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

["Ville Syrjälä" <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
> On Tue, 20 Nov 2018, Ville Syrjala <ville.syrjala@linux.intel.com> 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.
> 
> Overall looks about right and nice,
> 
> Reviewed-by: Jani Nikula <jani.nikula@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@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(-)
> >
> > 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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno