[2/8] drm/rockchip: Get rid of some unnecessary code
diff mbox

Message ID 1473857701-9250-3-git-send-email-tfiga@chromium.org
State New
Headers show

Commit Message

Tomasz Figa Sept. 14, 2016, 12:54 p.m. UTC
Current code implements prepare_fb and cleanup_fb callbacks only to
grab/release fb references, which is already done by atomic framework
when creating/destryoing plane state. Let's remove these
unused bits.

Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ------------------
 1 file changed, 18 deletions(-)

Comments

yao mark Sept. 18, 2016, 1:50 a.m. UTC | #1
On 2016年09月14日 20:54, Tomasz Figa wrote:
> Current code implements prepare_fb and cleanup_fb callbacks only to
> grab/release fb references, which is already done by atomic framework
> when creating/destryoing plane state. Let's remove these
> unused bits.
>
> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ------------------
>   1 file changed, 18 deletions(-)

Hi Tomasz

I think we can't get rid of the prepare_fb and cleanup_fb

see the reason:
commit 44d0237a26395ac94160cf23f32769013b365590
Author: Mark Yao <mark.yao@rock-chips.com>
Date:   Fri Apr 29 11:37:20 2016 +0800

     drm/rockchip: vop: fix iommu crash with async atomic

     After async atomic_commit callback, drm_atomic_clean_old_fb will
     clean all old fb, but because async, the old fb may be also on
     the vop hardware, dma will access the old fb buffer, clean old
     fb will cause iommu page fault.

     Reference the fb and unreference it when the fb actuall swap out
     from vop hardware.


> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 7e811cf..68988c6 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -641,22 +641,6 @@ static void vop_plane_destroy(struct drm_plane *plane)
>   	drm_plane_cleanup(plane);
>   }
>   
> -static int vop_plane_prepare_fb(struct drm_plane *plane,
> -				struct drm_plane_state *new_state)
> -{
> -	if (plane->state->fb)
> -		drm_framebuffer_reference(plane->state->fb);
> -
> -	return 0;
> -}
> -
> -static void vop_plane_cleanup_fb(struct drm_plane *plane,
> -				 struct drm_plane_state *old_state)
> -{
> -	if (old_state->fb)
> -		drm_framebuffer_unreference(old_state->fb);
> -}
> -
>   static int vop_plane_atomic_check(struct drm_plane *plane,
>   			   struct drm_plane_state *state)
>   {
> @@ -849,8 +833,6 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
>   }
>   
>   static const struct drm_plane_helper_funcs plane_helper_funcs = {
> -	.prepare_fb = vop_plane_prepare_fb,
> -	.cleanup_fb = vop_plane_cleanup_fb,
>   	.atomic_check = vop_plane_atomic_check,
>   	.atomic_update = vop_plane_atomic_update,
>   	.atomic_disable = vop_plane_atomic_disable,
Tomasz Figa Sept. 18, 2016, 4:01 a.m. UTC | #2
Hi Mark,

On Sun, Sep 18, 2016 at 10:50 AM, Mark yao <mark.yao@rock-chips.com> wrote:
> On 2016年09月14日 20:54, Tomasz Figa wrote:
>>
>> Current code implements prepare_fb and cleanup_fb callbacks only to
>> grab/release fb references, which is already done by atomic framework
>> when creating/destryoing plane state. Let's remove these
>> unused bits.
>>
>> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
>> ---
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ------------------
>>   1 file changed, 18 deletions(-)
>
>
> Hi Tomasz
>
> I think we can't get rid of the prepare_fb and cleanup_fb

I think I have to disagree. Please see below for detailed explanation.

>
> see the reason:
> commit 44d0237a26395ac94160cf23f32769013b365590
> Author: Mark Yao <mark.yao@rock-chips.com>
> Date:   Fri Apr 29 11:37:20 2016 +0800
>
>     drm/rockchip: vop: fix iommu crash with async atomic
>
>     After async atomic_commit callback, drm_atomic_clean_old_fb will
>     clean all old fb, but because async, the old fb may be also on
>     the vop hardware, dma will access the old fb buffer, clean old
>     fb will cause iommu page fault.

I think the above is not quite right. Atomic plane state holds a
reference to its fb and old state is not supposed to be destroyed
until the flip completes.

Indeed current rockchip_atomic_commit implementation has following
order of calls: rockchip_atomic_wait_for_complete(),
drm_atomic_helper_cleanup_planes(), drm_atomic_state_free(). This
means that .cleanup_fb() is called from
drm_atomic_helper_cleanup_planes() just before drm_atomic_state_free()
will release references by destroying old plane states. Note that both
are called already after rockchip_atomic_wait_for_complete(), so it
should be already safe to free the old fbs.

So the above fix doesn't really do anything, possibly just covers the
race condition of the original wait for vblank function by delaying
drm_atomic_state_free() a bit.

Moreover, the whole series has been thoroughly tested in Chrome OS 4.4
kernel, including async commits. (There is still a possibility some
newer upstream changes slightly modified the semantics, but I couldn't
find such difference. Actually one of the advantages of atomic helpers
was to avoid manually refcounting the fbs from the driver.)

Best regards,
Tomasz
yao mark Sept. 20, 2016, 1:36 a.m. UTC | #3
On 2016年09月18日 12:01, Tomasz Figa wrote:
> Hi Mark,
>
> On Sun, Sep 18, 2016 at 10:50 AM, Mark yao <mark.yao@rock-chips.com> wrote:
>> On 2016年09月14日 20:54, Tomasz Figa wrote:
>>> Current code implements prepare_fb and cleanup_fb callbacks only to
>>> grab/release fb references, which is already done by atomic framework
>>> when creating/destryoing plane state. Let's remove these
>>> unused bits.
>>>
>>> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
>>> ---
>>>    drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ------------------
>>>    1 file changed, 18 deletions(-)
>>
>> Hi Tomasz
>>
>> I think we can't get rid of the prepare_fb and cleanup_fb
> I think I have to disagree. Please see below for detailed explanation.
>
>> see the reason:
>> commit 44d0237a26395ac94160cf23f32769013b365590
>> Author: Mark Yao <mark.yao@rock-chips.com>
>> Date:   Fri Apr 29 11:37:20 2016 +0800
>>
>>      drm/rockchip: vop: fix iommu crash with async atomic
>>
>>      After async atomic_commit callback, drm_atomic_clean_old_fb will
>>      clean all old fb, but because async, the old fb may be also on
>>      the vop hardware, dma will access the old fb buffer, clean old
>>      fb will cause iommu page fault.
> I think the above is not quite right. Atomic plane state holds a
> reference to its fb and old state is not supposed to be destroyed
> until the flip completes.
>
> Indeed current rockchip_atomic_commit implementation has following
> order of calls: rockchip_atomic_wait_for_complete(),
> drm_atomic_helper_cleanup_planes(), drm_atomic_state_free(). This
> means that .cleanup_fb() is called from
> drm_atomic_helper_cleanup_planes() just before drm_atomic_state_free()
> will release references by destroying old plane states. Note that both
> are called already after rockchip_atomic_wait_for_complete(), so it
> should be already safe to free the old fbs.
>
> So the above fix doesn't really do anything, possibly just covers the
> race condition of the original wait for vblank function by delaying
> drm_atomic_state_free() a bit.
>
> Moreover, the whole series has been thoroughly tested in Chrome OS 4.4
> kernel, including async commits. (There is still a possibility some
> newer upstream changes slightly modified the semantics, but I couldn't
> find such difference. Actually one of the advantages of atomic helpers
> was to avoid manually refcounting the fbs from the driver.)
>
> Best regards,
> Tomasz
>
Hi Tomasz

You are right, plane_duplicate_state/plane_destroy_state already protect 
the old fbs.
we can get rid of prepare_fb and cleanup_fb.

Patch
diff mbox

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 7e811cf..68988c6 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -641,22 +641,6 @@  static void vop_plane_destroy(struct drm_plane *plane)
 	drm_plane_cleanup(plane);
 }
 
-static int vop_plane_prepare_fb(struct drm_plane *plane,
-				struct drm_plane_state *new_state)
-{
-	if (plane->state->fb)
-		drm_framebuffer_reference(plane->state->fb);
-
-	return 0;
-}
-
-static void vop_plane_cleanup_fb(struct drm_plane *plane,
-				 struct drm_plane_state *old_state)
-{
-	if (old_state->fb)
-		drm_framebuffer_unreference(old_state->fb);
-}
-
 static int vop_plane_atomic_check(struct drm_plane *plane,
 			   struct drm_plane_state *state)
 {
@@ -849,8 +833,6 @@  static void vop_plane_atomic_update(struct drm_plane *plane,
 }
 
 static const struct drm_plane_helper_funcs plane_helper_funcs = {
-	.prepare_fb = vop_plane_prepare_fb,
-	.cleanup_fb = vop_plane_cleanup_fb,
 	.atomic_check = vop_plane_atomic_check,
 	.atomic_update = vop_plane_atomic_update,
 	.atomic_disable = vop_plane_atomic_disable,