[v6,00/24] Associate ddc adapters with connectors
mbox series

Message ID cover.1564161140.git.andrzej.p@collabora.com
Headers show
Series
  • Associate ddc adapters with connectors
Related show

Message

Andrzej Pietrasiewicz July 26, 2019, 5:22 p.m. UTC
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.

Patch 2 adds a new variant of drm_connector_init(), see the changelog
below.

Patches 3..24 are examples 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.

v3..v4:

- added Reviewed-by for patch 01/23
- moved "ddc" field assignment to before drm_connector_init() is called
in msm, vc4, sti, mgag200, ast, amdgpu, radeon
- simplified the code in amdgpu and radeon at the expense of some lines
exceeding 80 characters as per Alex Deucher's suggestion
- added i915

v4..v5:

- changed "include <linux/i2c.h>" to "struct i2c_adapter;"
in drm_connector.h, consequently, added "include <linux/i2c.h>"
in drm_sysfs.c.
- added "drm_connector_init_with_ddc()" variant to ensure that the ddc
field of drm_connector is preserved accross its invocation
- accordingly changed invocations of drm_connector_init() in the
touched drivers to use the new variant

v5..v6:

- improved subject line of patch 1
- added kernel-doc for drm_connector_init_with_ddc()
- improved kernel-doc for the ddc field of struct drm_connector
- added Reviewed-by in patches 17 and 18
- added Acked-by in patch 2
- made the ownership of ddc i2c_adapter explicit in all patches,
this made the affected patches much simpler

@Benjamin
@Shawn

There were your Acked-by or Reviewed-by for some patches in v4, but now
that the patches use the newly added function I'm not sure I can still
include those tags without you actually confirming. Can I? Or can you
please re-review? 

TODO: nouveau, gma500, omapdrm, panel-simple - if applicable.
Other drivers are either already converted or don't mention neither
"ddc" nor "i2c_adapter".

Andrzej Pietrasiewicz (24):
  drm: Add ddc link in sysfs created by drm_connector
  drm: Add drm_connector_init() variant with ddc
  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
  drm/i915: Provide ddc symlink in hdmi connector sysfs directory

 .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    |  96 ++++++++----
 drivers/gpu/drm/ast/ast_mode.c                |  13 +-
 drivers/gpu/drm/bridge/dumb-vga-dac.c         |   6 +-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c     |   6 +-
 drivers/gpu/drm/bridge/ti-tfp410.c            |   6 +-
 drivers/gpu/drm/drm_connector.c               |  35 +++++
 drivers/gpu/drm/drm_sysfs.c                   |   8 +
 drivers/gpu/drm/exynos/exynos_hdmi.c          |   6 +-
 drivers/gpu/drm/i915/display/intel_hdmi.c     |  12 +-
 drivers/gpu/drm/imx/imx-ldb.c                 |   7 +-
 drivers/gpu/drm/imx/imx-tve.c                 |   6 +-
 drivers/gpu/drm/mediatek/mtk_hdmi.c           |   7 +-
 drivers/gpu/drm/mgag200/mgag200_mode.c        |  13 +-
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c     |   6 +-
 drivers/gpu/drm/radeon/radeon_connectors.c    | 142 +++++++++++++-----
 drivers/gpu/drm/rockchip/inno_hdmi.c          |   6 +-
 drivers/gpu/drm/rockchip/rk3066_hdmi.c        |   7 +-
 drivers/gpu/drm/sti/sti_hdmi.c                |   6 +-
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c        |   7 +-
 drivers/gpu/drm/tegra/hdmi.c                  |   7 +-
 drivers/gpu/drm/tegra/sor.c                   |   7 +-
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c        |   6 +-
 drivers/gpu/drm/vc4/vc4_hdmi.c                |  12 +-
 drivers/gpu/drm/zte/zx_hdmi.c                 |   6 +-
 drivers/gpu/drm/zte/zx_vga.c                  |   6 +-
 include/drm/drm_connector.h                   |  18 +++
 26 files changed, 336 insertions(+), 121 deletions(-)

Comments

Sam Ravnborg July 26, 2019, 6:35 p.m. UTC | #1
Hi Andezej.

On Fri, Jul 26, 2019 at 07:22:54PM +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.
> 
> Patch 2 adds a new variant of drm_connector_init(), see the changelog
> below.
> 
> Patches 3..24 are examples of how to convert a driver to this new scheme.
> 
...
> 
> v5..v6:
> 
> - improved subject line of patch 1
> - added kernel-doc for drm_connector_init_with_ddc()
> - improved kernel-doc for the ddc field of struct drm_connector
> - added Reviewed-by in patches 17 and 18
> - added Acked-by in patch 2
> - made the ownership of ddc i2c_adapter explicit in all patches,
> this made the affected patches much simpler

Looks good now.
Patch 1 and 2 are:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

The remaining patches are:
Acked-by: Sam Ravnborg <sam@ravnborg.org>

	Sam
Sam Ravnborg July 26, 2019, 6:55 p.m. UTC | #2
Hi all.

Andrzej have done a good job following up on feedback and this series is
now ready.

We need ack on the patches touching the individual drivers before we can
proceed.
Please check your drivers and get back.

	Sam

> Hi Andezej.
> 
> On Fri, Jul 26, 2019 at 07:22:54PM +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.
> > 
> > Patch 2 adds a new variant of drm_connector_init(), see the changelog
> > below.
> > 
> > Patches 3..24 are examples of how to convert a driver to this new scheme.
> > 
> ...
> > 
> > v5..v6:
> > 
> > - improved subject line of patch 1
> > - added kernel-doc for drm_connector_init_with_ddc()
> > - improved kernel-doc for the ddc field of struct drm_connector
> > - added Reviewed-by in patches 17 and 18
> > - added Acked-by in patch 2
> > - made the ownership of ddc i2c_adapter explicit in all patches,
> > this made the affected patches much simpler
> 
> Looks good now.
> Patch 1 and 2 are:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> The remaining patches are:
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> 
> 	Sam
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Emil Velikov July 30, 2019, 3:01 p.m. UTC | #3
On 2019/07/26, 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.
> 
> Patch 2 adds a new variant of drm_connector_init(), see the changelog
> below.
> 
> Patches 3..24 are examples 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.
> 
> v3..v4:
> 
> - added Reviewed-by for patch 01/23
> - moved "ddc" field assignment to before drm_connector_init() is called
> in msm, vc4, sti, mgag200, ast, amdgpu, radeon
> - simplified the code in amdgpu and radeon at the expense of some lines
> exceeding 80 characters as per Alex Deucher's suggestion
> - added i915
> 
> v4..v5:
> 
> - changed "include <linux/i2c.h>" to "struct i2c_adapter;"
> in drm_connector.h, consequently, added "include <linux/i2c.h>"
> in drm_sysfs.c.
> - added "drm_connector_init_with_ddc()" variant to ensure that the ddc
> field of drm_connector is preserved accross its invocation
> - accordingly changed invocations of drm_connector_init() in the
> touched drivers to use the new variant
> 
> v5..v6:
> 
> - improved subject line of patch 1
> - added kernel-doc for drm_connector_init_with_ddc()
> - improved kernel-doc for the ddc field of struct drm_connector
> - added Reviewed-by in patches 17 and 18
> - added Acked-by in patch 2
> - made the ownership of ddc i2c_adapter explicit in all patches,
> this made the affected patches much simpler
> 
> @Benjamin
> @Shawn
> 
> There were your Acked-by or Reviewed-by for some patches in v4, but now
> that the patches use the newly added function I'm not sure I can still
> include those tags without you actually confirming. Can I? Or can you
> please re-review? 
> 
> TODO: nouveau, gma500, omapdrm, panel-simple - if applicable.
> Other drivers are either already converted or don't mention neither
> "ddc" nor "i2c_adapter".
> 
Another way to check is to look for drm_get_edid. Sadly that also
highlights aux. dp/mst instances, which expose the DDC in another way.

For example comparing the diff stat wrt the following command shows
git grep -wc drm_get_edid -- drivers/gpu/drm/

> 
>  .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    |  96 ++++++++----
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c:1
- as pointed out by Alex - mix of aux dp/mst and normal

>  drivers/gpu/drm/ast/ast_mode.c                |  13 +-

drivers/gpu/drm/bridge/analogix-anx78xx.c:1
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1
- not applicable: aux dp/mst


