On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
> On Tue, 20 Nov 2018, Ville Syrjala <ville.syrj...@linux.intel.com> 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.
>
> Overall looks about right and nice,
>
> Reviewed-by: Jani Nikula <jani.nik...@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.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-...@lists.freedesktop.org
> > Cc: linux-arm-...@vger.kernel.org
> > Cc: freedreno@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(-)
> >
> > 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
