Re: [PATCH 06/21] drm/omap: check CRTC color format earlier
On Thu, Feb 26, 2015 at 03:20:14PM +0200, Tomi Valkeinen wrote: > When setting a color format to a DRM plane, the DRM core checks whether > the format is supported by the HW. However, it seems that when setting > the color format of a CRTC (i.e. a root plane), there's no checking > done. This causes omapdrm to configure omapdss with the bad color > format, which omapdss then rejects. > > While the above is ok, the error message is a bit confusing, and the > configuring of omapdss happens asynchronously from the ioctl that does > the color format change. > > This patch adds a color format check to omap_plane_mode_set() which > causes the ioctl to return an error for invalid color format. This means > that the userspace will get to know of the wrong setting, instead of the > current behavior where the error is not seen by the userspace. > > Signed-off-by: Tomi Valkeinen > --- > drivers/gpu/drm/omapdrm/omap_plane.c | 18 ++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c > b/drivers/gpu/drm/omapdrm/omap_plane.c > index 1f4f2b866379..bedd6f7af0f1 100644 > --- a/drivers/gpu/drm/omapdrm/omap_plane.c > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c > @@ -207,6 +207,24 @@ int omap_plane_mode_set(struct drm_plane *plane, > { > struct omap_plane *omap_plane = to_omap_plane(plane); > struct omap_drm_window *win = &omap_plane->win; > + int i; > + > + /* > + * Check whether this plane supports the fb pixel format. > + * I don't think this should really be needed, but it looks like the > + * drm core only checks the format for planes, not for the crtc. So > + * when setting the format for crtc, without this check we would > + * get an error later when trying to program the color format into the > + * HW. > + */ I think we should add a format check to the setcrtc ioctl if crtc->primary is set. Atm the code is in __setplane_internal but could easily be shared - there's already a copy in drm_atomi.c. Omapdrm wouldn't benefit from this yet since it doesn't support universal planes. But adding universal planes should be on your todo anyway ;-) -Daniel > + for (i = 0; i < plane->format_count; i++) > + if (fb->pixel_format == plane->format_types[i]) > + break; > + if (i == plane->format_count) { > + DBG("Invalid pixel format %s", > + drm_get_format_name(fb->pixel_format)); > + return -EINVAL; > + } > > win->crtc_x = crtc_x; > win->crtc_y = crtc_y; > -- > 2.3.0 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/21] drm/omap: handle mismatching color format and buffer width
On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote: > omapdrm doesn't check if the width of the framebuffer and the color > format's bits-per-pixel match. > > For example, using a display with a width of 1280, and a buffer > allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with > a 24 bits per pixel color format, leads to the following mismatch: > 5120/3 = 1706.666... bytes. This causes bad colors and a tilt on the > screen. > > Add a check into omapdrm to return an error if the user tries to use > such a combination. > > Signed-off-by: Tomi Valkeinen > --- > drivers/gpu/drm/omapdrm/omap_fb.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c > b/drivers/gpu/drm/omapdrm/omap_fb.c > index 2975096abdf5..bf98580223d0 100644 > --- a/drivers/gpu/drm/omapdrm/omap_fb.c > +++ b/drivers/gpu/drm/omapdrm/omap_fb.c > @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct > drm_device *dev, > goto fail; > } > > + if (mode_cmd->width % format->planes[i].stride_bpp != 0) { width is in pixels. No idea what you're trying to check here, but this probably isn't it. Also drm checks that things fit into the specified pitch (which is in bytes), see the pichtes[i] < width * cpp check in framebuffer_check. Cheers, Daniel > + dev_err(dev->dev, > + "buffer width (%d) is not a multiple of pixel > width (%d)\n", > + mode_cmd->width, format->planes[i].stride_bpp); > + ret = -EINVAL; > + goto fail; > + } > + > size = pitch * mode_cmd->height / format->planes[i].sub_y; > > if (size > (omap_gem_mmap_size(bos[i]) - mode_cmd->offsets[i])) > { > -- > 2.3.0 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/21] drm/omap: handle mismatching color format and buffer width
On Fri, Feb 27, 2015 at 02:40:20PM +, Daniel Stone wrote: > On 27 February 2015 at 13:01, Daniel Vetter wrote: > > On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote: > >> omapdrm doesn't check if the width of the framebuffer and the color > >> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c > >> b/drivers/gpu/drm/omapdrm/omap_fb.c > >> index 2975096abdf5..bf98580223d0 100644 > >> --- a/drivers/gpu/drm/omapdrm/omap_fb.c > >> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c > >> @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct > >> drm_device *dev, > >> goto fail; > >> } > >> > >> + if (mode_cmd->width % format->planes[i].stride_bpp != 0) { > > > > width is in pixels. No idea what you're trying to check here, but this > > probably isn't it. > > stride_bpp is very misnamed: it is the bits per pixel for that plane, > and not stride at all. I think the check should in fact be be (pitch % > format->planes[i].stride_bpp), which would achieve the desired result, > i.e. that the stride can be expressed as an integer number of pixels. I meant that mode_cmd->width is in pixels and so totally not what you want to check here. It probably should be mode_cmd->pitches[i]. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders
On Mon, Sep 28, 2015 at 12:26:28PM +0100, Russell King - ARM Linux wrote: > On Mon, Sep 28, 2015 at 11:01:34AM +0200, Arnaud Pouliquen wrote: > > few questions/remarks > > BR, > > Arnaud > > > > >+static void hdmi_codec_abort(struct device *dev) > > >+{ > > >+struct hdmi_codec_priv *hcp = dev_get_drvdata(dev); > > >+ > > >+dev_dbg(dev, "%s()\n", __func__); > > >+ > > >+mutex_lock(&hcp->current_stream_lock); > > >+if (hcp->current_stream && hcp->current_stream->runtime && > > >+snd_pcm_running(hcp->current_stream)) { > > >+dev_info(dev, "HDMI audio playback aborted\n"); > > >+snd_pcm_stream_lock_irq(hcp->current_stream); > > >+snd_pcm_stop(hcp->current_stream, SNDRV_PCM_STATE_DISCONNECTED); > > >+snd_pcm_stream_unlock_irq(hcp->current_stream); > > >+} > > >+mutex_unlock(&hcp->current_stream_lock); > > >+} > > Does driver should stop the stream in case of unplug? > > This could generate unexpected behavior at application level. > > Perhaps should be better if this part is managed in DRM driver. if HDMI > > master, I2S bus should be maintained ON to consume the audio stream until > > application action. > > If it does, that's really horrid. Atm the rule for display outputs is that nothing gets yanked until userspace approves, since otherwise compositors get stuck (or fall over with an unexpected -EINVAL from the kernel). The exception is DP MST because the current implementation is a complete hack for DP MST sink lifetimes and that's why we need to synchronously nuke them (which means shutting down everything). I'm surprised not a hole lot more people complain about this ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RESEND 0/9] drm: tilcdc driver fixes
On Sat, Jun 28, 2014 at 06:51:15AM -0400, Rob Clark wrote: > On Fri, Jun 27, 2014 at 6:08 PM, Darren Etheridge wrote: > > Guido, > > > > > > On 06/17/2014 09:17 AM, Guido MartÃnez wrote: > >> > >> The tilcdc driver could be compiled as a module, but was severely broken > >> and could not be used as such. This patchset attempts to fix the issues > >> preventing a proper load/unload of the module. > >> > >> Issues included dangling sysfs nodes, dangling devices, memory leaks and > >> a double kfree. > >> > >> It now seems to be working ok. We have tested this by loading and > >> unloading the driver repeteadly, with both panel and slave connectors > >> and found no flaws. > >> > >> There is still one warning left on tilcdc_crtc_destroy, caused by > >> destroying the connector while still in an ON status. We don't know why > >> this happens or why it's an issue, so we did not fix it. > >> > > > > Yes I see what you mean, it triggers the WARN_ON in tilcdc_crtc_destroy > > because DRM_MODE_DPMS_ON is still set. This WARN_ON does make some sense > > because DPMS_OFF would have the effect of turning off clocks and putting the > > monitor to sleep which seems logical considering we have torn down the > > display. Adding a tilcdc_crtc_dpms(DPMS_OFF) right before the WARN_ON > > confirms this, but it seems strange that this hasn't happened automatically > > (+ Russell doesn't need to do it in his Armada driver) - so I suspect there > > is a better way. > > tbh, I'm not entirely sure offhand why drm_mode_config_cleanup() > doesn't remove the fb's first (which should have the effect of > shutting down any lit crtc/encoder/connector).. that would seem like > the sensible way to shut down.. All userspace fbs should be cleared already before going into the driver unload. Which only leaves you with driver internal fbs (usually just one for fbdev emulation). It's the driver's job to clean that up explicitly. Then you can call mode_config_cleanup and the WARN_ON in there is a really nice space leak check. If we'd unconditionally clean up all fbs we'd have trouble with driver-private embedded fbs and their refcounting and would loose the space leak check. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] drm/lcdc: add TI LCD Controller DRM driver
On Fri, Dec 14, 2012 at 1:04 AM, Rob Clark wrote: > +static int lcdc_crtc_page_flip(struct drm_crtc *crtc, > + struct drm_framebuffer *fb, > + struct drm_pending_vblank_event *event) > +{ > + struct lcdc_crtc *lcdc_crtc = to_lcdc_crtc(crtc); > + struct drm_device *dev = crtc->dev; > + > + if (lcdc_crtc->event) { > + dev_err(dev->dev, "already pending page flip!\n"); > + return -EBUSY; > + } > + > + // TODO we should hold a ref to the fb somewhere.. Note that with the current fb refcounting nothing prevents you from fixing this. The ugly problems I've had to solve for the locking rework are all due to the drm core (i.e. setcrtc/pageflip/...) ioctl functions assuming that an fb can't suddenly disappear while holding the mode_config lock. Since I wanted to remove that requirement I've had to changed the refcounting in the drm core functions. But drivers can already extend the lifetime of an fb simply by grabbing a reference (as long as they grab that reference while holding the struct mutex all the time between fb lookup and grabbing the reference). And it will continue to work the same with the new locking scheme. -Daniel > + crtc->fb = fb; > + lcdc_crtc->event = event; > + update_scanout(crtc); > + > + return 0; > +} > + -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drm/omap: Take a fb reference in omap_plane_update()
On Tue, Apr 09, 2013 at 05:56:02PM +0530, Archit Taneja wrote: > When userspace calls SET_PLANE ioctl, drm core takes a reference of the fb and > passes control to the update_plane op defined by the drm driver. > > In omapdrm, we have a worker thread which queues framebuffers objects received > from update_plane and displays them at the appropriate time. > > It is possible that the framebuffer is destoryed by userspace between the time > of calling the ioctl and apply-worker being scheduled. If this happens, the > apply-worker holds a pointer to a framebuffer which is already destroyed. > > Take an extra refernece/unreference of the fb in omap_plane_update() to > prevent > this from happening. A reference is taken of the fb passed to update_plane(), > the previous framebuffer (held by plane->fb) is unreferenced. This will > prevent > drm from destroying the framebuffer till the time it's unreferenced by the > apply-worker. > > This is in addition to the exisitng reference/unreference in update_pin(), > which is taken for the scanout of the plane's current framebuffer, and an > unreference the previous framebuffer. > > Signed-off-by: Archit Taneja > Cc: Rob Clark > --- > drivers/gpu/drm/omapdrm/omap_plane.c |6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c > b/drivers/gpu/drm/omapdrm/omap_plane.c > index 2882cda..8d225d7 100644 > --- a/drivers/gpu/drm/omapdrm/omap_plane.c > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c > @@ -247,6 +247,12 @@ static int omap_plane_update(struct drm_plane *plane, > { > struct omap_plane *omap_plane = to_omap_plane(plane); > omap_plane->enabled = true; > + > + if (plane->fb) > + drm_framebuffer_unreference(plane->fb); Shouldn't the unref only happen once the flip has completed? Otherwise we might free the memory which is still being scanned out and put some other crap there. Note that I've been too lazy to read the code, but I expected the unref of the old one to happen after the ref-taking for the new one, with a vblank wait/delay in between ... Might be that I'm completely off ;-) -Daniel > + > + drm_framebuffer_reference(fb); > + > return omap_plane_mode_set(plane, crtc, fb, > crtc_x, crtc_y, crtc_w, crtc_h, > src_x, src_y, src_w, src_h, > -- > 1.7.10.4 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drm/omap: Take a fb reference in omap_plane_update()
On Tue, Apr 9, 2013 at 3:17 PM, Rob Clark wrote: >>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c >>> b/drivers/gpu/drm/omapdrm/omap_plane.c >>> index 2882cda..8d225d7 100644 >>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c >>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c >>> @@ -247,6 +247,12 @@ static int omap_plane_update(struct drm_plane *plane, >>> { >>> struct omap_plane *omap_plane = to_omap_plane(plane); >>> omap_plane->enabled = true; >>> + >>> + if (plane->fb) >>> + drm_framebuffer_unreference(plane->fb); >> >> Shouldn't the unref only happen once the flip has completed? Otherwise we >> might free the memory which is still being scanned out and put some other >> crap there. > > yup, there is a 2nd ref grabbed when we start scanout and dropped when > we finish scanout.. so that part was already covered. What wasn't > covered before was the time between the ioctl and the worker thread > (which was grabbing/dropping the scanout ref) Ah, I see. And the ordering doesn't seem to matter here since it's all protected by locks (against races with the worker thread) anyway. Thanks for the explanation. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
On Tue, Jan 22, 2013 at 04:36:22PM -0600, Rob Clark wrote: > A simple DRM/KMS driver for the TI LCD Controller found in various > smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the > CMA helpers. Currently only the TFP410 DVI encoder is supported > (tested with beaglebone + DVI cape). There are also various LCD > displays, for which support can be added (as I get hw to test on), > and an external i2c HDMI encoder found on some boards. > > The display controller supports a single CRTC. And the encoder+ > connector are split out into sub-devices. Depending on which LCD > or external encoder is actually present, the appropriate output > module(s) will be loaded. > > v1: original > v2: fix fb refcnting and few other cleanups > v3: get +/- vsync/hsync from timings rather than panel-info, add > option DT max-bandwidth field so driver doesn't attempt to > pick a display mode with too high memory bandwidth, and other > small cleanups > > Signed-off-by: Rob Clark Ok, read through it, looks nice. No idea whether this should use the panel stuff, but since no-one else does who cares. A few nits and questions below. Was a nice reading to learn about kfifo and the runtime power stuff. Reviewed-by: Daniel Vetter > --- > drivers/gpu/drm/Kconfig| 2 + > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/tilcdc/Kconfig | 10 + > drivers/gpu/drm/tilcdc/Makefile| 8 + > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 597 > drivers/gpu/drm/tilcdc/tilcdc_drv.c| 605 > + > drivers/gpu/drm/tilcdc/tilcdc_drv.h| 159 + > drivers/gpu/drm/tilcdc/tilcdc_regs.h | 154 + > drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 423 +++ > drivers/gpu/drm/tilcdc/tilcdc_tfp410.h | 26 ++ > 10 files changed, 1985 insertions(+) > create mode 100644 drivers/gpu/drm/tilcdc/Kconfig > create mode 100644 drivers/gpu/drm/tilcdc/Makefile > create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_crtc.c > create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.c > create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.h > create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_regs.h > create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c > create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.h > [cut] > +struct tilcdc_crtc { > + struct drm_crtc base; > + > + const struct tilcdc_panel_info *info; > + uint32_t dirty; > + dma_addr_t start, end; > + struct drm_pending_vblank_event *event; > + int dpms; > + wait_queue_head_t frame_done_wq; > + bool frame_done; > + > + /* fb currently set to scanout 0/1: */ > + struct drm_framebuffer *scanout[2]; > + > + /* for deferred fb unref's: */ > + DECLARE_KFIFO_PTR(unref_fifo, struct drm_framebuffer *); > + struct work_struct work; > +}; > +#define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base) > + > +static void unref_worker(struct work_struct *work) > +{ > + struct tilcdc_crtc *tilcdc_crtc = container_of(work, struct > tilcdc_crtc, work); > + struct drm_device *dev = tilcdc_crtc->base.dev; > + struct drm_framebuffer *fb; > + > + mutex_lock(&dev->mode_config.mutex); > + while (kfifo_get(&tilcdc_crtc->unref_fifo, &fb)) > + drm_framebuffer_unreference(fb); > + mutex_unlock(&dev->mode_config.mutex); Hm, just learned about the kfifo api. It looks like the locking here still works even with the new modeset locking, since kfifo explicitly allows concurrent readers and writers without locking, as long as there's only on of each kind. But maybe switch over to crtc->mutex to make things less tricky. Also, kfifo seems to have a new api which allows embedding of the kfifo thing and needs to be used with kfifo_in/out. [cut] > +static void update_scanout(struct drm_crtc *crtc) > +{ > + struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); > + struct drm_device *dev = crtc->dev; > + struct drm_framebuffer *fb = crtc->fb; > + struct drm_gem_cma_object *gem; > + unsigned int depth, bpp; > + > + drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp); > + gem = drm_fb_cma_get_gem_obj(fb, 0); > + > + tilcdc_crtc->start = gem->paddr + fb->offsets[0] + > + (crtc->y * fb->pitches[0]) + (crtc->x * bpp/8); > + > + tilcdc_crtc->end = tilcdc_crtc->start + > + (crtc->mode.vdisplay * fb->pitches[0]); > + > + if (tilcdc_crtc->dpms == DRM_MODE_DPMS_ON) { > + /* already enabled, so just mark the frames
Re: [PATCH 2/4] drm/i2c: nxp-tda998x (v2)
On Tue, Jan 22, 2013 at 04:36:23PM -0600, Rob Clark wrote: > Driver for the NXP TDA998X i2c hdmi encoder slave. > > v1: original > v2: fix npix/nline programming > > Signed-off-by: Rob Clark Just one bikeshed, otherwise Reviewed-by: Daniel Vetter [cut] > +static void > +reg_set(struct drm_encoder *encoder, uint16_t reg, uint8_t val) > +{ > + reg_write(encoder, reg, reg_read(encoder, reg) | val); > +} > + > +static void > +reg_clear(struct drm_encoder *encoder, uint16_t reg, uint8_t val) > +{ > + reg_write(encoder, reg, reg_read(encoder, reg) & ~val); > +} What about drivers/base/regmap? I haven't looked to closely yet and never used it in code, but there's a presentation [1] and it sounds like it provides some nice (and more important standardized) helper stuff for debug, tracing, ... Since encoder slave drivers tend to be utterly boring register bashing and we expect tons of time, I think high levels of standardization would be really useful. Care to look into this a bit? Cheers, Daniel 1: http://free-electrons.com/blog/fosdem2012-videos/ -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] drm/tilcdc: add encoder slave
On Tue, Jan 22, 2013 at 04:36:24PM -0600, Rob Clark wrote: > Add output panel driver for i2c encoder slaves. > > Signed-off-by: Rob Clark A few questions below, otherwise Reviewed-by: Daniel Vetter > --- > drivers/gpu/drm/tilcdc/Kconfig| 12 ++ > drivers/gpu/drm/tilcdc/Makefile | 1 + > drivers/gpu/drm/tilcdc/tilcdc_drv.c | 5 +- > drivers/gpu/drm/tilcdc/tilcdc_slave.c | 380 > ++ > drivers/gpu/drm/tilcdc/tilcdc_slave.h | 26 +++ > 5 files changed, 423 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.c > create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.h > > diff --git a/drivers/gpu/drm/tilcdc/Kconfig b/drivers/gpu/drm/tilcdc/Kconfig > index ee9b592..99beca2 100644 > --- a/drivers/gpu/drm/tilcdc/Kconfig > +++ b/drivers/gpu/drm/tilcdc/Kconfig > @@ -8,3 +8,15 @@ config DRM_TILCDC > Choose this option if you have an TI SoC with LCDC display > controller, for example AM33xx in beagle-bone, DA8xx, or > OMAP-L1xx. This driver replaces the FB_DA8XX fbdev driver. > + > +menu "I2C encoder or helper chips" > + depends on DRM && DRM_KMS_HELPER && I2C > + > +config DRM_I2C_NXP_TDA998X > + tristate "NXP Semiconductors TDA998X HDMI encoder" > + default m if DRM_TILCDC > + help > + Support for NXP Semiconductors TDA998X HDMI encoders. > + > +endmenu > + Shouldn't that hunk be in patch 2? > diff --git a/drivers/gpu/drm/tilcdc/Makefile b/drivers/gpu/drm/tilcdc/Makefile > index 1359cc2..aa9097e 100644 > --- a/drivers/gpu/drm/tilcdc/Makefile > +++ b/drivers/gpu/drm/tilcdc/Makefile > @@ -3,6 +3,7 @@ ccflags-y := -Iinclude/drm -Werror > tilcdc-y := \ > tilcdc_crtc.o \ > tilcdc_tfp410.o \ > + tilcdc_slave.o \ > tilcdc_drv.o > > obj-$(CONFIG_DRM_TILCDC) += tilcdc.o > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c > b/drivers/gpu/drm/tilcdc/tilcdc_drv.c > index cf1fddc..ca76dbe 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c > @@ -20,6 +20,7 @@ > #include "tilcdc_drv.h" > #include "tilcdc_regs.h" > #include "tilcdc_tfp410.h" > +#include "tilcdc_slave.h" > > #include "drm_fb_helper.h" > > @@ -587,6 +588,7 @@ static int __init tilcdc_drm_init(void) > { > DBG("init"); > tilcdc_tfp410_init(); > + tilcdc_slave_init(); > return platform_driver_register(&tilcdc_platform_driver); > } > > @@ -594,10 +596,11 @@ static void __exit tilcdc_drm_fini(void) > { > DBG("fini"); > tilcdc_tfp410_fini(); > + tilcdc_slave_fini(); > platform_driver_unregister(&tilcdc_platform_driver); > } > > -module_init(tilcdc_drm_init); > +late_initcall(tilcdc_drm_init); > module_exit(tilcdc_drm_fini); > > MODULE_AUTHOR("Rob Clark diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c > b/drivers/gpu/drm/tilcdc/tilcdc_slave.c > new file mode 100644 > index 000..b6f3e63 > --- /dev/null > +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c > @@ -0,0 +1,380 @@ > +/* > + * Copyright (C) 2012 Texas Instruments > + * Author: Rob Clark > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License version 2 as published > by > + * the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, but > WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along > with > + * this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include > +#include > +#include > +#include > +#include > + > +#include "tilcdc_drv.h" > + > +struct slave_module { > + struct tilcdc_module base; > + struct i2c_adapter *i2c; > +}; > +#define to_slave_module(x) container_of(x, struct slave_module, base) > + > +static const struct tilcdc_panel_info slave_info = { > + .min_bpp= 16, > + .max_bpp= 16, > + .bpp= 16, > + .ac_bias= 255, > + .ac_bias_intrpt = 0, > + .dma_burst_sz = 16, > + .fdd= 0x80, > + .tft_alt_mode = 0, > +
Re: [PATCH 3/4] drm/tilcdc: add encoder slave
On Tue, Jan 22, 2013 at 04:36:24PM -0600, Rob Clark wrote: > Add output panel driver for i2c encoder slaves. > > Signed-off-by: Rob Clark Found some more stuff ... [cut] > +static const struct drm_encoder_helper_funcs slave_encoder_helper_funcs = { > + .dpms = drm_i2c_encoder_dpms, > + .mode_fixup = drm_i2c_encoder_mode_fixup, > + .prepare= slave_encoder_prepare, > + .commit = drm_i2c_encoder_commit, > + .mode_set = drm_i2c_encoder_mode_set, > + .save = drm_i2c_encoder_save, > + .restore= drm_i2c_encoder_restore, > +}; I couldn't find these wrappers anywhere ... > + > +static const struct i2c_board_info info = { > + I2C_BOARD_INFO("tda998x", 0x70) > +}; Shouldn't there be some of/devicetree thing to tell us which one to load? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] drm/tilcdc: add support for LCD panels (v4)
On Tue, Jan 22, 2013 at 04:36:25PM -0600, Rob Clark wrote: > Add an output panel driver for LCD panels. Tested with LCD3 cape on > beaglebone. > > v1: original > v2: s/of_find_node_by_name()/of_get_child_by_name()/ from Pantelis > Antoniou > v3: add backlight support > v4: rebase to latest of video timing helpers > > Signed-off-by: Rob Clark So given that I'm utterly lacking clue about all things of (which seems to be where all the magic in this patch lies) I'm just gonna ask a few funny questions. [cut] > +static int panel_connector_get_modes(struct drm_connector *connector) > +{ > + struct drm_device *dev = connector->dev; > + struct panel_connector *panel_connector = to_panel_connector(connector); > + struct display_timings *timings = panel_connector->mod->timings; > + int i; > + > + for (i = 0; i < timings->num_timings; i++) { > + struct drm_display_mode *mode = drm_mode_create(dev); > + struct videomode vm; > + > + if (videomode_from_timing(timings, &vm, i)) > + break; > + > + drm_display_mode_from_videomode(&vm, mode); Why do we need to jump through the intermediate videomode thing here? Is that a deficiency of the of/videomode stuff? [cut] > + ret |= of_property_read_u32(info_np, "ac-bias", &info->ac_bias); > + ret |= of_property_read_u32(info_np, "ac-bias-intrpt", > &info->ac_bias_intrpt); > + ret |= of_property_read_u32(info_np, "dma-burst-sz", > &info->dma_burst_sz); > + ret |= of_property_read_u32(info_np, "bpp", &info->bpp); > + ret |= of_property_read_u32(info_np, "fdd", &info->fdd); > + ret |= of_property_read_u32(info_np, "sync-edge", &info->sync_edge); > + ret |= of_property_read_u32(info_np, "sync-ctrl", &info->sync_ctrl); > + ret |= of_property_read_u32(info_np, "raster-order", > &info->raster_order); > + ret |= of_property_read_u32(info_np, "fifo-th", &info->fifo_th); Shouldn't these values all be documented somewhere in the devictree docs? Or are they somewhat standardized? > + > + /* optional: */ > + info->tft_alt_mode = of_property_read_bool(info_np, > "tft-alt-mode"); > + info->stn_565_mode = of_property_read_bool(info_np, > "stn-565-mode"); > + info->mono_8bit_mode= of_property_read_bool(info_np, > "mono-8bit-mode"); > + info->invert_pxl_clk= of_property_read_bool(info_np, > "invert-pxl-clk"); > + > + if (of_property_read_u32(info_np, "max-bpp", &info->max_bpp)) > + info->max_bpp = info->bpp; > + if (of_property_read_u32(info_np, "min-bpp", &info->min_bpp)) > + info->min_bpp = info->bpp; > + > + if (ret) { > + pr_err("%s: error reading panel-info properties\n", __func__); > + kfree(info); > + return NULL; > + } > + > + return info; > +} > + > +static struct of_device_id panel_of_match[]; > + > +static int panel_probe(struct platform_device *pdev) > +{ > + struct device_node *node = pdev->dev.of_node; > + struct panel_module *panel_mod; > + struct tilcdc_module *mod; > + struct pinctrl *pinctrl; > + int ret = -EINVAL; > + > + > + /* bail out early if no DT data: */ > + if (!node) { > + dev_err(&pdev->dev, "device-tree data is missing\n"); > + return -ENXIO; > + } > + > + panel_mod = kzalloc(sizeof(*panel_mod), GFP_KERNEL); > + if (!panel_mod) > + return -ENOMEM; > + > + mod = &panel_mod->base; > + > + tilcdc_module_init(mod, "panel", &panel_module_ops); > + > + pinctrl = devm_pinctrl_get_select_default(&pdev->dev); > + if (IS_ERR(pinctrl)) > + dev_warn(&pdev->dev, "pins are not configured\n"); > + > + > + panel_mod->timings = of_get_display_timings(node); > + if (!panel_mod->timings) { > + dev_err(&pdev->dev, "could not get panel timings\n"); > + goto fail; > + } > + > + panel_mod->info = of_get_panel_info(node); > + if (!panel_mod->info) { > + dev_err(&pdev->dev, "could not get panel info\n"); > + goto fail; > + } > + > + panel_mod->backlight = of_find_backlight_by_node(node); If this _really_ works that easily, I'll have of-envy for the rest of my life :( /me hates the real-world abomination called Intel backlight handling Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 0/2] Disable planes on blanked CRTC and enable on unblank
On Fri, Nov 13, 2015 at 05:53:13PM +0200, Jyri Sarha wrote: > Since first RFC version: > - Added "drm/atomic: Track drm_plane's active state"-patch > > We would need something like this to get rid off OMAPDSS somewhat > messy runtime_resume code. How does this look, could this generic > solution be refined to be acceptable for mainline, or should we start > looking a local solution to omapdrm? One more comment besides what I've written in the older thread: You're not on latest drm-next, which has gained an active_only paramter to the plane commit helper. It might be best to discuss this topic on #dri-devel on freenode irc a bit. Cheers, Daniel > > Jyri Sarha (2): > drm/atomic: Track drm_plane's active state > drm/atomic: Disable planes on blanked CRTC and enable on unblank > > drivers/gpu/drm/drm_atomic_helper.c | 82 > + > drivers/gpu/drm/drm_plane_helper.c | 10 + > include/drm/drm_atomic_helper.h | 39 +- > include/drm/drm_crtc.h | 2 + > 4 files changed, 70 insertions(+), 63 deletions(-) > > -- > 1.9.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html