>  drivers/gpu/drm/bridge/dumb-vga-dac.c         |   6 +-

drivers/gpu/drm/bridge/sii902x.c:1
- normal instance(?) that should be updated at some point.

>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c     |   6 +-

drivers/gpu/drm/bridge/tc358767.c:1
- not applicable: aux dp/mst

>  drivers/gpu/drm/bridge/ti-tfp410.c            |   6 +-

drivers/gpu/drm/drm_dp_mst_topology.c:3
- not applicable: aux dp/mst

>  drivers/gpu/drm/drm_connector.c               |  35 +++++
>  drivers/gpu/drm/drm_sysfs.c                   |   8 +

drivers/gpu/drm/drm_edid.c:7
drivers/gpu/drm/drm_probe_helper.c:1
- unrelated

>  drivers/gpu/drm/exynos/exynos_hdmi.c          |   6 +-

drivers/gpu/drm/gma500/cdv_intel_dp.c:3
drivers/gpu/drm/gma500/cdv_intel_hdmi.c:2
drivers/gpu/drm/gma500/oaktrail_hdmi.c:1
drivers/gpu/drm/gma500/oaktrail_lvds.c:2
drivers/gpu/drm/gma500/psb_intel_modes.c:1
drivers/gpu/drm/gma500/psb_intel_sdvo.c:2
- should be updated at some point (as you pointed out).

>  drivers/gpu/drm/i915/display/intel_hdmi.c     |  12 +-
drivers/gpu/drm/i915/intel_connector.c:1
drivers/gpu/drm/i915/intel_crt.c:2
- not too sure here

drivers/gpu/drm/i915/intel_dp.c:2
- not applicable: aux dp/mst

drivers/gpu/drm/i915/intel_lvds.c:1
drivers/gpu/drm/i915/intel_sdvo.c:2
- not too sure here


>  drivers/gpu/drm/imx/imx-ldb.c                 |   7 +-
>  drivers/gpu/drm/imx/imx-tve.c                 |   6 +-
>  drivers/gpu/drm/mediatek/mtk_hdmi.c           |   7 +-
>  drivers/gpu/drm/mgag200/mgag200_mode.c        |  13 +-
>  drivers/gpu/drm/msm/hdmi/hdmi_connector.c     |   6 +-
drivers/gpu/drm/msm/edp/edp_ctrl.c:1
- not applicable: aux dp/mst

drivers/gpu/drm/nouveau/nouveau_connector.c:2
- should be updated at some point (as you pointed out).


drivers/gpu/drm/panel/panel-simple.c:1
- no applicable: panel driver


>  drivers/gpu/drm/radeon/radeon_connectors.c    | 142 +++++++++++++-----
>  drivers/gpu/drm/rockchip/inno_hdmi.c          |   6 +-
>  drivers/gpu/drm/rockchip/rk3066_hdmi.c        |   7 +-
>  drivers/gpu/drm/sti/sti_hdmi.c                |   6 +-
>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c        |   7 +-
>  drivers/gpu/drm/tegra/hdmi.c                  |   7 +-
>  drivers/gpu/drm/tegra/sor.c                   |   7 +-

drivers/gpu/drm/tegra/output.c:1
- already handled in hdmi/sor

>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c        |   6 +-
>  drivers/gpu/drm/vc4/vc4_hdmi.c                |  12 +-
>  drivers/gpu/drm/zte/zx_hdmi.c                 |   6 +-
>  drivers/gpu/drm/zte/zx_vga.c                  |   6 +-
>  include/drm/drm_connector.h                   |  18 +++
>  26 files changed, 336 insertions(+), 121 deletions(-)

In a Tl;Dr: I think this series covers 90%+ of the existing rather huge) driverset.

For the series:
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

Fwiw I'm in favour of Jani's suggestion to fold the dcc into the usual
helper drm_connector_init(). Although since we have 130+ instances it
might be better left for another day.

HTH
-Emil
Neil Armstrong July 31, 2019, 8 a.m. UTC | #4
Hi Sam,

On 26/07/2019 20:55, Sam Ravnborg wrote:
> Hi all.
> 
> Andrzej have done a good job following up on feedback and this series is
> now ready.
> 
> We need ack on the patches touching the individual drivers before we can
> proceed.
> Please check your drivers and get back.

