All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/4] drm/omap: fix output enable/disable sequence
@ 2014-04-02 12:37 Tomi Valkeinen
  2014-04-02 12:37 ` [PATCH 2/4] drm/omap: fix uninit order in pdev_remove() Tomi Valkeinen
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Tomi Valkeinen @ 2014-04-02 12:37 UTC (permalink / raw)
  To: Rob Clark, Archit Taneja, dri-devel; +Cc: Tomi Valkeinen

At the moment it's quite easy to get the following errors when the HDMI
output is enabled or disabled:

[drm:omap_crtc_error_irq] *ERROR* tv: errors: 00008000

The reason for the errors is that the omapdrm driver doesn't properly
handle the sync-lost irqs that happen when enabling the DIGIT crtc,
which is used for HDMI and analog TV. The driver does disable the
sync-lost irq properly, but it fails to wait until the output has been
fully enabled (i.e. the first vsync), so the sync-lost errors are still
seen occasionally.

This patch makes the omapdrm act the same way as the omapfb does:

- When enabling a display, we'll wait for the first vsync.
- When disabling a display, we'll wait for framedone if available, or
  odd and even vsyncs.

These changes make sure the output is fully enabled or disabled at the
end of the function.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reported-by: Sanjay Singh Rawat <sanjay.rawat@linaro.org>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 46 ++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 4313bb0a49a6..e7b643c178a6 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -527,38 +527,46 @@ static void set_enabled(struct drm_crtc *crtc, bool enable)
 	struct drm_device *dev = crtc->dev;
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
 	enum omap_channel channel = omap_crtc->channel;
-	struct omap_irq_wait *wait = NULL;
+	struct omap_irq_wait *wait;
+	u32 framedone_irq, vsync_irq;
+	int ret;
 
 	if (dispc_mgr_is_enabled(channel) == enable)
 		return;
 
-	/* ignore sync-lost irqs during enable/disable */
+	/*
+	 * Digit output produces some sync lost interrupts during the first
+	 * frame when enabling, so we need to ignore those.
+	 */
 	omap_irq_unregister(crtc->dev, &omap_crtc->error_irq);
 
-	if (dispc_mgr_get_framedone_irq(channel)) {
-		if (!enable) {
-			wait = omap_irq_wait_init(dev,
-					dispc_mgr_get_framedone_irq(channel), 1);
-		}
+	framedone_irq = dispc_mgr_get_framedone_irq(channel);
+	vsync_irq = dispc_mgr_get_vsync_irq(channel);
+
+	if (enable) {
+		wait = omap_irq_wait_init(dev, vsync_irq, 1);
 	} else {
 		/*
-		 * When we disable digit output, we need to wait until fields
-		 * are done.  Otherwise the DSS is still working, and turning
-		 * off the clocks prevents DSS from going to OFF mode. And when
-		 * enabling, we need to wait for the extra sync losts
+		 * When we disable the digit output, we need to wait for
+		 * FRAMEDONE to know that DISPC has finished with the output.
+		 *
+		 * OMAP2/3 does not have FRAMEDONE irq for digit output, and in
+		 * that case we need to use vsync interrupt, and wait for both
+		 * even and odd frames.
 		 */
-		wait = omap_irq_wait_init(dev,
-				dispc_mgr_get_vsync_irq(channel), 2);
+
+		if (framedone_irq)
+			wait = omap_irq_wait_init(dev, framedone_irq, 1);
+		else
+			wait = omap_irq_wait_init(dev, vsync_irq, 2);
 	}
 
 	dispc_mgr_enable(channel, enable);
 
-	if (wait) {
-		int ret = omap_irq_wait(dev, wait, msecs_to_jiffies(100));
-		if (ret) {
-			dev_err(dev->dev, "%s: timeout waiting for %s\n",
-					omap_crtc->name, enable ? "enable" : "disable");
-		}
+	ret = omap_irq_wait(dev, wait, msecs_to_jiffies(100));
+	if (ret) {
+		dev_err(dev->dev, "%s: timeout waiting for %s\n",
+				omap_crtc->name, enable ? "enable" : "disable");
 	}
 
 	omap_irq_register(crtc->dev, &omap_crtc->error_irq);
-- 
1.8.3.2

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/4] drm/omap: fix uninit order in pdev_remove()
  2014-04-02 12:37 [PATCH 1/4] drm/omap: fix output enable/disable sequence Tomi Valkeinen
@ 2014-04-02 12:37 ` Tomi Valkeinen
  2014-04-02 14:16   ` Rob Clark
  2014-04-02 12:37 ` [PATCH 3/4] drm/omap: fix DMM driver (un)registration Tomi Valkeinen
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Tomi Valkeinen @ 2014-04-02 12:37 UTC (permalink / raw)
  To: Rob Clark, Archit Taneja, dri-devel; +Cc: Tomi Valkeinen

When unloading omapdrm driver, the omapdrm platform device is
uninitialized last, after the displays have been disconnected omap_crtc
callbacks have been removed. As the omapdrm pdev uninitialization needs
the features uninitialized in earlier steps, a crash is guaranteed.

This patch fixes the uninitialize order so that the omapdrm pdev is
removed first.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index bf39fcc49e0f..df3e66416a30 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -696,10 +696,11 @@ static int pdev_remove(struct platform_device *device)
 {
 	DBG("");
 
+	drm_put_dev(platform_get_drvdata(device));
+
 	omap_disconnect_dssdevs();
 	omap_crtc_pre_uninit();
 
-	drm_put_dev(platform_get_drvdata(device));
 	return 0;
 }
 
-- 
1.8.3.2

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/4] drm/omap: fix DMM driver (un)registration
  2014-04-02 12:37 [PATCH 1/4] drm/omap: fix output enable/disable sequence Tomi Valkeinen
  2014-04-02 12:37 ` [PATCH 2/4] drm/omap: fix uninit order in pdev_remove() Tomi Valkeinen
@ 2014-04-02 12:37 ` Tomi Valkeinen
  2014-04-02 14:14   ` Rob Clark
  2014-04-02 12:38 ` [PATCH 4/4] drm/omap: fix race issue when unloading omapdrm Tomi Valkeinen
  2014-04-02 14:19 ` [PATCH 1/4] drm/omap: fix output enable/disable sequence Rob Clark
  3 siblings, 1 reply; 9+ messages in thread
From: Tomi Valkeinen @ 2014-04-02 12:37 UTC (permalink / raw)
  To: Rob Clark, Archit Taneja, dri-devel; +Cc: Tomi Valkeinen

At the moment the DMM driver is never unregistered, even if it's
registered in the omapdrm module's init function. This means we'll get
errors when reloading the omapdrm module.

Fix this by unregistering the DMM driver properly, and also change the
module init to fail if DMM driver cannot be registered, simplifying the
unregister path as we don't need to keep the state whether we registered
the DMM driver or not.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index df3e66416a30..f16a07d1668d 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -727,18 +727,33 @@ static struct platform_driver pdev = {
 
 static int __init omap_drm_init(void)
 {
+	int r;
+
 	DBG("init");
-	if (platform_driver_register(&omap_dmm_driver)) {
-		/* we can continue on without DMM.. so not fatal */
-		dev_err(NULL, "DMM registration failed\n");
+
+	r = platform_driver_register(&omap_dmm_driver);
+	if (r) {
+		pr_err("DMM driver registration failed\n");
+		return r;
 	}
-	return platform_driver_register(&pdev);
+
+	r = platform_driver_register(&pdev);
+	if (r) {
+		pr_err("omapdrm driver registration failed\n");
+		platform_driver_unregister(&omap_dmm_driver);
+		return r;
+	}
+
+	return 0;
 }
 
 static void __exit omap_drm_fini(void)
 {
 	DBG("fini");
+
 	platform_driver_unregister(&pdev);
+
+	platform_driver_unregister(&omap_dmm_driver);
 }
 
 /* need late_initcall() so we load after dss_driver's are loaded */
-- 
1.8.3.2

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/4] drm/omap: fix race issue when unloading omapdrm
  2014-04-02 12:37 [PATCH 1/4] drm/omap: fix output enable/disable sequence Tomi Valkeinen
  2014-04-02 12:37 ` [PATCH 2/4] drm/omap: fix uninit order in pdev_remove() Tomi Valkeinen
  2014-04-02 12:37 ` [PATCH 3/4] drm/omap: fix DMM driver (un)registration Tomi Valkeinen
@ 2014-04-02 12:38 ` Tomi Valkeinen
  2014-04-02 14:08   ` Rob Clark
  2014-04-02 14:19 ` [PATCH 1/4] drm/omap: fix output enable/disable sequence Rob Clark
  3 siblings, 1 reply; 9+ messages in thread
From: Tomi Valkeinen @ 2014-04-02 12:38 UTC (permalink / raw)
  To: Rob Clark, Archit Taneja, dri-devel; +Cc: Tomi Valkeinen

