From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joonyoung Shim Subject: Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC Date: Thu, 20 Sep 2012 17:08:18 +0900 Message-ID: <505ACEF2.201@samsung.com> References: <1347874683-21191-1-git-send-email-chris@chris-wilson.co.uk> <505A850A.4010205@samsung.com> <505AB35F.90608@samsung.com> <051c15$4c7nat@AZSMGA002.ch.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mailout3.samsung.com (mailout3.samsung.com [203.254.224.33]) by gabe.freedesktop.org (Postfix) with ESMTP id 88E8AA0A46 for ; Thu, 20 Sep 2012 01:08:12 -0700 (PDT) Received: from epcpsbgm1.samsung.com (epcpsbgm1 [203.254.230.26]) by mailout3.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0MAN00HRC2L5HYM0@mailout3.samsung.com> for dri-devel@lists.freedesktop.org; Thu, 20 Sep 2012 17:08:10 +0900 (KST) Received: from [10.90.51.60] by mmp1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTPA id <0MAN00BRT2LM2160@mmp1.samsung.com> for dri-devel@lists.freedesktop.org; Thu, 20 Sep 2012 17:08:10 +0900 (KST) In-reply-to: <051c15$4c7nat@AZSMGA002.ch.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: airlied@redhat.com, dri-devel@lists.freedesktop.org, stable@vger.kernel.org, Rob Clark List-Id: dri-devel@lists.freedesktop.org On 09/20/2012 04:49 PM, Chris Wilson wrote: > On Thu, 20 Sep 2012 15:10:39 +0900, Joonyoung Shim wrote: >> On 09/20/2012 02:38 PM, Rob Clark wrote: >>> On Wed, Sep 19, 2012 at 9:52 PM, Joonyoung Shim 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 >>>>> Cc: Jesse Barnes >>>>> 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 > Subject: Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC > To: Rob Clark , Joonyoung Shim > Cc: airlied@redhat.com, stable@vger.kernel.org, dri-devel@lists.freedesktop.org > In-Reply-To: > References: <1347874683-21191-1-git-send-email-chris@chris-wilson.co.uk> <505A850A.4010205@samsung.com> > > On Thu, 20 Sep 2012 00:38:04 -0500, Rob Clark wrote: >> On Wed, Sep 19, 2012 at 9:52 PM, Joonyoung Shim 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 >>>> Cc: Jesse Barnes >>>> 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 > Subject: Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC > To: Joonyoung Shim > 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 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.