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

On Tue, Dec 04, 2018 at 08:46:53AM +0100, Andrzej Hajda
wrote:
> 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?
void bridge_foo(...
+               ,struct drm_connector *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?
> > 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
That's not really how atomic works. One shouldn't go digging
through the obj->state pointers when we're not holding the
relevant locks anymore. The atomic way would be to pass either
both crtc state and connector state, or drm_atomic_state +
crtc/connector.
> 
> 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>
> >>>
-- 
Ville Syrjälä
Intel

On 05.12.2018 07:32, Laurent Pinchart wrote:
> Hi Ville,
>
> On Tuesday, 4 December 2018 21:13:20 EET Ville Syrjälä wrote:
>> On Tue, Dec 04, 2018 at 08:46:53AM +0100, Andrzej Hajda wrote:
>>> 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:
>>>>>> 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?
>> void bridge_foo(...
>> +               ,struct drm_connector *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?
>>>> 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
>> That's not really how atomic works. One shouldn't go digging
>> through the obj->state pointers when we're not holding the
>> relevant locks anymore. The atomic way would be to pass either
>> both crtc state and connector state, or drm_atomic_state +
>> crtc/connector.

Usually infoframe contents is generated in modesetting/enable callbacks
so the locks should be in place.

And the locks should be hold for
drm_hdmi_avi_infoframe_from_display_mode as well - it wouldn't be correct if

generated infoframe is not relevant to actual video mode. I guess that
if connector->state->crtc->mode

differs from mode passed to drm_hdmi_avi_infoframe_from_display_mode it
is a sign of possible problem.

And if they do not differ passing mode as an extra argument is redundant.

> Or a bridge state ? With chained bridges the mode can vary along the
pipeline,
> the CRTC adjusted mode will only cover the link between the CRTC and the
first
> bridge. It's only a matter of time until we need to store other
intermediate
> modes in states. I'd rather prepare for that instead of passing the
CRTC state
> to bridges.

Yes, optional bridge states seems reasonable, volunteers needed :)


Regards

Andrzej

>
>>> In moment of creating infoframe it should be set properly.
>>>
>>>>>> 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>