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

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

On Thu, Feb 20, 2025 at 05:27:10PM +0200, Ville Syrjälä wrote:
> 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ä <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.
> 
> 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 <simona.vetter@ffwll.ch>
> > 
> > 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.

Perfect!
-Sima

> 
> > -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ä <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
> > 
> > -- 
> > Simona Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch