Re: [PATCH 14/18] drm/i915: Read out HDMI infoframes

Daniel Vetter
2018-09-24T16:08:14Z

On Thu, Sep 20, 2018 at 09:51:41PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Add code to read the infoframes from the video DIP and unpack them into
> the crtc state.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 17 ++++
> drivers/gpu/drm/i915/intel_drv.h | 10 ++
> drivers/gpu/drm/i915/intel_hdmi.c | 203 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 230 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 5f3bd536d261..a56289f78326 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3459,6 +3459,23 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> bxt_ddi_phy_get_lane_lat_optim_mask(encoder);
>
> intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
> +
> + intel_hdmi_read_gcp_infoframe(encoder, pipe_config);
> +
> + if (!intel_read_infoframe(encoder, pipe_config,
> + HDMI_INFOFRAME_TYPE_AVI,
> + &pipe_config->infoframes.avi))
> + DRM_ERROR("failed to read AVI infoframe\n");
> +
> + if (!intel_read_infoframe(encoder, pipe_config,
> + HDMI_INFOFRAME_TYPE_SPD,
> + &pipe_config->infoframes.spd))
> + DRM_ERROR("failed to read SPD infoframe:\n");
> +
> + if (!intel_read_infoframe(encoder, pipe_config,
> + HDMI_INFOFRAME_TYPE_VENDOR,
> + &pipe_config->infoframes.hdmi))
> + DRM_ERROR("failed to read HDMI infoframe\n");
> }
>
> static enum intel_output_type
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 357624a6bfe2..75ec99b85232 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1185,6 +1185,10 @@ struct intel_digital_port {
> const struct intel_crtc_state *crtc_state,
> unsigned int type,
> const void *frame, ssize_t len);
> + ssize_t (*read_infoframe)(struct intel_encoder *encoder,
> + const struct intel_crtc_state *crtc_state,
> + unsigned int type,
> + void *frame, ssize_t len);
> void (*set_infoframes)(struct intel_encoder *encoder,
> bool enable,
> const struct intel_crtc_state *crtc_state,
> @@ -1867,6 +1871,12 @@ void intel_infoframe_init(struct intel_digital_port *intel_dig_port);
> u32 intel_hdmi_infoframes_enabled(struct intel_encoder *encoder,
> const struct intel_crtc_state *crtc_state);
> u32 intel_hdmi_infoframe_enable(unsigned int type);
> +void intel_hdmi_read_gcp_infoframe(struct intel_encoder *encoder,
> + struct intel_crtc_state *crtc_state);
> +bool intel_read_infoframe(struct intel_encoder *encoder,
> + const struct intel_crtc_state *crtc_state,
> + enum hdmi_infoframe_type type,
> + union hdmi_infoframe *frame);
>
>
> /* intel_lvds.c */
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 491001fc0fad..27cb6ec32e94 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -203,6 +203,31 @@ static void g4x_write_infoframe(struct intel_encoder *encoder,
> POSTING_READ(VIDEO_DIP_CTL);
> }
>
> +static ssize_t g4x_read_infoframe(struct intel_encoder *encoder,
> + const struct intel_crtc_state *crtc_state,
> + unsigned int type,
> + void *frame, ssize_t len)
> +{
> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> + u32 val, *data = frame;
> + int i;
> +
> + val = I915_READ(VIDEO_DIP_CTL);
> +
> + if ((val & g4x_infoframe_enable(type)) == 0)
> + return 0;
> +
> + val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
> + val |= g4x_infoframe_index(type);
> +
> + I915_WRITE(VIDEO_DIP_CTL, val);
> +
> + for (i = 0; i < len; i += 4)
> + *data++ = I915_READ(VIDEO_DIP_DATA);
> +
> + return len;
> +}
> +
> static u32 g4x_infoframes_enabled(struct intel_encoder *encoder,
> const struct intel_crtc_state *pipe_config)
> {
> @@ -258,6 +283,32 @@ static void ibx_write_infoframe(struct intel_encoder *encoder,
> POSTING_READ(reg);
> }
>
> +static ssize_t ibx_read_infoframe(struct intel_encoder *encoder,
> + const struct intel_crtc_state *crtc_state,
> + unsigned int type,
> + void *frame, ssize_t len)
> +{
> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> + u32 val, *data = frame;
> + int i;
> +
> + val = I915_READ(TVIDEO_DIP_CTL(crtc->pipe));
> +
> + if ((val & g4x_infoframe_enable(type)) == 0)
> + return 0;
> +
> + val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
> + val |= g4x_infoframe_index(type);
> +
> + I915_WRITE(TVIDEO_DIP_CTL(crtc->pipe), val);
> +
> + for (i = 0; i < len; i += 4)
> + *data++ = I915_READ(TVIDEO_DIP_DATA(crtc->pipe));
> +
> + return len;
> +}
> +
> static u32 ibx_infoframes_enabled(struct intel_encoder *encoder,
> const struct intel_crtc_state *pipe_config)
> {
> @@ -319,6 +370,32 @@ static void cpt_write_infoframe(struct intel_encoder *encoder,
> POSTING_READ(reg);
> }
>
> +static ssize_t cpt_read_infoframe(struct intel_encoder *encoder,
> + const struct intel_crtc_state *crtc_state,
> + unsigned int type,
> + void *frame, ssize_t len)
> +{
> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> + u32 val, *data = frame;
> + int i;
> +
> + val = I915_READ(TVIDEO_DIP_CTL(crtc->pipe));
> +
> + if ((val & g4x_infoframe_enable(type)) == 0)
> + return 0;
> +
> + val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
> + val |= g4x_infoframe_index(type);
> +
> + I915_WRITE(TVIDEO_DIP_CTL(crtc->pipe), val);
> +
> + for (i = 0; i < len; i += 4)
> + *data++ = I915_READ(TVIDEO_DIP_DATA(crtc->pipe));
> +
> + return len;
> +}
> +
> static u32 cpt_infoframes_enabled(struct intel_encoder *encoder,
> const struct intel_crtc_state *pipe_config)
> {
> @@ -373,6 +450,32 @@ static void vlv_write_infoframe(struct intel_encoder *encoder,
> POSTING_READ(reg);
> }
>
> +static ssize_t vlv_read_infoframe(struct intel_encoder *encoder,
> + const struct intel_crtc_state *crtc_state,
> + unsigned int type,
> + void *frame, ssize_t len)
> +{
> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> + u32 val, *data = frame;
> + int i;
> +
> + val = I915_READ(VLV_TVIDEO_DIP_CTL(crtc->pipe));
> +
> + if ((val & g4x_infoframe_enable(type)) == 0)
> + return 0;
> +
> + val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
> + val |= g4x_infoframe_index(type);
> +
> + I915_WRITE(VLV_TVIDEO_DIP_CTL(crtc->pipe), val);
> +
> + for (i = 0; i < len; i += 4)
> + *data++ = I915_READ(VLV_TVIDEO_DIP_DATA(crtc->pipe));
> +
> + return len;
> +}
> +
> static u32 vlv_infoframes_enabled(struct intel_encoder *encoder,
> const struct intel_crtc_state *pipe_config)
> {
> @@ -425,6 +528,28 @@ static void hsw_write_infoframe(struct intel_encoder *encoder,
> POSTING_READ(ctl_reg);
> }
>
> +static ssize_t hsw_read_infoframe(struct intel_encoder *encoder,
> + const struct intel_crtc_state *crtc_state,
> + unsigned int type,
> + void *frame, ssize_t len)
> +{
> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> + u32 val, *data = frame;
> + int i;
> +
> + val = I915_READ(HSW_TVIDEO_DIP_CTL(cpu_transcoder));
> +
> + if ((val & hsw_infoframe_enable(type)) == 0)
> + return 0;
> +
> + for (i = 0; i < len; i += 4)
> + *data++ = I915_READ(hsw_dip_data_reg(dev_priv, cpu_transcoder,
> + type, i >> 2));
> +
> + return len;
> +}
> +
> static u32 hsw_infoframes_enabled(struct intel_encoder *encoder,
> const struct intel_crtc_state *pipe_config)
> {
> @@ -530,6 +655,39 @@ static void intel_write_infoframe(struct intel_encoder *encoder,
> intel_dig_port->write_infoframe(encoder, crtc_state, type, buffer, len);
> }
>
> +bool intel_read_infoframe(struct intel_encoder *encoder,
> + const struct intel_crtc_state *crtc_state,
> + enum hdmi_infoframe_type type,
> + union hdmi_infoframe *frame)

Bit a bikeshed: I'd drop the boolean here and pull the debug output in.
That way you can give a bit better hint about what's going wrong. And less
duplicated code. You can still print the type.

> +{
> + struct intel_digital_port *intel_dig_port = enc_to_dig_port(&encoder->base);
> + u8 buffer[VIDEO_DIP_DATA_SIZE];
> + ssize_t len;
> + int ret;
> +
> + if ((crtc_state->infoframes.enable &
> + intel_hdmi_infoframe_enable(type)) == 0)
> + return true;

Afaiui you only need this because g4x doesn't check the pipe (well, port)
association. But then all the tests once again check this by confirming
that the infoframe they should read out is enabled. I'd drop the check in
the various hw-specific readout functions (it's duplicated). That should
still work with g4x here, as long as you filter at this level here.

With or without the bikesheds:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
> + len = intel_dig_port->read_infoframe(encoder, crtc_state,
> + type, buffer, sizeof(buffer));
> + if (len == 0)
> + return true;
> +
> + /* Fill the 'hole' (see big comment above) at position 3 */
> + memmove(&buffer[1], &buffer[0], 3);
> +
> + /* see comment above for the reason for this offset */
> + ret = hdmi_infoframe_unpack(frame, buffer + 1, sizeof(buffer) - 1);
> + if (ret)
> + return false;
> +
> + if (frame->any.type != type)
> + return false;
> +
> + return true;
> +}
> +
> static bool
> intel_hdmi_compute_avi_infoframe(struct intel_encoder *encoder,
> struct intel_crtc_state *crtc_state,
> @@ -784,6 +942,29 @@ static bool intel_hdmi_set_gcp_infoframe(struct intel_encoder *encoder,
> return true;
> }
>
> +void intel_hdmi_read_gcp_infoframe(struct intel_encoder *encoder,
> + struct intel_crtc_state *crtc_state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> + i915_reg_t reg;
> +
> + if ((crtc_state->infoframes.enable &
> + intel_hdmi_infoframe_enable(HDMI_PACKET_TYPE_GENERAL_CONTROL)) == 0)
> + return;
> +
> + if (HAS_DDI(dev_priv))
> + reg = HSW_TVIDEO_DIP_GCP(crtc_state->cpu_transcoder);
> + else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> + reg = VLV_TVIDEO_DIP_GCP(crtc->pipe);
> + else if (HAS_PCH_SPLIT(dev_priv))
> + reg = TVIDEO_DIP_GCP(crtc->pipe);
> + else
> + return;
> +
> + crtc_state->infoframes.gcp = I915_READ(reg);
> +}
> +
> static void intel_hdmi_compute_gcp_infoframe(struct intel_encoder *encoder,
> struct intel_crtc_state *crtc_state,
> struct drm_connector_state *conn_state)
> @@ -1382,6 +1563,23 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
> pipe_config->base.adjusted_mode.crtc_clock = dotclock;
>
> pipe_config->lane_count = 4;
> +
> + intel_hdmi_read_gcp_infoframe(encoder, pipe_config);
> +
> + if (!intel_read_infoframe(encoder, pipe_config,
> + HDMI_INFOFRAME_TYPE_AVI,
> + &pipe_config->infoframes.avi))
> + DRM_ERROR("failed to read AVI infoframe\n");
> +
> + if (!intel_read_infoframe(encoder, pipe_config,
> + HDMI_INFOFRAME_TYPE_SPD,
> + &pipe_config->infoframes.spd))
> + DRM_ERROR("failed to read SPD infoframe:\n");
> +
> + if (!intel_read_infoframe(encoder, pipe_config,
> + HDMI_INFOFRAME_TYPE_VENDOR,
> + &pipe_config->infoframes.hdmi))
> + DRM_ERROR("failed to read HDMI infoframe\n");
> }
>
> static void intel_enable_hdmi_audio(struct intel_encoder *encoder,
> @@ -2481,22 +2679,27 @@ void intel_infoframe_init(struct intel_digital_port *intel_dig_port)
>
> if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> intel_dig_port->write_infoframe = vlv_write_infoframe;
> + intel_dig_port->read_infoframe = vlv_read_infoframe;
> intel_dig_port->set_infoframes = vlv_set_infoframes;
> intel_dig_port->infoframes_enabled = vlv_infoframes_enabled;
> } else if (IS_G4X(dev_priv)) {
> intel_dig_port->write_infoframe = g4x_write_infoframe;
> + intel_dig_port->read_infoframe = g4x_read_infoframe;
> intel_dig_port->set_infoframes = g4x_set_infoframes;
> intel_dig_port->infoframes_enabled = g4x_infoframes_enabled;
> } else if (HAS_DDI(dev_priv)) {
> intel_dig_port->write_infoframe = hsw_write_infoframe;
> + intel_dig_port->read_infoframe = hsw_read_infoframe;
> intel_dig_port->set_infoframes = hsw_set_infoframes;
> intel_dig_port->infoframes_enabled = hsw_infoframes_enabled;
> } else if (HAS_PCH_IBX(dev_priv)) {
> intel_dig_port->write_infoframe = ibx_write_infoframe;
> + intel_dig_port->read_infoframe = ibx_read_infoframe;
> intel_dig_port->set_infoframes = ibx_set_infoframes;
> intel_dig_port->infoframes_enabled = ibx_infoframes_enabled;
> } else {
> intel_dig_port->write_infoframe = cpt_write_infoframe;
> + intel_dig_port->read_infoframe = cpt_read_infoframe;
> intel_dig_port->set_infoframes = cpt_set_infoframes;
> intel_dig_port->infoframes_enabled = cpt_infoframes_enabled;
> }
> --
> 2.16.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation