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@xxxxxxxxxxxxxxx> > > > > 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@xxxxxxxx> 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ä <ville.syrjala@xxxxxxxxxxxxxxx> > > --- > > 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
