| Message ID | cover.1561452052.git.andrzej.p@collabora.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Associate ddc adapters with connectors | expand |
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
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
v2..v3:
- converted as many drivers as possible.
PATCHES 3/22-22/22 SHOULD BE CONSIDERED RFC!
Andrzej Pietrasiewicz (22):
drm: Include ddc adapter pointer in struct drm_connector
drm/exynos: Provide ddc symlink in connector's sysfs
drm: rockchip: Provide ddc symlink in rk3066_hdmi sysfs directory
drm: rockchip: Provide ddc symlink in inno_hdmi sysfs directory
drm/msm/hdmi: Provide ddc symlink in hdmi connector sysfs directory
drm/sun4i: hdmi: Provide ddc symlink in sun4i hdmi connector sysfs
directory
drm/mediatek: Provide ddc symlink in hdmi connector sysfs directory
drm/tegra: Provide ddc symlink in output connector sysfs directory
drm/imx: imx-ldb: Provide ddc symlink in connector's sysfs
drm/imx: imx-tve: Provide ddc symlink in connector's sysfs
drm/vc4: Provide ddc symlink in connector sysfs directory
drm: zte: Provide ddc symlink in hdmi connector sysfs directory
drm: zte: Provide ddc symlink in vga connector sysfs directory
drm/tilcdc: Provide ddc symlink in connector sysfs directory
drm: sti: Provide ddc symlink in hdmi connector sysfs directory
drm/mgag200: Provide ddc symlink in connector sysfs directory
drm/ast: Provide ddc symlink in connector sysfs directory
drm/bridge: dumb-vga-dac: Provide ddc symlink in connector sysfs
directory
drm/bridge: dw-hdmi: Provide ddc symlink in connector sysfs directory
drm/bridge: ti-tfp410: Provide ddc symlink in connector sysfs
directory
drm/amdgpu: Provide ddc symlink in connector sysfs directory
drm/radeon: Provide ddc symlink in connector sysfs directory
.../gpu/drm/amd/amdgpu/amdgpu_connectors.c | 70 +++++++++++-----
drivers/gpu/drm/ast/ast_mode.c | 1 +
drivers/gpu/drm/bridge/dumb-vga-dac.c | 19 ++---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 40 ++++-----
drivers/gpu/drm/bridge/ti-tfp410.c | 19 ++---
drivers/gpu/drm/drm_sysfs.c | 7 ++
drivers/gpu/drm/exynos/exynos_hdmi.c | 11 ++-
drivers/gpu/drm/imx/imx-ldb.c | 13 ++-
drivers/gpu/drm/imx/imx-tve.c | 8 +-
drivers/gpu/drm/mediatek/mtk_hdmi.c | 9 +-
drivers/gpu/drm/mgag200/mgag200_mode.c | 1 +
drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 1 +
drivers/gpu/drm/radeon/radeon_connectors.c | 82 ++++++++++++++-----
drivers/gpu/drm/rockchip/inno_hdmi.c | 17 ++--
drivers/gpu/drm/rockchip/rk3066_hdmi.c | 17 ++--
drivers/gpu/drm/sti/sti_hdmi.c | 1 +
drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 -
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 14 ++--
drivers/gpu/drm/tegra/drm.h | 1 -
drivers/gpu/drm/tegra/output.c | 12 +--
drivers/gpu/drm/tegra/sor.c | 6 +-
drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 +
drivers/gpu/drm/vc4/vc4_hdmi.c | 16 ++--
drivers/gpu/drm/zte/zx_hdmi.c | 25 ++----
drivers/gpu/drm/zte/zx_vga.c | 25 ++----
include/drm/drm_connector.h | 11 +++
26 files changed, 252 insertions(+), 176 deletions(-)
Hi Andrzej, Just FYI, I have a patch series that reworks how bridges and connectors are handled, and it will heavily conflict with this. The purpose of the two series isn't the same, so both make sense. I will post the patches this weekend, and will then review this series in that context. Hopefully we'll get the best of both worlds :-) On Fri, Jun 28, 2019 at 06:01:14PM +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/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 > > v2..v3: > > - converted as many drivers as possible. > > PATCHES 3/22-22/22 SHOULD BE CONSIDERED RFC! > > Andrzej Pietrasiewicz (22): > drm: Include ddc adapter pointer in struct drm_connector > drm/exynos: Provide ddc symlink in connector's sysfs > drm: rockchip: Provide ddc symlink in rk3066_hdmi sysfs directory > drm: rockchip: Provide ddc symlink in inno_hdmi sysfs directory > drm/msm/hdmi: Provide ddc symlink in hdmi connector sysfs directory > drm/sun4i: hdmi: Provide ddc symlink in sun4i hdmi connector sysfs > directory > drm/mediatek: Provide ddc symlink in hdmi connector sysfs directory > drm/tegra: Provide ddc symlink in output connector sysfs directory > drm/imx: imx-ldb: Provide ddc symlink in connector's sysfs > drm/imx: imx-tve: Provide ddc symlink in connector's sysfs > drm/vc4: Provide ddc symlink in connector sysfs directory > drm: zte: Provide ddc symlink in hdmi connector sysfs directory > drm: zte: Provide ddc symlink in vga connector sysfs directory > drm/tilcdc: Provide ddc symlink in connector sysfs directory > drm: sti: Provide ddc symlink in hdmi connector sysfs directory > drm/mgag200: Provide ddc symlink in connector sysfs directory > drm/ast: Provide ddc symlink in connector sysfs directory > drm/bridge: dumb-vga-dac: Provide ddc symlink in connector sysfs > directory > drm/bridge: dw-hdmi: Provide ddc symlink in connector sysfs directory > drm/bridge: ti-tfp410: Provide ddc symlink in connector sysfs > directory > drm/amdgpu: Provide ddc symlink in connector sysfs directory > drm/radeon: Provide ddc symlink in connector sysfs directory > > .../gpu/drm/amd/amdgpu/amdgpu_connectors.c | 70 +++++++++++----- > drivers/gpu/drm/ast/ast_mode.c | 1 + > drivers/gpu/drm/bridge/dumb-vga-dac.c | 19 ++--- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 40 ++++----- > drivers/gpu/drm/bridge/ti-tfp410.c | 19 ++--- > drivers/gpu/drm/drm_sysfs.c | 7 ++ > drivers/gpu/drm/exynos/exynos_hdmi.c | 11 ++- > drivers/gpu/drm/imx/imx-ldb.c | 13 ++- > drivers/gpu/drm/imx/imx-tve.c | 8 +- > drivers/gpu/drm/mediatek/mtk_hdmi.c | 9 +- > drivers/gpu/drm/mgag200/mgag200_mode.c | 1 + > drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 1 + > drivers/gpu/drm/radeon/radeon_connectors.c | 82 ++++++++++++++----- > drivers/gpu/drm/rockchip/inno_hdmi.c | 17 ++-- > drivers/gpu/drm/rockchip/rk3066_hdmi.c | 17 ++-- > drivers/gpu/drm/sti/sti_hdmi.c | 1 + > drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 - > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 14 ++-- > drivers/gpu/drm/tegra/drm.h | 1 - > drivers/gpu/drm/tegra/output.c | 12 +-- > drivers/gpu/drm/tegra/sor.c | 6 +- > drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 + > drivers/gpu/drm/vc4/vc4_hdmi.c | 16 ++-- > drivers/gpu/drm/zte/zx_hdmi.c | 25 ++---- > drivers/gpu/drm/zte/zx_vga.c | 25 ++---- > include/drm/drm_connector.h | 11 +++ > 26 files changed, 252 insertions(+), 176 deletions(-) > > -- > 2.17.1 >
On Fri, Jun 28, 2019 at 06:01:14PM +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/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 > > v2..v3: > > - converted as many drivers as possible. > > PATCHES 3/22-22/22 SHOULD BE CONSIDERED RFC! There's a lot more drivers than this I think (i915 is absent as an example, but there should be tons more). Why are those not possible? -Daniel > > Andrzej Pietrasiewicz (22): > drm: Include ddc adapter pointer in struct drm_connector > drm/exynos: Provide ddc symlink in connector's sysfs > drm: rockchip: Provide ddc symlink in rk3066_hdmi sysfs directory > drm: rockchip: Provide ddc symlink in inno_hdmi sysfs directory > drm/msm/hdmi: Provide ddc symlink in hdmi connector sysfs directory > drm/sun4i: hdmi: Provide ddc symlink in sun4i hdmi connector sysfs > directory > drm/mediatek: Provide ddc symlink in hdmi connector sysfs directory > drm/tegra: Provide ddc symlink in output connector sysfs directory > drm/imx: imx-ldb: Provide ddc symlink in connector's sysfs > drm/imx: imx-tve: Provide ddc symlink in connector's sysfs > drm/vc4: Provide ddc symlink in connector sysfs directory > drm: zte: Provide ddc symlink in hdmi connector sysfs directory > drm: zte: Provide ddc symlink in vga connector sysfs directory > drm/tilcdc: Provide ddc symlink in connector sysfs directory > drm: sti: Provide ddc symlink in hdmi connector sysfs directory > drm/mgag200: Provide ddc symlink in connector sysfs directory > drm/ast: Provide ddc symlink in connector sysfs directory > drm/bridge: dumb-vga-dac: Provide ddc symlink in connector sysfs > directory > drm/bridge: dw-hdmi: Provide ddc symlink in connector sysfs directory > drm/bridge: ti-tfp410: Provide ddc symlink in connector sysfs > directory > drm/amdgpu: Provide ddc symlink in connector sysfs directory > drm/radeon: Provide ddc symlink in connector sysfs directory > > .../gpu/drm/amd/amdgpu/amdgpu_connectors.c | 70 +++++++++++----- > drivers/gpu/drm/ast/ast_mode.c | 1 + > drivers/gpu/drm/bridge/dumb-vga-dac.c | 19 ++--- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 40 ++++----- > drivers/gpu/drm/bridge/ti-tfp410.c | 19 ++--- > drivers/gpu/drm/drm_sysfs.c | 7 ++ > drivers/gpu/drm/exynos/exynos_hdmi.c | 11 ++- > drivers/gpu/drm/imx/imx-ldb.c | 13 ++- > drivers/gpu/drm/imx/imx-tve.c | 8 +- > drivers/gpu/drm/mediatek/mtk_hdmi.c | 9 +- > drivers/gpu/drm/mgag200/mgag200_mode.c | 1 + > drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 1 + > drivers/gpu/drm/radeon/radeon_connectors.c | 82 ++++++++++++++----- > drivers/gpu/drm/rockchip/inno_hdmi.c | 17 ++-- > drivers/gpu/drm/rockchip/rk3066_hdmi.c | 17 ++-- > drivers/gpu/drm/sti/sti_hdmi.c | 1 + > drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 - > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 14 ++-- > drivers/gpu/drm/tegra/drm.h | 1 - > drivers/gpu/drm/tegra/output.c | 12 +-- > drivers/gpu/drm/tegra/sor.c | 6 +- > drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 + > drivers/gpu/drm/vc4/vc4_hdmi.c | 16 ++-- > drivers/gpu/drm/zte/zx_hdmi.c | 25 ++---- > drivers/gpu/drm/zte/zx_vga.c | 25 ++---- > include/drm/drm_connector.h | 11 +++ > 26 files changed, 252 insertions(+), 176 deletions(-) > > -- > 2.17.1 >
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;
};
Two new sysfs functions would set up and remove the file.
int drm_sysfs_i2c_adapter_add(struct drm_i2c_adapter *i2c);
void drm_sysfs_i2c_adapter_remove(struct drm_i2c_adapter *i2c);
The i2c adapter, connector->ddc pointer and sysfs entry would be
initialized by
drm_i2c_adapter_init(struct drm_i2c_adapter *i2c, struct connector
*connector, void *algo_data);
and cleaned up by
drm_i2c_adapter_remove(struct drm_i2c_adapter *i2c);
Thoughts?
Best regards
Thomas
Am 28.06.19 um 18:01 schrieb 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>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> 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
>
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. Do you have any idea how to accommodate these various flavors of drivers in the scheme you propose? Andrzej
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. Best regards Thomas > Andrzej > >
On Fri, 28 Jun 2019 at 17:45, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Jun 28, 2019 at 06:01:14PM +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/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 > > > > v2..v3: > > > > - converted as many drivers as possible. > > > > PATCHES 3/22-22/22 SHOULD BE CONSIDERED RFC! > > There's a lot more drivers than this I think (i915 is absent as an > example, but there should be tons more). Why are those not possible? While I fully agree there are more drivers, at the same time I wonder. Is it a good idea to expect all of those to be fixed in one go and block patches addressing 15+ drivers? Personally I think it's reasonable to have this, alongside a TODO entry for other drivers. HTH Emil
W dniu 28.06.2019 o 18:11, Laurent Pinchart pisze: > Hi Andrzej, > > Just FYI, I have a patch series that reworks how bridges and connectors > are handled, and it will heavily conflict with this. The purpose of the > two series isn't the same, so both make sense. I will post the patches > this weekend, and will then review this series in that context. > Hopefully we'll get the best of both worlds :-) Hi Laurent, Did you have a chance to review my patch series? Andrzej
Hi Andrzej, On Fri, Jul 05, 2019 at 10:38:27AM +0200, Andrzej Pietrasiewicz wrote: > W dniu 28.06.2019 o 18:11, Laurent Pinchart pisze: > > Hi Andrzej, > > > > Just FYI, I have a patch series that reworks how bridges and connectors > > are handled, and it will heavily conflict with this. The purpose of the > > two series isn't the same, so both make sense. I will post the patches > > this weekend, and will then review this series in that context. > > Hopefully we'll get the best of both worlds :-) > > Hi Laurent, > > Did you have a chance to review my patch series? Not yet I'm afraid. I've been fairly busy this week, and coupled with some health issues (but I'm feeling better now, so nothing to worry about) it delayed my reviews. I'll get to it as soon as possible. Thank you for pinging me.
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 >> >>