At module unload, omap_fbdev_free() gets called which releases the
framebuffers. However, the framebuffers are still used by crtcs, and
will be released only later at vsync. The driver doesn't wait for this,
and goes on to release the rest of the resources, which often
causes a crash.

This patchs adds a omap_crtc_flush() function which waits until the crtc
has finished with its apply queue and page flips.

The function utilizes a simple polling while-loop, as the performance is
not an issue here.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 19 +++++++++++++++++++
 drivers/gpu/drm/omapdrm/omap_drv.c  |  6 ++++++
 drivers/gpu/drm/omapdrm/omap_drv.h  |  1 +
 3 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index e7b643c178a6..4f624c59a660 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -620,6 +620,25 @@ static void omap_crtc_post_apply(struct omap_drm_apply *apply)
 	/* nothing needed for post-apply */
 }
 
+void omap_crtc_flush(struct drm_crtc *crtc)
+{
+	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
+	int loops = 0;
+
+	while (!list_empty(&omap_crtc->pending_applies) ||
+		!list_empty(&omap_crtc->queued_applies) ||
+		omap_crtc->event || omap_crtc->old_fb) {
+
+		if (++loops > 10) {
+			dev_err(crtc->dev->dev,
+				"omap_crtc_flush() timeout\n");
+			break;
+		}
+
+		schedule_timeout_uninterruptible(msecs_to_jiffies(20));
+	}
+}
+
 static const char *channel_names[] = {
 		[OMAP_DSS_CHANNEL_LCD] = "lcd",
 		[OMAP_DSS_CHANNEL_DIGIT] = "tv",
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index f16a07d1668d..c8270e4b26f3 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -513,12 +513,18 @@ static int dev_load(struct drm_device *dev, unsigned long flags)
 static int dev_unload(struct drm_device *dev)
 {
 	struct omap_drm_private *priv = dev->dev_private;
+	int i;
 
 	DBG("unload: dev=%p", dev);
 
 	drm_kms_helper_poll_fini(dev);
 
 	omap_fbdev_free(dev);
+
+	/* flush crtcs so the fbs get released */
+	for (i = 0; i < priv->num_crtcs; i++)
+		omap_crtc_flush(priv->crtcs[i]);
+
 	omap_modeset_free(dev);
 	omap_gem_deinit(dev);
 
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 428b2981fd68..284b80fc3c54 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -163,6 +163,7 @@ void omap_crtc_pre_init(void);
 void omap_crtc_pre_uninit(void);
 struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 		struct drm_plane *plane, enum omap_channel channel, int id);
+void omap_crtc_flush(struct drm_crtc *crtc);
 
 struct drm_plane *omap_plane_init(struct drm_device *dev,
 		int plane_id, bool private_plane);
-- 
1.8.3.2

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 4/4] drm/omap: fix race issue when unloading omapdrm
  2014-04-02 12:38 ` [PATCH 4/4] drm/omap: fix race issue when unloading omapdrm Tomi Valkeinen
@ 2014-04-02 14:08   ` Rob Clark
  0 siblings, 0 replies; 9+ messages in thread
From: Rob Clark @ 2014-04-02 14:08 UTC (permalink / raw)
  To: Tomi Valkeinen; +Cc: dri-devel@lists.freedesktop.org

