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 <[email protected]> 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 <[email protected]> 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. 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. -Chris -- Chris Wilson, Intel Open Source Technology Centre
