Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC

[Joonyoung Shim <jy0922.shim@samsung.com>]

On 09/20/2012 04:49 PM, Chris Wilson wrote:
> On Thu, 20 Sep 2012 15:10:39 +0900, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>> On 09/20/2012 02:38 PM, Rob Clark wrote:
>>> On Wed, Sep 19, 2012 at 9:52 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>> On 09/17/2012 06:38 PM, Chris Wilson wrote:
>>>>> As during the plane cleanup, we wish to disable the hardware and
>>>>> so may modify state on the associated CRTC, that CRTC must continue to
>>>>> exist until we are finished.
>>>> A similar issue can occur in the drm_framebuffer_cleanup(). If crtc and
>>>> plane use same framebuffer and the framebuffer is destroyed, crtc is
>>>> turned off prior to turning off plane.
>>>>
>>> I imagine my patch to add refcnt'ing to fb would help in this case..
>>>
>>> BR,
>>> -R
>> Even if the patch to add refcnt'ing to fb is applied, same issue will
>> occur in the drm_framebuffer_remove(). It can delay to destroy the fb,
>> but cannot change crtc and plane disable order.
>>
>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54101
>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>>>>> Cc: stable@vger.kernel.org
>>>>> ---
>>>>>     drivers/gpu/drm/drm_crtc.c |    8 ++++----
>>>>>     1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>>>> index 6fbfc24..af81f77 100644
>>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>>> @@ -1034,15 +1034,15 @@ void drm_mode_config_cleanup(struct drm_device
>>>>> *dev)
>>>>>                   fb->funcs->destroy(fb);
>>>>>           }
>>>>>     -     list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list,
>>>>> head) {
>>>>> -               crtc->funcs->destroy(crtc);
>>>>> -       }
>>>>> -
>>>>>           list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
>>>>>                                    head) {
>>>>>                   plane->funcs->destroy(plane);
>>>>>           }
>>>>>     +     list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list,
>>>>> head) {
>>>>> +               crtc->funcs->destroy(crtc);
>>>>> +       }
>>>>> +
>>>>>           idr_remove_all(&dev->mode_config.crtc_idr);
>>>>>           idr_destroy(&dev->mode_config.crtc_idr);
>>>>>     }
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Subject: Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC
> To: Rob Clark <rob.clark@linaro.org>, Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: airlied@redhat.com, stable@vger.kernel.org, dri-devel@lists.freedesktop.org
> In-Reply-To: <CAF6AEGvOZa_ZhRJN212Rsn-gMMUyWoLT6UFY9iindi8AWx9GvA@mail.gmail.com>
> References: <1347874683-21191-1-git-send-email-chris@chris-wilson.co.uk> <505A850A.4010205@samsung.com> <CAF6AEGvOZa_ZhRJN212Rsn-gMMUyWoLT6UFY9iindi8AWx9GvA@mail.gmail.com>
>
> On Thu, 20 Sep 2012 00:38:04 -0500, Rob Clark <rob.clark@linaro.org> wrote:
>> On Wed, Sep 19, 2012 at 9:52 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>> On 09/17/2012 06:38 PM, Chris Wilson wrote:
>>>> As during the plane cleanup, we wish to disable the hardware and
>>>> so may modify state on the associated CRTC, that CRTC must continue to
>>>> exist until we are finished.
>>>
>>> A similar issue can occur in the drm_framebuffer_cleanup(). If crtc and
>>> plane use same framebuffer and the framebuffer is destroyed, crtc is
>>> turned off prior to turning off plane.
>>>
>> I imagine my patch to add refcnt'ing to fb would help in this case..
>>
>> BR,
>> -R
>>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54101
>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>>>> Cc: stable@vger.kernel.org
>>>> ---
>>>>    drivers/gpu/drm/drm_crtc.c |    8 ++++----
>>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>>> index 6fbfc24..af81f77 100644
>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>> @@ -1034,15 +1034,15 @@ void drm_mode_config_cleanup(struct drm_device
>>>> *dev)
>>>>                  fb->funcs->destroy(fb);
>>>>          }
>>>>    -     list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list,
>>>> head) {
>>>> -               crtc->funcs->destroy(crtc);
>>>> -       }
>>>> -
>>>>          list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
>>>>                                   head) {
>>>>                  plane->funcs->destroy(plane);
>>>>          }
>>>>    +     list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list,
>>>> head) {
>>>> +               crtc->funcs->destroy(crtc);
>>>> +       }
>>>> +
>>>>          idr_remove_all(&dev->mode_config.crtc_idr);
>>>>          idr_destroy(&dev->mode_config.crtc_idr);
>>>>    }
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Subject: Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC
> To: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: dri-devel@lists.freedesktop.org, airlied@redhat.com, stable@vger.kernel.org
> In-Reply-To: <505A850A.4010205@samsung.com>
> References: <1347874683-21191-1-git-send-email-chris@chris-wilson.co.uk> <505A850A.4010205@samsung.com>
>
> On Thu, 20 Sep 2012 11:52:58 +0900, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>> On 09/17/2012 06:38 PM, Chris Wilson wrote:
>>> As during the plane cleanup, we wish to disable the hardware and
>>> so may modify state on the associated CRTC, that CRTC must continue to
>>> exist until we are finished.
>> A similar issue can occur in the drm_framebuffer_cleanup(). If crtc and
>> plane use same framebuffer and the framebuffer is destroyed, crtc is
>> turned off prior to turning off plane.
> This is not an issue in our code as disabling the CRTCs should
> automatically disable any associated planes, and so the second pass over
> the planes should be a no-op.

Right, but it is decided by each hw specific driver implementation. If
drm core can prevent any problem, it's better.

> The issue during destroy drm_mode_config_cleanup() is that we actually
> free the CRTC objects and then try to decouple the planes which causes
> us to reference the just-freed objects in order to see if the hw needs
> updating.
>
> However, reordering the sequence to be CRTCs last would help for
> consistency.

Agree.