[PATCH] drm: Call sysfs_notify after changing drm_connector::dpms

Karsten Wiese fzuuzf at googlemail.com
Wed Sep 27 15:03:31 UTC 2017 

2017-09-27 14:58 GMT+02:00 Jani Nikula <jani.nikula at linux.intel.com>:

> On Wed, 27 Sep 2017, Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Wed, Sep 27, 2017 at 12:20:21PM +0200, Karsten Wiese wrote:
> >> 2017-09-27 9:18 GMT+02:00 Jani Nikula <jani.nikula at linux.intel.com>:
> >>
> >> > On Wed, 27 Sep 2017, Karsten Wiese <fzuuzf at googlemail.com> wrote:
> >> > > 2017-09-25 15:48 GMT+02:00 Jani Nikula <jani.nikula at linux.intel.com
> >:
> >> > >
> >> > >> On Tue, 19 Sep 2017, Karsten Wiese <fzuuzf at googlemail.com> wrote:
> >> > >> > This makes poll work for the
> >> > >> > /sys/class/drm/cardX/connectorY/dpms attributes.
> >> > >>
> >> > >> I guess the question is, what are you trying to achieve? What is
> the
> >> > >> problem that this solves?
> >> > >>
> >> > > ​
> >> > > The patch enables cpu cycle less waiting for dpms flag changes.
> >> > >
> >> > > Here it lets a screen brightness ​setting daemon know which monitor
> to
> >> > > handle.  One of the attached screens gets confused if it is set just
> >> > > after being switched on, hence the daemon leaves it untouched until
> it
> >> > > has been active for some seconds.
> >> >
> >> > Screen brightness settings daemon?
> >> >
> >> Running on a laptop lacking an ambient light sensor employing it's
> webcam
> >> instead to measure ambient light and adjust monitors' brightnesses
> >> accordingly.
> >>
> >>
> >> What exactly do you mean by "if it is
> >> > set"? What interface are you using to change brightness?
> >>
> >> The external monitor's brightness is adjusted by DDC/CI via the
> /dev/i2c-x
> >> the monitor is attached to.
> >>  I there a saner interface to use?
> >
> > There's supposed to be a backlight property on the panel, exposed by the
> X
> > driver. If there's not, then that needs to be fixed/adjusted.
>
> For eDP/LVDS in Intel driver yes, for external displays not. For
> external displays DDC/CI is currently probably the best bet.
>
> However, DDC/CI is overall not very robust. I think we have prior bug
> reports about it, but it's never really been very high on our list of
> priorities.
>
> > There's also plans to directly link that up on the kernel side, but
> that's
> > a bit more involved due to how screwed up the backlight driver design on
> > linux is right now.
>
> It's going to take forever and a half before DDC/CI for external
> displays get that treatment, if ever.
>
> It still feels wrong to add sysfs notify support for just one of our
> properties, for just this one use case. In particular if the reason is
> to workaround bugs in another interface.
>

​Would you accept a patch supporting​ all relevant attributes?

BR,
Karsten