On Wed, Apr 2, 2014 at 8:38 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> At module unload, omap_fbdev_free() gets called which releases the
> framebuffers. However, the framebuffers are still used by crtcs, and
> will be released only later at vsync. The driver doesn't wait for this,
> and goes on to release the rest of the resources, which often
> causes a crash.
>
> This patchs adds a omap_crtc_flush() function which waits until the crtc
> has finished with its apply queue and page flips.
>
> The function utilizes a simple polling while-loop, as the performance is
> not an issue here.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 19 +++++++++++++++++++
>  drivers/gpu/drm/omapdrm/omap_drv.c  |  6 ++++++
>  drivers/gpu/drm/omapdrm/omap_drv.h  |  1 +
>  3 files changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index e7b643c178a6..4f624c59a660 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -620,6 +620,25 @@ static void omap_crtc_post_apply(struct omap_drm_apply *apply)
>         /* nothing needed for post-apply */
>  }
>
> +void omap_crtc_flush(struct drm_crtc *crtc)
> +{
> +       struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> +       int loops = 0;
> +
> +       while (!list_empty(&omap_crtc->pending_applies) ||
> +               !list_empty(&omap_crtc->queued_applies) ||
> +               omap_crtc->event || omap_crtc->old_fb) {
> +
> +               if (++loops > 10) {
> +                       dev_err(crtc->dev->dev,
> +                               "omap_crtc_flush() timeout\n");
> +                       break;
> +               }
> +
> +               schedule_timeout_uninterruptible(msecs_to_jiffies(20));
> +       }
> +}
> +
>  static const char *channel_names[] = {
>                 [OMAP_DSS_CHANNEL_LCD] = "lcd",
>                 [OMAP_DSS_CHANNEL_DIGIT] = "tv",
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index f16a07d1668d..c8270e4b26f3 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -513,12 +513,18 @@ static int dev_load(struct drm_device *dev, unsigned long flags)
>  static int dev_unload(struct drm_device *dev)
>  {
>         struct omap_drm_private *priv = dev->dev_private;
> +       int i;
>
>         DBG("unload: dev=%p", dev);
>
>         drm_kms_helper_poll_fini(dev);
>
>         omap_fbdev_free(dev);
> +
> +       /* flush crtcs so the fbs get released */
> +       for (i = 0; i < priv->num_crtcs; i++)
> +               omap_crtc_flush(priv->crtcs[i]);
> +
>         omap_modeset_free(dev);
>         omap_gem_deinit(dev);
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
> index 428b2981fd68..284b80fc3c54 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -163,6 +163,7 @@ void omap_crtc_pre_init(void);
>  void omap_crtc_pre_uninit(void);
>  struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>                 struct drm_plane *plane, enum omap_channel channel, int id);
> +void omap_crtc_flush(struct drm_crtc *crtc);
>
>  struct drm_plane *omap_plane_init(struct drm_device *dev,
>                 int plane_id, bool private_plane);
> --
> 1.8.3.2
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/4] drm/omap: fix DMM driver (un)registration
  2014-04-02 12:37 ` [PATCH 3/4] drm/omap: fix DMM driver (un)registration Tomi Valkeinen
@ 2014-04-02 14:14   ` Rob Clark
  2014-04-02 14:18     ` Tomi Valkeinen
  0 siblings, 1 reply; 9+ messages in thread
From: Rob Clark @ 2014-04-02 14:14 UTC (permalink / raw)
  To: Tomi Valkeinen; +Cc: dri-devel@lists.freedesktop.org

On Wed, Apr 2, 2014 at 8:37 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> At the moment the DMM driver is never unregistered, even if it's
> registered in the omapdrm module's init function. This means we'll get
> errors when reloading the omapdrm module.
>
> Fix this by unregistering the DMM driver properly, and also change the
> module init to fail if DMM driver cannot be registered, simplifying the
> unregister path as we don't need to keep the state whether we registered
> the DMM driver or not.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index df3e66416a30..f16a07d1668d 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -727,18 +727,33 @@ static struct platform_driver pdev = {
>
>  static int __init omap_drm_init(void)
>  {
> +       int r;
> +
>         DBG("init");
> -       if (platform_driver_register(&omap_dmm_driver)) {
> -               /* we can continue on without DMM.. so not fatal */
> -               dev_err(NULL, "DMM registration failed\n");
> +
> +       r = platform_driver_register(&omap_dmm_driver);

the one thing I wonder slightly about, this is making omap_dmm_driver
register fail fatal, whereas it wasn't before..

That said, I don't remember in which case the dmm driver registration
would fail.  I think registering the driver should succeed even (for
example) on omap3 without dmm/tiler device.  But I guess you've
probably tested on o3 just to make sure?  Assuming you have:

Reviewed-by: Rob Clark <robdclark@gmail.com>

> +       if (r) {
> +               pr_err("DMM driver registration failed\n");
> +               return r;
>         }
> -       return platform_driver_register(&pdev);
> +
> +       r = platform_driver_register(&pdev);
> +       if (r) {
> +               pr_err("omapdrm driver registration failed\n");
> +               platform_driver_unregister(&omap_dmm_driver);
> +               return r;
> +       }
> +
> +       return 0;
>  }
>
>  static void __exit omap_drm_fini(void)
>  {
>         DBG("fini");
> +
>         platform_driver_unregister(&pdev);
> +
> +       platform_driver_unregister(&omap_dmm_driver);
>  }
>
>  /* need late_initcall() so we load after dss_driver's are loaded */
> --
> 1.8.3.2
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/4] drm/omap: fix uninit order in pdev_remove()
  2014-04-02 12:37 ` [PATCH 2/4] drm/omap: fix uninit order in pdev_remove() Tomi Valkeinen
@ 2014-04-02 14:16   ` Rob Clark
  0 siblings, 0 replies; 9+ messages in thread
From: Rob Clark @ 2014-04-02 14:16 UTC (permalink / raw)
  To: Tomi Valkeinen; +Cc: dri-devel@lists.freedesktop.org

On Wed, Apr 2, 2014 at 8:37 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> When unloading omapdrm driver, the omapdrm platform device is
> uninitialized last, after the displays have been disconnected omap_crtc
> callbacks have been removed. As the omapdrm pdev uninitialization needs
> the features uninitialized in earlier steps, a crash is guaranteed.
>
> This patch fixes the uninitialize order so that the omapdrm pdev is
> removed first.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Rob Clark <robdclark@gmail.com

> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index bf39fcc49e0f..df3e66416a30 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -696,10 +696,11 @@ static int pdev_remove(struct platform_device *device)
>  {
>         DBG("");
>
> +       drm_put_dev(platform_get_drvdata(device));
> +
>         omap_disconnect_dssdevs();
>         omap_crtc_pre_uninit();
>
> -       drm_put_dev(platform_get_drvdata(device));
>         return 0;
>  }
>
> --
> 1.8.3.2
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/4] drm/omap: fix DMM driver (un)registration
  2014-04-02 14:14   ` Rob Clark
@ 2014-04-02 14:18     ` Tomi Valkeinen
  0 siblings, 0 replies; 9+ messages in thread
From: Tomi Valkeinen @ 2014-04-02 14:18 UTC (permalink / raw)
  To: Rob Clark; +Cc: dri-devel@lists.freedesktop.org


[-- Attachment #1.1: Type: text/plain, Size: 2004 bytes --]

On 02/04/14 17:14, Rob Clark wrote:
> On Wed, Apr 2, 2014 at 8:37 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> At the moment the DMM driver is never unregistered, even if it's
>> registered in the omapdrm module's init function. This means we'll get
>> errors when reloading the omapdrm module.
>>
>> Fix this by unregistering the DMM driver properly, and also change the
>> module init to fail if DMM driver cannot be registered, simplifying the
>> unregister path as we don't need to keep the state whether we registered
>> the DMM driver or not.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_drv.c | 23 +++++++++++++++++++----
>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
>> index df3e66416a30..f16a07d1668d 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -727,18 +727,33 @@ static struct platform_driver pdev = {
>>
>>  static int __init omap_drm_init(void)
>>  {
>> +       int r;
>> +
>>         DBG("init");
>> -       if (platform_driver_register(&omap_dmm_driver)) {
>> -               /* we can continue on without DMM.. so not fatal */
>> -               dev_err(NULL, "DMM registration failed\n");
>> +
>> +       r = platform_driver_register(&omap_dmm_driver);
> 
> the one thing I wonder slightly about, this is making omap_dmm_driver
> register fail fatal, whereas it wasn't before..
> 
> That said, I don't remember in which case the dmm driver registration
> would fail.  I think registering the driver should succeed even (for
> example) on omap3 without dmm/tiler device.  But I guess you've
> probably tested on o3 just to make sure?  Assuming you have:

Yes. I think the driver registration could fail more or less only if
we're out of memory, or the driver is already register, or some other
similar situation.

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] drm/omap: fix output enable/disable sequence
  2014-04-02 12:37 [PATCH 1/4] drm/omap: fix output enable/disable sequence Tomi Valkeinen
                   ` (2 preceding siblings ...)
  2014-04-02 12:38 ` [PATCH 4/4] drm/omap: fix race issue when unloading omapdrm Tomi Valkeinen
@ 2014-04-02 14:19 ` Rob Clark
  3 siblings, 0 replies; 9+ messages in thread
From: Rob Clark @ 2014-04-02 14:19 UTC (permalink / raw)
  To: Tomi Valkeinen; +Cc: dri-devel@lists.freedesktop.org

