drm/atomic: Filter out redundant DPMS calls

Commit Message

Ville Syrjälä Feb. 19, 2025, 4:02 p.m. UTC

From: Ville Syrjälä <[e-mail]>

Video players (eg. mpv) do periodic XResetScreenSaver() calls to
keep the screen on while the video playing. The modesetting ddx
plumbs these straight through into the kernel as DPMS setproperty
ioctls, without any filtering whatsoever. When implemented via
atomic these end up as full commits on the crtc, which leads to a
dropped frame every time XResetScreenSaver() is called.

Let's just filter out redundant DPMS property changes in the
kernel to avoid this issue.

Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Hamza Mahfooz Feb. 19, 2025, 5:06 p.m. UTC | #1

On Wed, Feb 19, 2025 at 06:02:39PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <[e-mail]>
> 
> Video players (eg. mpv) do periodic XResetScreenSaver() calls to
> keep the screen on while the video playing. The modesetting ddx
> plumbs these straight through into the kernel as DPMS setproperty
> ioctls, without any filtering whatsoever. When implemented via
> atomic these end up as full commits on the crtc, which leads to a
> dropped frame every time XResetScreenSaver() is called.
> 
> Let's just filter out redundant DPMS property changes in the
> kernel to avoid this issue.

Do you know if this has any impact on the DPMS timeout (as set by
DPMSSetTimeouts())?

> 
> Signed-off-by: Ville Syrjälä <[e-mail]>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 2765ba90ad8f..c2726af6698e 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -957,6 +957,10 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
>  
>  	if (mode != DRM_MODE_DPMS_ON)
>  		mode = DRM_MODE_DPMS_OFF;
> +
> +	if (connector->dpms == mode)
> +		goto out;
> +
>  	connector->dpms = mode;
>  
>  	crtc = connector->state->crtc;
> -- 
> 2.45.3

Ville Syrjälä Feb. 19, 2025, 7:10 p.m. UTC | #2

On Wed, Feb 19, 2025 at 12:06:20PM -0500, Hamza Mahfooz wrote:
> On Wed, Feb 19, 2025 at 06:02:39PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <[e-mail]>
> > 
> > Video players (eg. mpv) do periodic XResetScreenSaver() calls to
> > keep the screen on while the video playing. The modesetting ddx
> > plumbs these straight through into the kernel as DPMS setproperty
> > ioctls, without any filtering whatsoever. When implemented via
> > atomic these end up as full commits on the crtc, which leads to a
> > dropped frame every time XResetScreenSaver() is called.
> > 
> > Let's just filter out redundant DPMS property changes in the
> > kernel to avoid this issue.
> 
> Do you know if this has any impact on the DPMS timeout (as set by
> DPMSSetTimeouts())?

That's all in userspace.

> 
> > 
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 2765ba90ad8f..c2726af6698e 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -957,6 +957,10 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
> >  
> >  	if (mode != DRM_MODE_DPMS_ON)
> >  		mode = DRM_MODE_DPMS_OFF;
> > +
> > +	if (connector->dpms == mode)
> > +		goto out;
> > +
> >  	connector->dpms = mode;
> >  
> >  	crtc = connector->state->crtc;
> > -- 
> > 2.45.3

Simona Vetter Feb. 20, 2025, 9:53 a.m. UTC | #3

On Wed, Feb 19, 2025 at 06:02:39PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <[e-mail]>
> 
> Video players (eg. mpv) do periodic XResetScreenSaver() calls to
> keep the screen on while the video playing. The modesetting ddx
> plumbs these straight through into the kernel as DPMS setproperty
> ioctls, without any filtering whatsoever. When implemented via
> atomic these end up as full commits on the crtc, which leads to a
> dropped frame every time XResetScreenSaver() is called.

I think you should add here that it's just an empty commit, because we do
filter out redundant commits where crtc->active_changed does nothing.
Except we still run the entire machinery with timestamps and drm_event and
everything.

And I don't think it's worth to filter that out at the atomic level,
because it's really only legacy ioctl that had this "complete noop"
behaviour.

With the commit message augmented:

Reviewed-by: Simona Vetter <[e-mail]>

Might also be nice to have a igt for this? Plus also wondering whether we
should cc: stable it.

Cheers, Sima

> Let's just filter out redundant DPMS property changes in the
> kernel to avoid this issue.
> 
> Signed-off-by: Ville Syrjälä <[e-mail]>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 2765ba90ad8f..c2726af6698e 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -957,6 +957,10 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
>  
>  	if (mode != DRM_MODE_DPMS_ON)
>  		mode = DRM_MODE_DPMS_OFF;
> +
> +	if (connector->dpms == mode)
> +		goto out;
> +
>  	connector->dpms = mode;
>  
>  	crtc = connector->state->crtc;
> -- 
> 2.45.3
>

Simona Vetter Feb. 20, 2025, 10 a.m. UTC | #4

On Thu, Feb 20, 2025 at 10:53:57AM +0100, Simona Vetter wrote:
> On Wed, Feb 19, 2025 at 06:02:39PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <[e-mail]>
> > 
> > Video players (eg. mpv) do periodic XResetScreenSaver() calls to
> > keep the screen on while the video playing. The modesetting ddx
> > plumbs these straight through into the kernel as DPMS setproperty
> > ioctls, without any filtering whatsoever. When implemented via
> > atomic these end up as full commits on the crtc, which leads to a
> > dropped frame every time XResetScreenSaver() is called.
> 
> I think you should add here that it's just an empty commit, because we do
> filter out redundant commits where crtc->active_changed does nothing.
> Except we still run the entire machinery with timestamps and drm_event and
> everything.
> 
> And I don't think it's worth to filter that out at the atomic level,
> because it's really only legacy ioctl that had this "complete noop"
> behaviour.
> 
> With the commit message augmented:
> 
> Reviewed-by: Simona Vetter <[e-mail]>

Ok, one more thing: Please also augment the dpms property uapi doc text
with a note that we make this guarantee. Otherwise this feels a bit too
much opaque magic. Maybe even a one-liner comment in the code that this is
uapi?
-Sima

> 
> Might also be nice to have a igt for this? Plus also wondering whether we
> should cc: stable it.
> 
> Cheers, Sima
> 
> > Let's just filter out redundant DPMS property changes in the
> > kernel to avoid this issue.
> > 
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 2765ba90ad8f..c2726af6698e 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -957,6 +957,10 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
> >  
> >  	if (mode != DRM_MODE_DPMS_ON)
> >  		mode = DRM_MODE_DPMS_OFF;
> > +
> > +	if (connector->dpms == mode)
> > +		goto out;
> > +
> >  	connector->dpms = mode;
> >  
> >  	crtc = connector->state->crtc;
> > -- 
> > 2.45.3
> > 
> 
> -- 
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Ville Syrjälä Feb. 20, 2025, 3:27 p.m. UTC | #5

On Thu, Feb 20, 2025 at 11:00:28AM +0100, Simona Vetter wrote:
> On Thu, Feb 20, 2025 at 10:53:57AM +0100, Simona Vetter wrote:
> > On Wed, Feb 19, 2025 at 06:02:39PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <[e-mail]>
> > > 
> > > Video players (eg. mpv) do periodic XResetScreenSaver() calls to
> > > keep the screen on while the video playing. The modesetting ddx
> > > plumbs these straight through into the kernel as DPMS setproperty
> > > ioctls, without any filtering whatsoever. When implemented via
> > > atomic these end up as full commits on the crtc, which leads to a
> > > dropped frame every time XResetScreenSaver() is called.
> > 
> > I think you should add here that it's just an empty commit, because we do
> > filter out redundant commits where crtc->active_changed does nothing.
> > Except we still run the entire machinery with timestamps and drm_event and
> > everything.

Yeah, it'll take at least one frame. And it's a blocking ioctl as well.

> > 
> > And I don't think it's worth to filter that out at the atomic level,
> > because it's really only legacy ioctl that had this "complete noop"
> > behaviour.

Yep, I think we can expect atomic userspace to do better.
Oh, and you can't even set the DPMS property via the atomic uapi
directly.

> > 
> > With the commit message augmented:
> > 
> > Reviewed-by: Simona Vetter <[e-mail]>
> 
> Ok, one more thing: Please also augment the dpms property uapi doc text
> with a note that we make this guarantee. Otherwise this feels a bit too
> much opaque magic. Maybe even a one-liner comment in the code that this is
> uapi?

Something like this perhaps?
+ *     On atomic drivers any DPMS setproperty ioctl where the value does not
+ *     change is completely skipped, otherwise an atomic commit will occur.
+ *     On legacy drivers the exact behavior is driver specific.

> -Sima
> 
> > 
> > Might also be nice to have a igt for this? Plus also wondering whether we
> > should cc: stable it.
> > 
> > Cheers, Sima
> > 
> > > Let's just filter out redundant DPMS property changes in the
> > > kernel to avoid this issue.
> > > 
> > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index 2765ba90ad8f..c2726af6698e 100644
> > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > @@ -957,6 +957,10 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
> > >  
> > >  	if (mode != DRM_MODE_DPMS_ON)
> > >  		mode = DRM_MODE_DPMS_OFF;
> > > +
> > > +	if (connector->dpms == mode)
> > > +		goto out;
> > > +
> > >  	connector->dpms = mode;
> > >  
> > >  	crtc = connector->state->crtc;
> > > -- 
> > > 2.45.3
> > > 
> > 
> > -- 
> > Simona Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Patch

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 2765ba90ad8f..c2726af6698e 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -957,6 +957,10 @@  int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
 
 	if (mode != DRM_MODE_DPMS_ON)
 		mode = DRM_MODE_DPMS_OFF;
+
+	if (connector->dpms == mode)
+		goto out;
+
 	connector->dpms = mode;
 
 	crtc = connector->state->crtc;