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

If this information is useful, please help other people find it:
Share via:

Andrzej Hajda

2018-Dec-04 07:46 UTC

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

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>
>>>

Ville Syrjälä

2018-Dec-04 19:13 UTC

head link

[Nouveau] [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

Laurent Pinchart

2018-Dec-05 06:32 UTC

head link

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

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.
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.
> > 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>
-- 
Regards,

Laurent Pinchart

Reasonably Related Threads

Search for more apparently analagous threads

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

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

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

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

Reasonably Related Threads