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

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

On Mon, Oct 01, 2018 at 08:55:51AM +0200, Daniel Vetter wrote:
> On Mon, Sep 24, 2018 at 07:36:08PM +0300, Ville Syrjälä wrote:
> > 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.
> 
> >From a functionality pov, do we actually care about the null DIP? HDMI
> mode y/n for sure, but we track that already. Feels like just never
> reading out the null packet state and never setting it in the state
> structure is the cheap way out here, and also the least confusing way out
> here. Trying to second guess what the hw does for something that doesn't
> seem to matter feels a bit silly.

I suppose. I just liked the idea of the bitmask reflecting reality, and
the fact that it gave one extra sanity check for free seemed like a nice
bonus. But the practical benefit is likely near zero so I don't see a
problem leaving it behind. I'll go rip it out.


> -Daniel
> 
> > 
> > > 
> > > 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
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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