I can apply all core and maintainer-acked patches for now :
1, 2, 7, 10, 11, 16, 17, 18, 19, 20, 21, 22, 23

and Andrzej can resend not applied patches with Yours and Emil's Reviewed-by,
so we can wait a few more days to apply them.

Neil

> 
> 	Sam
> 
>> Hi Andezej.
>>
>> On Fri, Jul 26, 2019 at 07:22:54PM +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.
>>>
>>> Patch 2 adds a new variant of drm_connector_init(), see the changelog
>>> below.
>>>
>>> Patches 3..24 are examples of how to convert a driver to this new scheme.
>>>
>> ...
>>>
>>> v5..v6:
>>>
>>> - improved subject line of patch 1
>>> - added kernel-doc for drm_connector_init_with_ddc()
>>> - improved kernel-doc for the ddc field of struct drm_connector
>>> - added Reviewed-by in patches 17 and 18
>>> - added Acked-by in patch 2
>>> - made the ownership of ddc i2c_adapter explicit in all patches,
>>> this made the affected patches much simpler
>>
>> Looks good now.
>> Patch 1 and 2 are:
>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>>
>> The remaining patches are:
>> Acked-by: Sam Ravnborg <sam@ravnborg.org>
>>
>> 	Sam
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Sam Ravnborg July 31, 2019, 10:40 a.m. UTC | #5
Hi Neil.

On Wed, Jul 31, 2019 at 10:00:14AM +0200, Neil Armstrong wrote:
> Hi Sam,
> 
> On 26/07/2019 20:55, Sam Ravnborg wrote:
> > Hi all.
> > 
> > Andrzej have done a good job following up on feedback and this series is
> > now ready.
> > 
> > We need ack on the patches touching the individual drivers before we can
> > proceed.
> > Please check your drivers and get back.
> 
> I can apply all core and maintainer-acked patches for now :
> 1, 2, 7, 10, 11, 16, 17, 18, 19, 20, 21, 22, 23
> 
> and Andrzej can resend not applied patches with Yours and Emil's Reviewed-by,
> so we can wait a few more days to apply them.

Sounds like a good plan.
Thanks for thaking care of this.

	Sam
Andrzej Pietrasiewicz July 31, 2019, 1:10 p.m. UTC | #6
W dniu 31.07.2019 o 12:40, Sam Ravnborg pisze:
> Hi Neil.
> 
> On Wed, Jul 31, 2019 at 10:00:14AM +0200, Neil Armstrong wrote:
>> Hi Sam,
>>
>> On 26/07/2019 20:55, Sam Ravnborg wrote:
>>> Hi all.
>>>
>>> Andrzej have done a good job following up on feedback and this series is
>>> now ready.
>>>
>>> We need ack on the patches touching the individual drivers before we can
>>> proceed.
>>> Please check your drivers and get back.
>>
>> I can apply all core and maintainer-acked patches for now :
>> 1, 2, 7, 10, 11, 16, 17, 18, 19, 20, 21, 22, 23
>>
>> and Andrzej can resend not applied patches with Yours and Emil's Reviewed-by,
>> so we can wait a few more days to apply them.
> 
> Sounds like a good plan.
> Thanks for thaking care of this.

When is it good time to resend patches 3, 4, 5, 6, 8, 9, 12, 13, 14, 15, 24 as a
new series?

Andrzej
Neil Armstrong July 31, 2019, 2:22 p.m. UTC | #7
On 31/07/2019 15:10, Andrzej Pietrasiewicz wrote:
> W dniu 31.07.2019 o 12:40, Sam Ravnborg pisze:
>> Hi Neil.
>>
>> On Wed, Jul 31, 2019 at 10:00:14AM +0200, Neil Armstrong wrote:
>>> Hi Sam,
>>>
>>> On 26/07/2019 20:55, Sam Ravnborg wrote:
>>>> Hi all.
>>>>
>>>> Andrzej have done a good job following up on feedback and this series is
>>>> now ready.
>>>>
>>>> We need ack on the patches touching the individual drivers before we can
>>>> proceed.
>>>> Please check your drivers and get back.
>>>
>>> I can apply all core and maintainer-acked patches for now :
>>> 1, 2, 7, 10, 11, 16, 17, 18, 19, 20, 21, 22, 23
>>>
>>> and Andrzej can resend not applied patches with Yours and Emil's Reviewed-by,
>>> so we can wait a few more days to apply them.
>>
>> Sounds like a good plan.
>> Thanks for thaking care of this.
> 
> When is it good time to resend patches 3, 4, 5, 6, 8, 9, 12, 13, 14, 15, 24 as a
> new series?

