Re: [PATCH] drm/atomic: Filter out redundant DPMS calls

[Simona Vetter <simona.vetter@ffwll.ch>]

On Wed, Feb 19, 2025 at 06:02:39PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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 <simona.vetter@ffwll.ch>

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ä <ville.syrjala@linux.intel.com>
> ---
>  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