Am 19.06.2014 05:49, schrieb Michel D?nzer:
> This patch only applies to 3.15, right?
Yes correct. For 3.16 we have the reworked flip which I think is still a
good idea to keep for now.
I've addresses your comments and send out a v2 to the list CCing you,
please review.
Thanks,
Christian.
>
>
> On 19.06.2014 02:11, Christian K?nig wrote:
>> From: Christian K?nig <christian.koenig at amd.com>
>>
>> radeon_crtc_handle_flip can be called concurrently and if
>> we set the unpin_work to early try to flip an unpinned BO or
>> worse.
> Spelling: 'too early'
>
> Maybe something like:
>
> radeon_crtc_handle_flip can be called concurrently, and if
> we set the unpin_work too early, it may try to flip an unpinned BO or
> worse.
>
>
>> Signed-off-by: Christian K?nig <christian.koenig at amd.com>
>> Cc: stable at vger.kernel.org
>> ---
>> drivers/gpu/drm/radeon/radeon_display.c | 31
>> ++++++++++++++++---------------
>> 1 file changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_display.c
>> b/drivers/gpu/drm/radeon/radeon_display.c
>> index 356b733..cf22741 100644
>> --- a/drivers/gpu/drm/radeon/radeon_display.c
>> +++ b/drivers/gpu/drm/radeon/radeon_display.c
>> @@ -393,17 +393,6 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
>>
>> INIT_WORK(&work->work, radeon_unpin_work_func);
>>
>> - /* We borrow the event spin lock for protecting unpin_work */
>> - spin_lock_irqsave(&dev->event_lock, flags);
>> - if (radeon_crtc->unpin_work) {
>> - DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
>> - r = -EBUSY;
>> - goto unlock_free;
>> - }
>> - radeon_crtc->unpin_work = work;
>> - radeon_crtc->deferred_flip_completion = 0;
>> - spin_unlock_irqrestore(&dev->event_lock, flags);
>> -
>> /* pin the new buffer */
>> DRM_DEBUG_DRIVER("flip-ioctl() cur_fbo = %p, cur_bbo = %p\n",
>> work->old_rbo, rbo);
>> @@ -461,10 +450,6 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
>> base &= ~7;
>> }
>>
>> - spin_lock_irqsave(&dev->event_lock, flags);
>> - work->new_crtc_base = base;
>> - spin_unlock_irqrestore(&dev->event_lock, flags);
>> -
>> /* update crtc fb */
>> crtc->primary->fb = fb;
>>
>> @@ -477,6 +462,22 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
>> /* set the proper interrupt */
>> radeon_pre_page_flip(rdev, radeon_crtc->crtc_id);
>>
>> + /* We borrow the event spin lock for protecting unpin_work */
>> + spin_lock_irqsave(&dev->event_lock, flags);
>> + if (radeon_crtc->unpin_work) {
>> + spin_unlock_irqrestore(&dev->event_lock, flags);
>> + radeon_post_page_flip(rdev, radeon_crtc->crtc_id);
>> + drm_vblank_put(dev, radeon_crtc->crtc_id);
>> +
>> + DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
>> + r = -EBUSY;
>> + goto pflip_cleanup1;
>> + }
>> + radeon_crtc->unpin_work = work;
>> + radeon_crtc->deferred_flip_completion = 0;
>> + work->new_crtc_base = base;
>> + spin_unlock_irqrestore(&dev->event_lock, flags);
>> +
> This introduces a path where crtc->primary->fb is updated, but then we
> return -EBUSY.
>
>
> It also introduces a warning:
>
> drivers/gpu/drm/radeon/radeon_display.c: In function ?radeon_crtc_page_flip?:
> drivers/gpu/drm/radeon/radeon_display.c:496:1: warning: label ?unlock_free?
> defined but not used [-Wunused-label]
> unlock_free:
> ^
>
>
> Apart from that, looks good.
>
>
