2019-06-25 12:04:41

by Andrzej Pietrasiewicz

[permalink] [raw]
Subject: [PATCH 0/2] Associate ddc adapters with connectors

It is difficult for a user to know which of the i2c adapters is for which
drm connector. This series addresses this problem.

The idea is to have a symbolic link in connector's sysfs directory, e.g.:

ls -l /sys/class/drm/card0-HDMI-A-1/i2c-2
lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/i2c-2 \
-> ../../../../soc/13880000.i2c/i2c-2

The user then knows that their card0-HDMI-A-1 uses i2c-2 and can e.g. run
ddcutil:

ddcutil -b 2 getvcp 0x10
VCP code 0x10 (Brightness ): current value = 90, max value = 100

The first patch in the series adds struct i2c_adapter pointer to struct
drm_connector. If the field is used by a particular driver, then an
appropriate symbolic link is created by the generic code, which is also added
by this patch.

The second patch is an example of how to convert a driver to this new scheme.

Andrzej Pietrasiewicz (2):
drm: Include ddc adapter pointer in struct drm_connector
drm/exynos: Provide ddc symlink in connector's sysfs

drivers/gpu/drm/drm_sysfs.c | 9 +++++++++
drivers/gpu/drm/exynos/exynos_hdmi.c | 11 +++++------
include/drm/drm_connector.h | 11 +++++++++++
3 files changed, 25 insertions(+), 6 deletions(-)

--
2.17.1


2019-06-25 12:05:03

by Andrzej Pietrasiewicz

[permalink] [raw]
Subject: [PATCH 2/2] drm/exynos: Provide ddc symlink in connector's sysfs

Switch to using the ddc provided by the generic connector.

Signed-off-by: Andrzej Pietrasiewicz <[email protected]>
---
drivers/gpu/drm/exynos/exynos_hdmi.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 894a99793633..6816e37861b7 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -126,7 +126,6 @@ struct hdmi_context {
void __iomem *regs;
void __iomem *regs_hdmiphy;
struct i2c_client *hdmiphy_port;
- struct i2c_adapter *ddc_adpt;
struct gpio_desc *hpd_gpio;
int irq;
struct regmap *pmureg;
@@ -872,10 +871,10 @@ static int hdmi_get_modes(struct drm_connector *connector)
struct edid *edid;
int ret;

- if (!hdata->ddc_adpt)
+ if (!connector->ddc)
return -ENODEV;

- edid = drm_get_edid(connector, hdata->ddc_adpt);
+ edid = drm_get_edid(connector, connector->ddc);
if (!edid)
return -ENODEV;

@@ -1893,7 +1892,7 @@ static int hdmi_get_ddc_adapter(struct hdmi_context *hdata)
return -EPROBE_DEFER;
}

- hdata->ddc_adpt = adpt;
+ hdata->connector.ddc = adpt;

return 0;
}
@@ -2045,7 +2044,7 @@ static int hdmi_probe(struct platform_device *pdev)
if (hdata->regs_hdmiphy)
iounmap(hdata->regs_hdmiphy);
err_ddc:
- put_device(&hdata->ddc_adpt->dev);
+ put_device(&hdata->connector.ddc->dev);

return ret;
}
@@ -2072,7 +2071,7 @@ static int hdmi_remove(struct platform_device *pdev)
if (hdata->regs_hdmiphy)
iounmap(hdata->regs_hdmiphy);

- put_device(&hdata->ddc_adpt->dev);
+ put_device(&hdata->connector.ddc->dev);

mutex_destroy(&hdata->mutex);

--
2.17.1

2019-06-25 12:05:29

by Andrzej Pietrasiewicz

[permalink] [raw]
Subject: [PATCH 1/2] drm: Include ddc adapter pointer in struct drm_connector

Add generic code which creates symbolic links in sysfs, pointing to ddc
interface used by a particular video output. For example:

ls -l /sys/class/drm/card0-HDMI-A-1/i2c-2
lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/i2c-2 \
-> ../../../../soc/13880000.i2c/i2c-2

This makes it easy for user to associate a display with its ddc adapter
and use e.g. ddcutil to control the chosen monitor.

This patch adds an i2c_adapter pointer to struct drm_connector. Particular
drivers can then use it instead of using their own private instance. If a
connector contains a ddc, then create a symbolic link in sysfs.

