Nouveau - Dec 2018 - [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions

On 21.11.2018 19:19, Laurent Pinchart wrote:
> Hi Ville,
>
> Thank you for the patch.
>
> On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote:
>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>
>> Make life easier for drivers by simply passing the connector
>> to drm_hdmi_avi_infoframe_from_display_mode() and
>> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
>> need to worry about is_hdmi2_sink mess.
> While this is good for display controller drivers, the change isn't
great for
> bridge drivers. Down the road we're looking at moving connector support
out of
> the bridge drivers. Adding an additional dependency to connectors in the 
> bridges will make that more difficult. Ideally bridges should retrieve the 
> information from their sink, regardless of whether it is a connector or 
> another bridge.

I agree with it, and case of sii8620 shows that there are cases where
bridge has no direct access to the connector.

On the other side,  since you are passing connector to
drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode
parameter and rename the function to
drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and
mode set on the connector differs?


Regards

Andrzej

>
> Please see below for an additional comment.
>
>> Cc: Alex Deucher <alexander.deucher at amd.com>
>> Cc: "Christian König" <christian.koenig at amd.com>
>> Cc: "David (ChunMing) Zhou" <David1.Zhou at amd.com>
>> Cc: Archit Taneja <architt at codeaurora.org>
>> Cc: Andrzej Hajda <a.hajda at samsung.com>
>> Cc: Laurent Pinchart <Laurent.pinchart at ideasonboard.com>
>> Cc: Inki Dae <inki.dae at samsung.com>
>> Cc: Joonyoung Shim <jy0922.shim at samsung.com>
>> Cc: Seung-Woo Kim <sw0312.kim at samsung.com>
>> Cc: Kyungmin Park <kyungmin.park at samsung.com>
>> Cc: Russell King <linux at armlinux.org.uk>
>> Cc: CK Hu <ck.hu at mediatek.com>
>> Cc: Philipp Zabel <p.zabel at pengutronix.de>
>> Cc: Rob Clark <robdclark at gmail.com>
>> Cc: Ben Skeggs <bskeggs at redhat.com>
>> Cc: Tomi Valkeinen <tomi.valkeinen at ti.com>
>> Cc: Sandy Huang <hjc at rock-chips.com>
>> Cc: "Heiko Stübner" <heiko at sntech.de>
>> Cc: Benjamin Gaignard <benjamin.gaignard at linaro.org>
>> Cc: Vincent Abriou <vincent.abriou at st.com>
>> Cc: Thierry Reding <thierry.reding at gmail.com>
>> Cc: Eric Anholt <eric at anholt.net>
>> Cc: Shawn Guo <shawnguo at kernel.org>
>> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
>> Cc: amd-gfx at lists.freedesktop.org
>> Cc: linux-arm-msm at vger.kernel.org
>> Cc: freedreno at lists.freedesktop.org
>> Cc: nouveau at lists.freedesktop.org
>> Cc: linux-tegra at vger.kernel.org
>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c    |  2 +-
>>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c    |  2 +-
>>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c     |  3 ++-
>>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c     |  2 +-
>>  drivers/gpu/drm/bridge/analogix-anx78xx.c |  5 ++--
>>  drivers/gpu/drm/bridge/sii902x.c          |  3 ++-
>>  drivers/gpu/drm/bridge/sil-sii8620.c      |  3 +--
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 ++-
>>  drivers/gpu/drm/drm_edid.c                | 33 ++++++++++++++---------
>>  drivers/gpu/drm/exynos/exynos_hdmi.c      |  3 ++-
>>  drivers/gpu/drm/i2c/tda998x_drv.c         |  3 ++-
>>  drivers/gpu/drm/i915/intel_hdmi.c         | 14 +++++-----
>>  drivers/gpu/drm/i915/intel_lspcon.c       | 15 ++++++-----
>>  drivers/gpu/drm/i915/intel_sdvo.c         | 10 ++++---
>>  drivers/gpu/drm/mediatek/mtk_hdmi.c       |  3 ++-
>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c    |  3 ++-
>>  drivers/gpu/drm/nouveau/dispnv50/disp.c   |  7 +++--
>>  drivers/gpu/drm/omapdrm/omap_encoder.c    |  5 ++--
>>  drivers/gpu/drm/radeon/radeon_audio.c     |  2 +-
>>  drivers/gpu/drm/rockchip/inno_hdmi.c      |  4 ++-
>>  drivers/gpu/drm/sti/sti_hdmi.c            |  3 ++-
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c    |  3 ++-
>>  drivers/gpu/drm/tegra/hdmi.c              |  3 ++-
>>  drivers/gpu/drm/tegra/sor.c               |  3 ++-
>>  drivers/gpu/drm/vc4/vc4_hdmi.c            | 11 +++++---
>>  drivers/gpu/drm/zte/zx_hdmi.c             |  4 ++-
>>  include/drm/drm_edid.h                    |  8 +++---
>>  27 files changed, 94 insertions(+), 66 deletions(-)
> For dw-hdmi and omapdrm,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>

On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda
wrote:
> On 21.11.2018 19:19, Laurent Pinchart wrote:
> > Hi Ville,
> >
> > Thank you for the patch.
> >
> > On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote:
> >> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >>
> >> Make life easier for drivers by simply passing the connector
> >> to drm_hdmi_avi_infoframe_from_display_mode() and
> >> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
> >> need to worry about is_hdmi2_sink mess.
> > While this is good for display controller drivers, the change
isn't great for
> > bridge drivers. Down the road we're looking at moving connector
support out of
> > the bridge drivers. Adding an additional dependency to connectors in
the
> > bridges will make that more difficult. Ideally bridges should retrieve
the
> > information from their sink, regardless of whether it is a connector
or
> > another bridge.
> 
> 
> I agree with it, and case of sii8620 shows that there are cases where
> bridge has no direct access to the connector.
It's just a matter of plumbing it through.
> 
> On the other side,  since you are passing connector to
> drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode
> parameter and rename the function to
> drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and
> mode set on the connector differs?
Connectors don't have a mode.
> 
> 
> Regards
> 
> Andrzej
> 
> 
> >
> > Please see below for an additional comment.
> >
> >> Cc: Alex Deucher <alexander.deucher at amd.com>
> >> Cc: "Christian König" <christian.koenig at
amd.com>
> >> Cc: "David (ChunMing) Zhou" <David1.Zhou at
amd.com>
> >> Cc: Archit Taneja <architt at codeaurora.org>
> >> Cc: Andrzej Hajda <a.hajda at samsung.com>
> >> Cc: Laurent Pinchart <Laurent.pinchart at ideasonboard.com>
> >> Cc: Inki Dae <inki.dae at samsung.com>
> >> Cc: Joonyoung Shim <jy0922.shim at samsung.com>
> >> Cc: Seung-Woo Kim <sw0312.kim at samsung.com>
> >> Cc: Kyungmin Park <kyungmin.park at samsung.com>
> >> Cc: Russell King <linux at armlinux.org.uk>
> >> Cc: CK Hu <ck.hu at mediatek.com>
> >> Cc: Philipp Zabel <p.zabel at pengutronix.de>
> >> Cc: Rob Clark <robdclark at gmail.com>
> >> Cc: Ben Skeggs <bskeggs at redhat.com>
> >> Cc: Tomi Valkeinen <tomi.valkeinen at ti.com>
> >> Cc: Sandy Huang <hjc at rock-chips.com>
> >> Cc: "Heiko Stübner" <heiko at sntech.de>
> >> Cc: Benjamin Gaignard <benjamin.gaignard at linaro.org>
> >> Cc: Vincent Abriou <vincent.abriou at st.com>
> >> Cc: Thierry Reding <thierry.reding at gmail.com>
> >> Cc: Eric Anholt <eric at anholt.net>
> >> Cc: Shawn Guo <shawnguo at kernel.org>
> >> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> >> Cc: amd-gfx at lists.freedesktop.org
> >> Cc: linux-arm-msm at vger.kernel.org
> >> Cc: freedreno at lists.freedesktop.org
> >> Cc: nouveau at lists.freedesktop.org
> >> Cc: linux-tegra at vger.kernel.org
> >> Signed-off-by: Ville Syrjälä <ville.syrjala at
linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c    |  2 +-
> >>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c    |  2 +-
> >>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c     |  3 ++-
> >>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c     |  2 +-
> >>  drivers/gpu/drm/bridge/analogix-anx78xx.c |  5 ++--
> >>  drivers/gpu/drm/bridge/sii902x.c          |  3 ++-
> >>  drivers/gpu/drm/bridge/sil-sii8620.c      |  3 +--
> >>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 ++-
> >>  drivers/gpu/drm/drm_edid.c                | 33
++++++++++++++---------
> >>  drivers/gpu/drm/exynos/exynos_hdmi.c      |  3 ++-
> >>  drivers/gpu/drm/i2c/tda998x_drv.c         |  3 ++-
> >>  drivers/gpu/drm/i915/intel_hdmi.c         | 14 +++++-----
> >>  drivers/gpu/drm/i915/intel_lspcon.c       | 15 ++++++-----
> >>  drivers/gpu/drm/i915/intel_sdvo.c         | 10 ++++---
> >>  drivers/gpu/drm/mediatek/mtk_hdmi.c       |  3 ++-
> >>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c    |  3 ++-
> >>  drivers/gpu/drm/nouveau/dispnv50/disp.c   |  7 +++--
> >>  drivers/gpu/drm/omapdrm/omap_encoder.c    |  5 ++--
> >>  drivers/gpu/drm/radeon/radeon_audio.c     |  2 +-
> >>  drivers/gpu/drm/rockchip/inno_hdmi.c      |  4 ++-
> >>  drivers/gpu/drm/sti/sti_hdmi.c            |  3 ++-
> >>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c    |  3 ++-
> >>  drivers/gpu/drm/tegra/hdmi.c              |  3 ++-
> >>  drivers/gpu/drm/tegra/sor.c               |  3 ++-
> >>  drivers/gpu/drm/vc4/vc4_hdmi.c            | 11 +++++---
> >>  drivers/gpu/drm/zte/zx_hdmi.c             |  4 ++-
> >>  include/drm/drm_edid.h                    |  8 +++---
> >>  27 files changed, 94 insertions(+), 66 deletions(-)
> > For dw-hdmi and omapdrm,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at
ideasonboard.com>
> >
-- 
Ville Syrjälä
Intel

On 03.12.2018 22:38, Ville Syrjälä wrote:
> On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote:
>> On 21.11.2018 19:19, Laurent Pinchart wrote:
>>> Hi Ville,
>>>
>>> Thank you for the patch.
>>>
>>> On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote:
>>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>
>>>> Make life easier for drivers by simply passing the connector
>>>> to drm_hdmi_avi_infoframe_from_display_mode() and
>>>> drm_hdmi_avi_infoframe_quant_range(). That way drivers
don't
>>>> need to worry about is_hdmi2_sink mess.
>>> While this is good for display controller drivers, the change
isn't great for
>>> bridge drivers. Down the road we're looking at moving connector
support out of
>>> the bridge drivers. Adding an additional dependency to connectors
in the
>>> bridges will make that more difficult. Ideally bridges should
retrieve the
>>> information from their sink, regardless of whether it is a
connector or
>>> another bridge.
>>
>> I agree with it, and case of sii8620 shows that there are cases where
>> bridge has no direct access to the connector.
> It's just a matter of plumbing it through.

What do you mean exactly?

>
>> On the other side,  since you are passing connector to
>> drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode
>> parameter and rename the function to
>> drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and
>> mode set on the connector differs?
> Connectors don't have a mode.

As they are passing video stream they should have it, even if not
directly, for example:

connector->state->crtc->mode

In moment of creating infoframe it should be set properly.


Regards

Andrzej

>
>>
>> Regards
>>
>> Andrzej
>>
>>
>>> Please see below for an additional comment.
>>>
>>>> Cc: Alex Deucher <alexander.deucher at amd.com>
>>>> Cc: "Christian König" <christian.koenig at
amd.com>
>>>> Cc: "David (ChunMing) Zhou" <David1.Zhou at
amd.com>
>>>> Cc: Archit Taneja <architt at codeaurora.org>
>>>> Cc: Andrzej Hajda <a.hajda at samsung.com>
>>>> Cc: Laurent Pinchart <Laurent.pinchart at
ideasonboard.com>
>>>> Cc: Inki Dae <inki.dae at samsung.com>
>>>> Cc: Joonyoung Shim <jy0922.shim at samsung.com>
>>>> Cc: Seung-Woo Kim <sw0312.kim at samsung.com>
>>>> Cc: Kyungmin Park <kyungmin.park at samsung.com>
>>>> Cc: Russell King <linux at armlinux.org.uk>
>>>> Cc: CK Hu <ck.hu at mediatek.com>
>>>> Cc: Philipp Zabel <p.zabel at pengutronix.de>
>>>> Cc: Rob Clark <robdclark at gmail.com>
>>>> Cc: Ben Skeggs <bskeggs at redhat.com>
>>>> Cc: Tomi Valkeinen <tomi.valkeinen at ti.com>
>>>> Cc: Sandy Huang <hjc at rock-chips.com>
>>>> Cc: "Heiko Stübner" <heiko at sntech.de>
>>>> Cc: Benjamin Gaignard <benjamin.gaignard at linaro.org>
>>>> Cc: Vincent Abriou <vincent.abriou at st.com>
>>>> Cc: Thierry Reding <thierry.reding at gmail.com>
>>>> Cc: Eric Anholt <eric at anholt.net>
>>>> Cc: Shawn Guo <shawnguo at kernel.org>
>>>> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
>>>> Cc: amd-gfx at lists.freedesktop.org
>>>> Cc: linux-arm-msm at vger.kernel.org
>>>> Cc: freedreno at lists.freedesktop.org
>>>> Cc: nouveau at lists.freedesktop.org
>>>> Cc: linux-tegra at vger.kernel.org
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala at
linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c    |  2 +-
>>>>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c    |  2 +-
>>>>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c     |  3 ++-
>>>>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c     |  2 +-
>>>>  drivers/gpu/drm/bridge/analogix-anx78xx.c |  5 ++--
>>>>  drivers/gpu/drm/bridge/sii902x.c          |  3 ++-
>>>>  drivers/gpu/drm/bridge/sil-sii8620.c      |  3 +--
>>>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 ++-
>>>>  drivers/gpu/drm/drm_edid.c                | 33
++++++++++++++---------
>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c      |  3 ++-
>>>>  drivers/gpu/drm/i2c/tda998x_drv.c         |  3 ++-
>>>>  drivers/gpu/drm/i915/intel_hdmi.c         | 14 +++++-----
>>>>  drivers/gpu/drm/i915/intel_lspcon.c       | 15 ++++++-----
>>>>  drivers/gpu/drm/i915/intel_sdvo.c         | 10 ++++---
>>>>  drivers/gpu/drm/mediatek/mtk_hdmi.c       |  3 ++-
>>>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c    |  3 ++-
>>>>  drivers/gpu/drm/nouveau/dispnv50/disp.c   |  7 +++--
>>>>  drivers/gpu/drm/omapdrm/omap_encoder.c    |  5 ++--
>>>>  drivers/gpu/drm/radeon/radeon_audio.c     |  2 +-
>>>>  drivers/gpu/drm/rockchip/inno_hdmi.c      |  4 ++-
>>>>  drivers/gpu/drm/sti/sti_hdmi.c            |  3 ++-
>>>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c    |  3 ++-
>>>>  drivers/gpu/drm/tegra/hdmi.c              |  3 ++-
>>>>  drivers/gpu/drm/tegra/sor.c               |  3 ++-
>>>>  drivers/gpu/drm/vc4/vc4_hdmi.c            | 11 +++++---
>>>>  drivers/gpu/drm/zte/zx_hdmi.c             |  4 ++-
>>>>  include/drm/drm_edid.h                    |  8 +++---
>>>>  27 files changed, 94 insertions(+), 66 deletions(-)
>>> For dw-hdmi and omapdrm,
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at
ideasonboard.com>
>>>