I'll ping you when everything is applied, build-tested and pushed on drm-misc-next

Neil

> 
> Andrzej
Neil Armstrong July 31, 2019, 3:32 p.m. UTC | #8
Hi Andrzej,

On 31/07/2019 16:22, Neil Armstrong wrote:
> On 31/07/2019 15:10, Andrzej Pietrasiewicz wrote:
>> W dniu 31.07.2019 o 12:40, Sam Ravnborg pisze:
>>> Hi Neil.
>>>
>>> On Wed, Jul 31, 2019 at 10:00:14AM +0200, Neil Armstrong wrote:
>>>> Hi Sam,
>>>>
>>>> On 26/07/2019 20:55, Sam Ravnborg wrote:
>>>>> Hi all.
>>>>>
>>>>> Andrzej have done a good job following up on feedback and this series is
>>>>> now ready.
>>>>>
>>>>> We need ack on the patches touching the individual drivers before we can
>>>>> proceed.
>>>>> Please check your drivers and get back.
>>>>
>>>> I can apply all core and maintainer-acked patches for now :
>>>> 1, 2, 7, 10, 11, 16, 17, 18, 19, 20, 21, 22, 23
>>>>
>>>> and Andrzej can resend not applied patches with Yours and Emil's Reviewed-by,
>>>> so we can wait a few more days to apply them.
>>>
>>> Sounds like a good plan.
>>> Thanks for thaking care of this.
>>
>> When is it good time to resend patches 3, 4, 5, 6, 8, 9, 12, 13, 14, 15, 24 as a
>> new series?
> 
> I'll ping you when everything is applied, build-tested and pushed on drm-misc-next

I pushed 1, 2, 7, 10, 11, 16, 17, 18, 19, 20, 21, 22, 23 :
bed7a2182de6 drm/radeon: Provide ddc symlink in connector sysfs directory
5b50fa2b35a4 drm/amdgpu: Provide ddc symlink in connector sysfs directory
cfb444552926 drm/bridge: ti-tfp410: Provide ddc symlink in connector sysfs directory
9ebc4d2140ad drm/bridge: dw-hdmi: Provide ddc symlink in connector sysfs directory
a4f9087e85de drm/bridge: dumb-vga-dac: Provide ddc symlink in connector sysfs directory
350fd554ee44 drm/ast: Provide ddc symlink in connector sysfs directory
9572ae176a10 drm/mgag200: Provide ddc symlink in connector sysfs directory
7058e76682d7 drm: sti: Provide ddc symlink in hdmi connector sysfs directory
2ae7eb372ed4 drm/imx: imx-tve: Provide ddc symlink in connector's sysfs
be0ec35940bc drm/imx: imx-ldb: Provide ddc symlink in connector's sysfs
1e8f17855ff8 drm/sun4i: hdmi: Provide ddc symlink in sun4i hdmi connector sysfs directory
100163df4203 drm: Add drm_connector_init() variant with ddc
e1a29c6c5955 drm: Add ddc link in sysfs created by drm_connector

Neil

> 
> Neil
> 
>>
>> Andrzej
>
Andrzej Pietrasiewicz July 31, 2019, 4:58 p.m. UTC | #9
Now that some of the patches of the previous v6 series are applied,
I'm resending the remaining ones (patches 3-13) with Acked-by and
Reviewed-by added.

I'm also taking this opportunity to provide the symlink for another
connector in amdgpu (patch 1), and to fix a small but nasty bug
which can cause a use of an uninitialized variable (patch 2).

