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

Ville Syrjälä
2018-12-04T19:13:20Z

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@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@amd.com>
> >>>> Cc: "Christian König" <christian.koenig@amd.com>
> >>>> Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
> >>>> Cc: Archit Taneja <architt@codeaurora.org>
> >>>> Cc: Andrzej Hajda <a.hajda@samsung.com>
> >>>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> >>>> Cc: Inki Dae <inki.dae@samsung.com>
> >>>> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> >>>> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> >>>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> >>>> Cc: Russell King <linux@armlinux.org.uk>
> >>>> Cc: CK Hu <ck.hu@mediatek.com>
> >>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> >>>> Cc: Rob Clark <robdclark@gmail.com>
> >>>> Cc: Ben Skeggs <bskeggs@redhat.com>
> >>>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >>>> Cc: Sandy Huang <hjc@rock-chips.com>
> >>>> Cc: "Heiko Stübner" <heiko@sntech.de>
> >>>> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> >>>> Cc: Vincent Abriou <vincent.abriou@st.com>
> >>>> Cc: Thierry Reding <thierry.reding@gmail.com>
> >>>> Cc: Eric Anholt <eric@anholt.net>
> >>>> Cc: Shawn Guo <shawnguo@kernel.org>
> >>>> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> >>>> Cc: amd-gfx@lists.freedesktop.org
> >>>> Cc: linux-arm-msm@vger.kernel.org
> >>>> Cc: freedreno@lists.freedesktop.org
> >>>> Cc: nouveau@lists.freedesktop.org
> >>>> Cc: linux-tegra@vger.kernel.org
> >>>> Signed-off-by: Ville Syrjälä <ville.syrjala@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@ideasonboard.com>
> >>>

--
Ville Syrjälä
Intel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org