Re: [PATCH 6/6] drm/omap: protect omap_crtc's event with event_lock spinlock

[Archit Taneja <archit@ti.com>]

Hi,

On Monday 14 April 2014 06:06 PM, Tomi Valkeinen wrote:
> On 11/04/14 10:23, Archit Taneja wrote:
>> The vblank_cb callback and the page_flip ioctl can occur together in different
>> CPU contexts. vblank_cb uses takes tje drm device's event_lock spinlock when
>> sending the vblank event and updating omap_crtc->event and omap_crtc->od_fb.
>>
>> Use the same spinlock in page_flip, to make sure the above omap_crtc parameters
>> are configured sequentially.
>>
>> Signed-off-by: Archit Taneja <archit@ti.com>
>> ---
>>   drivers/gpu/drm/omapdrm/omap_crtc.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> index d74c72c..9c7af42 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> @@ -350,6 +350,7 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc,
>>   	struct drm_device *dev = crtc->dev;
>>   	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>>   	struct drm_gem_object *bo;
>> +	unsigned long flags;
>>
>>   	DBG("%d -> %d (event=%p)", crtc->fb ? crtc->fb->base.id : -1,
>>   			fb->base.id, event);
>> @@ -359,9 +360,12 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc,
>>   		return -EINVAL;
>>   	}
>>
>> +	spin_lock_irqsave(&dev->event_lock, flags);
>> +
>>   	omap_crtc->event = event;
>>   	omap_crtc->old_fb = crtc->fb = fb;
>>
>> +	spin_unlock_irqrestore(&dev->event_lock, flags);
>
> There's
>
> if (omap_crtc->old_fb)
>
> just above the code you changed. Shouldn't that also be inside the spinlock?

Yes, that should come within the spinlock too.

On a related note, the mdp5_crtc->event in mdp5_crtc_page_flip(and for 
mdp4 crtc) should also be inside the spinlock.

>
> Also, the description contains a few typos.

I'll fix these.

Thanks,
Archit