On Thu, Oct 25, 2018 at 05:08:39PM +0300, Ville Syrjälä wrote:
> On Wed, Oct 24, 2018 at 03:28:33PM -0700, Manasi Navare wrote:
> > Infoframes are used to send secondary data packets. This patch
> > adds support for DSC Picture parameter set secondary data packets
> > in the existing write_infoframe helpers.
> >
> > v2:
> > * Rebase on drm-tip (Manasi)
> >
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++++++++--
> > 2 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 64cca0a83cf7..0ecdc95f56d8 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4545,6 +4545,7 @@ enum {
> > * of the infoframe structure specified by CEA-861. */
> > #define VIDEO_DIP_DATA_SIZE 32
> > #define VIDEO_DIP_VSC_DATA_SIZE 36
> > +#define VIDEO_DIP_PPS_DATA_SIZE 132
> > #define VIDEO_DIP_CTL _MMIO(0x61170)
> > /* Pre HSW: */
> > #define VIDEO_DIP_ENABLE (1 << 31)
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index d3e653640ce7..02fb54737d92 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -115,6 +115,8 @@ static u32 hsw_infoframe_enable(unsigned int type)
> > switch (type) {
> > case DP_SDP_VSC:
> > return VIDEO_DIP_ENABLE_VSC_HSW;
> > + case DP_SDP_PPS:
> > + return VDIP_ENABLE_PPS;
>
> Hmm. Why is that bit named so differently to the rest?
Will have to address the mess around VDIP_CVTL reg earlier defs in a separate patch set
>
> > case HDMI_INFOFRAME_TYPE_AVI:
> > return VIDEO_DIP_ENABLE_AVI_HSW;
> > case HDMI_INFOFRAME_TYPE_SPD:
> > @@ -136,6 +138,8 @@ hsw_dip_data_reg(struct drm_i915_private *dev_priv,
> > switch (type) {
> > case DP_SDP_VSC:
> > return HSW_TVIDEO_DIP_VSC_DATA(cpu_transcoder, i);
> > + case DP_SDP_PPS:
> > + return ICL_VIDEO_DIP_PPS_DATA(cpu_transcoder, i);
> > case HDMI_INFOFRAME_TYPE_AVI:
> > return HSW_TVIDEO_DIP_AVI_DATA(cpu_transcoder, i);
> > case HDMI_INFOFRAME_TYPE_SPD:
> > @@ -148,6 +152,18 @@ hsw_dip_data_reg(struct drm_i915_private *dev_priv,
> > }
> > }
> >
> > +static int hsw_dip_data_size(unsigned int type)
> > +{
> > + switch (type) {
> > + case DP_SDP_VSC:
> > + return VIDEO_DIP_VSC_DATA_SIZE;
> > + case DP_SDP_PPS:
> > + return VIDEO_DIP_PPS_DATA_SIZE;
> > + default:
> > + return VIDEO_DIP_DATA_SIZE;
> > + }
> > +}
> > +
> > static void g4x_write_infoframe(struct intel_encoder *encoder,
> > const struct intel_crtc_state *crtc_state,
> > unsigned int type,
> > @@ -382,11 +398,14 @@ static void hsw_write_infoframe(struct intel_encoder *encoder,
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > i915_reg_t ctl_reg = HSW_TVIDEO_DIP_CTL(cpu_transcoder);
> > - int data_size = type == DP_SDP_VSC ?
> > - VIDEO_DIP_VSC_DATA_SIZE : VIDEO_DIP_DATA_SIZE;
> > + i915_reg_t data_reg;
> > + int data_size = 0;
>
> =0 is unnecessary.
This was added to adderss a warning that data_size is uninitialized.
But will double check again if its really needed.
>
> > int i;
> > u32 val = I915_READ(ctl_reg);
> >
> > + data_size = hsw_dip_data_size(type);
> > + data_reg = hsw_dip_data_reg(dev_priv, cpu_transcoder, type, 0);
>
> data_reg is unused.
Yes I think the cleanup patch series that was sent sometime recently now
uses hsw_dip_data_reg directly in I915_WRITE call.
I will remove the data_reg.
Manasi
>
> > +
> > val &= ~hsw_infoframe_enable(type);
> > I915_WRITE(ctl_reg, val);
> >
> > --
> > 2.18.0
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