Signed-off-by: Andrzej Pietrasiewicz <[email protected]>
---
drivers/gpu/drm/drm_sysfs.c | 9 +++++++++
include/drm/drm_connector.h | 11 +++++++++++
2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index ad10810bc972..627f8ebfc87a 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -294,6 +294,10 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
/* Let userspace know we have a new connector */
drm_sysfs_hotplug_event(dev);

+ if (connector->ddc)
+ return sysfs_create_link(&connector->kdev->kobj,
+ &connector->ddc->dev.kobj,
+ connector->ddc->dev.kobj.name);
return 0;
}

@@ -301,6 +305,11 @@ void drm_sysfs_connector_remove(struct drm_connector *connector)
{
if (!connector->kdev)
return;
+
+ if (connector->ddc)
+ sysfs_remove_link(&connector->kdev->kobj,
+ connector->ddc->dev.kobj.name);
+
DRM_DEBUG("removing \"%s\" from sysfs\n",
connector->name);

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index ca745d9feaf5..1ad3d1d54ba7 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -23,6 +23,7 @@
#ifndef __DRM_CONNECTOR_H__
#define __DRM_CONNECTOR_H__

+#include <linux/i2c.h>
#include <linux/list.h>
#include <linux/llist.h>
#include <linux/ctype.h>
@@ -1308,6 +1309,16 @@ struct drm_connector {
* [0]: progressive, [1]: interlaced
*/
int audio_latency[2];
+
+ /**
+ * @ddc: associated ddc adapter.
+ * A connector usually has its associated ddc adapter. If a driver uses
+ * this field, then an appropriate symbolic link is created in connector
+ * sysfs directory to make it easy for the user to tell which i2c
+ * adapter is for a particular display.
+ */
+ struct i2c_adapter *ddc;
+
/**
* @null_edid_counter: track sinks that give us all zeros for the EDID.
* Needed to workaround some HW bugs where we get all 0s
--
2.17.1

2019-06-25 12:09:58

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH 0/2] Associate ddc adapters with connectors

On Tue, Jun 25, 2019 at 11:46:34AM +0200, Andrzej Pietrasiewicz wrote:
> It is difficult for a user to know which of the i2c adapters is for which
> drm connector. This series addresses this problem.
>
> The idea is to have a symbolic link in connector's sysfs directory, e.g.:
>
> ls -l /sys/class/drm/card0-HDMI-A-1/i2c-2
> lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/i2c-2 \
> -> ../../../../soc/13880000.i2c/i2c-2

Don't you want the symlink name to be "i2c" or something fixed, rather
than the name of the i2c adapter? Otherwise, you seem to be encumbering
userspace with searching the directory to try and find the symlink.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

2019-06-25 12:14:05

by Andrzej Pietrasiewicz

[permalink] [raw]
Subject: Re: [PATCH 0/2] Associate ddc adapters with connectors

Hi Russell,

W dniu 25.06.2019 o 12:03, Russell King - ARM Linux admin pisze:
> On Tue, Jun 25, 2019 at 11:46:34AM +0200, Andrzej Pietrasiewicz wrote:
>> It is difficult for a user to know which of the i2c adapters is for which
>> drm connector. This series addresses this problem.
>>
>> The idea is to have a symbolic link in connector's sysfs directory, e.g.:
>>
>> ls -l /sys/class/drm/card0-HDMI-A-1/i2c-2
>> lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/i2c-2 \
>> -> ../../../../soc/13880000.i2c/i2c-2
>
> Don't you want the symlink name to be "i2c" or something fixed, rather
> than the name of the i2c adapter? Otherwise, you seem to be encumbering
> userspace with searching the directory to try and find the symlink.
>

Thank you for your comment. So you imagine something on the lines of:

lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \
-> ../../../../soc/13880000.i2c/i2c-2

?

This makes sense to me, I will send a v2.

Andrzej

2019-06-25 12:16:05

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH 0/2] Associate ddc adapters with connectors

On Tue, Jun 25, 2019 at 12:14:27PM +0200, Andrzej Pietrasiewicz wrote:
> Hi Russell,
>
> W dniu 25.06.2019 o?12:03, Russell King - ARM Linux admin pisze:
> > On Tue, Jun 25, 2019 at 11:46:34AM +0200, Andrzej Pietrasiewicz wrote:
> > > It is difficult for a user to know which of the i2c adapters is for which
> > > drm connector. This series addresses this problem.
> > >
> > > The idea is to have a symbolic link in connector's sysfs directory, e.g.:
> > >
> > > ls -l /sys/class/drm/card0-HDMI-A-1/i2c-2
> > > lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/i2c-2 \
> > > -> ../../../../soc/13880000.i2c/i2c-2
> >
> > Don't you want the symlink name to be "i2c" or something fixed, rather
> > than the name of the i2c adapter? Otherwise, you seem to be encumbering
> > userspace with searching the directory to try and find the symlink.
> >
>
> Thank you for your comment. So you imagine something on the lines of:
>
> lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \
> -> ../../../../soc/13880000.i2c/i2c-2
>
> ?

Exactly.

> This makes sense to me, I will send a v2.

Thanks.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

2019-06-25 12:16:51

by Andrzej Pietrasiewicz

[permalink] [raw]
Subject: [PATCH v2 0/2] Associate ddc adapters with connectors

It is difficult for a user to know which of the i2c adapters is for which
drm connector. This series addresses this problem.

The idea is to have a symbolic link in connector's sysfs directory, e.g.:

ls -l /sys/class/drm/card0-HDMI-A-1/ddc
lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \
-> ../../../../soc/13880000.i2c/i2c-2

The user then knows that their card0-HDMI-A-1 uses i2c-2 and can e.g. run
ddcutil:

ddcutil -b 2 getvcp 0x10
VCP code 0x10 (Brightness ): current value = 90, max value = 100

The first patch in the series adds struct i2c_adapter pointer to struct
drm_connector. If the field is used by a particular driver, then an
appropriate symbolic link is created by the generic code, which is also added
by this patch.

The second patch is an example of how to convert a driver to this new scheme.

v1..v2:

- used fixed name "ddc" for the symbolic link in order to make it easy for
userspace to find the i2c adapter

Andrzej Pietrasiewicz (2):
drm: Include ddc adapter pointer in struct drm_connector
drm/exynos: Provide ddc symlink in connector's sysfs

drivers/gpu/drm/drm_sysfs.c | 7 +++++++
drivers/gpu/drm/exynos/exynos_hdmi.c | 11 +++++------
include/drm/drm_connector.h | 11 +++++++++++
3 files changed, 23 insertions(+), 6 deletions(-)

--
2.17.1

2019-06-25 12:19:21

by Andrzej Pietrasiewicz

[permalink] [raw]
Subject: [PATCH v2 2/2] drm/exynos: Provide ddc symlink in connector's sysfs

Switch to using the ddc provided by the generic connector.

Signed-off-by: Andrzej Pietrasiewicz <[email protected]>
---
drivers/gpu/drm/exynos/exynos_hdmi.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 894a99793633..6816e37861b7 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -126,7 +126,6 @@ struct hdmi_context {
void __iomem *regs;
void __iomem *regs_hdmiphy;
struct i2c_client *hdmiphy_port;
- struct i2c_adapter *ddc_adpt;
struct gpio_desc *hpd_gpio;
int irq;
struct regmap *pmureg;
@@ -872,10 +871,10 @@ static int hdmi_get_modes(struct drm_connector *connector)
struct edid *edid;
int ret;

- if (!hdata->ddc_adpt)
+ if (!connector->ddc)
return -ENODEV;

- edid = drm_get_edid(connector, hdata->ddc_adpt);
+ edid = drm_get_edid(connector, connector->ddc);
if (!edid)
return -ENODEV;

@@ -1893,7 +1892,7 @@ static int hdmi_get_ddc_adapter(struct hdmi_context *hdata)
return -EPROBE_DEFER;
}

- hdata->ddc_adpt = adpt;
+ hdata->connector.ddc = adpt;

return 0;
}
@@ -2045,7 +2044,7 @@ static int hdmi_probe(struct platform_device *pdev)
if (hdata->regs_hdmiphy)
iounmap(hdata->regs_hdmiphy);
err_ddc:
- put_device(&hdata->ddc_adpt->dev);
+ put_device(&hdata->connector.ddc->dev);

return ret;
}
@@ -2072,7 +2071,7 @@ static int hdmi_remove(struct platform_device *pdev)
if (hdata->regs_hdmiphy)
iounmap(hdata->regs_hdmiphy);

- put_device(&hdata->ddc_adpt->dev);
+ put_device(&hdata->connector.ddc->dev);

mutex_destroy(&hdata->mutex);

--
2.17.1

2019-06-25 12:20:02

by Andrzej Pietrasiewicz

[permalink] [raw]
Subject: [PATCH v2 1/2] drm: Include ddc adapter pointer in struct drm_connector

Add generic code which creates symbolic links in sysfs, pointing to ddc
interface used by a particular video output. For example:

ls -l /sys/class/drm/card0-HDMI-A-1/ddc
lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \
-> ../../../../soc/13880000.i2c/i2c-2

This makes it easy for user to associate a display with its ddc adapter
and use e.g. ddcutil to control the chosen monitor.

This patch adds an i2c_adapter pointer to struct drm_connector. Particular
drivers can then use it instead of using their own private instance. If a
connector contains a ddc, then create a symbolic link in sysfs.

Signed-off-by: Andrzej Pietrasiewicz <[email protected]>
---
drivers/gpu/drm/drm_sysfs.c | 7 +++++++
include/drm/drm_connector.h | 11 +++++++++++
2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index ad10810bc972..26d359b39785 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -294,6 +294,9 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
/* Let userspace know we have a new connector */
drm_sysfs_hotplug_event(dev);

+ if (connector->ddc)
+ return sysfs_create_link(&connector->kdev->kobj,
+ &connector->ddc->dev.kobj, "ddc");
return 0;
}

@@ -301,6 +304,10 @@ void drm_sysfs_connector_remove(struct drm_connector *connector)
{
if (!connector->kdev)
return;
+
+ if (connector->ddc)
+ sysfs_remove_link(&connector->kdev->kobj, "ddc");
+
DRM_DEBUG("removing \"%s\" from sysfs\n",
connector->name);

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index ca745d9feaf5..1ad3d1d54ba7 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -23,6 +23,7 @@
#ifndef __DRM_CONNECTOR_H__
#define __DRM_CONNECTOR_H__

+#include <linux/i2c.h>
#include <linux/list.h>
#include <linux/llist.h>
#include <linux/ctype.h>
@@ -1308,6 +1309,16 @@ struct drm_connector {
* [0]: progressive, [1]: interlaced
*/
int audio_latency[2];
+
+ /**
+ * @ddc: associated ddc adapter.
+ * A connector usually has its associated ddc adapter. If a driver uses
+ * this field, then an appropriate symbolic link is created in connector
+ * sysfs directory to make it easy for the user to tell which i2c
+ * adapter is for a particular display.
+ */
+ struct i2c_adapter *ddc;
+
/**
* @null_edid_counter: track sinks that give us all zeros for the EDID.
* Needed to workaround some HW bugs where we get all 0s
--
2.17.1

2019-06-25 13:44:02

by Emil Velikov

[permalink] [raw]
Subject: Re: [PATCH 0/2] Associate ddc adapters with connectors

On 2019/06/25, Andrzej Pietrasiewicz wrote:
> Hi Russell,
>
> W dniu 25.06.2019 o?12:03, Russell King - ARM Linux admin pisze:
> > On Tue, Jun 25, 2019 at 11:46:34AM +0200, Andrzej Pietrasiewicz wrote:
> > > It is difficult for a user to know which of the i2c adapters is for which
> > > drm connector. This series addresses this problem.
> > >
> > > The idea is to have a symbolic link in connector's sysfs directory, e.g.:
> > >
> > > ls -l /sys/class/drm/card0-HDMI-A-1/i2c-2
> > > lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/i2c-2 \
> > > -> ../../../../soc/13880000.i2c/i2c-2
> >
> > Don't you want the symlink name to be "i2c" or something fixed, rather
> > than the name of the i2c adapter? Otherwise, you seem to be encumbering
> > userspace with searching the directory to try and find the symlink.
> >
>
> Thank you for your comment. So you imagine something on the lines of:
>
> lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \
> -> ../../../../soc/13880000.i2c/i2c-2
>
> ?
>
Fwiw my Intel machine lists a number of i2c devices:
/sys/class/drm/card0-DP-1/i2c-6
/sys/class/drm/card0-DP-2/i2c-7
/sys/class/drm/card0-eDP-1/i2c-5

Note: I haven't looked _if_ they relate to ones you're proposing here.

One thing worth mentioning is, the ones I've seen are not symlinks to
another sysfs entries. And there aren't any i2c nodes in /dev ...

Just a random food for thought :-)

HTH
Emil




2019-06-25 14:10:12

by Daniel Vetter

[permalink] [raw]
Subject: Re: [PATCH 0/2] Associate ddc adapters with connectors

On Tue, Jun 25, 2019 at 02:36:39PM +0100, Emil Velikov wrote:
> On 2019/06/25, Andrzej Pietrasiewicz wrote:
> > Hi Russell,
> >
> > W dniu 25.06.2019 o?12:03, Russell King - ARM Linux admin pisze:
> > > On Tue, Jun 25, 2019 at 11:46:34AM +0200, Andrzej Pietrasiewicz wrote:
> > > > It is difficult for a user to know which of the i2c adapters is for which
> > > > drm connector. This series addresses this problem.
> > > >
> > > > The idea is to have a symbolic link in connector's sysfs directory, e.g.:
> > > >
> > > > ls -l /sys/class/drm/card0-HDMI-A-1/i2c-2
> > > > lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/i2c-2 \
> > > > -> ../../../../soc/13880000.i2c/i2c-2
> > >
> > > Don't you want the symlink name to be "i2c" or something fixed, rather
> > > than the name of the i2c adapter? Otherwise, you seem to be encumbering
> > > userspace with searching the directory to try and find the symlink.
> > >
> >
> > Thank you for your comment. So you imagine something on the lines of:
> >
> > lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \
> > -> ../../../../soc/13880000.i2c/i2c-2
> >
> > ?
> >
> Fwiw my Intel machine lists a number of i2c devices:
> /sys/class/drm/card0-DP-1/i2c-6
> /sys/class/drm/card0-DP-2/i2c-7
> /sys/class/drm/card0-eDP-1/i2c-5
>
> Note: I haven't looked _if_ they relate to ones you're proposing here.
>
> One thing worth mentioning is, the ones I've seen are not symlinks to
> another sysfs entries. And there aren't any i2c nodes in /dev ...
>
> Just a random food for thought :-)

Those are the i2c-over-dp-aux controllers. I think we want to list these
too.

Btw to make this more useful maybe some default implementations for
get_modes which automatically dtrt, as a helper? Probably could use that
to squash quite a bit of boilerplate.

Otherwise I like this. Biggest problem I'm seeing here is rolling this out
everywhere, this is a lot of work. And without widespread adoptions it's
not terribly useful for userspace.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2019-06-25 14:11:22

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH 0/2] Associate ddc adapters with connectors

On Tue, Jun 25, 2019 at 04:07:55PM +0200, Daniel Vetter wrote:
> Otherwise I like this. Biggest problem I'm seeing here is rolling this out
> everywhere, this is a lot of work. And without widespread adoptions it's
> not terribly useful for userspace.

There will be cases where it's not possible, because the I2C bus is
hidden behind a chip that doesn't give you direct access to the DDC
bus.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

2019-06-25 14:21:18

by Daniel Vetter

[permalink] [raw]
Subject: Re: [PATCH 0/2] Associate ddc adapters with connectors

On Tue, Jun 25, 2019 at 03:10:32PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Jun 25, 2019 at 04:07:55PM +0200, Daniel Vetter wrote:
> > Otherwise I like this. Biggest problem I'm seeing here is rolling this out
> > everywhere, this is a lot of work. And without widespread adoptions it's
> > not terribly useful for userspace.
>
> There will be cases where it's not possible, because the I2C bus is
> hidden behind a chip that doesn't give you direct access to the DDC
> bus.

Oh sure, plus lots of connectors where there's just not ddc bus at all.
But if we only roll this out for a handful of drivers it's also not great,
that's what I meant. Looking at

$ git grep drm_do_get_edid

there's only very few drivers where the ddc bus is hidden. There's a lot
more where it's not, and I think a big series to tackle those would serve
extremely well to make a case for this sysfs link.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2019-06-25 14:22:20

by Daniel Vetter

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] drm: Include ddc adapter pointer in struct drm_connector

On Tue, Jun 25, 2019 at 12:24:40PM +0200, Andrzej Pietrasiewicz wrote:
> Add generic code which creates symbolic links in sysfs, pointing to ddc
> interface used by a particular video output. For example:
>
> ls -l /sys/class/drm/card0-HDMI-A-1/ddc
> lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \
> -> ../../../../soc/13880000.i2c/i2c-2
>
> This makes it easy for user to associate a display with its ddc adapter
> and use e.g. ddcutil to control the chosen monitor.
>
> This patch adds an i2c_adapter pointer to struct drm_connector. Particular
> drivers can then use it instead of using their own private instance. If a
> connector contains a ddc, then create a symbolic link in sysfs.
>
> Signed-off-by: Andrzej Pietrasiewicz <[email protected]>

Acked-by: Daniel Vetter <[email protected]>

As mentioned in the other subthread, I think the important bit to sell
this is rolling it out to as many drivers as feasible and collecting all
the acks from driver maintainers.
-Daniel
> ---
> drivers/gpu/drm/drm_sysfs.c | 7 +++++++
> include/drm/drm_connector.h | 11 +++++++++++
> 2 files changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index ad10810bc972..26d359b39785 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -294,6 +294,9 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
> /* Let userspace know we have a new connector */
> drm_sysfs_hotplug_event(dev);
>
> + if (connector->ddc)
> + return sysfs_create_link(&connector->kdev->kobj,
> + &connector->ddc->dev.kobj, "ddc");
> return 0;
> }
>
> @@ -301,6 +304,10 @@ void drm_sysfs_connector_remove(struct drm_connector *connector)
> {
> if (!connector->kdev)
> return;
> +
> + if (connector->ddc)
> + sysfs_remove_link(&connector->kdev->kobj, "ddc");
> +
> DRM_DEBUG("removing \"%s\" from sysfs\n",
> connector->name);
>
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index ca745d9feaf5..1ad3d1d54ba7 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -23,6 +23,7 @@
> #ifndef __DRM_CONNECTOR_H__
> #define __DRM_CONNECTOR_H__
>
> +#include <linux/i2c.h>
> #include <linux/list.h>
> #include <linux/llist.h>
> #include <linux/ctype.h>
> @@ -1308,6 +1309,16 @@ struct drm_connector {
> * [0]: progressive, [1]: interlaced
> */
> int audio_latency[2];
> +
> + /**
> + * @ddc: associated ddc adapter.
> + * A connector usually has its associated ddc adapter. If a driver uses
> + * this field, then an appropriate symbolic link is created in connector
> + * sysfs directory to make it easy for the user to tell which i2c
> + * adapter is for a particular display.
> + */
> + struct i2c_adapter *ddc;
> +
> /**
> * @null_edid_counter: track sinks that give us all zeros for the EDID.
> * Needed to workaround some HW bugs where we get all 0s
> --
> 2.17.1
>

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2019-06-26 06:51:32

by Andrzej Pietrasiewicz

[permalink] [raw]
Subject: Re: [PATCH 0/2] Associate ddc adapters with connectors

W dniu 25.06.2019 o 16:20, Daniel Vetter pisze:
> On Tue, Jun 25, 2019 at 03:10:32PM +0100, Russell King - ARM Linux admin wrote:
>> On Tue, Jun 25, 2019 at 04:07:55PM +0200, Daniel Vetter wrote:
>>> Otherwise I like this. Biggest problem I'm seeing here is rolling this out
>>> everywhere, this is a lot of work. And without widespread adoptions it's
>>> not terribly useful for userspace.
>>
>> There will be cases where it's not possible, because the I2C bus is
>> hidden behind a chip that doesn't give you direct access to the DDC
>> bus.
>
> Oh sure, plus lots of connectors where there's just not ddc bus at all.
> But if we only roll this out for a handful of drivers it's also not great,
> that's what I meant. Looking at
>
> $ git grep drm_do_get_edid
>
> there's only very few drivers where the ddc bus is hidden. There's a lot
> more where it's not, and I think a big series to tackle those would serve
> extremely well to make a case for this sysfs link.
> -Daniel
>

I will respond with a v3 then, including as many drivers as possible.
Those will be compile-tested only, though.

Andrzej