On Wed, Apr 2, 2014 at 8:37 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> At the moment it's quite easy to get the following errors when the HDMI
> output is enabled or disabled:
>
> [drm:omap_crtc_error_irq] *ERROR* tv: errors: 00008000
>
> The reason for the errors is that the omapdrm driver doesn't properly
> handle the sync-lost irqs that happen when enabling the DIGIT crtc,
> which is used for HDMI and analog TV. The driver does disable the
> sync-lost irq properly, but it fails to wait until the output has been
> fully enabled (i.e. the first vsync), so the sync-lost errors are still
> seen occasionally.
>
> This patch makes the omapdrm act the same way as the omapfb does:
>
> - When enabling a display, we'll wait for the first vsync.
> - When disabling a display, we'll wait for framedone if available, or
>   odd and even vsyncs.
>
> These changes make sure the output is fully enabled or disabled at the
> end of the function.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reported-by: Sanjay Singh Rawat <sanjay.rawat@linaro.org>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 46 ++++++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 4313bb0a49a6..e7b643c178a6 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -527,38 +527,46 @@ static void set_enabled(struct drm_crtc *crtc, bool enable)
>         struct drm_device *dev = crtc->dev;
>         struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>         enum omap_channel channel = omap_crtc->channel;
> -       struct omap_irq_wait *wait = NULL;
> +       struct omap_irq_wait *wait;
> +       u32 framedone_irq, vsync_irq;
> +       int ret;
>
>         if (dispc_mgr_is_enabled(channel) == enable)
>                 return;
>
> -       /* ignore sync-lost irqs during enable/disable */
> +       /*
> +        * Digit output produces some sync lost interrupts during the first
> +        * frame when enabling, so we need to ignore those.
> +        */
>         omap_irq_unregister(crtc->dev, &omap_crtc->error_irq);
>
> -       if (dispc_mgr_get_framedone_irq(channel)) {
> -               if (!enable) {
> -                       wait = omap_irq_wait_init(dev,
> -                                       dispc_mgr_get_framedone_irq(channel), 1);
> -               }
> +       framedone_irq = dispc_mgr_get_framedone_irq(channel);
> +       vsync_irq = dispc_mgr_get_vsync_irq(channel);
> +
> +       if (enable) {
> +               wait = omap_irq_wait_init(dev, vsync_irq, 1);
>         } else {
>                 /*
> -                * When we disable digit output, we need to wait until fields
> -                * are done.  Otherwise the DSS is still working, and turning
> -                * off the clocks prevents DSS from going to OFF mode. And when
> -                * enabling, we need to wait for the extra sync losts
> +                * When we disable the digit output, we need to wait for
> +                * FRAMEDONE to know that DISPC has finished with the output.
> +                *
> +                * OMAP2/3 does not have FRAMEDONE irq for digit output, and in
> +                * that case we need to use vsync interrupt, and wait for both
> +                * even and odd frames.
>                  */
> -               wait = omap_irq_wait_init(dev,
> -                               dispc_mgr_get_vsync_irq(channel), 2);
> +
> +               if (framedone_irq)
> +                       wait = omap_irq_wait_init(dev, framedone_irq, 1);
> +               else
> +                       wait = omap_irq_wait_init(dev, vsync_irq, 2);
>         }
>
>         dispc_mgr_enable(channel, enable);
>
> -       if (wait) {
> -               int ret = omap_irq_wait(dev, wait, msecs_to_jiffies(100));
> -               if (ret) {
> -                       dev_err(dev->dev, "%s: timeout waiting for %s\n",
> -                                       omap_crtc->name, enable ? "enable" : "disable");
> -               }
> +       ret = omap_irq_wait(dev, wait, msecs_to_jiffies(100));
> +       if (ret) {
> +               dev_err(dev->dev, "%s: timeout waiting for %s\n",
> +                               omap_crtc->name, enable ? "enable" : "disable");
>         }
>
>         omap_irq_register(crtc->dev, &omap_crtc->error_irq);
> --
> 1.8.3.2
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2014-04-02 14:19 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-02 12:37 [PATCH 1/4] drm/omap: fix output enable/disable sequence Tomi Valkeinen
2014-04-02 12:37 ` [PATCH 2/4] drm/omap: fix uninit order in pdev_remove() Tomi Valkeinen
2014-04-02 14:16   ` Rob Clark
2014-04-02 12:37 ` [PATCH 3/4] drm/omap: fix DMM driver (un)registration Tomi Valkeinen
2014-04-02 14:14   ` Rob Clark
2014-04-02 14:18     ` Tomi Valkeinen
2014-04-02 12:38 ` [PATCH 4/4] drm/omap: fix race issue when unloading omapdrm Tomi Valkeinen
2014-04-02 14:08   ` Rob Clark
2014-04-02 14:19 ` [PATCH 1/4] drm/omap: fix output enable/disable sequence Rob Clark

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.