Re: [PATCH] backlight: add DDC/CI brightness driver

[Daniel Thompson <daniel.thompson@linaro.org>]

On 10/08/17 13:56, Miłosz Rachwał wrote:
> On 13.02.2017 13:12, Daniel Thompson wrote:
>> On 21/01/17 18:31, Miłosz Rachwał wrote:
>>> This patch contains driver for exposing VESA DDC/CI MCCS brightness
>>> settings via kernel backlight API. This allows to control monitor
>>> brightness using standard software which uses /sys/class/backlight/
>>> interface, same as for laptop backlight drivers.
>>> Because some monitors don't correctly implement DDC/CI standard module
>>> parameter allows to override maximum brightness value reported by
>>> monitor.
>>> This module doesn't support autodetection, so driver must be manually
>>> instantiated on monitor I²C bus under 0x37 address.
>> 
>> This driver is well written and very clear but I'm afraid I have a
>> few high level concerns about this driver.
>> 
>> Firstly, have you discussed this driver with the DRM developers?
>> DDC/CI is closely related to EDID (monitor usually implemented
>> both, same I2C bus, etc). It seems odd for this driver to be
>> standalone rather than integrated into the DRM connector model
>> (allowing it to probe automatically).
> 
> Yes, it should be somehow integrated into DRM EDID probing, but I
> haven't looked into that code yet. But I think that this driver
> should stay mostly unchanged, and DRM code should only do probing of
> this driver and pass fb_info* so that it is possible to check
> relation of framebuffer and backlight device via
> backlight_ops.check_fb, yes? Or whole driver should be moved into DRM
> code?

To be honest I was hoping you'd be able to point at discussion with the 
DRM developers!

My gut reaction is that this code is best implemented within the DRM 
code. I could probably be lobbied to change this view but not without 
discussion involving the DRM folks.


>> Secondly, it assumes the Luminance VCD is the best way to control
>> the backlight (ignoring both the current and legacy backlight
>> controls). I'm not totally sure that falling back to luminance is
>> sensible for a backlight control but it is certainly wrong to go
>> directly to it, ignoring the other VCPs.
> 
> Oops, I used VCD that worked on my monitor and forgotten about the
> others. It is probably necessary to query and parse capability string
> to get supported VCP codes. But anyway in MCCS specifiaction the only
> other code that seems revelant is 0x6B "backlight level: white", what
> are other "current and legacy backlight controls"?

0x6B is the current one and 0x13 "backlight control (legacy)" is the 
legacy one.


>> Finally, I'm also not sure about they way the maxbr module
>> parameter works because it makes a per-device configuration setting
>> and applies it system-wide.
> 
> maxbr is written to backlight_properties.max_brightness during
> probing, so it could be changed between device instantiations and
> each instance would get their own max_brightness value. Ugly
> solution, but I don't had any other idea as i2c probe function
> doesn't have any extra arguments. Though while that approach was
> doable with driver instantiatiated by userspace sysfs interface it
> won't be with automatic probing by DRM. I think alternative solution
> is passing quirk list with values for specific monitor models, or
> maybe just hardcoded quirk list.

My take is that the MCCS exhibits most of the common backlight problems 
that make it hard to hook up to a GUI (does 0 turn off the backlight 
completely or is it just the least bright settings? is the scale linear 
or logarithmic?). In fairness the current sysfs interfaces for the 
backlight system also exhibit this problems ;-).

Perhaps shelve this until we know whether the code can be properly 
integrated with DRM (IIUC that would also allow individual properties to 
exist).


Daniel.