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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Dec 05, 2018 at 09:46:40AM +0100, Andrzej Hajda wrote:
> 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@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.

Not when doing a nonblocking modeset.

> 
> 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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel