From: Daniel Vetter <daniel@ffwll.ch> To: David Herrmann <dh.herrmann@gmail.com> Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, Dave Airlie <airlied@gmail.com>, Jingoo Han <jg1.han@samsung.com>, Bryan Wu <cooloney@gmail.com>, Lee Jones <lee.jones@linaro.org>, Matthew Garrett <matthew.garrett@nebula.com>, Adam Jackson <ajax@redhat.com>, linux-kernel <linux-kernel@vger.kernel.org>, Daniel Vetter <daniel.vetter@ffwll.ch> Subject: Re: [PATCH RFC 4/4] drm: link connectors to backlight devices Date: Thu, 11 Sep 2014 15:06:24 +0200 [thread overview] Message-ID: <20140911130624.GJ15520@phenom.ffwll.local> (raw) In-Reply-To: <CANq1E4Q1RJCPACB0Cg7A79BB7Ly-zEUDik7PKyQ9oNuEizFPxA@mail.gmail.com> On Thu, Sep 11, 2014 at 02:22:55PM +0200, David Herrmann wrote: > Hi > > On Thu, Sep 11, 2014 at 8:48 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > Nice you skid around all the pitfalls and trapdoors, I guess we've all > > been rather blind ;-) > > > > Two high-level comments: > > - We also want to forward "bl_power". cros was totally not happy when we > > stopped treating brightness == 0 as completely off (it upsets some > > panels terminally, so there's a vbt lower limit). Instead we expose this > > now through the bl_power knob. > > > > While at it I think we should expose all the other backlight properties > > too (read-only ofc for actual/max_brightness). > > bl_power is easy to add. I guess v2 will have: > "BACKLIGHT-POWER" (range 0-4) > > actual-brightness is a bit more tricky. Currently, DRM caches property > values, so there is no read_property() hook. We'd have to add this. > But it'll be quite nasty as we have to call into the backlight driver. > So I think we want to run an async-interruptible worker on the > backlight, drop the locks in the ioctl and wait for the job to finish. > Not sure whether it's worth it.. maybe we can add this later. See Jani's reply - we probably don't need it, at least not in version 1. > > > - How does udev match on the drm connector name? They are not terribly > > stable atm, and if you reload your drm driver, or much more likely, have > > two gpus with two drm drivers they change. We probably should change the > > name allocation scheme to be per device instance instead of global > > first. Within a driver probe order is hopefully deterministic on a given > > platform, since even with super dynamic setups (based on dt/acpi) the > > firmware tables should change really. > > You can match on EDID attributes. Ok, so far this is pretty ugly as > the EDID property is binary. But we can add rather trivial udev > extensions to make EDID binary against text matching possible. Why EDID? This is purely about the drm connector name, e.g. if I have 2 gpus, both with an eDP connector (optimus, so just one panel) then the first driver gets eDP-1 as the name of it and the 2nd one eDP-2. Which is ok if both should control backlight brightness through the same driver, but a total mess if not just gpus get switched, but also backlight controllers. And if you reload you get then eDP-2 and eDP-3. Well at least in the past, that hilarity at least was fixed in commit b21e3afe2357c0f49348a5fb61247012bf8262ec Author: Ilia Mirkin <imirkin@alum.mit.edu> Date: Wed Aug 7 22:34:48 2013 -0400 drm: use ida to allocate connector id What I think we need to do is to make these ida allocators per-device, so that both drivers have an eDP-1 connector. Otherwise you need to either match both or do funny tricks like "the first eDP connector, no matter which one on this gpu". After all we can now support more than one eDP (and more than one LVDS since a long time actually). Or how exactly is the udev hw db supposed to match this stuff for special cases. In general we need to duplicate the existing logic from libbacklight, like Matthew suggested. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: David Herrmann <dh.herrmann@gmail.com> Cc: Matthew Garrett <matthew.garrett@nebula.com>, Bryan Wu <cooloney@gmail.com>, linux-kernel <linux-kernel@vger.kernel.org>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, Daniel Vetter <daniel.vetter@ffwll.ch>, Lee Jones <lee.jones@linaro.org> Subject: Re: [PATCH RFC 4/4] drm: link connectors to backlight devices Date: Thu, 11 Sep 2014 15:06:24 +0200 [thread overview] Message-ID: <20140911130624.GJ15520@phenom.ffwll.local> (raw) In-Reply-To: <CANq1E4Q1RJCPACB0Cg7A79BB7Ly-zEUDik7PKyQ9oNuEizFPxA@mail.gmail.com> On Thu, Sep 11, 2014 at 02:22:55PM +0200, David Herrmann wrote: > Hi > > On Thu, Sep 11, 2014 at 8:48 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > Nice you skid around all the pitfalls and trapdoors, I guess we've all > > been rather blind ;-) > > > > Two high-level comments: > > - We also want to forward "bl_power". cros was totally not happy when we > > stopped treating brightness == 0 as completely off (it upsets some > > panels terminally, so there's a vbt lower limit). Instead we expose this > > now through the bl_power knob. > > > > While at it I think we should expose all the other backlight properties > > too (read-only ofc for actual/max_brightness). > > bl_power is easy to add. I guess v2 will have: > "BACKLIGHT-POWER" (range 0-4) > > actual-brightness is a bit more tricky. Currently, DRM caches property > values, so there is no read_property() hook. We'd have to add this. > But it'll be quite nasty as we have to call into the backlight driver. > So I think we want to run an async-interruptible worker on the > backlight, drop the locks in the ioctl and wait for the job to finish. > Not sure whether it's worth it.. maybe we can add this later. See Jani's reply - we probably don't need it, at least not in version 1. > > > - How does udev match on the drm connector name? They are not terribly > > stable atm, and if you reload your drm driver, or much more likely, have > > two gpus with two drm drivers they change. We probably should change the > > name allocation scheme to be per device instance instead of global > > first. Within a driver probe order is hopefully deterministic on a given > > platform, since even with super dynamic setups (based on dt/acpi) the > > firmware tables should change really. > > You can match on EDID attributes. Ok, so far this is pretty ugly as > the EDID property is binary. But we can add rather trivial udev > extensions to make EDID binary against text matching possible. Why EDID? This is purely about the drm connector name, e.g. if I have 2 gpus, both with an eDP connector (optimus, so just one panel) then the first driver gets eDP-1 as the name of it and the 2nd one eDP-2. Which is ok if both should control backlight brightness through the same driver, but a total mess if not just gpus get switched, but also backlight controllers. And if you reload you get then eDP-2 and eDP-3. Well at least in the past, that hilarity at least was fixed in commit b21e3afe2357c0f49348a5fb61247012bf8262ec Author: Ilia Mirkin <imirkin@alum.mit.edu> Date: Wed Aug 7 22:34:48 2013 -0400 drm: use ida to allocate connector id What I think we need to do is to make these ida allocators per-device, so that both drivers have an eDP-1 connector. Otherwise you need to either match both or do funny tricks like "the first eDP connector, no matter which one on this gpu". After all we can now support more than one eDP (and more than one LVDS since a long time actually). Or how exactly is the udev hw db supposed to match this stuff for special cases. In general we need to duplicate the existing logic from libbacklight, like Matthew suggested. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
next prev parent reply other threads:[~2014-09-11 13:06 UTC|newest] Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-09-10 15:54 [PATCH RFC 0/4] Linking DRM Connectors to Backlight Devices David Herrmann 2014-09-10 15:54 ` David Herrmann 2014-09-10 15:54 ` [PATCH RFC 1/4] backlight: use static initializers David Herrmann 2014-09-10 15:54 ` David Herrmann 2014-09-11 8:59 ` Jani Nikula 2014-09-11 8:59 ` Jani Nikula 2014-09-10 15:54 ` [PATCH RFC 2/4] backlight: use spin-lock to protect device list David Herrmann 2014-09-10 15:54 ` David Herrmann 2014-09-11 9:00 ` Jani Nikula 2014-09-11 9:00 ` Jani Nikula 2014-09-10 15:54 ` [PATCH RFC 3/4] backlight: add kernel-internal backlight API David Herrmann 2014-09-11 11:10 ` Thierry Reding 2014-09-11 11:10 ` Thierry Reding 2014-09-11 11:14 ` David Herrmann 2014-09-11 11:14 ` David Herrmann 2014-09-11 11:21 ` Thierry Reding 2014-09-11 11:21 ` Thierry Reding 2014-09-10 15:54 ` [PATCH RFC 4/4] drm: link connectors to backlight devices David Herrmann 2014-09-10 15:54 ` David Herrmann 2014-09-11 6:48 ` Daniel Vetter 2014-09-11 6:48 ` Daniel Vetter 2014-09-11 12:22 ` David Herrmann 2014-09-11 13:06 ` Daniel Vetter [this message] 2014-09-11 13:06 ` Daniel Vetter 2014-09-11 16:07 ` David Herrmann 2014-09-11 12:46 ` Jani Nikula 2014-09-10 20:40 ` [PATCH RFC 0/4] Linking DRM Connectors to Backlight Devices Matthew Garrett 2014-09-10 20:40 ` Matthew Garrett 2014-09-11 12:48 ` David Herrmann 2014-09-11 12:48 ` David Herrmann 2016-10-24 13:08 ` [PATCH 0/6] Rebase of David Herrmann drm connector link to backlight device Marta Lofstedt 2016-10-24 13:08 ` [PATCH 1/6] backlight: use static initializers Marta Lofstedt 2016-10-24 13:08 ` [PATCH 2/6] backlight: use spin-lock to protect device list Marta Lofstedt 2016-10-24 13:08 ` [PATCH 3/6] backlight: add kernel-internal backlight API Marta Lofstedt 2016-10-24 13:08 ` [PATCH 4/6] drm: link connectors to backlight devices Marta Lofstedt 2016-10-24 13:08 ` [PATCH 5/6] i915: Use drm backlight Marta Lofstedt 2016-10-24 13:08 ` [PATCH 6/6] drm: drm_backlight use the connect value to set brightness property Marta Lofstedt 2016-10-24 14:33 ` [PATCH 0/6] Rebase of David Herrmann drm connector link to backlight device Daniel Vetter 2014-09-11 6:51 [PATCH RFC 4/4] drm: link connectors to backlight devices Matthew Garrett
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20140911130624.GJ15520@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=airlied@gmail.com \ --cc=ajax@redhat.com \ --cc=cooloney@gmail.com \ --cc=daniel.vetter@ffwll.ch \ --cc=dh.herrmann@gmail.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=jg1.han@samsung.com \ --cc=lee.jones@linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=matthew.garrett@nebula.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.