RE: [PATCH] RFC: omapdrm DRM/KMS driver for TI OMAP platforms
Hello, Rob.
> -Original Message-
> From: robdcl...@gmail.com [mailto:robdcl...@gmail.com] On Behalf Of Rob
> Clark
> Sent: Tuesday, September 06, 2011 1:05 AM
> To: Inki Dae
> Cc: dri-de...@lists.freedesktop.org; linaro-dev@lists.linaro.org;
> Valkeinen, Tomi
> Subject: Re: [PATCH] RFC: omapdrm DRM/KMS driver for TI OMAP platforms
>
> On Mon, Sep 5, 2011 at 4:58 AM, Inki Dae wrote:
> > Hello, Rob.
> > I didn't look into any comments from other develops yet , so my comments
> > could be duplicated with other ones. below is my opinions and I
> commented
> > your codes also.
> >
> > We can discuss crtc, encoder and connector. in x86 world, my
> understanding,
> > each one means the following.
>
> just fwiw, some of current implementation in the KMS code is result of
> how the DSS2 API works.. ie. a dssdev (panel driver) is really part
> encoder and part connector.
>
> But on other hand, there are customers who write their own panel
> drivers in DSS2 so I'm not sure that we want to completely change
> this. And the encoder/connector distinction gets a bit more blurry w/
> smart-panel type displays.
>
> [snip]
> >> +static int omap_connector_mode_valid(struct drm_connector *connector,
> >> + struct drm_display_mode *mode)
> >> +{
> >> + struct omap_connector *omap_connector =
> >> to_omap_connector(connector);
> >> + struct omap_dss_device *dssdev = omap_connector->dssdev;
> >> + struct omap_dss_driver *dssdrv = dssdev->driver;
> >> + struct omap_video_timings timings = {0};
> >> + struct drm_device *dev = connector->dev;
> >> + struct drm_display_mode *new_mode;
> >> + int ret = MODE_BAD;
> >> +
> >> + copy_timings_drm_to_omap(&timings, mode);
> >> + mode->vrefresh = drm_mode_vrefresh(mode);
> >> +
> >> + if (!dssdrv->check_timings(dssdev, &timings)) {
> >> + /* check if vrefresh is still valid */
> >> + new_mode = drm_mode_duplicate(dev, mode);
> >> + new_mode->clock = timings.pixel_clock;
> >> + new_mode->vrefresh = 0;
> >> + if (mode->vrefresh == drm_mode_vrefresh(new_mode))
> >> + ret = MODE_OK;
> >> + drm_mode_destroy(dev, new_mode);
> >> + }
> >> +
> >
> > is there any reason that you call drm_mode_duplicate() to get a clone of
> a
> > mode? I just wonder it.
>
> yes, because the new_mode is modified and I didn't want this fxn to
> have unintended side-effects.
>
> this works in a slightly funny way, because dssdev->check_timings() is
> actually a dssdev->check_and_modify_timings(), modifying the pixel
> clock to the closest thing that is supported. We need to compare the
> resulting vrefresh to see if it is something matching.. otherwise we
> ask for some resolution @ 60Hz and maybe get 30Hz instead.
>
> [snip]
> >> +/* initialize connector */
> >> +struct drm_connector * omap_connector_init(struct drm_device *dev,
> >> + int connector_type, struct omap_dss_device *dssdev)
> >> +{
> >> + struct drm_connector *connector = NULL;
> >
> > It appears that this connector is initialized with NULL. This is just
> minor
> > issue. :)
>
> it is so it isn't uninitialized if kzalloc fails and we 'goto fail'..
> I guess in this particular case there is only a single fail point,
> although it is a coding pattern that I follow elsewhere where there
> are multiple fail points. And at least this way if I added some other
> step later where it might fail after the kzalloc(), I won't
> accidentally miss the appropriate cleanup. I mostly prefer to have a
> single cleanup path when possible so I don't confuse myself later
>
> [snip]
> >> +static void page_flip_cb(void *arg)
> >> +{
> >> + struct drm_crtc *crtc = arg;
> >> + struct drm_device *dev = crtc->dev;
> >> + struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> >> + struct drm_pending_vblank_event *event = omap_crtc->event;
> >> + struct timeval now;
> >> + unsigned long flags;
> >> +
> >> + WARN_ON(!event);
> >> +
> >> + omap_crtc->event = NULL;
> >> +
> >> + update_scanout(crtc);
> >> + commit(crtc);
> >> +
> >> + /* wakeup userspace */
> >> + // TODO: this should happen *after* flip.. someh
RE: [PATCH] RFC: omapdrm DRM/KMS driver for TI OMAP platforms
Hello Rob.
Sorry for being late. here was a national holiday.
> -Original Message-
> From: robdcl...@gmail.com [mailto:robdcl...@gmail.com] On Behalf Of Rob
> Clark
> Sent: Thursday, September 08, 2011 3:44 AM
> To: Inki Dae
> Cc: linaro-dev@lists.linaro.org; dri-de...@lists.freedesktop.org
> Subject: Re: [PATCH] RFC: omapdrm DRM/KMS driver for TI OMAP platforms
>
> On Wed, Sep 7, 2011 at 1:00 AM, Inki Dae wrote:
> > Hello, Rob.
> >
> [snip]
> >> >> +static void page_flip_cb(void *arg)
> >> >> +{
> >> >> + struct drm_crtc *crtc = arg;
> >> >> + struct drm_device *dev = crtc->dev;
> >> >> + struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> >> >> + struct drm_pending_vblank_event *event = omap_crtc->event;
> >> >> + struct timeval now;
> >> >> + unsigned long flags;
> >> >> +
> >> >> + WARN_ON(!event);
> >> >> +
> >> >> + omap_crtc->event = NULL;
> >> >> +
> >> >> + update_scanout(crtc);
> >> >> + commit(crtc);
> >> >> +
> >> >> + /* wakeup userspace */
> >> >> + // TODO: this should happen *after* flip.. somehow..
> >> >> + if (event) {
> >> >> + spin_lock_irqsave(&dev->event_lock, flags);
> >> >> + event->event.sequence =
> >> >> + drm_vblank_count_and_time(dev,
> >> > omap_crtc->id,
> >> >> &now);
> >> >> + event->event.tv_sec = now.tv_sec;
> >> >> + event->event.tv_usec = now.tv_usec;
> >> >> + list_add_tail(&event->base.link,
> >> >> + &event->base.file_priv->event_list);
> >> >> +
> > wake_up_interruptible(&event->base.file_priv->event_wait);
> >> >> + spin_unlock_irqrestore(&dev->event_lock, flags);
> >> >> + }
> >> >
> >> > How about moving codes above into interrupt handler for vblank?
> >> > maybe there
> >>
> >> I should mention a couple of things:
> >>
> >> 1) drm vblank stuff isn't really used currently.. it is actually DSS2
> >> layer that is registering for the display related interrupt(s). I'm
> >> not really sure how to handle this best. I suppose the DRM driver
> >> could *also* register for these interrupts, but that seems a bit odd..
> >>
> >
> > DRM Framework supports only one interrupt handler. this issue should be
> > resolved. and currently I made our driver to use its own request_irq,
> not
> > DRM Framework side. we only sets drm_device->irq_enabled to 1 and
> interrupt
> > handler is registered at display controller or hdmi driver respectively.
> but
> > I'm not sure that this way is best so I will look over more. Anyway
> current
> > drm framework should be fixed to be considered with multiple irq.
>
> Or perhaps even callbacks (some other driver handling the irq's directly)?
>
Yes.
> >> Also, I guess it is also worth mentioning.. when it comes to vblank,
> >> there are two fundamentally different sorts of displays we deal with.
> >> Something like DVI/HDMI/etc which need constant refreshing. In these
> >> cases we constantly scan-out the buffer until the next page
> >> flip+vsync. And smart panels, where they get scanned out once and
> >> then DSS is no longer reading the scanout buffer until we manually
> >> trigger an update.
> >>
> >
> > Is the Smart panel CPU interface based lcd panel that has its own
> > framebuffer internally.?
>
> yes
>
CPU Interface based lcd panel needs manual updating once all contents of
internal framebuffer are transferred to panel. thus, the Smart Panel(cpu
interface based lcd panel) has advantage of power consumption with
user-requested update. this means application should trigger display
controller to be transferred to lcd panel at framedone signal of lcd panel
because only the application is aware of this moment. this way would be
getting confused, especially on general os, such as linux system, based
platform so in case of using Smart Panel, I think we should make the panel
driver to be worked like RGB interface.
- framedone interrupt handler of the panel should be placed in the driver.
- at booting time, display controller driver triggers just one time
- the panel driver triggers ever
RE: [PATCH] RFC: omapdrm DRM/KMS driver for TI OMAP platforms
Hi, Rob.
> -Original Message-
> From: robdcl...@gmail.com [mailto:robdcl...@gmail.com] On Behalf Of Rob
> Clark
> Sent: Thursday, September 15, 2011 4:42 AM
> To: Inki Dae
> Cc: linaro-dev@lists.linaro.org; dri-de...@lists.freedesktop.org
> Subject: Re: [PATCH] RFC: omapdrm DRM/KMS driver for TI OMAP platforms
>
> On Wed, Sep 14, 2011 at 12:44 AM, Inki Dae wrote:
> > Hello Rob.
> > Sorry for being late. here was a national holiday.
> >
> >
> >> -Original Message-
> >> From: robdcl...@gmail.com [mailto:robdcl...@gmail.com] On Behalf Of Rob
> >> Clark
> >> Sent: Thursday, September 08, 2011 3:44 AM
> >> To: Inki Dae
> >> Cc: linaro-dev@lists.linaro.org; dri-de...@lists.freedesktop.org
> >> Subject: Re: [PATCH] RFC: omapdrm DRM/KMS driver for TI OMAP platforms
> >>
> >> On Wed, Sep 7, 2011 at 1:00 AM, Inki Dae wrote:
> >> > Hello, Rob.
> >> >
> >> [snip]
> >> >> >> +static void page_flip_cb(void *arg)
> >> >> >> +{
> >> >> >> + struct drm_crtc *crtc = arg;
> >> >> >> + struct drm_device *dev = crtc->dev;
> >> >> >> + struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> >> >> >> + struct drm_pending_vblank_event *event = omap_crtc->event;
> >> >> >> + struct timeval now;
> >> >> >> + unsigned long flags;
> >> >> >> +
> >> >> >> + WARN_ON(!event);
> >> >> >> +
> >> >> >> + omap_crtc->event = NULL;
> >> >> >> +
> >> >> >> + update_scanout(crtc);
> >> >> >> + commit(crtc);
> >> >> >> +
> >> >> >> + /* wakeup userspace */
> >> >> >> + // TODO: this should happen *after* flip.. somehow..
> >> >> >> + if (event) {
> >> >> >> + spin_lock_irqsave(&dev->event_lock, flags);
> >> >> >> + event->event.sequence =
> >> >> >> + drm_vblank_count_and_time(dev,
> >> >> > omap_crtc->id,
> >> >> >> &now);
> >> >> >> + event->event.tv_sec = now.tv_sec;
> >> >> >> + event->event.tv_usec = now.tv_usec;
> >> >> >> + list_add_tail(&event->base.link,
> >> >> >> +
&event->base.file_priv->event_list);
> >> >> >> +
> >> > wake_up_interruptible(&event->base.file_priv->event_wait);
> >> >> >> + spin_unlock_irqrestore(&dev->event_lock, flags);
> >> >> >> + }
> >> >> >
> >> >> > How about moving codes above into interrupt handler for vblank?
> >> >> > maybe there
> >> >>
> >> >> I should mention a couple of things:
> >> >>
> >> >> 1) drm vblank stuff isn't really used currently.. it is actually
> DSS2
> >> >> layer that is registering for the display related interrupt(s). I'm
> >> >> not really sure how to handle this best. I suppose the DRM driver
> >> >> could *also* register for these interrupts, but that seems a bit
> odd..
> >> >>
> >> >
> >> > DRM Framework supports only one interrupt handler. this issue should
> be
> >> > resolved. and currently I made our driver to use its own request_irq,
> >> not
> >> > DRM Framework side. we only sets drm_device->irq_enabled to 1 and
> >> interrupt
> >> > handler is registered at display controller or hdmi driver
> respectively.
> >> but
> >> > I'm not sure that this way is best so I will look over more. Anyway
> >> current
> >> > drm framework should be fixed to be considered with multiple irq.
> >>
> >> Or perhaps even callbacks (some other driver handling the irq's
> directly)?
> >>
> >
> > Yes.
> >
> >> >> Also, I guess it is also worth mentioning.. when it comes to vblank,
> >> >> there are two fundamentally different sorts of displays we deal
with.
> >> >> Something like DVI/HDMI/etc which need constant refreshing. In
> these
> >> >> cases we constantly scan-out the buffer until
RE: [PATCH] drm/exynos: Add fallback option to get non physically contiguous memory for gem_dumb_create
Applied.
Thanks,
Inki Dae
> -Original Message-
> From: Vikas Sajjan [mailto:vikas.saj...@linaro.org]
> Sent: Friday, August 23, 2013 3:35 PM
> To: dri-de...@lists.freedesktop.org; inki@samsung.com
> Cc: kgene@samsung.com; s.nawro...@samsung.com; robdcl...@gmail.com;
> tomasz.f...@gmail.com; laurent.pinch...@ideasonboard.com;
> patc...@linaro.org; linaro-dev@lists.linaro.org
> Subject: [PATCH] drm/exynos: Add fallback option to get non physically
> contiguous memory for gem_dumb_create
>
> To address the case where physically contiguous memory MAY NOT be a
> mandatory
> requirement for framebuffer for the application calling
> exynos_drm_gem_dumb_create,
> the patch adds a feature to get non physically contiguous memory for
> framebuffer,
> if physically contiguous memory allocation fails and if IOMMU is
supported.
>
> Signed-off-by: Vikas Sajjan
> Signed-off-by: Arun Kumar
> ---
> drivers/gpu/drm/exynos/exynos_drm_gem.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index 2eabe1a..66d1b40 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -17,6 +17,7 @@
> #include "exynos_drm_drv.h"
> #include "exynos_drm_gem.h"
> #include "exynos_drm_buf.h"
> +#include "exynos_drm_iommu.h"
>
> static unsigned int convert_to_vm_err_msg(int msg)
> {
> @@ -666,6 +667,18 @@ int exynos_drm_gem_dumb_create(struct drm_file
> *file_priv,
>
> exynos_gem_obj = exynos_drm_gem_create(dev, EXYNOS_BO_CONTIG |
> EXYNOS_BO_WC, args->size);
> + /*
> + * If physically contiguous memory allocation fails and if IOMMU is
> + * supported then try to get buffer from non physically contiguous
> + * memory area.
> + */
> + if (IS_ERR(exynos_gem_obj) && is_drm_iommu_supported(dev)) {
> + dev_warn(dev->dev, "contiguous FB allocation failed, falling
> back to non-contiguous\n");
> + exynos_gem_obj = exynos_drm_gem_create(dev,
> + EXYNOS_BO_NONCONTIG | EXYNOS_BO_WC,
> + args->size);
> + }
> +
> if (IS_ERR(exynos_gem_obj))
> return PTR_ERR(exynos_gem_obj);
>
> --
> 1.7.9.5
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH] drm/exynos: Add fallback option to get non physically contiguous memory for gem_dumb_create
One more thing, changed the subject to "Consider fallback option to
allocation fail". The subject is too long :)
Thanks,
Inki Dae
> -Original Message-
> From: linux-samsung-soc-ow...@vger.kernel.org [mailto:linux-samsung-soc-
> ow...@vger.kernel.org] On Behalf Of Vikas Sajjan
> Sent: Tuesday, August 27, 2013 12:05 PM
> To: Inki Dae
> Cc: DRI mailing list; kgene.kim; Sylwester Nawrocki; Rob Clark; Tomasz
> Figa; Laurent Pinchart; Patch Tracking; linaro-dev@lists.linaro.org; sunil
> joshi; linux-samsung-...@vger.kernel.org
> Subject: Re: [PATCH] drm/exynos: Add fallback option to get non physically
> contiguous memory for gem_dumb_create
>
> Thanks.
>
> On 27 August 2013 08:14, Inki Dae wrote:
> > Applied.
> >
> > Thanks,
> > Inki Dae
> >
> >> -Original Message-
> >> From: Vikas Sajjan [mailto:vikas.saj...@linaro.org]
> >> Sent: Friday, August 23, 2013 3:35 PM
> >> To: dri-de...@lists.freedesktop.org; inki@samsung.com
> >> Cc: kgene@samsung.com; s.nawro...@samsung.com; robdcl...@gmail.com;
> >> tomasz.f...@gmail.com; laurent.pinch...@ideasonboard.com;
> >> patc...@linaro.org; linaro-dev@lists.linaro.org
> >> Subject: [PATCH] drm/exynos: Add fallback option to get non physically
> >> contiguous memory for gem_dumb_create
> >>
> >> To address the case where physically contiguous memory MAY NOT be a
> >> mandatory
> >> requirement for framebuffer for the application calling
> >> exynos_drm_gem_dumb_create,
> >> the patch adds a feature to get non physically contiguous memory for
> >> framebuffer,
> >> if physically contiguous memory allocation fails and if IOMMU is
> > supported.
> >>
> >> Signed-off-by: Vikas Sajjan
> >> Signed-off-by: Arun Kumar
> >> ---
> >> drivers/gpu/drm/exynos/exynos_drm_gem.c | 13 +
> >> 1 file changed, 13 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> >> b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> >> index 2eabe1a..66d1b40 100644
> >> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> >> @@ -17,6 +17,7 @@
> >> #include "exynos_drm_drv.h"
> >> #include "exynos_drm_gem.h"
> >> #include "exynos_drm_buf.h"
> >> +#include "exynos_drm_iommu.h"
> >>
> >> static unsigned int convert_to_vm_err_msg(int msg)
> >> {
> >> @@ -666,6 +667,18 @@ int exynos_drm_gem_dumb_create(struct drm_file
> >> *file_priv,
> >>
> >> exynos_gem_obj = exynos_drm_gem_create(dev, EXYNOS_BO_CONTIG |
> >> EXYNOS_BO_WC,
args->size);
> >> + /*
> >> + * If physically contiguous memory allocation fails and if IOMMU
> is
> >> + * supported then try to get buffer from non physically
contiguous
> >> + * memory area.
> >> + */
> >> + if (IS_ERR(exynos_gem_obj) && is_drm_iommu_supported(dev)) {
> >> + dev_warn(dev->dev, "contiguous FB allocation failed,
falling
> >> back to non-contiguous\n");
> >> + exynos_gem_obj = exynos_drm_gem_create(dev,
> >> + EXYNOS_BO_NONCONTIG |
EXYNOS_BO_WC,
> >> + args->size);
> >> + }
> >> +
> >> if (IS_ERR(exynos_gem_obj))
> >> return PTR_ERR(exynos_gem_obj);
> >>
> >> --
> >> 1.7.9.5
> >
>
>
>
> --
> Thanks and Regards
> Vikas Sajjan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-
> soc" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
