Re: [Intel-gfx] [PATCH 11/18] drm/i915: Return the mask of enabled infoframes from ->inforame_enabled()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 24, 2018 at 05:51:16PM +0200, Daniel Vetter wrote:
> On Thu, Sep 20, 2018 at 09:51:38PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We want to start tracking which infoframes are enabled, so let's replace
> > the boolean flag with a bitmask.
> > 
> > We'll abstract the bitmask so that it's not platform dependent. That
> > will allow us to examine the bitmask later in platform independent code.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c  |  2 +-
> >  drivers/gpu/drm/i915/intel_drv.h  |  4 +-
> >  drivers/gpu/drm/i915/intel_hdmi.c | 87 ++++++++++++++++++++++++++++-----------
> >  3 files changed, 68 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 086e3f940586..098a0e4edf2a 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -3390,7 +3390,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> >  		pipe_config->has_hdmi_sink = true;
> >  		intel_dig_port = enc_to_dig_port(&encoder->base);
> >  
> > -		if (intel_dig_port->infoframe_enabled(encoder, pipe_config))
> > +		if (intel_hdmi_infoframes_enabled(encoder, pipe_config))
> >  			pipe_config->has_infoframe = true;
> >  
> >  		if ((temp & TRANS_DDI_HDMI_SCRAMBLING_MASK) ==
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index e0f3a79fc75e..6815c69aac2f 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1181,7 +1181,7 @@ struct intel_digital_port {
> >  			       bool enable,
> >  			       const struct intel_crtc_state *crtc_state,
> >  			       const struct drm_connector_state *conn_state);
> > -	bool (*infoframe_enabled)(struct intel_encoder *encoder,
> > +	u32 (*infoframes_enabled)(struct intel_encoder *encoder,
> >  				  const struct intel_crtc_state *pipe_config);
> >  };
> >  
> > @@ -1856,6 +1856,8 @@ bool intel_hdmi_handle_sink_scrambling(struct intel_encoder *encoder,
> >  				       bool scrambling);
> >  void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
> >  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);
> >  
> >  
> >  /* intel_lvds.c */
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index c3c2a638d062..a8fcddb199ae 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -100,10 +100,14 @@ static u32 g4x_infoframe_index(unsigned int type)
> >  static u32 g4x_infoframe_enable(unsigned int type)
> >  {
> >  	switch (type) {
> > +	case HDMI_PACKET_TYPE_NULL:
> > +		return VIDEO_DIP_ENABLE; /* slight lie */
> 
> Not exactly sure why we're tracking this one here, but not for hsw.

HSW+ doesn't have a DIP enable bit like this. It only has the
bits to enable specific infoframes.

> Shouldn't we include that one if the DDI port is in hdmi mode? Would be
> more consistent I think.

Yes that would seem like the more correct thing. I think the reason
I did this here was so that I could map the DIP_ENABLE bit to
something unique. Would allow us to differentiate between the
"DIP enabled with no infoframes enabled" vs. "DIP enabled with
some infoframes enabled" cases. But seeing as we always enable some
infoframes I guess this doesn't really provide us with anything
particularly useful.

That said, I'm actually not sure whether the hw will send the null
packets if we don't enable the DIP. Would require a HDMI analyzer
to confirm.

Hmm. Actually gen4 bspec tells me:
"If DIP is enabled but DIP types are all disabled, no DIP is sent.
 However, a single Null DIP will be sent at the same point in the
 stream that DIP packets would have been sent. This is done to
 keep the port in HDMI mode, otherwise it would revert to DVI mode.
 The "Null packets enabled during vsync" mode (bit #9 of port
 control register) overrides this behavior."

So I guess mapping the null packet to the DIP enable bit is more or
less correct. Although the spec doesn't quite say whether the null
packet is also sent when some DIP types are also enabled, or if it
is only send when no DIP types are enabled.

So to match the hw I guess the readout should really do something
like:

if (hdmi & HDMI_MODE || dip_ctl & DIP_ENABLE)
	infoframes |= TYPE_NULL;

but that would again mean that we can't tell the two cases
apart.

> 
> Aside from that lgtm, has my
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> once we figured the TYPE_NULL thing out.
> -Daniel
> 
> >  	case HDMI_PACKET_TYPE_GENERAL_CONTROL:
> >  		return VIDEO_DIP_ENABLE_GCP;
> >  	case HDMI_PACKET_TYPE_GAMUT_METADATA:
> >  		return VIDEO_DIP_ENABLE_GAMUT;
> > +	case DP_SDP_VSC:
> > +		return 0;
> >  	case HDMI_INFOFRAME_TYPE_AVI:
> >  		return VIDEO_DIP_ENABLE_AVI;
> >  	case HDMI_INFOFRAME_TYPE_SPD:
> > @@ -119,6 +123,8 @@ static u32 g4x_infoframe_enable(unsigned int type)
> >  static u32 hsw_infoframe_enable(unsigned int type)
> >  {
> >  	switch (type) {
> > +	case HDMI_PACKET_TYPE_NULL:
> > +		return 0;
> >  	case HDMI_PACKET_TYPE_GENERAL_CONTROL:
> >  		return VIDEO_DIP_ENABLE_GCP_HSW;
> >  	case HDMI_PACKET_TYPE_GAMUT_METADATA:
> > @@ -197,19 +203,19 @@ static void g4x_write_infoframe(struct intel_encoder *encoder,
> >  	POSTING_READ(VIDEO_DIP_CTL);
> >  }
> >  
> > -static bool g4x_infoframe_enabled(struct intel_encoder *encoder,
> > +static u32 g4x_infoframes_enabled(struct intel_encoder *encoder,
> >  				  const struct intel_crtc_state *pipe_config)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	u32 val = I915_READ(VIDEO_DIP_CTL);
> >  
> >  	if ((val & VIDEO_DIP_ENABLE) == 0)
> > -		return false;
> > +		return 0;
> >  
> >  	if ((val & VIDEO_DIP_PORT_MASK) != VIDEO_DIP_PORT(encoder->port))
> > -		return false;
> > +		return 0;
> >  
> > -	return val & (VIDEO_DIP_ENABLE_AVI |
> > +	return val & (VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> >  		      VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
> >  }
> >  
> > @@ -252,7 +258,7 @@ static void ibx_write_infoframe(struct intel_encoder *encoder,
> >  	POSTING_READ(reg);
> >  }
> >  
> > -static bool ibx_infoframe_enabled(struct intel_encoder *encoder,
> > +static u32 ibx_infoframes_enabled(struct intel_encoder *encoder,
> >  				  const struct intel_crtc_state *pipe_config)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > @@ -261,12 +267,12 @@ static bool ibx_infoframe_enabled(struct intel_encoder *encoder,
> >  	u32 val = I915_READ(reg);
> >  
> >  	if ((val & VIDEO_DIP_ENABLE) == 0)
> > -		return false;
> > +		return 0;
> >  
> >  	if ((val & VIDEO_DIP_PORT_MASK) != VIDEO_DIP_PORT(encoder->port))
> > -		return false;
> > +		return 0;
> >  
> > -	return val & (VIDEO_DIP_ENABLE_AVI |
> > +	return val & (VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> >  		      VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> >  		      VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> >  }
> > @@ -313,7 +319,7 @@ static void cpt_write_infoframe(struct intel_encoder *encoder,
> >  	POSTING_READ(reg);
> >  }
> >  
> > -static bool cpt_infoframe_enabled(struct intel_encoder *encoder,
> > +static u32 cpt_infoframes_enabled(struct intel_encoder *encoder,
> >  				  const struct intel_crtc_state *pipe_config)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > @@ -321,9 +327,9 @@ static bool cpt_infoframe_enabled(struct intel_encoder *encoder,
> >  	u32 val = I915_READ(TVIDEO_DIP_CTL(pipe));
> >  
> >  	if ((val & VIDEO_DIP_ENABLE) == 0)
> > -		return false;
> > +		return 0;
> >  
> > -	return val & (VIDEO_DIP_ENABLE_AVI |
> > +	return val & (VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> >  		      VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> >  		      VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> >  }
> > @@ -367,7 +373,7 @@ static void vlv_write_infoframe(struct intel_encoder *encoder,
> >  	POSTING_READ(reg);
> >  }
> >  
> > -static bool vlv_infoframe_enabled(struct intel_encoder *encoder,
> > +static u32 vlv_infoframes_enabled(struct intel_encoder *encoder,
> >  				  const struct intel_crtc_state *pipe_config)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > @@ -375,12 +381,12 @@ static bool vlv_infoframe_enabled(struct intel_encoder *encoder,
> >  	u32 val = I915_READ(VLV_TVIDEO_DIP_CTL(pipe));
> >  
> >  	if ((val & VIDEO_DIP_ENABLE) == 0)
> > -		return false;
> > +		return 0;
> >  
> >  	if ((val & VIDEO_DIP_PORT_MASK) != VIDEO_DIP_PORT(encoder->port))
> > -		return false;
> > +		return 0;
> >  
> > -	return val & (VIDEO_DIP_ENABLE_AVI |
> > +	return val & (VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> >  		      VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> >  		      VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> >  }
> > @@ -419,7 +425,7 @@ static void hsw_write_infoframe(struct intel_encoder *encoder,
> >  	POSTING_READ(ctl_reg);
> >  }
> >  
> > -static bool hsw_infoframe_enabled(struct intel_encoder *encoder,
> > +static u32 hsw_infoframes_enabled(struct intel_encoder *encoder,
> >  				  const struct intel_crtc_state *pipe_config)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > @@ -430,6 +436,42 @@ static bool hsw_infoframe_enabled(struct intel_encoder *encoder,
> >  		      VIDEO_DIP_ENABLE_GMP_HSW | VIDEO_DIP_ENABLE_SPD_HSW);
> >  }
> >  
> > +static const u8 infoframe_type_to_idx[] = {
> > +	HDMI_PACKET_TYPE_NULL,
> > +	HDMI_PACKET_TYPE_GENERAL_CONTROL,
> > +	HDMI_PACKET_TYPE_GAMUT_METADATA,
> > +	DP_SDP_VSC,
> > +	HDMI_INFOFRAME_TYPE_AVI,
> > +	HDMI_INFOFRAME_TYPE_SPD,
> > +	HDMI_INFOFRAME_TYPE_VENDOR,
> > +};
> > +
> > +u32 intel_hdmi_infoframes_enabled(struct intel_encoder *encoder,
> > +				  const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > +	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> > +	u32 val, ret = 0;
> > +	int i;
> > +
> > +	val = dig_port->infoframes_enabled(encoder, crtc_state);
> > +
> > +	/* map from hardware bits to dip idx */
> > +	for (i = 0; i < ARRAY_SIZE(infoframe_type_to_idx); i++) {
> > +		unsigned int type = infoframe_type_to_idx[i];
> > +
> > +		if (HAS_DDI(dev_priv)) {
> > +			if (val & hsw_infoframe_enable(type))
> > +				ret |= BIT(i);
> > +		} else {
> > +			if (val & g4x_infoframe_enable(type))
> > +				ret |= BIT(i);
> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  /*
> >   * The data we write to the DIP data buffer registers is 1 byte bigger than the
> >   * HDMI infoframe size because of an ECC/reserved byte at position 3 (starting
> > @@ -1200,7 +1242,6 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
> >  				  struct intel_crtc_state *pipe_config)
> >  {
> >  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> > -	struct intel_digital_port *intel_dig_port = hdmi_to_dig_port(intel_hdmi);
> >  	struct drm_device *dev = encoder->base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	u32 tmp, flags = 0;
> > @@ -1223,7 +1264,7 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
> >  	if (tmp & HDMI_MODE_SELECT_HDMI)
> >  		pipe_config->has_hdmi_sink = true;
> >  
> > -	if (intel_dig_port->infoframe_enabled(encoder, pipe_config))
> > +	if (intel_hdmi_infoframes_enabled(encoder, pipe_config))
> >  		pipe_config->has_infoframe = true;
> >  
> >  	if (tmp & SDVO_AUDIO_ENABLE)
> > @@ -2325,23 +2366,23 @@ 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->set_infoframes = vlv_set_infoframes;
> > -		intel_dig_port->infoframe_enabled = vlv_infoframe_enabled;
> > +		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->set_infoframes = g4x_set_infoframes;
> > -		intel_dig_port->infoframe_enabled = g4x_infoframe_enabled;
> > +		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->set_infoframes = hsw_set_infoframes;
> > -		intel_dig_port->infoframe_enabled = hsw_infoframe_enabled;
> > +		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->set_infoframes = ibx_set_infoframes;
> > -		intel_dig_port->infoframe_enabled = ibx_infoframe_enabled;
> > +		intel_dig_port->infoframes_enabled = ibx_infoframes_enabled;
> >  	} else {
> >  		intel_dig_port->write_infoframe = cpt_write_infoframe;
> >  		intel_dig_port->set_infoframes = cpt_set_infoframes;
> > -		intel_dig_port->infoframe_enabled = cpt_infoframe_enabled;
> > +		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
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel