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.syrj...@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.vet...@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.syrj...@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
