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ä <[email protected]> > >>>>> > >>>>> 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 <[email protected]> > >>>>> Cc: "Christian König" <[email protected]> > >>>>> Cc: "David (ChunMing) Zhou" <[email protected]> > >>>>> Cc: Archit Taneja <[email protected]> > >>>>> Cc: Andrzej Hajda <[email protected]> > >>>>> Cc: Laurent Pinchart <[email protected]> > >>>>> Cc: Inki Dae <[email protected]> > >>>>> Cc: Joonyoung Shim <[email protected]> > >>>> Cc: Seung-Woo Kim <[email protected]> > >>>>> Cc: Kyungmin Park <[email protected]> > >>>>> Cc: Russell King <[email protected]> > >>>>> Cc: CK Hu <[email protected]> > >>>>> Cc: Philipp Zabel <[email protected]> > >>>>> Cc: Rob Clark <[email protected]> > >>>>> Cc: Ben Skeggs <[email protected]> > >>>>> Cc: Tomi Valkeinen <[email protected]> > >>>>> Cc: Sandy Huang <[email protected]> > >>>>> Cc: "Heiko Stübner" <[email protected]> > >>>>> Cc: Benjamin Gaignard <[email protected]> > >>>>> Cc: Vincent Abriou <[email protected]> > >>>>> Cc: Thierry Reding <[email protected]> > >>>>> Cc: Eric Anholt <[email protected]> > >>>>> Cc: Shawn Guo <[email protected]> > >>>>> Cc: Ilia Mirkin <[email protected]> > >>>>> Cc: [email protected] > >>>>> Cc: [email protected] > >>>>> Cc: [email protected] > >>>>> Cc: [email protected] > >>>>> Cc: [email protected] > >>>>> Signed-off-by: Ville Syrjälä <[email protected]> > >>>>> --- > >>>>> > >>>>> 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 <[email protected]> -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