Andrzej Pietrasiewicz (13):
  drm/amdgpu: Provide ddc symlink in dm connector's sysfs directory
  drm/radeon: Eliminate possible use of an uninitialized variable
  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/mediatek: Provide ddc symlink in hdmi connector sysfs directory
  drm/tegra: Provide ddc symlink in output connector sysfs directory
  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/i915: Provide ddc symlink in hdmi connector sysfs directory

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  5 +++--
 drivers/gpu/drm/exynos/exynos_hdmi.c              |  6 ++++--
 drivers/gpu/drm/i915/display/intel_hdmi.c         | 12 ++++++++----
 drivers/gpu/drm/mediatek/mtk_hdmi.c               |  7 ++++---
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c         |  6 ++++--
 drivers/gpu/drm/radeon/radeon_connectors.c        |  2 +-
 drivers/gpu/drm/rockchip/inno_hdmi.c              |  6 ++++--
 drivers/gpu/drm/rockchip/rk3066_hdmi.c            |  7 ++++---
 drivers/gpu/drm/tegra/hdmi.c                      |  7 ++++---
 drivers/gpu/drm/tegra/sor.c                       |  7 ++++---
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c            |  6 ++++--
 drivers/gpu/drm/vc4/vc4_hdmi.c                    | 12 ++++++++----
 drivers/gpu/drm/zte/zx_hdmi.c                     |  6 ++++--
 drivers/gpu/drm/zte/zx_vga.c                      |  6 ++++--
 14 files changed, 60 insertions(+), 35 deletions(-)
Laurent Pinchart Aug. 4, 2019, 12:33 p.m. UTC | #10
Hi Andrzej,

On Fri, Jul 26, 2019 at 07:22:54PM +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.
> 
> Patch 2 adds a new variant of drm_connector_init(), see the changelog
> below.
> 
> Patches 3..24 are examples 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.
> 
> v3..v4:
> 
> - added Reviewed-by for patch 01/23
> - moved "ddc" field assignment to before drm_connector_init() is called
> in msm, vc4, sti, mgag200, ast, amdgpu, radeon
> - simplified the code in amdgpu and radeon at the expense of some lines
> exceeding 80 characters as per Alex Deucher's suggestion
> - added i915
> 
> v4..v5:
> 
> - changed "include <linux/i2c.h>" to "struct i2c_adapter;"
> in drm_connector.h, consequently, added "include <linux/i2c.h>"
> in drm_sysfs.c.
> - added "drm_connector_init_with_ddc()" variant to ensure that the ddc
> field of drm_connector is preserved accross its invocation
> - accordingly changed invocations of drm_connector_init() in the
> touched drivers to use the new variant
> 
> v5..v6:
> 
> - improved subject line of patch 1
> - added kernel-doc for drm_connector_init_with_ddc()
> - improved kernel-doc for the ddc field of struct drm_connector
> - added Reviewed-by in patches 17 and 18
> - added Acked-by in patch 2
> - made the ownership of ddc i2c_adapter explicit in all patches,
> this made the affected patches much simpler
> 
> @Benjamin
> @Shawn
> 
> There were your Acked-by or Reviewed-by for some patches in v4, but now
> that the patches use the newly added function I'm not sure I can still
> include those tags without you actually confirming. Can I? Or can you
> please re-review? 
> 
> TODO: nouveau, gma500, omapdrm, panel-simple - if applicable.

omapdrm is moving to a new helper that creates connectors for a set of
bridges, so I'll handle it there. It may require adding a ddc field to
drm_bridge.

> Other drivers are either already converted or don't mention neither
> "ddc" nor "i2c_adapter".
> 
> Andrzej Pietrasiewicz (24):
>   drm: Add ddc link in sysfs created by drm_connector
>   drm: Add drm_connector_init() variant with ddc
>   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
>   drm/i915: Provide ddc symlink in hdmi connector sysfs directory
> 
>  .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    |  96 ++++++++----
>  drivers/gpu/drm/ast/ast_mode.c                |  13 +-
>  drivers/gpu/drm/bridge/dumb-vga-dac.c         |   6 +-
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c     |   6 +-
>  drivers/gpu/drm/bridge/ti-tfp410.c            |   6 +-
>  drivers/gpu/drm/drm_connector.c               |  35 +++++
>  drivers/gpu/drm/drm_sysfs.c                   |   8 +
>  drivers/gpu/drm/exynos/exynos_hdmi.c          |   6 +-
>  drivers/gpu/drm/i915/display/intel_hdmi.c     |  12 +-
>  drivers/gpu/drm/imx/imx-ldb.c                 |   7 +-
>  drivers/gpu/drm/imx/imx-tve.c                 |   6 +-
>  drivers/gpu/drm/mediatek/mtk_hdmi.c           |   7 +-
>  drivers/gpu/drm/mgag200/mgag200_mode.c        |  13 +-
>  drivers/gpu/drm/msm/hdmi/hdmi_connector.c     |   6 +-
>  drivers/gpu/drm/radeon/radeon_connectors.c    | 142 +++++++++++++-----
>  drivers/gpu/drm/rockchip/inno_hdmi.c          |   6 +-
>  drivers/gpu/drm/rockchip/rk3066_hdmi.c        |   7 +-
>  drivers/gpu/drm/sti/sti_hdmi.c                |   6 +-
>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c        |   7 +-
>  drivers/gpu/drm/tegra/hdmi.c                  |   7 +-
>  drivers/gpu/drm/tegra/sor.c                   |   7 +-
>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c        |   6 +-
>  drivers/gpu/drm/vc4/vc4_hdmi.c                |  12 +-
>  drivers/gpu/drm/zte/zx_hdmi.c                 |   6 +-
>  drivers/gpu/drm/zte/zx_vga.c                  |   6 +-
>  include/drm/drm_connector.h                   |  18 +++
>  26 files changed, 336 insertions(+), 121 deletions(-)
Laurent Pinchart Aug. 4, 2019, 12:36 p.m. UTC | #11
On Tue, Jul 30, 2019 at 04:01:23PM +0100, Emil Velikov wrote:
> On 2019/07/26, 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.
> > 
> > Patch 2 adds a new variant of drm_connector_init(), see the changelog
> > below.
> > 
> > Patches 3..24 are examples 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.
> > 
> > v3..v4:
> > 
> > - added Reviewed-by for patch 01/23
> > - moved "ddc" field assignment to before drm_connector_init() is called
> > in msm, vc4, sti, mgag200, ast, amdgpu, radeon
> > - simplified the code in amdgpu and radeon at the expense of some lines
> > exceeding 80 characters as per Alex Deucher's suggestion
> > - added i915
> > 
> > v4..v5:
> > 
> > - changed "include <linux/i2c.h>" to "struct i2c_adapter;"
> > in drm_connector.h, consequently, added "include <linux/i2c.h>"
> > in drm_sysfs.c.
> > - added "drm_connector_init_with_ddc()" variant to ensure that the ddc
> > field of drm_connector is preserved accross its invocation
> > - accordingly changed invocations of drm_connector_init() in the
> > touched drivers to use the new variant
> > 
> > v5..v6:
> > 
> > - improved subject line of patch 1
> > - added kernel-doc for drm_connector_init_with_ddc()
> > - improved kernel-doc for the ddc field of struct drm_connector
> > - added Reviewed-by in patches 17 and 18
> > - added Acked-by in patch 2
> > - made the ownership of ddc i2c_adapter explicit in all patches,
> > this made the affected patches much simpler
> > 
> > @Benjamin
> > @Shawn
> > 
> > There were your Acked-by or Reviewed-by for some patches in v4, but now
> > that the patches use the newly added function I'm not sure I can still
> > include those tags without you actually confirming. Can I? Or can you
> > please re-review? 
> > 
> > TODO: nouveau, gma500, omapdrm, panel-simple - if applicable.
> > Other drivers are either already converted or don't mention neither
> > "ddc" nor "i2c_adapter".
> > 
> Another way to check is to look for drm_get_edid. Sadly that also
> highlights aux. dp/mst instances, which expose the DDC in another way.
> 
> For example comparing the diff stat wrt the following command shows
> git grep -wc drm_get_edid -- drivers/gpu/drm/

There's also drm_do_get_edid, which points to the adv7511 bridge as a
good candidate.

> > 
> >  .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    |  96 ++++++++----
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c:1
> - as pointed out by Alex - mix of aux dp/mst and normal
> 
> >  drivers/gpu/drm/ast/ast_mode.c                |  13 +-
> 
> drivers/gpu/drm/bridge/analogix-anx78xx.c:1
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1
> - not applicable: aux dp/mst
> 
> 
> >  drivers/gpu/drm/bridge/dumb-vga-dac.c         |   6 +-
> 
> drivers/gpu/drm/bridge/sii902x.c:1
> - normal instance(?) that should be updated at some point.
> 
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c     |   6 +-
> 
> drivers/gpu/drm/bridge/tc358767.c:1
> - not applicable: aux dp/mst
> 
> >  drivers/gpu/drm/bridge/ti-tfp410.c            |   6 +-
> 
> drivers/gpu/drm/drm_dp_mst_topology.c:3
> - not applicable: aux dp/mst
> 
> >  drivers/gpu/drm/drm_connector.c               |  35 +++++
> >  drivers/gpu/drm/drm_sysfs.c                   |   8 +
> 
> drivers/gpu/drm/drm_edid.c:7
> drivers/gpu/drm/drm_probe_helper.c:1
> - unrelated
> 
> >  drivers/gpu/drm/exynos/exynos_hdmi.c          |   6 +-
> 
> drivers/gpu/drm/gma500/cdv_intel_dp.c:3
> drivers/gpu/drm/gma500/cdv_intel_hdmi.c:2
> drivers/gpu/drm/gma500/oaktrail_hdmi.c:1
> drivers/gpu/drm/gma500/oaktrail_lvds.c:2
> drivers/gpu/drm/gma500/psb_intel_modes.c:1
> drivers/gpu/drm/gma500/psb_intel_sdvo.c:2
> - should be updated at some point (as you pointed out).
> 
> >  drivers/gpu/drm/i915/display/intel_hdmi.c     |  12 +-
> drivers/gpu/drm/i915/intel_connector.c:1
> drivers/gpu/drm/i915/intel_crt.c:2
> - not too sure here
> 
> drivers/gpu/drm/i915/intel_dp.c:2
> - not applicable: aux dp/mst
> 
> drivers/gpu/drm/i915/intel_lvds.c:1
> drivers/gpu/drm/i915/intel_sdvo.c:2
> - not too sure here
> 
> 
> >  drivers/gpu/drm/imx/imx-ldb.c                 |   7 +-
> >  drivers/gpu/drm/imx/imx-tve.c                 |   6 +-
> >  drivers/gpu/drm/mediatek/mtk_hdmi.c           |   7 +-
> >  drivers/gpu/drm/mgag200/mgag200_mode.c        |  13 +-
> >  drivers/gpu/drm/msm/hdmi/hdmi_connector.c     |   6 +-
> drivers/gpu/drm/msm/edp/edp_ctrl.c:1
> - not applicable: aux dp/mst
> 
> drivers/gpu/drm/nouveau/nouveau_connector.c:2
> - should be updated at some point (as you pointed out).
> 
> 
> drivers/gpu/drm/panel/panel-simple.c:1
> - no applicable: panel driver
> 
> 
> >  drivers/gpu/drm/radeon/radeon_connectors.c    | 142 +++++++++++++-----
> >  drivers/gpu/drm/rockchip/inno_hdmi.c          |   6 +-
> >  drivers/gpu/drm/rockchip/rk3066_hdmi.c        |   7 +-
> >  drivers/gpu/drm/sti/sti_hdmi.c                |   6 +-
> >  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c        |   7 +-
> >  drivers/gpu/drm/tegra/hdmi.c                  |   7 +-
> >  drivers/gpu/drm/tegra/sor.c                   |   7 +-
> 
> drivers/gpu/drm/tegra/output.c:1
> - already handled in hdmi/sor
> 
> >  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c        |   6 +-
> >  drivers/gpu/drm/vc4/vc4_hdmi.c                |  12 +-
> >  drivers/gpu/drm/zte/zx_hdmi.c                 |   6 +-
> >  drivers/gpu/drm/zte/zx_vga.c                  |   6 +-
> >  include/drm/drm_connector.h                   |  18 +++
> >  26 files changed, 336 insertions(+), 121 deletions(-)
> 
> In a Tl;Dr: I think this series covers 90%+ of the existing rather huge) driverset.
> 
> For the series:
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> 
> Fwiw I'm in favour of Jani's suggestion to fold the dcc into the usual
> helper drm_connector_init(). Although since we have 130+ instances it
> might be better left for another day.
> 
> HTH
> -Emil