On Wed, Nov 01, 2017 at 10:27:23AM +0100, Maarten Lankhorst wrote: > Op 09-08-17 om 08:46 schreef Shashank Sharma: > > To pass AVI infoframes from display controller to LSPCON, we > > have to write infoframe packets into vendor specified AUX address, > > in vendor specified way. > > > > Also, LSPCON vendors provide AUX offsets, to inform the LSPCON > > chip that the AVI IF packets are written, so that the firmware > > can pick it up and apply. > > > > This patch adds function to write AVI infoframes for both MCA as > > well as Parade Tech LSPCON chips. These two vendors provide different > > methods for writing infoframes, so this patch contains two different > > functions, one for each. > > > > V2: Rebase > > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Cc: Imre Deak <imre.deak@xxxxxxxxx> > > Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> > > --- > This patch will fail to compile without 7/7 applied: > - enc_to_intel_lspcon missing. > - crtc_state->lspcon_active missing. > > > drivers/gpu/drm/i915/intel_ddi.c | 12 +- > > drivers/gpu/drm/i915/intel_drv.h | 16 +++ > > drivers/gpu/drm/i915/intel_hdmi.c | 15 ++- > > drivers/gpu/drm/i915/intel_lspcon.c | 246 ++++++++++++++++++++++++++++++++++++ > > 4 files changed, 284 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index 9384080..08f3567 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -2193,6 +2193,15 @@ static void intel_ddi_pre_enable(struct intel_encoder *encoder, > > pipe_config->shared_dpll, > > intel_crtc_has_type(pipe_config, > > INTEL_OUTPUT_DP_MST)); > > + > > + if (pipe_config->lspcon_active) { > > + struct intel_digital_port *dig_port = > > + enc_to_dig_port(&encoder->base); > > + > > + dig_port->set_infoframes(&encoder->base, > > + pipe_config->has_infoframe, > > + pipe_config, conn_state); > > + } > > } > > if (type == INTEL_OUTPUT_HDMI) { > > intel_ddi_pre_enable_hdmi(encoder, > > @@ -2734,8 +2743,6 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > > intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); > > intel_encoder->cloneable = 0; > > > > - intel_infoframe_init(intel_dig_port); > > - > > if (init_dp) { > > if (!intel_ddi_init_dp_connector(intel_dig_port)) > > goto err; > > @@ -2765,6 +2772,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > > port_name(port)); > > } > > > > + intel_infoframe_init(intel_dig_port); > > return; > > > > err: > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index adab635..99eaab6 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1052,6 +1052,10 @@ struct intel_lspcon { > > bool active; > > enum drm_lspcon_mode mode; > > enum lspcon_vendor vendor; > > + > > + void (*write_infoframe)(struct drm_encoder *encoder, > > + const struct intel_crtc_state *crtc_state, > > + union hdmi_infoframe *frame); > > }; > > > > struct intel_digital_port { > > @@ -1978,6 +1982,18 @@ void intel_color_load_luts(struct drm_crtc_state *crtc_state); > > bool lspcon_init(struct intel_digital_port *intel_dig_port); > > void lspcon_resume(struct intel_lspcon *lspcon); > > void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon); > > +bool lspcon_ycbcr420_config(struct drm_connector *connector, > > + struct intel_crtc_state *config); > > +void lspcon_write_infoframe(struct drm_encoder *encoder, > > + const struct intel_crtc_state *crtc_state, > > + enum hdmi_infoframe_type type, > > + const void *buf, ssize_t len); > > +void lspcon_set_infoframes(struct drm_encoder *encoder, > > + bool enable, > > + const struct intel_crtc_state *crtc_state, > > + const struct drm_connector_state *conn_state); > > +bool lspcon_infoframe_enabled(struct drm_encoder *encoder, > > + const struct intel_crtc_state *pipe_config); > > > > /* intel_pipe_crc.c */ > > int intel_pipe_crc_create(struct drm_minor *minor); > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > > index e4a27e1..5710029 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -1909,9 +1909,18 @@ void intel_infoframe_init(struct intel_digital_port *intel_dig_port) > > intel_dig_port->set_infoframes = g4x_set_infoframes; > > intel_dig_port->infoframe_enabled = g4x_infoframe_enabled; > > } else if (HAS_DDI(dev_priv)) { > > - intel_dig_port->write_infoframe = hsw_write_infoframe; > > - intel_dig_port->set_infoframes = hsw_set_infoframes; > > - intel_dig_port->infoframe_enabled = hsw_infoframe_enabled; > > + if (intel_dig_port->lspcon.active) { > > + intel_dig_port->write_infoframe = > > + lspcon_write_infoframe; > > + intel_dig_port->set_infoframes = lspcon_set_infoframes; > > + intel_dig_port->infoframe_enabled = > > + lspcon_infoframe_enabled; > > + } else { > > + intel_dig_port->set_infoframes = hsw_set_infoframes; > > + intel_dig_port->infoframe_enabled = > > + hsw_infoframe_enabled; > > + intel_dig_port->write_infoframe = hsw_write_infoframe; > > + } > > } else if (HAS_PCH_IBX(dev_priv)) { > > intel_dig_port->write_infoframe = ibx_write_infoframe; > > intel_dig_port->set_infoframes = ibx_set_infoframes; > > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c > > index 93507c5..b4fcd30 100644 > > --- a/drivers/gpu/drm/i915/intel_lspcon.c > > +++ b/drivers/gpu/drm/i915/intel_lspcon.c > > @@ -31,6 +31,18 @@ > > #define LSPCON_VENDOR_PARADE_OUI 0x001CF8 > > #define LSPCON_VENDOR_MCA_OUI 0x0060AD > > > > +/* AUX addresses to write AVI IF into */ > > +#define LSPCON_MCA_AVI_IF_WRITE_OFFSET 0x5C0 > > +#define LSPCON_MCA_AVI_IF_CTRL 0x5DF > > +#define LSPCON_MCA_AVI_IF_KICKOFF (1 << 0) > > +#define LSPCON_MCA_AVI_IF_HANDLED (1 << 1) > > + > > +#define LSPCON_PARADE_AVI_IF_WRITE_OFFSET 0x516 > > +#define LSPCON_PARADE_AVI_IF_CTRL 0x51E > > +#define LSPCON_PARADE_AVI_IF_KICKOFF (1 << 7) > > +#define LSPCON_PARADE_AVI_IF_STATUS 0x51F > > +#define LSPCON_PARADE_AVI_IF_HANDLED (2 << 6) > > + > > static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon) > > { > > struct intel_digital_port *dig_port = > > @@ -227,6 +239,240 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon) > > DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n"); > > } > > > > +bool lspcon_ycbcr420_config(struct drm_connector *connector, > > + struct intel_crtc_state *config) > > +{ > > + struct drm_display_info *info = &connector->display_info; > > + struct drm_display_mode *mode = &config->base.adjusted_mode; > > + > > + if (drm_mode_is_420_only(info, mode)) { > > + > > + if (!connector->ycbcr_420_allowed) { > > + DRM_ERROR("Platform doesn't support YCBCR420 output\n"); > > + return false; > > + } > > + > > + config->port_clock /= 2; > > + return true; > > + } > > + > > + return false; > > +} > > + > > +static bool _lspcon_write_avi_infoframe_parade(struct drm_dp_aux *aux, > > + const uint8_t *buffer, > > + ssize_t len) > > +{ > > + u8 avi_if_ctrl; > > + u8 avi_if_status; > > + u8 count = 0; > > + u8 retry = 5; > > + u8 avi_buf[8] = {0, }; > Initialize the first byte to 1, and you can remove the special case for count == 0? > > + uint16_t reg; > > + ssize_t ret; > > + > > + while (count++ < 4) { > > + > > + do { > > + /* Is LSPCON FW ready */ > > + reg = LSPCON_PARADE_AVI_IF_CTRL; > > + ret = drm_dp_dpcd_read(aux, reg, &avi_if_ctrl, 1); > > + if (ret < 0) { > > + DRM_ERROR("DPCD read failed, add:0x%x\n", reg); > > + return false; > > + } > > + > > + if (avi_if_ctrl & LSPCON_PARADE_AVI_IF_KICKOFF) > > + break; > > + usleep_range(100, 200); > > + } while (--retry); > > + > > + if (!(avi_if_ctrl & LSPCON_PARADE_AVI_IF_KICKOFF)) { > > + DRM_ERROR("LSPCON FW not ready for infoframes\n"); > > + return false; > > + } > > + > > + /* > > + * AVI Infoframe contains 31 bytes of data: > > + * HB0 to HB2 (3 bytes header) > > + * PB0 to PB27 (28 bytes data) > > + * As per Parade spec, while sending first block (8bytes), > > + * byte 0 is kept for request token no, and byte1 - byte7 > > + * contain frame data. So we have to pack frame like this: > > + * first block of 8 bytes: <token> <HB0-HB2> <PB0-PB3> > > + * next 3 blocks: <PB4-PB27> > > + */ > > + if (count) > > + memcpy(avi_buf, buffer + count * 8 - 1, 8); > > + else { > > + avi_buf[0] = 1; > > + memcpy(&avi_buf[1], buffer, 7); > > + } > ^This won't work, I think. > > for (count = 0; count < 4; count++) > > + > > + /* Write 8 bytes of data at a time */ > > + reg = LSPCON_PARADE_AVI_IF_WRITE_OFFSET; > > + ret = drm_dp_dpcd_write(aux, reg, avi_buf, 8); > > + if (ret < 0) { > > + DRM_ERROR("DPCD write failed, add:0x%x\n", reg); > > + return false; > > + } > > + > > + /* > > + * While sending a block of 8 byes, we need to inform block > > + * number to FW, by programming bits[1:0] of ctrl reg with > > + * block number > > + */ > > + avi_if_ctrl = 0x80 + count; > > + reg = LSPCON_PARADE_AVI_IF_CTRL; > > + ret = drm_dp_dpcd_write(aux, reg, &avi_if_ctrl, 1); > > + if (ret < 0) { > > + DRM_ERROR("DPCD write failed, add:0x%x\n", reg); > > + return false; > > + } > > + } > > + > > + /* Check LSPCON FW status */ > > + reg = LSPCON_PARADE_AVI_IF_STATUS; > > + ret = drm_dp_dpcd_read(aux, reg, &avi_if_status, 1); > > + if (ret < 0) { > > + DRM_ERROR("DPCD write failed, address 0x%x\n", reg); > > + return false; > > + } > > + > > + if (avi_if_status & LSPCON_PARADE_AVI_IF_HANDLED) > > + DRM_DEBUG_KMS("AVI IF handled by FW\n"); > you should check for (avi_if_status & AVI_IF_STATUS_MASK) == IF_HANDLED, > afaict from specs, if the status completes with an error, you still report success here. > > > + return true; > > +} > > + > > +static bool _lspcon_write_avi_infoframe_mca(struct drm_dp_aux *aux, > > + const uint8_t *buffer, ssize_t len) > > +{ > > + int ret; > > + uint32_t val = 0; > > + uint16_t reg; > > + const uint8_t *data = buffer; > > + > > + reg = LSPCON_MCA_AVI_IF_WRITE_OFFSET; > > + while (val < len) { > > + ret = drm_dp_dpcd_write(aux, reg, (void *)data, 1); > > + if (ret < 0) { > > + DRM_ERROR("DPCD write failed, add:0x%x\n", reg); > > + return false; > > + } > > + val++; reg++; data++; > > + } > > + > > + val = 0; > > + reg = LSPCON_MCA_AVI_IF_CTRL; > > + ret = drm_dp_dpcd_read(aux, reg, &val, 1); > > + if (ret < 0) { > > + DRM_ERROR("DPCD read failed, address 0x%x\n", reg); > > + return false; > > + } > > + > > + /* Indicate LSPCON chip about infoframe, clear bit 1 and set bit 0 */ > > + val &= ~LSPCON_MCA_AVI_IF_HANDLED; > > + val |= LSPCON_MCA_AVI_IF_KICKOFF; > > + > > + ret = drm_dp_dpcd_write(aux, reg, &val, 1); > > + if (ret < 0) { > > + DRM_ERROR("DPCD read failed, address 0x%x\n", reg); > > + return false; > > + } > > + > > + val = 0; > > + ret = drm_dp_dpcd_read(aux, reg, &val, 1); > > + if (ret < 0) { > > + DRM_ERROR("DPCD read failed, address 0x%x\n", reg); > > + return false; > > + } > > + > > + if (val == LSPCON_MCA_AVI_IF_HANDLED) > > + DRM_DEBUG_KMS("AVI IF handled by FW\n"); > > + > > + return true; > > +} > > + > > +void lspcon_write_infoframe(struct drm_encoder *encoder, > > + const struct intel_crtc_state *crtc_state, > > + enum hdmi_infoframe_type type, > > + const void *frame, ssize_t len) > > +{ > > + bool ret; > > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > + struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder); > > + > > + /* LSPCON only needs AVI IF */ > > + if (type != HDMI_INFOFRAME_TYPE_AVI) > > + return; > > + > > + if (lspcon->vendor == LSPCON_VENDOR_MCA) > > + ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux, > > + frame, len); > > + else > > + ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux, > > + frame, len); > > + > > + if (!ret) > > + DRM_ERROR("Failed to write AVI infoframes\n"); > Well, both infoframe write functions already report a DRM_ERROR, this print could be lower or removed. :) > > + else > > + DRM_DEBUG_DRIVER("AVI infoframes updated successfully\n"); > > +} > > + > > +void lspcon_set_infoframes(struct drm_encoder *encoder, > > + bool enable, > > + const struct intel_crtc_state *crtc_state, > > + const struct drm_connector_state *conn_state) > > +{ > > + ssize_t ret; > > + union hdmi_infoframe frame; > > + uint8_t buf[VIDEO_DIP_DATA_SIZE]; > > + struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > > + struct intel_dp *intel_dp = &dig_port->dp; > > + struct drm_connector *connector = &intel_dp->attached_connector->base; > > + const struct drm_display_mode *mode = &crtc_state->base.adjusted_mode; > > + bool is_hdmi2_sink = connector->display_info.hdmi.scdc.supported; > > + > > + if (!crtc_state->lspcon_active) { > > + DRM_ERROR("Writing infoframes while LSPCON disabled ?\n"); > > + return; > > + } > > + > > + ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, > > + mode, is_hdmi2_sink); > > + if (ret < 0) { > > + DRM_ERROR("couldn't fill AVI infoframe\n"); > > + return; > > + } > > + > > + if (crtc_state->ycbcr420) > > + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; > > + else > > + frame.avi.colorspace = HDMI_COLORSPACE_RGB; > > + > > + drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode, > > + crtc_state->limited_color_range ? > > + HDMI_QUANTIZATION_RANGE_LIMITED : > > + HDMI_QUANTIZATION_RANGE_FULL, > > + false); > > + > > + ret = hdmi_infoframe_pack(&frame, buf, sizeof(buf)); > > + if (ret < 0) { > > + DRM_ERROR("Failed to pack AVI IF\n"); > In general, it would be nice if we also get the returned error code for debugging. :) Speaking of infoframe related errors, I've started on a branch where I move the infoframe setup/validation into .compute_config(). I guess I should finish it and send it out. > > + return; > > + } > > + > > + dig_port->write_infoframe(encoder, crtc_state, HDMI_INFOFRAME_TYPE_AVI, > > + buf, ret); > > +} > > + > > +bool lspcon_infoframe_enabled(struct drm_encoder *encoder, > > + const struct intel_crtc_state *pipe_config) > > +{ > > + return enc_to_intel_lspcon(encoder)->active; > > +} > > + > > void lspcon_resume(struct intel_lspcon *lspcon) > > { > > enum drm_lspcon_mode expected_mode; > -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx