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.syrj...@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.deuc...@amd.com> >>>>>>> Cc: "Christian König" <christian.koe...@amd.com> >>>>>>> Cc: "David (ChunMing) Zhou" <david1.z...@amd.com> >>>>>>> Cc: Archit Taneja <arch...@codeaurora.org> >>>>>>> Cc: Andrzej Hajda <a.ha...@samsung.com> >>>>>>> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com> >>>>>>> Cc: Inki Dae <inki....@samsung.com> >>>>>>> Cc: Joonyoung Shim <jy0922.s...@samsung.com> >>>>>> Cc: Seung-Woo Kim <sw0312....@samsung.com> >>>>>>> Cc: Kyungmin Park <kyungmin.p...@samsung.com> >>>>>>> Cc: Russell King <li...@armlinux.org.uk> >>>>>>> Cc: CK Hu <ck...@mediatek.com> >>>>>>> Cc: Philipp Zabel <p.za...@pengutronix.de> >>>>>>> Cc: Rob Clark <robdcl...@gmail.com> >>>>>>> Cc: Ben Skeggs <bske...@redhat.com> >>>>>>> Cc: Tomi Valkeinen <tomi.valkei...@ti.com> >>>>>>> Cc: Sandy Huang <h...@rock-chips.com> >>>>>>> Cc: "Heiko Stübner" <he...@sntech.de> >>>>>>> Cc: Benjamin Gaignard <benjamin.gaign...@linaro.org> >>>>>>> Cc: Vincent Abriou <vincent.abr...@st.com> >>>>>>> Cc: Thierry Reding <thierry.red...@gmail.com> >>>>>>> Cc: Eric Anholt <e...@anholt.net> >>>>>>> Cc: Shawn Guo <shawn...@kernel.org> >>>>>>> Cc: Ilia Mirkin <imir...@alum.mit.edu> >>>>>>> Cc: amd-gfx@lists.freedesktop.org >>>>>>> Cc: linux-arm-...@vger.kernel.org >>>>>>> Cc: freedr...@lists.freedesktop.org >>>>>>> Cc: nouv...@lists.freedesktop.org >>>>>>> Cc: linux-te...@vger.kernel.org >>>>>>> Signed-off-by: Ville Syrjälä <ville.syrj...@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.pinch...@ideasonboard.com> _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
