[PATCH 0/2] Associate ddc adapters with connectors

[PATCH 0/2] Associate ddc adapters with connectors

[Andrzej Pietrasiewicz]

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

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

[Andrzej Pietrasiewicz]

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 <andrzej.p@collabora.com>
---
 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

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

[Andrzej Pietrasiewicz]

Switch to using the ddc provided by the generic connector.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 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

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

[Russell King - ARM Linux admin]

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

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

[Andrzej Pietrasiewicz]

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

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

[Russell King - ARM Linux admin]

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

[PATCH v2 0/2] Associate ddc adapters with connectors

[Andrzej Pietrasiewicz]

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

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

[Andrzej Pietrasiewicz]

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 <andrzej.p@collabora.com>
---
 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

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

[Daniel Vetter]

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 <andrzej.p@collabora.com>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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

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

[Andrzej Pietrasiewicz]

Switch to using the ddc provided by the generic connector.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 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

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

[Emil Velikov]

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

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

[Daniel Vetter]

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

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

[Russell King - ARM Linux admin]

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

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

[Daniel Vetter]

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

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

[Andrzej Pietrasiewicz]

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

Re: [PATCH v3 01/22] drm: Include ddc adapter pointer in struct drm_connector

[Andrzej Hajda]

On 01.07.2019 16:41, Thomas Zimmermann wrote:
> Hi
>
> Am 01.07.19 um 15:27 schrieb Andrzej Pietrasiewicz:
>> Hi Thomas,
>>
>> Thank you for your comments. Please see inline.
>>
>> W dniu 30.06.2019 o 10:12, Thomas Zimmermann pisze:
>>> Hi
>>>
>>> I like the idea, but would prefer a more structured approach.
>>>
>>> Setting connector->ddc before calling drm_sysfs_connector_add() seems
>>> error prone. The dependency is not really clear from the code or
>>> interfaces.
>>>
>>> The other thing is that drivers I mostly work on, ast and mgag200, have
>>> code like this:
>>>
>>>    struct ast_i2c_chan {
>>>     struct i2c_adapter adapter;
>>>     struct drm_device *dev;
>>>     struct i2c_algo_bit_data bit;
>>>    };
>>>
>>>    struct ast_connector {
>>>     struct drm_connector base;
>>>     struct ast_i2c_chan *i2c;
>>>    };
>>>
>>> It already encodes the connection between connector and ddc adapter.
>>>
>>> I suggest to introduce struct drm_i2c_adapter
>>>
>>>    struct drm_i2c_adapter {
>>>     struct i2c_adapter base;
>>>     struct drm_connector *connector;
>>>    };
>>>
>>> and convert drivers over to it. Ast would look like this:
>>>
>>>    struct ast_i2c_chan {
>>>     struct drm_i2c_adapter adapter;
>>>     struct i2c_algo_bit_data bit;
>>>    };
>>>
>> There are few flavors of these drivers. In some of them
>> the i2c_adapter for ddc is allocated and initialized by
>> the driver, which must provide a place in memory to hold
>> the adapter. ast is an example of this approach.
>>
>> But there are others, such as for example exynos_hdmi.c.
>> It gets its ddc adapter with of_find_i2c_adapter_by_node()
>> and merely stores a pointer to it, while not managing the
>> memory needed to hold the i2c_adapter.
> I see.
>
>> Do you have any idea how to accommodate these various
>> flavors of drivers in the scheme you propose?
> Something like
>
>   struct drm_i2c_adapter {
> 	struct i2c_adapter *adapter;
> 	struct drm_connector *connector;
>   };
>
> with adapter either being allocated dynamically or acquired via
> of_find_i2c_adapter_by_node(); with separate init and finish functions
> for each variant. But it looks like over-abstraction to me. Without
> prototyping the idea, I cannot say if it's worth the effort.
>
> For something more simple, maybe just have a function to attach an i2c
> adapter to a connector:
>
>   drm_connector_attach_i2c_adapter(connector, i2c_adapter)
>
> which sets the connector's ddc pointer and creates the sysfs entry if
> the connector's entry is already present.


I am not sure if such function is really necessary. Assigning ddc field
before connector registration seems to be much simpler and quite
standard - many fields are initialized this way.


Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej



> Best regards
> Thomas
>
>> Andrzej
>>
>>