linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/atomic: Check new_crtc_state->active to determine if CRTC needs disable in self refresh mode
@ 2021-12-30  4:06 Liu Ying
  2022-01-07 19:53 ` Alex Deucher
  0 siblings, 1 reply; 4+ messages in thread
From: Liu Ying @ 2021-12-30  4:06 UTC (permalink / raw)
  To: dri-devel, linux-kernel
  Cc: linux-imx, Sean Paul, Rob Clark, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter

Actual hardware state of CRTC is controlled by the member 'active' in
struct drm_crtc_state instead of the member 'enable', according to the
kernel doc of the member 'enable'.  In fact, the drm client modeset
and atomic helpers are using the member 'active' to do the control.

Referencing the member 'enable' of new_crtc_state, the function
crtc_needs_disable() may fail to reflect if CRTC needs disable in
self refresh mode, e.g., when the framebuffer emulation will be blanked
through the client modeset helper with the next commit, the member
'enable' of new_crtc_state is still true while the member 'active' is
false, hence the relevant potential encoder and bridges won't be disabled.

So, let's check new_crtc_state->active to determine if CRTC needs disable
in self refresh mode instead of new_crtc_state->enable.

Fixes: 1452c25b0e60 ("drm: Add helpers to kick off self refresh mode in drivers")
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Rob Clark <robdclark@chromium.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index a7a05e1e26bb..9603193d2fa1 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1016,7 +1016,7 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
 	 * it's in self refresh mode and needs to be fully disabled.
 	 */
 	return old_state->active ||
-	       (old_state->self_refresh_active && !new_state->enable) ||
+	       (old_state->self_refresh_active && !new_state->active) ||
 	       new_state->self_refresh_active;
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/atomic: Check new_crtc_state->active to determine if CRTC needs disable in self refresh mode
  2021-12-30  4:06 [PATCH] drm/atomic: Check new_crtc_state->active to determine if CRTC needs disable in self refresh mode Liu Ying
@ 2022-01-07 19:53 ` Alex Deucher
  2022-01-08  2:07   ` Liu Ying
  0 siblings, 1 reply; 4+ messages in thread
From: Alex Deucher @ 2022-01-07 19:53 UTC (permalink / raw)
  To: Liu Ying
  Cc: Maling list - DRI developers, LKML, Rob Clark, David Airlie,
	Sean Paul, NXP Linux Team, Thomas Zimmermann

On Wed, Dec 29, 2021 at 11:07 PM Liu Ying <victor.liu@nxp.com> wrote:
>
> Actual hardware state of CRTC is controlled by the member 'active' in
> struct drm_crtc_state instead of the member 'enable', according to the
> kernel doc of the member 'enable'.  In fact, the drm client modeset
> and atomic helpers are using the member 'active' to do the control.
>
> Referencing the member 'enable' of new_crtc_state, the function
> crtc_needs_disable() may fail to reflect if CRTC needs disable in
> self refresh mode, e.g., when the framebuffer emulation will be blanked
> through the client modeset helper with the next commit, the member
> 'enable' of new_crtc_state is still true while the member 'active' is
> false, hence the relevant potential encoder and bridges won't be disabled.
>
> So, let's check new_crtc_state->active to determine if CRTC needs disable
> in self refresh mode instead of new_crtc_state->enable.
>
> Fixes: 1452c25b0e60 ("drm: Add helpers to kick off self refresh mode in drivers")
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Rob Clark <robdclark@chromium.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Do you need someone to push this for you?

Alex

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index a7a05e1e26bb..9603193d2fa1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1016,7 +1016,7 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
>          * it's in self refresh mode and needs to be fully disabled.
>          */
>         return old_state->active ||
> -              (old_state->self_refresh_active && !new_state->enable) ||
> +              (old_state->self_refresh_active && !new_state->active) ||
>                new_state->self_refresh_active;
>  }
>
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/atomic: Check new_crtc_state->active to determine if CRTC needs disable in self refresh mode
  2022-01-07 19:53 ` Alex Deucher
@ 2022-01-08  2:07   ` Liu Ying
  2022-01-11 15:47     ` Alex Deucher
  0 siblings, 1 reply; 4+ messages in thread
From: Liu Ying @ 2022-01-08  2:07 UTC (permalink / raw)
  To: Alex Deucher
  Cc: Maling list - DRI developers, LKML, Rob Clark, David Airlie,
	Sean Paul, NXP Linux Team, Thomas Zimmermann

On Fri, 2022-01-07 at 14:53 -0500, Alex Deucher wrote:
> On Wed, Dec 29, 2021 at 11:07 PM Liu Ying <victor.liu@nxp.com> wrote:
> > 
> > Actual hardware state of CRTC is controlled by the member 'active'
> > in
> > struct drm_crtc_state instead of the member 'enable', according to
> > the
> > kernel doc of the member 'enable'.  In fact, the drm client modeset
> > and atomic helpers are using the member 'active' to do the control.
> > 
> > Referencing the member 'enable' of new_crtc_state, the function
> > crtc_needs_disable() may fail to reflect if CRTC needs disable in
> > self refresh mode, e.g., when the framebuffer emulation will be
> > blanked
> > through the client modeset helper with the next commit, the member
> > 'enable' of new_crtc_state is still true while the member 'active'
> > is
> > false, hence the relevant potential encoder and bridges won't be
> > disabled.
> > 
> > So, let's check new_crtc_state->active to determine if CRTC needs
> > disable
> > in self refresh mode instead of new_crtc_state->enable.
> > 
> > Fixes: 1452c25b0e60 ("drm: Add helpers to kick off self refresh
> > mode in drivers")
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: Rob Clark <robdclark@chromium.org>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> 
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> 
> Do you need someone to push this for you?

Yes, please.  Thanks.

Liu Ying

> 
> Alex
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index a7a05e1e26bb..9603193d2fa1 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1016,7 +1016,7 @@ crtc_needs_disable(struct drm_crtc_state
> > *old_state,
> >          * it's in self refresh mode and needs to be fully
> > disabled.
> >          */
> >         return old_state->active ||
> > -              (old_state->self_refresh_active && !new_state-
> > >enable) ||
> > +              (old_state->self_refresh_active && !new_state-
> > >active) ||
> >                new_state->self_refresh_active;
> >  }
> > 
> > --
> > 2.25.1
> > 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/atomic: Check new_crtc_state->active to determine if CRTC needs disable in self refresh mode
  2022-01-08  2:07   ` Liu Ying
@ 2022-01-11 15:47     ` Alex Deucher
  0 siblings, 0 replies; 4+ messages in thread
From: Alex Deucher @ 2022-01-11 15:47 UTC (permalink / raw)
  To: Liu Ying
  Cc: Maling list - DRI developers, LKML, Rob Clark, David Airlie,
	Sean Paul, NXP Linux Team, Thomas Zimmermann

Pushed out to drm-misc-next-fixes.

Alex

On Fri, Jan 7, 2022 at 9:07 PM Liu Ying <victor.liu@nxp.com> wrote:
>
> On Fri, 2022-01-07 at 14:53 -0500, Alex Deucher wrote:
> > On Wed, Dec 29, 2021 at 11:07 PM Liu Ying <victor.liu@nxp.com> wrote:
> > >
> > > Actual hardware state of CRTC is controlled by the member 'active'
> > > in
> > > struct drm_crtc_state instead of the member 'enable', according to
> > > the
> > > kernel doc of the member 'enable'.  In fact, the drm client modeset
> > > and atomic helpers are using the member 'active' to do the control.
> > >
> > > Referencing the member 'enable' of new_crtc_state, the function
> > > crtc_needs_disable() may fail to reflect if CRTC needs disable in
> > > self refresh mode, e.g., when the framebuffer emulation will be
> > > blanked
> > > through the client modeset helper with the next commit, the member
> > > 'enable' of new_crtc_state is still true while the member 'active'
> > > is
> > > false, hence the relevant potential encoder and bridges won't be
> > > disabled.
> > >
> > > So, let's check new_crtc_state->active to determine if CRTC needs
> > > disable
> > > in self refresh mode instead of new_crtc_state->enable.
> > >
> > > Fixes: 1452c25b0e60 ("drm: Add helpers to kick off self refresh
> > > mode in drivers")
> > > Cc: Sean Paul <seanpaul@chromium.org>
> > > Cc: Rob Clark <robdclark@chromium.org>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Maxime Ripard <mripard@kernel.org>
> > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: David Airlie <airlied@linux.ie>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> >
> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> >
> > Do you need someone to push this for you?
>
> Yes, please.  Thanks.
>
> Liu Ying
>
> >
> > Alex
> >
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > index a7a05e1e26bb..9603193d2fa1 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1016,7 +1016,7 @@ crtc_needs_disable(struct drm_crtc_state
> > > *old_state,
> > >          * it's in self refresh mode and needs to be fully
> > > disabled.
> > >          */
> > >         return old_state->active ||
> > > -              (old_state->self_refresh_active && !new_state-
> > > >enable) ||
> > > +              (old_state->self_refresh_active && !new_state-
> > > >active) ||
> > >                new_state->self_refresh_active;
> > >  }
> > >
> > > --
> > > 2.25.1
> > >
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-01-11 15:47 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-30  4:06 [PATCH] drm/atomic: Check new_crtc_state->active to determine if CRTC needs disable in self refresh mode Liu Ying
2022-01-07 19:53 ` Alex Deucher
2022-01-08  2:07   ` Liu Ying
2022-01-11 15:47     ` Alex Deucher

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).