>
> BR,
> Jani.
>
>
> > -Daniel
> >
> >>
> >> What happens
> >> > when the display "gets confused"?
> >> >
> >> The monitor wrongly toggles sharpness if it receives the brightness
> >> adjusting​
> >> ​ DDC/CI soon after resuming from power saving state.
> >>
> >> >
> >> > My first instinct is that you're proposing a new kernel ABI to solve
> an
> >> > issue you shouldn't be solving in userspace to begin with.
> >>
> >> Calculating ambient light from pictures acquired via ​
> >> ​v4l​2 in kernel seamed wrong to me.
> >>
> >> Or that
> >> > perhaps the userspace should be doing this in cooperation with the drm
> >> > master, not standalone.
> >> >
> >> Is there a way to call into the drm-master (xscreensaver/xserver​ here​
> >> ​) ​with the call only returning if a monitor's power state changed?
> >>
> >> There is DPMSInfo, but it returns immediately rendering the daemon less
> >> efficient.
> >>
> >>
> >> BR,
> >>
> >> Karsten
> >>
> >> >
> >> > BR,
> >> > Jani.
> >> >
> >> > >
> >> > > Without the p​atch the daemon would have to actively read the dpms
> flag
> >> > > every once in a while.
> >> > >
> >> > >
> >> > >>
> >> > >> We have zero sysfs_notify in all of drm AFAICT.
> >> > >
> >> > >
> >> > > Yes I noticed too and looked for some dbus signal to listen to​ but
> >> > didn't
> >> > > find any.
> >> > >
> >> > > BR,
> >> > > Karsten
> >> > >
> >> > >>
> >> > >> BR,
> >> > >> Jani.
> >> > >>
> >> > >>
> >> > >> >
> >> > >> > Tested with i915 suspended by XScreenServer and
> >> > >> > suspend to RAM.
> >> > >> >
> >> > >> > Signed-off-by: Karsten Wiese <fzuuzf at googlemail.com>
> >> > >> > ---
> >> > >> >  drivers/gpu/drm/drm_atomic.c        | 4 ++++
> >> > >> >  drivers/gpu/drm/drm_atomic_helper.c | 6 +++++-
> >> > >> >  2 files changed, 9 insertions(+), 1 deletion(-)
> >> > >> >
> >> > >> > diff --git a/drivers/gpu/drm/drm_atomic.c
> >> > b/drivers/gpu/drm/drm_atomic.c
> >> > >> > index 2fd383d..b6fa87b 100644
> >> > >> > --- a/drivers/gpu/drm/drm_atomic.c
> >> > >> > +++ b/drivers/gpu/drm/drm_atomic.c
> >> > >> > @@ -1880,6 +1880,10 @@ int drm_atomic_connector_commit_
> dpms(struct
> >> > >> drm_atomic_state *state,
> >> > >> >  out:
> >> > >> >       if (ret != 0)
> >> > >> >               connector->dpms = old_mode;
> >> > >> > +     else
> >> > >> > +             if (connector->dpms != old_mode)
> >> > >> > +                     sysfs_notify(&connector->kdev->kobj, NULL,
> >> > >> "dpms");
> >> > >> > +
> >> > >> >       return ret;
> >> > >> >  }
> >> > >> >
> >> > >> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> >> > >> b/drivers/gpu/drm/drm_atomic_helper.c
> >> > >> > index 4e53aae..6198772 100644
> >> > >> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> > >> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> > >> > @@ -921,12 +921,16 @@ drm_atomic_helper_update_
> >> > legacy_modeset_state(struct
> >> > >> drm_device *dev,
> >> > >> >               crtc = new_conn_state->crtc;
> >> > >> >               if ((!crtc && old_conn_state->crtc) ||
> >> > >> >                   (crtc && drm_atomic_crtc_needs_modeset(
> >> > crtc->state)))
> >> > >> {
> >> > >> > -                     int mode = DRM_MODE_DPMS_OFF;
> >> > >> > +                     int old_mode, mode = DRM_MODE_DPMS_OFF;
> >> > >> >
> >> > >> >                       if (crtc && crtc->state->active)
> >> > >> >                               mode = DRM_MODE_DPMS_ON;
> >> > >> >
> >> > >> > +                     old_mode = connector->dpms;
> >> > >> >                       connector->dpms = mode;
> >> > >> > +                     if (old_mode != mode)
> >> > >> > +                             sysfs_notify(&connector->kdev-
> >kobj,
> >> > >> > +                                             NULL, "dpms");
> >> > >> >               }
> >> > >> >       }
> >> > >>
> >> > >> --
> >> > >> Jani Nikula, Intel Open Source Technology Center
> >> > >>
> >> >
> >> > --
> >> > Jani Nikula, Intel Open Source Technology Center
> >> >
> >
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Jani Nikula, Intel Open Source Technology Center
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20170927/500b5516/attachment.html>

More information about the dri-devel mailing list