On 09/20/2012 04:49 PM, Chris Wilson wrote: > On Thu, 20 Sep 2012 15:10:39 +0900, Joonyoung Shim <jy0922.shim at > 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 at >>> 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 at chris-wilson.co.uk> >>>>> Cc: Jesse Barnes <jbarnes at virtuousgeek.org> >>>>> Cc: stable at 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 at lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel > From: Chris Wilson <chris at chris-wilson.co.uk> > Subject: Re: [PATCH] drm: Destroy the planes prior to destroying the > associated CRTC > To: Rob Clark <rob.clark at linaro.org>, Joonyoung Shim <jy0922.shim at > samsung.com> > Cc: airlied at redhat.com, stable at vger.kernel.org, dri-devel at > lists.freedesktop.org > In-Reply-To: <CAF6AEGvOZa_ZhRJN212Rsn-gMMUyWoLT6UFY9iindi8AWx9GvA at > mail.gmail.com> > References: <1347874683-21191-1-git-send-email-chris at chris-wilson.co.uk> > <505A850A.4010205 at samsung.com> > <CAF6AEGvOZa_ZhRJN212Rsn-gMMUyWoLT6UFY9iindi8AWx9GvA at mail.gmail.com> > > On Thu, 20 Sep 2012 00:38:04 -0500, Rob Clark <rob.clark at linaro.org> wrote: >> On Wed, Sep 19, 2012 at 9:52 PM, Joonyoung Shim <jy0922.shim at 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 at chris-wilson.co.uk> >>>> Cc: Jesse Barnes <jbarnes at virtuousgeek.org> >>>> Cc: stable at 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 at lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel > From: Chris Wilson <chris at chris-wilson.co.uk> > Subject: Re: [PATCH] drm: Destroy the planes prior to destroying the > associated CRTC > To: Joonyoung Shim <jy0922.shim at samsung.com> > Cc: dri-devel at lists.freedesktop.org, airlied at redhat.com, stable at > vger.kernel.org > In-Reply-To: <505A850A.4010205 at samsung.com> > References: <1347874683-21191-1-git-send-email-chris at chris-wilson.co.uk> > <505A850A.4010205 at samsung.com> > > On Thu, 20 Sep 2012 11:52:58 +0900, Joonyoung Shim <jy0922.shim at > 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.
