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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 27 Sep 2017, Daniel Vetter <daniel@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@linux.intel.com>:
>> 
>> > On Wed, 27 Sep 2017, Karsten Wiese <fzuuzf@googlemail.com> wrote:
>> > > 2017-09-25 15:48 GMT+02:00 Jani Nikula <jani.nikula@linux.intel.com>:
>> > >
>> > >> On Tue, 19 Sep 2017, Karsten Wiese <fzuuzf@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.

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@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel