* [PATCH] drm: Destroy the planes prior to destroying the associated CRTC
@ 2012-09-17 9:38 Chris Wilson
2012-09-17 14:55 ` Jesse Barnes
2012-09-20 2:52 ` Joonyoung Shim
0 siblings, 2 replies; 11+ messages in thread
From: Chris Wilson @ 2012-09-17 9:38 UTC (permalink / raw)
To: dri-devel; +Cc: airlied, stable
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.
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);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC
2012-09-17 9:38 [PATCH] drm: Destroy the planes prior to destroying the associated CRTC Chris Wilson
@ 2012-09-17 14:55 ` Jesse Barnes
2012-09-17 15:04 ` Chris Wilson
2012-09-20 2:52 ` Joonyoung Shim
1 sibling, 1 reply; 11+ messages in thread
From: Jesse Barnes @ 2012-09-17 14:55 UTC (permalink / raw)
To: Chris Wilson; +Cc: airlied, stable, dri-devel
On Mon, 17 Sep 2012 10:38:03 +0100
Chris Wilson <chris@chris-wilson.co.uk> 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.
>
> 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);
> }
Yeah, looks good. Did we used to not touch the crtc in the plane
cleanup? Either way this is safer.
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC
2012-09-17 14:55 ` Jesse Barnes
@ 2012-09-17 15:04 ` Chris Wilson
2012-09-19 9:34 ` Daniel Vetter
0 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2012-09-17 15:04 UTC (permalink / raw)
To: Jesse Barnes; +Cc: airlied, stable, dri-devel
On Mon, 17 Sep 2012 07:55:59 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Yeah, looks good. Did we used to not touch the crtc in the plane
> cleanup? Either way this is safer.
I think the bug only came to light because QA is now routinely running
with slub_debug.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC
2012-09-17 15:04 ` Chris Wilson
@ 2012-09-19 9:34 ` Daniel Vetter
2012-09-19 9:45 ` Paul Menzel
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2012-09-19 9:34 UTC (permalink / raw)
To: Chris Wilson; +Cc: airlied, dri-devel, stable
On Mon, Sep 17, 2012 at 04:04:23PM +0100, Chris Wilson wrote:
> On Mon, 17 Sep 2012 07:55:59 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > Yeah, looks good. Did we used to not touch the crtc in the plane
> > cleanup? Either way this is safer.
>
> I think the bug only came to light because QA is now routinely running
> with slub_debug.
And QA now confirmed that this patch fixes things:
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54101
Tested-by: lu hua <huax.lu@intel.com>
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC
2012-09-19 9:34 ` Daniel Vetter
@ 2012-09-19 9:45 ` Paul Menzel
2012-09-19 10:26 ` Chris Wilson
0 siblings, 1 reply; 11+ messages in thread
From: Paul Menzel @ 2012-09-19 9:45 UTC (permalink / raw)
Cc: airlied, Lu Huax, stable, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 640 bytes --]
Am Mittwoch, den 19.09.2012, 11:34 +0200 schrieb Daniel Vetter:
> On Mon, Sep 17, 2012 at 04:04:23PM +0100, Chris Wilson wrote:
> > On Mon, 17 Sep 2012 07:55:59 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > Yeah, looks good. Did we used to not touch the crtc in the plane
> > > cleanup? Either way this is safer.
> >
> > I think the bug only came to light because QA is now routinely running
> > with slub_debug.
>
> And QA now confirmed that this patch fixes things:
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54101
> Tested-by: lu hua <huax.lu@intel.com>
Lu Hua*x*?
Thanks,
Paul
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC
2012-09-19 9:45 ` Paul Menzel
@ 2012-09-19 10:26 ` Chris Wilson
0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2012-09-19 10:26 UTC (permalink / raw)
To: Paul Menzel; +Cc: airlied, stable, dri-devel, Lu Huax
On Wed, 19 Sep 2012 11:45:34 +0200, Paul Menzel <paulepanter@users.sourceforge.net> wrote:
> Am Mittwoch, den 19.09.2012, 11:34 +0200 schrieb Daniel Vetter:
> > On Mon, Sep 17, 2012 at 04:04:23PM +0100, Chris Wilson wrote:
> > > On Mon, 17 Sep 2012 07:55:59 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > > Yeah, looks good. Did we used to not touch the crtc in the plane
> > > > cleanup? Either way this is safer.
> > >
> > > I think the bug only came to light because QA is now routinely running
> > > with slub_debug.
> >
> > And QA now confirmed that this patch fixes things:
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54101
> > Tested-by: lu hua <huax.lu@intel.com>
>
> Lu Hua*x*?
The 'x' is just part of Intel's policy for indicating a contractor in
email addresses, which can be quite confusing.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC
2012-09-17 9:38 [PATCH] drm: Destroy the planes prior to destroying the associated CRTC Chris Wilson
2012-09-17 14:55 ` Jesse Barnes
@ 2012-09-20 2:52 ` Joonyoung Shim
2012-09-20 5:38 ` Rob Clark
1 sibling, 1 reply; 11+ messages in thread
From: Joonyoung Shim @ 2012-09-20 2:52 UTC (permalink / raw)
To: Chris Wilson; +Cc: airlied, stable, dri-devel
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.
> 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);
> }
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC
2012-09-20 2:52 ` Joonyoung Shim
@ 2012-09-20 5:38 ` Rob Clark
2012-09-20 6:10 ` Joonyoung Shim
0 siblings, 1 reply; 11+ messages in thread
From: Rob Clark @ 2012-09-20 5:38 UTC (permalink / raw)
To: Joonyoung Shim; +Cc: airlied, dri-devel, stable
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC
2012-09-20 5:38 ` Rob Clark
@ 2012-09-20 6:10 ` Joonyoung Shim
2012-09-20 7:49 ` Chris Wilson
0 siblings, 1 reply; 11+ messages in thread
From: Joonyoung Shim @ 2012-09-20 6:10 UTC (permalink / raw)
To: Rob Clark; +Cc: airlied, dri-devel, stable
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC
2012-09-20 6:10 ` Joonyoung Shim
@ 2012-09-20 7:49 ` Chris Wilson
2012-09-20 8:08 ` Joonyoung Shim
0 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2012-09-20 7:49 UTC (permalink / raw)
To: Joonyoung Shim, Rob Clark; +Cc: airlied, dri-devel, stable
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.
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm: Destroy the planes prior to destroying the associated CRTC
2012-09-20 7:49 ` Chris Wilson
@ 2012-09-20 8:08 ` Joonyoung Shim
0 siblings, 0 replies; 11+ messages in thread
From: Joonyoung Shim @ 2012-09-20 8:08 UTC (permalink / raw)
To: Chris Wilson; +Cc: airlied, dri-devel, stable, Rob Clark
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-09-20 8:08 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-17 9:38 [PATCH] drm: Destroy the planes prior to destroying the associated CRTC Chris Wilson
2012-09-17 14:55 ` Jesse Barnes
2012-09-17 15:04 ` Chris Wilson
2012-09-19 9:34 ` Daniel Vetter
2012-09-19 9:45 ` Paul Menzel
2012-09-19 10:26 ` Chris Wilson
2012-09-20 2:52 ` Joonyoung Shim
2012-09-20 5:38 ` Rob Clark
2012-09-20 6:10 ` Joonyoung Shim
2012-09-20 7:49 ` Chris Wilson
2012-09-20 8:08 ` Joonyoung Shim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.