Ville Syrjälä
2018-Nov-21 11:51 UTC
[Nouveau] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:> On Tue, 20 Nov 2018, Ville Syrjala <ville.syrjala at linux.intel.com> 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. > > Overall looks about right and nice, > > Reviewed-by: Jani Nikula <jani.nikula at intel.com> > > But please do take that with a grain of salt for everything outside of > i915 and drm core. > > Please also find a few comments inline below. > > > 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(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > > index 4cfecdce29a3..1f0426d2fc2a 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > > @@ -1682,7 +1682,7 @@ static void dce_v10_0_afmt_setmode(struct drm_encoder *encoder, > > dce_v10_0_audio_write_sad_regs(encoder); > > dce_v10_0_audio_write_latency_fields(encoder, mode); > > > > - err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); > > + err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode); > > if (err < 0) { > > DRM_ERROR("failed to setup AVI infoframe: %zd\n", err); > > return; > > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c > > index 7c868916d90f..2280b971d758 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c > > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c > > @@ -1724,7 +1724,7 @@ static void dce_v11_0_afmt_setmode(struct drm_encoder *encoder, > > dce_v11_0_audio_write_sad_regs(encoder); > > dce_v11_0_audio_write_latency_fields(encoder, mode); > > > > - err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); > > + err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode); > > if (err < 0) { > > DRM_ERROR("failed to setup AVI infoframe: %zd\n", err); > > return; > > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > > index 17eaaba36017..db443ec53d3a 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > > @@ -1423,6 +1423,7 @@ static void dce_v6_0_audio_set_avi_infoframe(struct drm_encoder *encoder, > > struct amdgpu_device *adev = dev->dev_private; > > struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder); > > struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv; > > + struct drm_connector *connector = amdgpu_get_connector_for_encoder(encoder); > > struct hdmi_avi_infoframe frame; > > u8 buffer[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE]; > > uint8_t *payload = buffer + 3; > > @@ -1430,7 +1431,7 @@ static void dce_v6_0_audio_set_avi_infoframe(struct drm_encoder *encoder, > > ssize_t err; > > u32 tmp; > > > > - err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); > > + err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode); > > if (err < 0) { > > DRM_ERROR("failed to setup AVI infoframe: %zd\n", err); > > return; > > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > > index 8c0576978d36..13da915991dd 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > > @@ -1616,7 +1616,7 @@ static void dce_v8_0_afmt_setmode(struct drm_encoder *encoder, > > dce_v8_0_audio_write_sad_regs(encoder); > > dce_v8_0_audio_write_latency_fields(encoder, mode); > > > > - err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); > > + err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode); > > if (err < 0) { > > DRM_ERROR("failed to setup AVI infoframe: %zd\n", err); > > return; > > diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c > > index f8433c93f463..e11309e9bc4f 100644 > > --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c > > +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c > > @@ -1094,8 +1094,9 @@ static void anx78xx_bridge_mode_set(struct drm_bridge *bridge, > > > > mutex_lock(&anx78xx->lock); > > > > - err = drm_hdmi_avi_infoframe_from_display_mode(&frame, adjusted_mode, > > - false); > > + err = drm_hdmi_avi_infoframe_from_display_mode(&frame, > > + &anx78xx->connector, > > + adjusted_mode); > > if (err) { > > DRM_ERROR("Failed to setup AVI infoframe: %d\n", err); > > goto unlock; > > diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c > > index bfa902013aa4..a9b4f45ae87c 100644 > > --- a/drivers/gpu/drm/bridge/sii902x.c > > +++ b/drivers/gpu/drm/bridge/sii902x.c > > @@ -258,7 +258,8 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, > > if (ret) > > return; > > > > - ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj, false); > > + ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, > > + &sii902x->connector, adj); > > if (ret < 0) { > > DRM_ERROR("couldn't fill AVI infoframe\n"); > > return; > > diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c > > index a6e8f4591e63..0cc293a6ac24 100644 > > --- a/drivers/gpu/drm/bridge/sil-sii8620.c > > +++ b/drivers/gpu/drm/bridge/sil-sii8620.c > > @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620 *ctx, > > int ret; > > > > ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi, > > - mode, > > - true); > > + NULL, mode); > > if (ctx->use_packed_pixel) > > frm.avi.colorspace = HDMI_COLORSPACE_YUV422; > > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > index 64c3cf027518..88b720b63126 100644 > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode) > > u8 val; > > > > /* Initialise info frame from DRM mode */ > > - drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); > > + drm_hdmi_avi_infoframe_from_display_mode(&frame, > > + &hdmi->connector, mode); > > > > if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format)) > > frame.colorspace = HDMI_COLORSPACE_YUV444; > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > index b506e3622b08..501ac05ba7da 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct drm_connector *connector, > > } > > EXPORT_SYMBOL(drm_set_preferred_mode); > > > > +static bool is_hdmi2_sink(struct drm_connector *connector) > > You're usually known for adding const all around, why not const pointer > here and in all the other drm_* functions that call this?My current approach is to constify states/fbs/etc. but not so much crtcs/connectors/etc. Too much const can sometimes get in the way of things requiring that you remove the const later. But I guess in this case the const shouldn't really get in the way of anything because these are pretty much supposed to be pure functions.> > > +{ > > + /* > > + * FIXME: sil-sii8620 doesn't have a connector around when > > + * we need one, so we have to be prepared for a NULL connector. > > + */ > > + if (!connector) > > + return false; > > This actually changes the is_hdmi2_sink value for sil-sii8620.Hmm. No idea why they would have set that to true when everyone else is passing false. I guess I can change this to true to not change it. IIRC that was the only driver that didn't have a connector around. That said, I was actually thinking of removing this hdmi2 vs. not stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it "just in case", but we already have a similar issue with earlier cea-861 revisions and haven't seen any bugs where an older monitor would get confused by a VIC not yet defined when the monitor was produced. -- Ville Syrjälä Intel
Andrzej Hajda
2018-Nov-29 08:46 UTC
[Nouveau] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
Quite late, hopefully not too late. On 21.11.2018 12:51, Ville Syrjälä wrote:> On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote: >> >>> return; >>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c >>> index a6e8f4591e63..0cc293a6ac24 100644 >>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c >>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c >>> @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620 *ctx, >>> int ret; >>> >>> ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi, >>> - mode, >>> - true); >>> + NULL, mode); >>> if (ctx->use_packed_pixel) >>> frm.avi.colorspace = HDMI_COLORSPACE_YUV422; >>> >>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> index 64c3cf027518..88b720b63126 100644 >>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode) >>> u8 val; >>> >>> /* Initialise info frame from DRM mode */ >>> - drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); >>> + drm_hdmi_avi_infoframe_from_display_mode(&frame, >>> + &hdmi->connector, mode); >>> >>> if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format)) >>> frame.colorspace = HDMI_COLORSPACE_YUV444; >>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >>> index b506e3622b08..501ac05ba7da 100644 >>> --- a/drivers/gpu/drm/drm_edid.c >>> +++ b/drivers/gpu/drm/drm_edid.c >>> @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct drm_connector *connector, >>> } >>> EXPORT_SYMBOL(drm_set_preferred_mode); >>> >>> +static bool is_hdmi2_sink(struct drm_connector *connector) >> You're usually known for adding const all around, why not const pointer >> here and in all the other drm_* functions that call this? > My current approach is to constify states/fbs/etc. but not so much > crtcs/connectors/etc. Too much const can sometimes get in the way > of things requiring that you remove the const later. But I guess > in this case the const shouldn't really get in the way of anything > because these are pretty much supposed to be pure functions. > >>> +{ >>> + /* >>> + * FIXME: sil-sii8620 doesn't have a connector around when >>> + * we need one, so we have to be prepared for a NULL connector. >>> + */ >>> + if (!connector) >>> + return false; >> This actually changes the is_hdmi2_sink value for sil-sii8620. > Hmm. No idea why they would have set that to true when everyone else is > passing false.Because false does not work :) More precisely MHLv3 (used in Sii8620) uses CTA-861-F standard for infoframes, which is specific to HDMI2.0. Unfortunately I have no access to MHL specs, but my experiments and vendor drivers strongly suggests it is done this way. This is important in case of 4K modes which are handled differently by HDMI 1.4 and HDMI2.0. The pipeline looks like (in parenthesis HDMI version on the stream): exynos_hdmi --(1.4)--> SII8620 --(2.0)--> MHL_dongle --(1.4)--> TV> I guess I can change this to true to not change it. IIRC > that was the only driver that didn't have a connector around. > > That said, I was actually thinking of removing this hdmi2 vs. not > stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it > "just in case", but we already have a similar issue with earlier > cea-861 revisions and haven't seen any bugs where an older monitor > would get confused by a VIC not yet defined when the monitor was > produced. >Are you sure this is a good idea? As I said earlier 4K modes are present in HDMI 1.4 and 2.0 but they are handled differently, so this is not only matter of unknown VIC in AVIF. Regards Andrzej
Ville Syrjälä
2018-Dec-03 21:48 UTC
[Nouveau] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Thu, Nov 29, 2018 at 09:46:16AM +0100, Andrzej Hajda wrote:> Quite late, hopefully not too late. > > > On 21.11.2018 12:51, Ville Syrjälä wrote: > > On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote: > >> > >>> return; > >>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c > >>> index a6e8f4591e63..0cc293a6ac24 100644 > >>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c > >>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c > >>> @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620 *ctx, > >>> int ret; > >>> > >>> ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi, > >>> - mode, > >>> - true); > >>> + NULL, mode); > >>> if (ctx->use_packed_pixel) > >>> frm.avi.colorspace = HDMI_COLORSPACE_YUV422; > >>> > >>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >>> index 64c3cf027518..88b720b63126 100644 > >>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >>> @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode) > >>> u8 val; > >>> > >>> /* Initialise info frame from DRM mode */ > >>> - drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); > >>> + drm_hdmi_avi_infoframe_from_display_mode(&frame, > >>> + &hdmi->connector, mode); > >>> > >>> if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format)) > >>> frame.colorspace = HDMI_COLORSPACE_YUV444; > >>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > >>> index b506e3622b08..501ac05ba7da 100644 > >>> --- a/drivers/gpu/drm/drm_edid.c > >>> +++ b/drivers/gpu/drm/drm_edid.c > >>> @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct drm_connector *connector, > >>> } > >>> EXPORT_SYMBOL(drm_set_preferred_mode); > >>> > >>> +static bool is_hdmi2_sink(struct drm_connector *connector) > >> You're usually known for adding const all around, why not const pointer > >> here and in all the other drm_* functions that call this? > > My current approach is to constify states/fbs/etc. but not so much > > crtcs/connectors/etc. Too much const can sometimes get in the way > > of things requiring that you remove the const later. But I guess > > in this case the const shouldn't really get in the way of anything > > because these are pretty much supposed to be pure functions. > > > >>> +{ > >>> + /* > >>> + * FIXME: sil-sii8620 doesn't have a connector around when > >>> + * we need one, so we have to be prepared for a NULL connector. > >>> + */ > >>> + if (!connector) > >>> + return false; > >> This actually changes the is_hdmi2_sink value for sil-sii8620. > > Hmm. No idea why they would have set that to true when everyone else is > > passing false. > > > Because false does not work :) More precisely MHLv3 (used in Sii8620) > uses CTA-861-F standard for infoframes, which is specific to HDMI2.0. > > Unfortunately I have no access to MHL specs, but my experiments and > vendor drivers strongly suggests it is done this way. > > This is important in case of 4K modes which are handled differently by > HDMI 1.4 and HDMI2.0.HDMI 2.0 handles 4k just like 1.4 handled it when you use one of the 4k modes defined in 1.4. Only if you use features beyond 1.4 do we switch over to the HDMI 2.0 specific signalling.> > The pipeline looks like (in parenthesis HDMI version on the stream): > > exynos_hdmi --(1.4)--> SII8620 --(2.0)--> MHL_dongle --(1.4)--> TV > > > > I guess I can change this to true to not change it. IIRC > > that was the only driver that didn't have a connector around. > > > > That said, I was actually thinking of removing this hdmi2 vs. not > > stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it > > "just in case", but we already have a similar issue with earlier > > cea-861 revisions and haven't seen any bugs where an older monitor > > would get confused by a VIC not yet defined when the monitor was > > produced. > > > Are you sure this is a good idea? As I said earlier 4K modes are present > in HDMI 1.4 and 2.0 but they are handled differently, so this is not > only matter of unknown VIC in AVIF. > > > Regards > > Andrzej-- Ville Syrjälä Intel
Apparently Analagous Threads
- [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
- [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
- [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
- [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
- [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions