Re: [PATCH v10 0/3] drivers: ddcci: add drivers for DDCCI

[Hans de Goede]

Hi Yusuf,

On 4/4/22 01:08, Yusuf Khan wrote:
> This patch adds the DDCCI driver by Christoph Grenz into the kernel.
> The original gitlab page is loacted at https://gitlab.com/ddcci-driv
> er-linux/ddcci-driver-linux/-/tree/master.
> 
> DDC/CI is a control protocol for monitor settings supported by most
> monitors since about 2005. A chardev and sysfs interface is provided.
> A backlight driver using DDCCI is also provided in the seccond patch.
> 
> Signed-off-by: Yusuf Khan <yusisamerican@xxxxxxxxx>
> Signed-off-by: Christoph Grenz <christophg+lkml@xxxxxxxxxxxxx>

Thank you for your submission of this patch series.

I must say that I'm a bit surprised that this series was NOT
also send to the drm/kms subsystem maintainers and mailinglists
since this deals with monitors and thus is highly relevant for
those folks. Luckily I saw an article about this at Phoronix
(you write in the changelog that you believe that there is
no interaction with the DRM/KMS subsystems but that is wrong).

One very important thing which I'm missing in this cover-letter
is why you want to have this in the kernel at all? There are
already various tools which do DDCCI from userspace just fine.

I guess you may be interested in offering /sys/class/backlight
functionality for external monitors. That is actually something
which I'm interested in too, but it is not that simple.

The current /sys/class/backlight interface does not offer a
way for userspace to map a /sys/class/backlight device to
a specific display-output / monitor. So userspace currently
assumes that there is only 1 (1 valid) /sys/class/backlight
device and that that *always* belongs to the internal LCD
panel of a laptop.

So just adding /sys/class/backlight device(s) for internal
monitors without addressing the short-comings of the existing
userspace interface is simply not possible because it would
break existing userspace which is something which is not
allowed.

So NACK from me for the backlight part at least and without
that, I really see no reason to do this in the kernel rather
then userspace.

I've recently been discussing this with a colleague of mine,
Sebastian Wick and as a result of that I'm giving a talk
with a proposal for a better userspace API for this at
kernel-recipes:
https://kernel-recipes.org/en/2022/talks/new-userspace-api-for-display-panel-brightness-control/

I hope to start working on a RFC patch series for this soon.

IMHO merging this series should be put on hold until we
have a better idea of how we want to solve the userspace
API challenges, esp. the monitor <-> /sys/class/backlight
mapping problem.

Regards,

Hans



p.s.

This is not the first time this has come up:

https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@xxxxxxxxxxxxxxx/
https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/



> ---
> v2: Fix typos.
> 
> v3: Add documentation, move files around, replace semaphores with
> mutexes, and replaced <asm-generic/fcntl.h> with <linux/fcntl.h>.
> "imirkin"(which due to his involvement in the dri-devel irc channel
> I cant help but assume to be a DRM developer) said that the DDC/CI
> bus does not intefere with the buses that DRM is involved with.
> 
> v4: Move some documentation, fix grammer mistakes, remove usages of
> likely(), and clarify some documentation.
> 
> v5: Fix grammer mistakes, remove usages of likely(), and clarify
> some documentation.
> 
> v6: Change contact information to reference Christoph Grenz.
> 
> v7: Remove all instances of the unlikely() macro.
> 
> v8: Modify documentation to provide updated date and kernel
> documentation, fix SPDX lines, use isalpha instead of redefining
> logic, change maximum amount of bytes that can be written to be
> conformant with DDC/CI specification, prevent userspace from holding
> locks with the open file descriptor, remove ddcci_cdev_seek, dont
> refine sysfs_emit() logic, use EXPORT_SYMBOL_GPL instead of
> EXPORT_SYMBOL, remove ddcci_device_remove_void, remove module
> paramaters and version, and split into 2 patches.
> 
> v9: Fix IS_ANY_ID matching for compilers and archs where char is
> unsigned char and use the cannonical patch format.
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> 
> v10: Change patch title to "drivers: ddcci: add drivers for DDCCI
> and change" and change patch descriptions to add more detailed
> explanations of function.
> 
> Patch 1: Add the main DDCCI component.
> 
> Patch 2: Add the backlight driver that utilizes the DDCCI driver.
> 
> Patch 3: Add documentation for the DDCCI drivers.
> 
> Yusuf Khan (3):
>   drivers: ddcci: add drivers for DDCCI
>   drivers: ddcci: add drivers for DDCCI
>   drivers: ddcci: add drivers for DDCCI
> 
>  Documentation/ABI/testing/sysfs-driver-ddcci |   57 +
>  Documentation/driver-api/ddcci.rst           |  122 ++
>  drivers/char/Kconfig                         |   11 +
>  drivers/char/Makefile                        |    1 +
>  drivers/char/ddcci.c                         | 1805 ++++++++++++++++++
>  drivers/video/backlight/Kconfig              |   11 +
>  drivers/video/backlight/Makefile             |    1 +
>  drivers/video/backlight/ddcci-backlight.c    |  411 ++++
>  include/linux/ddcci.h                        |  163 ++
>  9 files changed, 2582 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-ddcci
>  create mode 100644 Documentation/driver-api/ddcci.rst
>  create mode 100644 drivers/char/ddcci.c
>  create mode 100644 drivers/video/backlight/ddcci-backlight.c
>  create mode 100644 include/linux/ddcci.h
>