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

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- 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