| Message ID | cover.1561452052.git.andrzej.p@collabora.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Associate ddc adapters with connectors | expand |
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.
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
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.
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
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
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.
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
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
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 >> >>