[PATCH 1/9] drm/omap: fix encoder-crtc mapping

[PATCH 1/9] drm/omap: fix encoder-crtc mapping

[Tomi Valkeinen]

OMAP DSS hardware supports changing the output port to which an overlay
manager's video stream goes. For example, DPI video stream can come from
any of the four overlay managers on OMAP5.

However, as it's difficult to manage the change in the driver, the
omapdss driver does not support that at the moment, and has a hardcoded
overlay manager per output.

omapdrm, on the other hand, uses the hardware features to find out which
overlay manager to use for an output, which causes problems. For
example, on OMAP5, omapdrm tries to use DIGIT overlay manager for DPI
output, instead of the LCD3 required by the omapdss driver.

This patch changes the omapdrm to use the omapdss driver's hardcoded
overlay managers, which fixes the issue.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 002b9721e85a..26fda74c1e48 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -286,14 +286,13 @@ static int omap_modeset_init(struct drm_device *dev)
 		for (id = 0; id < priv->num_crtcs; id++) {
 			struct drm_crtc *crtc = priv->crtcs[id];
 			enum omap_channel crtc_channel;
-			enum omap_dss_output_id supported_outputs;
 
 			crtc_channel = omap_crtc_channel(crtc);
-			supported_outputs =
-				dss_feat_get_supported_outputs(crtc_channel);
 
-			if (supported_outputs & output->id)
+			if (output->dispc_channel == crtc_channel) {
 				encoder->possible_crtcs |= (1 << id);
+				break;
+			}
 		}
 
 		omap_dss_put_device(output);
-- 
1.9.1

[PATCH 2/9] drm/omap: page_flip: return -EBUSY if flip pending

[Tomi Valkeinen]

The DRM documentation says:

"If a page flip is already pending, the page_flip operation must return
-EBUSY."

Currently omapdrm returns -EINVAL instead. Fix omapdrm by returning
-EBUSY.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 2d28dc337cfb..0812b0f80167 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -360,7 +360,7 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc,
 	if (omap_crtc->old_fb) {
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 		dev_err(dev->dev, "already a pending flip\n");
-		return -EINVAL;
+		return -EBUSY;
 	}
 
 	omap_crtc->event = event;
-- 
1.9.1

[PATCH 3/9] drm/omap: fix race issue with vsync irq and apply

[Tomi Valkeinen]

omap_crtc's apply_worker does:

	omap_irq_register(dev, &omap_crtc->apply_irq);
	dispc_mgr_go(channel);

This is supposed to enable the vsync irq, and set the GO bit. The vsync
handler will later check if the HW has cleared the GO bit and queue
apply work.

However, what may happen is that the vsync irq is enabled, and it gets
ran before the GO bit is set by the apply_worker. In this case the vsync
handler will see the GO bit as cleared, and queues the apply work too
early.

This causes WARN'ings from dispc driver, and possibly other problems.

This patch fixes the issue by adding a new variable, 'go_bit_set' which
is used to track the supposed state of GO bit and helps the apply worker
and irq handler handle the job without a race.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 0812b0f80167..193979f97bdb 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -47,6 +47,9 @@ struct omap_crtc {
 	bool enabled;
 	bool full_update;
 
+	/* tracks the state of GO bit between irq handler and apply worker */
+	bool go_bit_set;
+
 	struct omap_drm_apply apply;
 
 	struct omap_drm_irq apply_irq;
@@ -442,11 +445,16 @@ static void omap_crtc_apply_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
 	if (!omap_crtc->error_irq.registered)
 		__omap_irq_register(crtc->dev, &omap_crtc->error_irq);
 
-	if (!dispc_mgr_go_busy(omap_crtc->channel)) {
+	/* make sure we see the most recent 'go_bit_set' */
+	rmb();
+	if (omap_crtc->go_bit_set && !dispc_mgr_go_busy(omap_crtc->channel)) {
 		struct omap_drm_private *priv =
 				crtc->dev->dev_private;
 		DBG("%s: apply done", omap_crtc->name);
 		__omap_irq_unregister(crtc->dev, &omap_crtc->apply_irq);
+		omap_crtc->go_bit_set = false;
+		/* make sure apple_worker sees 'go_bit_set = false' */
+		wmb();
 		queue_work(priv->wq, &omap_crtc->apply_work);
 	}
 }
@@ -472,7 +480,9 @@ static void apply_worker(struct work_struct *work)
 	 * If we are still pending a previous update, wait.. when the
 	 * pending update completes, we get kicked again.
 	 */
-	if (omap_crtc->apply_irq.registered)
+	/* make sure we see the most recent 'go_bit_set' */
+	rmb();
+	if (omap_crtc->go_bit_set)
 		goto out;
 
 	/* finish up previous apply's: */
@@ -502,6 +512,9 @@ static void apply_worker(struct work_struct *work)
 		if (dispc_mgr_is_enabled(channel)) {
 			omap_irq_register(dev, &omap_crtc->apply_irq);
 			dispc_mgr_go(channel);
+			omap_crtc->go_bit_set = true;
+			/* make sure the irq handler sees 'go_bit_set' */
+			wmb();
 		} else {
 			struct omap_drm_private *priv = dev->dev_private;
 			queue_work(priv->wq, &omap_crtc->apply_work);
-- 
1.9.1

[PATCH 4/9] drm/omap: make modesetting synchronous

[Tomi Valkeinen]

Currently modesetting is not done synchronously, but it queues work that
is done later. In theory this is fine, but the driver doesn't handle it
at properly. This means that if an application first does a full
modeset, then immediately afterwards starts page flipping, the page
flipping will not work properly as there's modeset work still in the
queue.

The result with my test application was that a backbuffer was shown on
the screen.

Fixing this properly would be rather big undertaking. Thus this patch
fixes the issue by making the modesetting synchronous, by waiting for
the queued work to be done at the end of omap_crtc->commit().

The ugly part here is that the background work takes crtc->mutex, and
the modesetting also holds that lock, which means that the background
work never gets done. To get around this, we unclock, wait, and lock
again.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 193979f97bdb..3261fbf94957 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -277,8 +277,13 @@ static void omap_crtc_prepare(struct drm_crtc *crtc)
 static void omap_crtc_commit(struct drm_crtc *crtc)
 {
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
+	struct drm_device *dev = crtc->dev;
 	DBG("%s", omap_crtc->name);
 	omap_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
+
+	drm_modeset_unlock_all(dev);
+	omap_crtc_flush(crtc);
+	drm_modeset_lock_all(dev);
 }
 
 static int omap_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
-- 
1.9.1

Re: [PATCH 4/9] drm/omap: make modesetting synchronous

[Daniel Vetter]

On Wed, Sep 03, 2014 at 02:55:05PM +0300, Tomi Valkeinen wrote:
> Currently modesetting is not done synchronously, but it queues work that
> is done later. In theory this is fine, but the driver doesn't handle it
> at properly. This means that if an application first does a full
> modeset, then immediately afterwards starts page flipping, the page
> flipping will not work properly as there's modeset work still in the
> queue.
> 
> The result with my test application was that a backbuffer was shown on
> the screen.
> 
> Fixing this properly would be rather big undertaking. Thus this patch
> fixes the issue by making the modesetting synchronous, by waiting for
> the queued work to be done at the end of omap_crtc->commit().
> 
> The ugly part here is that the background work takes crtc->mutex, and
> the modesetting also holds that lock, which means that the background
> work never gets done. To get around this, we unclock, wait, and lock
> again.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 193979f97bdb..3261fbf94957 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -277,8 +277,13 @@ static void omap_crtc_prepare(struct drm_crtc *crtc)
>  static void omap_crtc_commit(struct drm_crtc *crtc)
>  {
>  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> +	struct drm_device *dev = crtc->dev;
>  	DBG("%s", omap_crtc->name);
>  	omap_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
> +
> +	drm_modeset_unlock_all(dev);

This will run afoul of the upcoming locking rework in the atomic work. And
I'm fairly sure that the crtc helpers will fall over badly if someone
submits a concurrent setCrtc while you've dropped the locks here.

Can't you instead just drop the locking from the worker? As long as you
synchronize like here at the right places it can't race. I expect that you
might want to synchronize in the crtc_prepare hook, too. But beyond that
it should work.

As-is nacked because future headaches for me ;-)

Cheers, Daniel

> +	omap_crtc_flush(crtc);
> +	drm_modeset_lock_all(dev);
>  }
>  
>  static int omap_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@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

Re: [PATCH 4/9] drm/omap: make modesetting synchronous

[Rob Clark]

On Wed, Sep 3, 2014 at 10:25 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Sep 03, 2014 at 02:55:05PM +0300, Tomi Valkeinen wrote:
>> Currently modesetting is not done synchronously, but it queues work that
>> is done later. In theory this is fine, but the driver doesn't handle it
>> at properly. This means that if an application first does a full
>> modeset, then immediately afterwards starts page flipping, the page
>> flipping will not work properly as there's modeset work still in the
>> queue.
>>
>> The result with my test application was that a backbuffer was shown on
>> the screen.
>>
>> Fixing this properly would be rather big undertaking. Thus this patch
>> fixes the issue by making the modesetting synchronous, by waiting for
>> the queued work to be done at the end of omap_crtc->commit().
>>
>> The ugly part here is that the background work takes crtc->mutex, and
>> the modesetting also holds that lock, which means that the background
>> work never gets done. To get around this, we unclock, wait, and lock
>> again.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_crtc.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> index 193979f97bdb..3261fbf94957 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> @@ -277,8 +277,13 @@ static void omap_crtc_prepare(struct drm_crtc *crtc)
>>  static void omap_crtc_commit(struct drm_crtc *crtc)
>>  {
>>       struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>> +     struct drm_device *dev = crtc->dev;
>>       DBG("%s", omap_crtc->name);
>>       omap_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
>> +
>> +     drm_modeset_unlock_all(dev);
>
> This will run afoul of the upcoming locking rework in the atomic work. And
> I'm fairly sure that the crtc helpers will fall over badly if someone
> submits a concurrent setCrtc while you've dropped the locks here.

what about just dropping and re-acquiring crtc->mutex, since that is
all the worker actually needs..

it would be a bigger task to get rid of the mutex on the worker, since
we'd have to double buffer state.  It seemed like rather than
re-invent atomic, it would be better just to wait for atomic to land
and then restructure the driver

BR,
-R

> Can't you instead just drop the locking from the worker? As long as you
> synchronize like here at the right places it can't race. I expect that you
> might want to synchronize in the crtc_prepare hook, too. But beyond that
> it should work.
>
> As-is nacked because future headaches for me ;-)
>
> Cheers, Daniel
>
>> +     omap_crtc_flush(crtc);
>> +     drm_modeset_lock_all(dev);
>>  }
>>
>>  static int omap_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@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

Re: [PATCH 4/9] drm/omap: make modesetting synchronous

[Daniel Vetter]

On Wed, Sep 03, 2014 at 11:05:08AM -0400, Rob Clark wrote:
> On Wed, Sep 3, 2014 at 10:25 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Sep 03, 2014 at 02:55:05PM +0300, Tomi Valkeinen wrote:
> >> Currently modesetting is not done synchronously, but it queues work that
> >> is done later. In theory this is fine, but the driver doesn't handle it
> >> at properly. This means that if an application first does a full
> >> modeset, then immediately afterwards starts page flipping, the page
> >> flipping will not work properly as there's modeset work still in the
> >> queue.
> >>
> >> The result with my test application was that a backbuffer was shown on
> >> the screen.
> >>
> >> Fixing this properly would be rather big undertaking. Thus this patch
> >> fixes the issue by making the modesetting synchronous, by waiting for
> >> the queued work to be done at the end of omap_crtc->commit().
> >>
> >> The ugly part here is that the background work takes crtc->mutex, and
> >> the modesetting also holds that lock, which means that the background
> >> work never gets done. To get around this, we unclock, wait, and lock
> >> again.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/omapdrm/omap_crtc.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> index 193979f97bdb..3261fbf94957 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> @@ -277,8 +277,13 @@ static void omap_crtc_prepare(struct drm_crtc *crtc)
> >>  static void omap_crtc_commit(struct drm_crtc *crtc)
> >>  {
> >>       struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> >> +     struct drm_device *dev = crtc->dev;
> >>       DBG("%s", omap_crtc->name);
> >>       omap_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
> >> +
> >> +     drm_modeset_unlock_all(dev);
> >
> > This will run afoul of the upcoming locking rework in the atomic work. And
> > I'm fairly sure that the crtc helpers will fall over badly if someone
> > submits a concurrent setCrtc while you've dropped the locks here.
> 
> what about just dropping and re-acquiring crtc->mutex, since that is
> all the worker actually needs..
> 
> it would be a bigger task to get rid of the mutex on the worker, since
> we'd have to double buffer state.  It seemed like rather than
> re-invent atomic, it would be better just to wait for atomic to land
> and then restructure the driver

Well atomic kinda has the same idea to not take locks in the worker and
instead relying on ordering.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/9] drm/omap: make modesetting synchronous

[Rob Clark]

On Wed, Sep 3, 2014 at 11:21 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Sep 03, 2014 at 11:05:08AM -0400, Rob Clark wrote:
>> On Wed, Sep 3, 2014 at 10:25 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Wed, Sep 03, 2014 at 02:55:05PM +0300, Tomi Valkeinen wrote:
>> >> Currently modesetting is not done synchronously, but it queues work that
>> >> is done later. In theory this is fine, but the driver doesn't handle it
>> >> at properly. This means that if an application first does a full
>> >> modeset, then immediately afterwards starts page flipping, the page
>> >> flipping will not work properly as there's modeset work still in the
>> >> queue.
>> >>
>> >> The result with my test application was that a backbuffer was shown on
>> >> the screen.
>> >>
>> >> Fixing this properly would be rather big undertaking. Thus this patch
>> >> fixes the issue by making the modesetting synchronous, by waiting for
>> >> the queued work to be done at the end of omap_crtc->commit().
>> >>
>> >> The ugly part here is that the background work takes crtc->mutex, and
>> >> the modesetting also holds that lock, which means that the background
>> >> work never gets done. To get around this, we unclock, wait, and lock
>> >> again.
>> >>
>> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> >> ---
>> >>  drivers/gpu/drm/omapdrm/omap_crtc.c | 5 +++++
>> >>  1 file changed, 5 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> >> index 193979f97bdb..3261fbf94957 100644
>> >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>> >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> >> @@ -277,8 +277,13 @@ static void omap_crtc_prepare(struct drm_crtc *crtc)
>> >>  static void omap_crtc_commit(struct drm_crtc *crtc)
>> >>  {
>> >>       struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>> >> +     struct drm_device *dev = crtc->dev;
>> >>       DBG("%s", omap_crtc->name);
>> >>       omap_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
>> >> +
>> >> +     drm_modeset_unlock_all(dev);
>> >
>> > This will run afoul of the upcoming locking rework in the atomic work. And
>> > I'm fairly sure that the crtc helpers will fall over badly if someone
>> > submits a concurrent setCrtc while you've dropped the locks here.
>>
>> what about just dropping and re-acquiring crtc->mutex, since that is
>> all the worker actually needs..
>>
>> it would be a bigger task to get rid of the mutex on the worker, since
>> we'd have to double buffer state.  It seemed like rather than
>> re-invent atomic, it would be better just to wait for atomic to land
>> and then restructure the driver
>
> Well atomic kinda has the same idea to not take locks in the worker and
> instead relying on ordering.

so, I am not quite sure that would work..  (and maybe this is an extra
thing to think about for atomic).

The problem is that hw has no hw cursor.  So cursor is implemented
using plane.  But thanks to retarded userspace that rapidly
enables/disables cursor in some cases, we really need plane updates to
be async.  The way it currently works is userspace thread updates
plane state as rapidly as it wants.  When worker eventually gets to
run, it takes the most current state, possibly skipping 1000's of
enable/disable that happened in the mean time.

But for that to work, at least the way the driver is structured
currently, the worker needs to synchronize against userspace somehow.

BR,
-R

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/9] drm/omap: make modesetting synchronous

[Daniel Vetter]

On Wed, Sep 3, 2014 at 6:00 PM, Rob Clark <robdclark@gmail.com> wrote:
> so, I am not quite sure that would work..  (and maybe this is an extra
> thing to think about for atomic).
>
> The problem is that hw has no hw cursor.  So cursor is implemented
> using plane.  But thanks to retarded userspace that rapidly
> enables/disables cursor in some cases, we really need plane updates to
> be async.  The way it currently works is userspace thread updates
> plane state as rapidly as it wants.  When worker eventually gets to
> run, it takes the most current state, possibly skipping 1000's of
> enable/disable that happened in the mean time.
>
> But for that to work, at least the way the driver is structured
> currently, the worker needs to synchronize against userspace somehow.

We could cancel the async worker before the update. But that still
leaves the problem that in general this needs full driver support to
work, and we can't really assume that. I guess we need to make a
special hack for the cursor emulation code to make sure this keeps
working: Cursor updates would only update the sw state and then rearm
a fully async cursor update work item, except when the cursor size
changed since the driver might reject that. I need to think more about
this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/9] drm/omap: make modesetting synchronous

[Tomi Valkeinen]

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

On 03/09/14 17:25, Daniel Vetter wrote:
> On Wed, Sep 03, 2014 at 02:55:05PM +0300, Tomi Valkeinen wrote:
>> Currently modesetting is not done synchronously, but it queues work that
>> is done later. In theory this is fine, but the driver doesn't handle it
>> at properly. This means that if an application first does a full
>> modeset, then immediately afterwards starts page flipping, the page
>> flipping will not work properly as there's modeset work still in the
>> queue.
>>
>> The result with my test application was that a backbuffer was shown on
>> the screen.
>>
>> Fixing this properly would be rather big undertaking. Thus this patch
>> fixes the issue by making the modesetting synchronous, by waiting for
>> the queued work to be done at the end of omap_crtc->commit().
>>
>> The ugly part here is that the background work takes crtc->mutex, and
>> the modesetting also holds that lock, which means that the background
>> work never gets done. To get around this, we unclock, wait, and lock
>> again.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_crtc.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> index 193979f97bdb..3261fbf94957 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> @@ -277,8 +277,13 @@ static void omap_crtc_prepare(struct drm_crtc *crtc)
>>  static void omap_crtc_commit(struct drm_crtc *crtc)
>>  {
>>  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>> +	struct drm_device *dev = crtc->dev;
>>  	DBG("%s", omap_crtc->name);
>>  	omap_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
>> +
>> +	drm_modeset_unlock_all(dev);
> 
> This will run afoul of the upcoming locking rework in the atomic work. And
> I'm fairly sure that the crtc helpers will fall over badly if someone
> submits a concurrent setCrtc while you've dropped the locks here.
> 
> Can't you instead just drop the locking from the worker? As long as you
> synchronize like here at the right places it can't race. I expect that you
> might want to synchronize in the crtc_prepare hook, too. But beyond that
> it should work.
> 
> As-is nacked because future headaches for me ;-)

Yes, it's ugly. But doing it with either queuing or caching would be a
major change. I was just trying to do smallish fixes to the driver for now.

How about only unlocking/locking the crtc->mutex as Rob suggested? I
think it should work, but I didn't try it yet. Would that be as bad for
the locking rework?

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

Re: [PATCH 4/9] drm/omap: make modesetting synchronous

[Daniel Vetter]

On Mon, Sep 08, 2014 at 03:53:18PM +0300, Tomi Valkeinen wrote:
> On 03/09/14 17:25, Daniel Vetter wrote:
> > On Wed, Sep 03, 2014 at 02:55:05PM +0300, Tomi Valkeinen wrote:
> >> Currently modesetting is not done synchronously, but it queues work that
> >> is done later. In theory this is fine, but the driver doesn't handle it
> >> at properly. This means that if an application first does a full
> >> modeset, then immediately afterwards starts page flipping, the page
> >> flipping will not work properly as there's modeset work still in the
> >> queue.
> >>
> >> The result with my test application was that a backbuffer was shown on
> >> the screen.
> >>
> >> Fixing this properly would be rather big undertaking. Thus this patch
> >> fixes the issue by making the modesetting synchronous, by waiting for
> >> the queued work to be done at the end of omap_crtc->commit().
> >>
> >> The ugly part here is that the background work takes crtc->mutex, and
> >> the modesetting also holds that lock, which means that the background
> >> work never gets done. To get around this, we unclock, wait, and lock
> >> again.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/omapdrm/omap_crtc.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> index 193979f97bdb..3261fbf94957 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> @@ -277,8 +277,13 @@ static void omap_crtc_prepare(struct drm_crtc *crtc)
> >>  static void omap_crtc_commit(struct drm_crtc *crtc)
> >>  {
> >>  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> >> +	struct drm_device *dev = crtc->dev;
> >>  	DBG("%s", omap_crtc->name);
> >>  	omap_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
> >> +
> >> +	drm_modeset_unlock_all(dev);
> > 
> > This will run afoul of the upcoming locking rework in the atomic work. And
> > I'm fairly sure that the crtc helpers will fall over badly if someone
> > submits a concurrent setCrtc while you've dropped the locks here.
> > 
> > Can't you instead just drop the locking from the worker? As long as you
> > synchronize like here at the right places it can't race. I expect that you
> > might want to synchronize in the crtc_prepare hook, too. But beyond that
> > it should work.
> > 
> > As-is nacked because future headaches for me ;-)
> 
> Yes, it's ugly. But doing it with either queuing or caching would be a
> major change. I was just trying to do smallish fixes to the driver for now.
> 
> How about only unlocking/locking the crtc->mutex as Rob suggested? I
> think it should work, but I didn't try it yet. Would that be as bad for
> the locking rework?

Same problem really, you shouldn't drop ww mutexes and reacquire them in
the atomic world. ww mutexes have some debug infrastructure for that
(ww_acquire_done) to catch abusers of this.

So if you want to go forward with this it needs at least a big FIXME
comment explaining that this is wrong. If you use locking to enforce
ordering constraints that usually doesn't work well, and dropping locks to
wait for async workers is plainly a locking design bug.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/9] drm/omap: make modesetting synchronous

[Rob Clark]

On Mon, Sep 8, 2014 at 9:24 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Sep 08, 2014 at 03:53:18PM +0300, Tomi Valkeinen wrote:
>> On 03/09/14 17:25, Daniel Vetter wrote:
>> > On Wed, Sep 03, 2014 at 02:55:05PM +0300, Tomi Valkeinen wrote:
>> >> Currently modesetting is not done synchronously, but it queues work that
>> >> is done later. In theory this is fine, but the driver doesn't handle it
>> >> at properly. This means that if an application first does a full
>> >> modeset, then immediately afterwards starts page flipping, the page
>> >> flipping will not work properly as there's modeset work still in the
>> >> queue.
>> >>
>> >> The result with my test application was that a backbuffer was shown on
>> >> the screen.
>> >>
>> >> Fixing this properly would be rather big undertaking. Thus this patch
>> >> fixes the issue by making the modesetting synchronous, by waiting for
>> >> the queued work to be done at the end of omap_crtc->commit().
>> >>
>> >> The ugly part here is that the background work takes crtc->mutex, and
>> >> the modesetting also holds that lock, which means that the background
>> >> work never gets done. To get around this, we unclock, wait, and lock
>> >> again.
>> >>
>> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> >> ---
>> >>  drivers/gpu/drm/omapdrm/omap_crtc.c | 5 +++++
>> >>  1 file changed, 5 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> >> index 193979f97bdb..3261fbf94957 100644
>> >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>> >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> >> @@ -277,8 +277,13 @@ static void omap_crtc_prepare(struct drm_crtc *crtc)
>> >>  static void omap_crtc_commit(struct drm_crtc *crtc)
>> >>  {
>> >>    struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>> >> +  struct drm_device *dev = crtc->dev;
>> >>    DBG("%s", omap_crtc->name);
>> >>    omap_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
>> >> +
>> >> +  drm_modeset_unlock_all(dev);
>> >
>> > This will run afoul of the upcoming locking rework in the atomic work. And
>> > I'm fairly sure that the crtc helpers will fall over badly if someone
>> > submits a concurrent setCrtc while you've dropped the locks here.
>> >
>> > Can't you instead just drop the locking from the worker? As long as you
>> > synchronize like here at the right places it can't race. I expect that you
>> > might want to synchronize in the crtc_prepare hook, too. But beyond that
>> > it should work.
>> >
>> > As-is nacked because future headaches for me ;-)
>>
>> Yes, it's ugly. But doing it with either queuing or caching would be a
>> major change. I was just trying to do smallish fixes to the driver for now.
>>
>> How about only unlocking/locking the crtc->mutex as Rob suggested? I
>> think it should work, but I didn't try it yet. Would that be as bad for
>> the locking rework?
>
> Same problem really, you shouldn't drop ww mutexes and reacquire them in
> the atomic world. ww mutexes have some debug infrastructure for that
> (ww_acquire_done) to catch abusers of this.
>
> So if you want to go forward with this it needs at least a big FIXME
> comment explaining that this is wrong. If you use locking to enforce
> ordering constraints that usually doesn't work well, and dropping locks to
> wait for async workers is plainly a locking design bug.

well, the locking in core makes it in some ways a bit of a midlayer.. ;-)

Some crtc->funcs->wait_until_flushed() sort of thing that
drm_mode_setcrtc() could call after dropping locks would go a long
ways to fix this case.  (Although post-atomic, may no longer be
required.)

BR,
-R


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/9] drm/omap: make modesetting synchronous

[Daniel Vetter]

On Mon, Sep 08, 2014 at 09:39:40AM -0400, Rob Clark wrote:
> On Mon, Sep 8, 2014 at 9:24 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Sep 08, 2014 at 03:53:18PM +0300, Tomi Valkeinen wrote:
> >> On 03/09/14 17:25, Daniel Vetter wrote:
> >> > On Wed, Sep 03, 2014 at 02:55:05PM +0300, Tomi Valkeinen wrote:
> >> >> Currently modesetting is not done synchronously, but it queues work that
> >> >> is done later. In theory this is fine, but the driver doesn't handle it
> >> >> at properly. This means that if an application first does a full
> >> >> modeset, then immediately afterwards starts page flipping, the page
> >> >> flipping will not work properly as there's modeset work still in the
> >> >> queue.
> >> >>
> >> >> The result with my test application was that a backbuffer was shown on
> >> >> the screen.
> >> >>
> >> >> Fixing this properly would be rather big undertaking. Thus this patch
> >> >> fixes the issue by making the modesetting synchronous, by waiting for
> >> >> the queued work to be done at the end of omap_crtc->commit().
> >> >>
> >> >> The ugly part here is that the background work takes crtc->mutex, and
> >> >> the modesetting also holds that lock, which means that the background
> >> >> work never gets done. To get around this, we unclock, wait, and lock
> >> >> again.
> >> >>
> >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> >> ---
> >> >>  drivers/gpu/drm/omapdrm/omap_crtc.c | 5 +++++
> >> >>  1 file changed, 5 insertions(+)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> >> index 193979f97bdb..3261fbf94957 100644
> >> >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> >> @@ -277,8 +277,13 @@ static void omap_crtc_prepare(struct drm_crtc *crtc)
> >> >>  static void omap_crtc_commit(struct drm_crtc *crtc)
> >> >>  {
> >> >>    struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> >> >> +  struct drm_device *dev = crtc->dev;
> >> >>    DBG("%s", omap_crtc->name);
> >> >>    omap_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
> >> >> +
> >> >> +  drm_modeset_unlock_all(dev);
> >> >
> >> > This will run afoul of the upcoming locking rework in the atomic work. And
> >> > I'm fairly sure that the crtc helpers will fall over badly if someone
> >> > submits a concurrent setCrtc while you've dropped the locks here.
> >> >
> >> > Can't you instead just drop the locking from the worker? As long as you
> >> > synchronize like here at the right places it can't race. I expect that you
> >> > might want to synchronize in the crtc_prepare hook, too. But beyond that
> >> > it should work.
> >> >
> >> > As-is nacked because future headaches for me ;-)
> >>
> >> Yes, it's ugly. But doing it with either queuing or caching would be a
> >> major change. I was just trying to do smallish fixes to the driver for now.
> >>
> >> How about only unlocking/locking the crtc->mutex as Rob suggested? I
> >> think it should work, but I didn't try it yet. Would that be as bad for
> >> the locking rework?
> >
> > Same problem really, you shouldn't drop ww mutexes and reacquire them in
> > the atomic world. ww mutexes have some debug infrastructure for that
> > (ww_acquire_done) to catch abusers of this.
> >
> > So if you want to go forward with this it needs at least a big FIXME
> > comment explaining that this is wrong. If you use locking to enforce
> > ordering constraints that usually doesn't work well, and dropping locks to
> > wait for async workers is plainly a locking design bug.
> 
> well, the locking in core makes it in some ways a bit of a midlayer.. ;-)

It's the locking for the drm core layer, it better be done in drm core.
Drivers are free to use it, but if it doesn't fit they can always do their
own. And if you look at i915 we actually have a metric pile of mutexes
taken from modeset code and other places exactly because the drm core
modeset locking can't work for everyone.

> Some crtc->funcs->wait_until_flushed() sort of thing that
> drm_mode_setcrtc() could call after dropping locks would go a long
> ways to fix this case.  (Although post-atomic, may no longer be
> required.)

Now _this_ smells like a proper midlayer - it enforces behaviour
sequencing by providing a bunch of callbacks for drivers to hook into.

Also, locking isn't to enforce ordering constraints at all, that's the job
of flush_workqueue, completion and friends. And if such a sync point would
result in deadlocks if your worker also uses the modeset locks then your
driver needs to grow internal mutexes to coordinate the data accessed by
workers. In i915 we've added a pile of such cases in the past (and will
add more), e.g. the pps mutex, backlight spinlock, edp mutex, ...

ADF does the same mistake of "hey let's do the synchronization in the core
because driver writers get it wrong all the time". Which doesn't make it
better ;-)

Now if your idea was to provide this as a helper, which either enforces
the ordering correctly to not need locks, or has it's own locking of
in-flux requests or similar, then I'm in favour. But adding such a
callback at the core is nacked with extreme prejudice from me ...

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/9] drm/omap: make modesetting synchronous

[Rob Clark]

On Mon, Sep 8, 2014 at 9:50 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Sep 08, 2014 at 09:39:40AM -0400, Rob Clark wrote:
>> On Mon, Sep 8, 2014 at 9:24 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Mon, Sep 08, 2014 at 03:53:18PM +0300, Tomi Valkeinen wrote:
>> >> On 03/09/14 17:25, Daniel Vetter wrote:
>> >> > On Wed, Sep 03, 2014 at 02:55:05PM +0300, Tomi Valkeinen wrote:
>> >> >> Currently modesetting is not done synchronously, but it queues work that
>> >> >> is done later. In theory this is fine, but the driver doesn't handle it
>> >> >> at properly. This means that if an application first does a full
>> >> >> modeset, then immediately afterwards starts page flipping, the page
>> >> >> flipping will not work properly as there's modeset work still in the
>> >> >> queue.
>> >> >>
>> >> >> The result with my test application was that a backbuffer was shown on
>> >> >> the screen.
>> >> >>
>> >> >> Fixing this properly would be rather big undertaking. Thus this patch
>> >> >> fixes the issue by making the modesetting synchronous, by waiting for
>> >> >> the queued work to be done at the end of omap_crtc->commit().
>> >> >>
>> >> >> The ugly part here is that the background work takes crtc->mutex, and
>> >> >> the modesetting also holds that lock, which means that the background
>> >> >> work never gets done. To get around this, we unclock, wait, and lock
>> >> >> again.
>> >> >>
>> >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> >> >> ---
>> >> >>  drivers/gpu/drm/omapdrm/omap_crtc.c | 5 +++++
>> >> >>  1 file changed, 5 insertions(+)
>> >> >>
>> >> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> >> >> index 193979f97bdb..3261fbf94957 100644
>> >> >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>> >> >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> >> >> @@ -277,8 +277,13 @@ static void omap_crtc_prepare(struct drm_crtc *crtc)
>> >> >>  static void omap_crtc_commit(struct drm_crtc *crtc)
>> >> >>  {
>> >> >>    struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>> >> >> +  struct drm_device *dev = crtc->dev;
>> >> >>    DBG("%s", omap_crtc->name);
>> >> >>    omap_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
>> >> >> +
>> >> >> +  drm_modeset_unlock_all(dev);
>> >> >
>> >> > This will run afoul of the upcoming locking rework in the atomic work. And
>> >> > I'm fairly sure that the crtc helpers will fall over badly if someone
>> >> > submits a concurrent setCrtc while you've dropped the locks here.
>> >> >
>> >> > Can't you instead just drop the locking from the worker? As long as you
>> >> > synchronize like here at the right places it can't race. I expect that you
>> >> > might want to synchronize in the crtc_prepare hook, too. But beyond that
>> >> > it should work.
>> >> >
>> >> > As-is nacked because future headaches for me ;-)
>> >>
>> >> Yes, it's ugly. But doing it with either queuing or caching would be a
>> >> major change. I was just trying to do smallish fixes to the driver for now.
>> >>
>> >> How about only unlocking/locking the crtc->mutex as Rob suggested? I
>> >> think it should work, but I didn't try it yet. Would that be as bad for
>> >> the locking rework?
>> >
>> > Same problem really, you shouldn't drop ww mutexes and reacquire them in
>> > the atomic world. ww mutexes have some debug infrastructure for that
>> > (ww_acquire_done) to catch abusers of this.
>> >
>> > So if you want to go forward with this it needs at least a big FIXME
>> > comment explaining that this is wrong. If you use locking to enforce
>> > ordering constraints that usually doesn't work well, and dropping locks to
>> > wait for async workers is plainly a locking design bug.
>>
>> well, the locking in core makes it in some ways a bit of a midlayer.. ;-)
>
> It's the locking for the drm core layer, it better be done in drm core.
> Drivers are free to use it, but if it doesn't fit they can always do their
> own. And if you look at i915 we actually have a metric pile of mutexes
> taken from modeset code and other places exactly because the drm core
> modeset locking can't work for everyone.

note that I'm not actually advocating removing/changing the locking in
core.. I was just pointing out that the way it is currently structured
can get in the way.

>> Some crtc->funcs->wait_until_flushed() sort of thing that
>> drm_mode_setcrtc() could call after dropping locks would go a long
>> ways to fix this case.  (Although post-atomic, may no longer be
>> required.)
>
> Now _this_ smells like a proper midlayer - it enforces behaviour
> sequencing by providing a bunch of callbacks for drivers to hook into.

well, I was thinking more in terms of an optional callback so that
drivers which (at least currently) need to work like omapdrm can
actually do what they need to do.

that said, I think when atomic eventually lands, it would be a good
idea to revisit how omapdrm works.. I think atomic should give a
better way for it to do what it needs to do..

In the mean time, we need *something*.  Although I think dropping and
re-grabbing locks is an ok temporary solution.  It wouldn't exactly be
the only driver that does this.

BR,
-R


> Also, locking isn't to enforce ordering constraints at all, that's the job
> of flush_workqueue, completion and friends. And if such a sync point would
> result in deadlocks if your worker also uses the modeset locks then your
> driver needs to grow internal mutexes to coordinate the data accessed by
> workers. In i915 we've added a pile of such cases in the past (and will
> add more), e.g. the pps mutex, backlight spinlock, edp mutex, ...
>
> ADF does the same mistake of "hey let's do the synchronization in the core
> because driver writers get it wrong all the time". Which doesn't make it
> better ;-)
>
> Now if your idea was to provide this as a helper, which either enforces
> the ordering correctly to not need locks, or has it's own locking of
> in-flux requests or similar, then I'm in favour. But adding such a
> callback at the core is nacked with extreme prejudice from me ...
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 5/9] drm/omap: clear omap_obj->paddr in omap_gem_put_paddr()

[Tomi Valkeinen]

Clear omap_obj's paddr when unmapping the memory, so that it's easier to
catch bad use of the paddr.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index e4849413ee80..b342e7b3bf00 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -829,6 +829,7 @@ int omap_gem_put_paddr(struct drm_gem_object *obj)
 				dev_err(obj->dev->dev,
 					"could not release unmap: %d\n", ret);
 			}
+			omap_obj->paddr = 0;
 			omap_obj->block = NULL;
 		}
 	}
-- 
1.9.1

[PATCH 6/9] drm/omap: add pin refcounting to omap_framebuffer

[Tomi Valkeinen]

omap_framebuffer_pin() and omap_framebuffer_unpin() are currently
broken, as they cannot be called multiple times (i.e. pin, pin, unpin,
unpin), which is what happens in certain cases. This issue causes the
driver to possibly use 0 as an address for a displayed buffer, leading
to OCP error from DSS.

This patch fixes the issue by adding a simple pin_count, used to track
the number of pins.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 2a5cacdc344b..58f2af32ede8 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -86,6 +86,7 @@ struct plane {
 
 struct omap_framebuffer {
 	struct drm_framebuffer base;
+	int pin_count;
 	const struct format *format;
 	struct plane planes[4];
 };
@@ -261,6 +262,11 @@ int omap_framebuffer_pin(struct drm_framebuffer *fb)
 	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
 	int ret, i, n = drm_format_num_planes(fb->pixel_format);
 
+	if (omap_fb->pin_count > 0) {
+		omap_fb->pin_count++;
+		return 0;
+	}
+
 	for (i = 0; i < n; i++) {
 		struct plane *plane = &omap_fb->planes[i];
 		ret = omap_gem_get_paddr(plane->bo, &plane->paddr, true);
@@ -269,6 +275,8 @@ int omap_framebuffer_pin(struct drm_framebuffer *fb)
 		omap_gem_dma_sync(plane->bo, DMA_TO_DEVICE);
 	}
 
+	omap_fb->pin_count++;
+
 	return 0;
 
 fail:
@@ -287,6 +295,11 @@ int omap_framebuffer_unpin(struct drm_framebuffer *fb)
 	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
 	int ret, i, n = drm_format_num_planes(fb->pixel_format);
 
+	omap_fb->pin_count--;
+
+	if (omap_fb->pin_count > 0)
+		return 0;
+
 	for (i = 0; i < n; i++) {
 		struct plane *plane = &omap_fb->planes[i];
 		ret = omap_gem_put_paddr(plane->bo);
-- 
1.9.1

[PATCH 7/9] drm/omap: fix omap_crtc_flush() to handle the workqueue

[Tomi Valkeinen]

omap_crtc_flush() is used to wait for scheduled work to be done for the
give crtc. However, it's not quite right at the moment.

omap_crtc_flush() does wait for work that is ran via vsync irq to be
done. However, work is also queued to the driver's priv->wq workqueue,
which is not handled by omap_crtc_flush().

Improve omap_crtc_flush() to flush the workqueue so that work there will
be ran.

This fixes a race issue on module unload, where an unpin work may be on
the work queue, but does not get ran before drm core starts tearing the
driver down, leading to a WARN.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 3261fbf94957..506cf8bc804f 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -655,21 +655,42 @@ static void omap_crtc_post_apply(struct omap_drm_apply *apply)
 	/* nothing needed for post-apply */
 }
 
+static bool omap_crtc_work_pending(struct omap_crtc *omap_crtc)
+{
+	return !list_empty(&omap_crtc->pending_applies) ||
+		!list_empty(&omap_crtc->queued_applies) ||
+		omap_crtc->event || omap_crtc->old_fb;
+}
+
+/*
+ * Wait for any work on the workqueue to be finished, and any work which will
+ * be run via vsync irq to be done.
+ *
+ * Note that work on workqueue could schedule new vsync work, and vice versa.
+ */
 void omap_crtc_flush(struct drm_crtc *crtc)
 {
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
+	struct omap_drm_private *priv = crtc->dev->dev_private;
 	int loops = 0;
 
-	while (!list_empty(&omap_crtc->pending_applies) ||
-		!list_empty(&omap_crtc->queued_applies) ||
-		omap_crtc->event || omap_crtc->old_fb) {
+	while (true) {
+		/* first flush the wq, so that any scheduled work is done */
+		flush_workqueue(priv->wq);
+
+		/* if we have nothing queued for this crtc, we're done */
+		if (!omap_crtc_work_pending(omap_crtc))
+			break;
 
 		if (++loops > 10) {
-			dev_err(crtc->dev->dev,
-				"omap_crtc_flush() timeout\n");
+			dev_err(crtc->dev->dev, "omap_crtc_flush() timeout\n");
 			break;
 		}
 
+		/*
+		 * wait for a bit so that a vsync has (probably) happened, and
+		 * that the crtc work is (probably) done.
+		 */
 		schedule_timeout_uninterruptible(msecs_to_jiffies(20));
 	}
 }
-- 
1.9.1

Re: [PATCH 7/9] drm/omap: fix omap_crtc_flush() to handle the workqueue

[Daniel Vetter]

On Wed, Sep 03, 2014 at 02:55:08PM +0300, Tomi Valkeinen wrote:
> omap_crtc_flush() is used to wait for scheduled work to be done for the
> give crtc. However, it's not quite right at the moment.
> 
> omap_crtc_flush() does wait for work that is ran via vsync irq to be
> done. However, work is also queued to the driver's priv->wq workqueue,
> which is not handled by omap_crtc_flush().
> 
> Improve omap_crtc_flush() to flush the workqueue so that work there will
> be ran.
> 
> This fixes a race issue on module unload, where an unpin work may be on
> the work queue, but does not get ran before drm core starts tearing the
> driver down, leading to a WARN.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

I didn't really dig into details, but isn't that the same workqueue as
used by the async modeset code? So the same deadlocks might happen ...

lockdep won't complain though since you essentially open-code a
workqueue_flush, and lockdep also doesn't complain about all possible
deadlocks (due to some design issues with lockdep).
-Daniel

> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 3261fbf94957..506cf8bc804f 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -655,21 +655,42 @@ static void omap_crtc_post_apply(struct omap_drm_apply *apply)
>  	/* nothing needed for post-apply */
>  }
>  
> +static bool omap_crtc_work_pending(struct omap_crtc *omap_crtc)
> +{
> +	return !list_empty(&omap_crtc->pending_applies) ||
> +		!list_empty(&omap_crtc->queued_applies) ||
> +		omap_crtc->event || omap_crtc->old_fb;
> +}
> +
> +/*
> + * Wait for any work on the workqueue to be finished, and any work which will
> + * be run via vsync irq to be done.
> + *
> + * Note that work on workqueue could schedule new vsync work, and vice versa.
> + */
>  void omap_crtc_flush(struct drm_crtc *crtc)
>  {
>  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> +	struct omap_drm_private *priv = crtc->dev->dev_private;
>  	int loops = 0;
>  
> -	while (!list_empty(&omap_crtc->pending_applies) ||
> -		!list_empty(&omap_crtc->queued_applies) ||
> -		omap_crtc->event || omap_crtc->old_fb) {
> +	while (true) {
> +		/* first flush the wq, so that any scheduled work is done */
> +		flush_workqueue(priv->wq);
> +
> +		/* if we have nothing queued for this crtc, we're done */
> +		if (!omap_crtc_work_pending(omap_crtc))
> +			break;
>  
>  		if (++loops > 10) {
> -			dev_err(crtc->dev->dev,
> -				"omap_crtc_flush() timeout\n");
> +			dev_err(crtc->dev->dev, "omap_crtc_flush() timeout\n");
>  			break;
>  		}
>  
> +		/*
> +		 * wait for a bit so that a vsync has (probably) happened, and
> +		 * that the crtc work is (probably) done.
> +		 */
>  		schedule_timeout_uninterruptible(msecs_to_jiffies(20));
>  	}
>  }
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@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

Re: [PATCH 7/9] drm/omap: fix omap_crtc_flush() to handle the workqueue

[Tomi Valkeinen]

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

On 03/09/14 17:27, Daniel Vetter wrote:
> On Wed, Sep 03, 2014 at 02:55:08PM +0300, Tomi Valkeinen wrote:
>> omap_crtc_flush() is used to wait for scheduled work to be done for the
>> give crtc. However, it's not quite right at the moment.
>>
>> omap_crtc_flush() does wait for work that is ran via vsync irq to be
>> done. However, work is also queued to the driver's priv->wq workqueue,
>> which is not handled by omap_crtc_flush().
>>
>> Improve omap_crtc_flush() to flush the workqueue so that work there will
>> be ran.
>>
>> This fixes a race issue on module unload, where an unpin work may be on
>> the work queue, but does not get ran before drm core starts tearing the
>> driver down, leading to a WARN.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> I didn't really dig into details, but isn't that the same workqueue as
> used by the async modeset code? So the same deadlocks might happen ...

Yes, we have just one workqueue in the driver.

Hmm, deadlocks with what lock? The modeconfig or crtc->mutex? I don't
think they are locked at any place where omap_crtc_flush is called.

> lockdep won't complain though since you essentially open-code a
> workqueue_flush, and lockdep also doesn't complain about all possible
> deadlocks (due to some design issues with lockdep).

What do you mean "open-code a workqueue_flush"?. I use flush_workqueue
there. We have two things to wait for: work on the workqueue and work
which is triggered by the vsync irq. So we loop and test for both of
those, until there's no more work.

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

Re: [PATCH 7/9] drm/omap: fix omap_crtc_flush() to handle the workqueue

[Daniel Vetter]

On Mon, Sep 08, 2014 at 04:03:18PM +0300, Tomi Valkeinen wrote:
> On 03/09/14 17:27, Daniel Vetter wrote:
> > On Wed, Sep 03, 2014 at 02:55:08PM +0300, Tomi Valkeinen wrote:
> >> omap_crtc_flush() is used to wait for scheduled work to be done for the
> >> give crtc. However, it's not quite right at the moment.
> >>
> >> omap_crtc_flush() does wait for work that is ran via vsync irq to be
> >> done. However, work is also queued to the driver's priv->wq workqueue,
> >> which is not handled by omap_crtc_flush().
> >>
> >> Improve omap_crtc_flush() to flush the workqueue so that work there will
> >> be ran.
> >>
> >> This fixes a race issue on module unload, where an unpin work may be on
> >> the work queue, but does not get ran before drm core starts tearing the
> >> driver down, leading to a WARN.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > 
> > I didn't really dig into details, but isn't that the same workqueue as
> > used by the async modeset code? So the same deadlocks might happen ...
> 
> Yes, we have just one workqueue in the driver.
> 
> Hmm, deadlocks with what lock? The modeconfig or crtc->mutex? I don't
> think they are locked at any place where omap_crtc_flush is called.

Oh, I presumed you're using _flush in the relevant modeset functions - we
do that in i915 to make sure that all the pageflips and other stuff
completed before we do another modeset. But omap only calls this at driver
unload, so no direct problem.

> > lockdep won't complain though since you essentially open-code a
> > workqueue_flush, and lockdep also doesn't complain about all possible
> > deadlocks (due to some design issues with lockdep).
> 
> What do you mean "open-code a workqueue_flush"?. I use flush_workqueue
> there. We have two things to wait for: work on the workqueue and work
> which is triggered by the vsync irq. So we loop and test for both of
> those, until there's no more work.

Oops, missed that. Ordering looks wrong though since if the irq can latch
the workqueue you need to wait for irqs to happen first before flushing.
And obviously queue the work before signalling the completion of the
interrupt. But since this seems to lack locking anyway and is only used
for unload it doesn't really matter.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/9] drm/omap: fix omap_crtc_flush() to handle the workqueue

[Tomi Valkeinen]

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

On 08/09/14 16:31, Daniel Vetter wrote:
> On Mon, Sep 08, 2014 at 04:03:18PM +0300, Tomi Valkeinen wrote:
>> On 03/09/14 17:27, Daniel Vetter wrote:
>>> On Wed, Sep 03, 2014 at 02:55:08PM +0300, Tomi Valkeinen wrote:
>>>> omap_crtc_flush() is used to wait for scheduled work to be done for the
>>>> give crtc. However, it's not quite right at the moment.
>>>>
>>>> omap_crtc_flush() does wait for work that is ran via vsync irq to be
>>>> done. However, work is also queued to the driver's priv->wq workqueue,
>>>> which is not handled by omap_crtc_flush().
>>>>
>>>> Improve omap_crtc_flush() to flush the workqueue so that work there will
>>>> be ran.
>>>>
>>>> This fixes a race issue on module unload, where an unpin work may be on
>>>> the work queue, but does not get ran before drm core starts tearing the
>>>> driver down, leading to a WARN.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>
>>> I didn't really dig into details, but isn't that the same workqueue as
>>> used by the async modeset code? So the same deadlocks might happen ...
>>
>> Yes, we have just one workqueue in the driver.
>>
>> Hmm, deadlocks with what lock? The modeconfig or crtc->mutex? I don't
>> think they are locked at any place where omap_crtc_flush is called.
> 
> Oh, I presumed you're using _flush in the relevant modeset functions - we

No. That's the locking issue again. We can't flush when holding the crtc
mutex, as the works in the workqueue also try to grab it...

> do that in i915 to make sure that all the pageflips and other stuff
> completed before we do another modeset. But omap only calls this at driver
> unload, so no direct problem.

At the moment yes, but in this series I add the same omap_crtc_flush()
call to two new places: dev_preclose and omap_crtc_commit. Of which the
omap_crtc_commit is the problematic one, discussed in the mail thread
for patch 4.

>>> lockdep won't complain though since you essentially open-code a
>>> workqueue_flush, and lockdep also doesn't complain about all possible
>>> deadlocks (due to some design issues with lockdep).
>>
>> What do you mean "open-code a workqueue_flush"?. I use flush_workqueue
>> there. We have two things to wait for: work on the workqueue and work
>> which is triggered by the vsync irq. So we loop and test for both of
>> those, until there's no more work.
> 
> Oops, missed that. Ordering looks wrong though since if the irq can latch
> the workqueue you need to wait for irqs to happen first before flushing.
> And obviously queue the work before signalling the completion of the
> interrupt. But since this seems to lack locking anyway and is only used
> for unload it doesn't really matter.

Yeah, well, the workqueue can create work for the irq also. I don't know
if it does, currently, but I think it's safer to presume that both
workqueue and the irq can create work to the other.

But that's why I have a loop there. So we flush, then check if there is
work for the irq. If yes, sleep a bit and go back to start. So if the
irq work created new work for the wq, we flush that. And if that work
created new work for the irq, we check that again. Etc.

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

Re: [PATCH 7/9] drm/omap: fix omap_crtc_flush() to handle the workqueue

[Rob Clark]

On Tue, Sep 9, 2014 at 3:07 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>>>> lockdep won't complain though since you essentially open-code a
>>>> workqueue_flush, and lockdep also doesn't complain about all possible
>>>> deadlocks (due to some design issues with lockdep).
>>>
>>> What do you mean "open-code a workqueue_flush"?. I use flush_workqueue
>>> there. We have two things to wait for: work on the workqueue and work
>>> which is triggered by the vsync irq. So we loop and test for both of
>>> those, until there's no more work.
>>
>> Oops, missed that. Ordering looks wrong though since if the irq can latch
>> the workqueue you need to wait for irqs to happen first before flushing.
>> And obviously queue the work before signalling the completion of the
>> interrupt. But since this seems to lack locking anyway and is only used
>> for unload it doesn't really matter.

from a quick look at omap_crtc_flush(), you probably also race w/
apply worker list manipulation (like moving from queued_applies to
pending_applies)

> Yeah, well, the workqueue can create work for the irq also. I don't know
> if it does, currently, but I think it's safer to presume that both
> workqueue and the irq can create work to the other.
>
> But that's why I have a loop there. So we flush, then check if there is
> work for the irq. If yes, sleep a bit and go back to start. So if the
> irq work created new work for the wq, we flush that. And if that work
> created new work for the irq, we check that again. Etc.

I think adding an internal per-crtc apply_lock would solve a lot of
problems.  I was originally shying away from recommending that, mostly
because I didn't want to think about it and I though there was an
easier solution.

But with an apply_lock we could at least add some locking to _flush()
and make it not racy as well..

Although, I wonder if some waitqueue scheme would be a bit more sane..
ie. end of apply_worker, if there is nothing more queued up, signal
the event that _flush() is waiting on.

BR,
-R

Re: [PATCH 7/9] drm/omap: fix omap_crtc_flush() to handle the workqueue

[Tomi Valkeinen]

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

On 09/09/14 13:43, Rob Clark wrote:

> Although, I wonder if some waitqueue scheme would be a bit more sane..
> ie. end of apply_worker, if there is nothing more queued up, signal
> the event that _flush() is waiting on.

Maybe, but we would still need either the separate apply_lock, or unlock
the crtc->mutex to allow the workqueue to run.

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

[PATCH 8/9] drm/omap: fix preclose to wait for scheduled work

[Tomi Valkeinen]

We already wait for all scheduled work to be done on the driver's unload
function. However, I think we need to wait on preclose() also, so that
when an application closes the drm file descriptor, we are sure that
there's no left around.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 26fda74c1e48..f59fa6600cf8 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -594,7 +594,20 @@ static void dev_lastclose(struct drm_device *dev)
 
 static void dev_preclose(struct drm_device *dev, struct drm_file *file)
 {
+	struct omap_drm_private *priv = dev->dev_private;
+	int i;
+
 	DBG("preclose: dev=%p", dev);
+
+	/*
+	 * Flush crtcs to finish any pending work.
+	 * Note: this may not be correct if there are multiple applications
+	 * using the drm device, and could possibly result in a timeout from
+	 * omap_crtc_flush() if an other application is actively queuing new
+	 * work.
+	 */
+	for (i = 0; i < priv->num_crtcs; i++)
+		omap_crtc_flush(priv->crtcs[i]);
 }
 
 static void dev_postclose(struct drm_device *dev, struct drm_file *file)
-- 
1.9.1

Re: [PATCH 8/9] drm/omap: fix preclose to wait for scheduled work

[Daniel Vetter]

On Wed, Sep 03, 2014 at 02:55:09PM +0300, Tomi Valkeinen wrote:
> We already wait for all scheduled work to be done on the driver's unload
> function. However, I think we need to wait on preclose() also, so that
> when an application closes the drm file descriptor, we are sure that
> there's no left around.

The justification (likely, didn't check omapdrm code) for this is that we
need to clear out all outstanding drm_events. Currently i915 and some
other drivers (but not all) have that open-coded. We should probably track
drm_events on a per-fd list and move this logic into the core instead of
adding more driver-specific hacks.
-Daniel
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 26fda74c1e48..f59fa6600cf8 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -594,7 +594,20 @@ static void dev_lastclose(struct drm_device *dev)
>  
>  static void dev_preclose(struct drm_device *dev, struct drm_file *file)
>  {
> +	struct omap_drm_private *priv = dev->dev_private;
> +	int i;
> +
>  	DBG("preclose: dev=%p", dev);
> +
> +	/*
> +	 * Flush crtcs to finish any pending work.
> +	 * Note: this may not be correct if there are multiple applications
> +	 * using the drm device, and could possibly result in a timeout from
> +	 * omap_crtc_flush() if an other application is actively queuing new
> +	 * work.
> +	 */
> +	for (i = 0; i < priv->num_crtcs; i++)
> +		omap_crtc_flush(priv->crtcs[i]);
>  }
>  
>  static void dev_postclose(struct drm_device *dev, struct drm_file *file)
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@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

Re: [PATCH 8/9] drm/omap: fix preclose to wait for scheduled work

[Tomi Valkeinen]

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

On 03/09/14 17:32, Daniel Vetter wrote:
> On Wed, Sep 03, 2014 at 02:55:09PM +0300, Tomi Valkeinen wrote:
>> We already wait for all scheduled work to be done on the driver's unload
>> function. However, I think we need to wait on preclose() also, so that
>> when an application closes the drm file descriptor, we are sure that
>> there's no left around.

Hmm, that's supposed to say "there's no work left around".

> The justification (likely, didn't check omapdrm code) for this is that we
> need to clear out all outstanding drm_events. Currently i915 and some
> other drivers (but not all) have that open-coded. We should probably track
> drm_events on a per-fd list and move this logic into the core instead of
> adding more driver-specific hacks.

To be honest, I'm not 100% sure what we need to wait for =). I'm still
learning DRM, and omapdrm's work handling is not the easiest one to
follow. But yes, at least the page-flip event is one that rings alarm
bells in my head, if the fb is allowed to be closed and there's still an
event outstanding.

With this patch we wait until all work is done in the preclose. The
performance is not an issue here, so I went for "better safe than sorry".

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

[PATCH 9/9] drm/omap: add a comment why locking is missing

[Tomi Valkeinen]

unpin_worker() calls omap_framebuffer_unpin() without any locks, which
looks very suspicious. However, both pin and unpin are always called via
the driver's private workqueue, so the access is synchronized that way.

Add a comment to make this clear.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 891a4dc608af..743d04981d71 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -71,6 +71,10 @@ static void unpin_worker(struct drm_flip_work *work, void *val)
 			container_of(work, struct omap_plane, unpin_work);
 	struct drm_device *dev = omap_plane->base.dev;
 
+	/*
+	 * omap_framebuffer_pin/unpin are always called from priv->wq,
+	 * so there's no need for locking here.
+	 */
 	omap_framebuffer_unpin(val);
 	mutex_lock(&dev->mode_config.mutex);
 	drm_framebuffer_unreference(val);
-- 
1.9.1