Re: [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it

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

On Thu, Feb 22, 2018 at 07:25:22AM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Tue, 2018-02-20 at 11:31 -0800, Rodrigo Vivi wrote:
> > On Tue, Feb 20, 2018 at 07:05:22PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Since we no longer have a 1:1 correspondence between ports and AUX
> > > channels, let's give AUX channels their own enum. Makes it easier
> > > to tell the apples from the oranges, and we get rid of the
> > > port E AUX power domain FIXME since we now derive the power domain
> > > from the actual AUX CH.
> > 
> > 
> > Beautiful clean-up. I had this in a todo list years ago and
> > after staying on the bottom for so long I had removed it from there...
> > > 
> > > v2: Rebase due to AUX F
> > 
> > sorry and thanks! :)
> > 
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > I wondered if at least aux_power_domain part could be in a
> > separated patch because by the end I was a bit confused...
> > But in the end I could put all pieces together and it made sense
> > again. So one patch for easy back porting seems better.
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h      |   8 +-
> > >  drivers/gpu/drm/i915/intel_display.h |  11 ++
> > >  drivers/gpu/drm/i915/intel_dp.c      | 240 +++++++++++++++++------------------
> > >  drivers/gpu/drm/i915/intel_drv.h     |   1 +
> > >  4 files changed, 131 insertions(+), 129 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 1412abcb27d4..39d624083a17 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -5347,8 +5347,8 @@ enum {
> > >  #define _DPF_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64520)
> > >  #define _DPF_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64524)
> > >  
> > > -#define DP_AUX_CH_CTL(port)	_MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > > -#define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > > +#define DP_AUX_CH_CTL(aux_ch)	_MMIO_PORT(aux_ch, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > > +#define DP_AUX_CH_DATA(aux_ch, i)	_MMIO(_PORT(aux_ch, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > >  
> > >  #define   DP_AUX_CH_CTL_SEND_BUSY	    (1 << 31)
> > >  #define   DP_AUX_CH_CTL_DONE		    (1 << 30)
> > > @@ -7875,8 +7875,8 @@ enum {
> > >  #define _PCH_DPD_AUX_CH_DATA4	0xe4320
> > >  #define _PCH_DPD_AUX_CH_DATA5	0xe4324
> > >  
> > > -#define PCH_DP_AUX_CH_CTL(port)		_MMIO_PORT((port) - PORT_B, _PCH_DPB_AUX_CH_CTL, _PCH_DPC_AUX_CH_CTL)
> > > -#define PCH_DP_AUX_CH_DATA(port, i)	_MMIO(_PORT((port) - PORT_B, _PCH_DPB_AUX_CH_DATA1, _PCH_DPC_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > > +#define PCH_DP_AUX_CH_CTL(aux_ch)		_MMIO_PORT((aux_ch) - AUX_CH_B, _PCH_DPB_AUX_CH_CTL, _PCH_DPC_AUX_CH_CTL)
> > > +#define PCH_DP_AUX_CH_DATA(aux_ch, i)	_MMIO(_PORT((aux_ch) - AUX_CH_B, _PCH_DPB_AUX_CH_DATA1, _PCH_DPC_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > >  
> > >  /* CPT */
> > >  #define  PORT_TRANS_A_SEL_CPT	0
> > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > index c4042e342f50..f5733a2576e7 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > @@ -139,6 +139,17 @@ enum dpio_phy {
> > >  
> > >  #define I915_NUM_PHYS_VLV 2
> > >  
> > > +enum aux_ch {
> > > +	AUX_CH_A,
> > > +	AUX_CH_B,
> > > +	AUX_CH_C,
> > > +	AUX_CH_D,
> > > +	_AUX_CH_E, /* does not exist */
> > > +	AUX_CH_F,
> > > +};
> > > +
> > > +#define aux_ch_name(a) ((a) + 'A')
> > > +
> > >  enum intel_display_power_domain {
> > >  	POWER_DOMAIN_PIPE_A,
> > >  	POWER_DOMAIN_PIPE_B,
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index f20b25f98e5a..eeb8a026fd08 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1331,171 +1331,194 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > >  	return ret;
> > >  }
> > >  
> > > -static enum port intel_aux_port(struct drm_i915_private *dev_priv,
> > > -				enum port port)
> > > +static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > >  {
> > > +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > +	enum port port = encoder->port;
> > >  	const struct ddi_vbt_port_info *info =
> > >  		&dev_priv->vbt.ddi_port_info[port];
> > > -	enum port aux_port;
> > > +	enum aux_ch aux_ch;
> > >  
> > >  	if (!info->alternate_aux_channel) {
> > > +		aux_ch = (enum aux_ch) port;
> > > +
> > >  		DRM_DEBUG_KMS("using AUX %c for port %c (platform default)\n",
> > > -			      port_name(port), port_name(port));
> > > -		return port;
> > > +			      aux_ch_name(aux_ch), port_name(port));
> > > +		return aux_ch;
> > >  	}
> > >  
> > >  	switch (info->alternate_aux_channel) {
> > >  	case DP_AUX_A:
> > > -		aux_port = PORT_A;
> > > +		aux_ch = AUX_CH_A;
> > >  		break;
> > >  	case DP_AUX_B:
> > > -		aux_port = PORT_B;
> > > +		aux_ch = AUX_CH_B;
> > >  		break;
> > >  	case DP_AUX_C:
> > > -		aux_port = PORT_C;
> > > +		aux_ch = AUX_CH_C;
> > >  		break;
> > >  	case DP_AUX_D:
> > > -		aux_port = PORT_D;
> > > +		aux_ch = AUX_CH_D;
> > >  		break;
> > >  	case DP_AUX_F:
> > > -		aux_port = PORT_F;
> > > +		aux_ch = AUX_CH_F;
> > >  		break;
> > >  	default:
> > >  		MISSING_CASE(info->alternate_aux_channel);
> > > -		aux_port = PORT_A;
> > > +		aux_ch = AUX_CH_A;
> > >  		break;
> > >  	}
> > >  
> > >  	DRM_DEBUG_KMS("using AUX %c for port %c (VBT)\n",
> > > -		      port_name(aux_port), port_name(port));
> > > +		      aux_ch_name(aux_ch), port_name(port));
> > >  
> > > -	return aux_port;
> > > +	return aux_ch;
> > > +}
> > > +
> > > +static enum intel_display_power_domain
> > > +intel_aux_power_domain(struct intel_dp *intel_dp)
> > > +{
> > > +	switch (intel_dp->aux_ch) {
> > > +	case AUX_CH_A:
> > > +		return POWER_DOMAIN_AUX_A;
> > > +	case AUX_CH_B:
> > > +		return POWER_DOMAIN_AUX_B;
> > > +	case AUX_CH_C:
> > > +		return POWER_DOMAIN_AUX_C;
> > > +	case AUX_CH_D:
> > > +		return POWER_DOMAIN_AUX_D;
> > > +	case AUX_CH_F:
> > > +		return POWER_DOMAIN_AUX_F;
> > > +	default:
> > > +		MISSING_CASE(intel_dp->aux_ch);
> > > +		return POWER_DOMAIN_AUX_A;
> > > +	}
> > >  }
> > >  
> > >  static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
> > > -				  enum port port)
> > > +				  enum aux_ch aux_ch)
> > >  {
> > > -	switch (port) {
> > > -	case PORT_B:
> > > -	case PORT_C:
> > > -	case PORT_D:
> > > -		return DP_AUX_CH_CTL(port);
> > > +	switch (aux_ch) {
> > > +	case AUX_CH_B:
> > > +	case AUX_CH_C:
> > > +	case AUX_CH_D:
> > > +		return DP_AUX_CH_CTL(aux_ch);
> > >  	default:
> > > -		MISSING_CASE(port);
> > > -		return DP_AUX_CH_CTL(PORT_B);
> > > +		MISSING_CASE(aux_ch);
> > > +		return DP_AUX_CH_CTL(AUX_CH_B);
> 
> Shouldn't intel_dp->aux_ch change too if we are switching to fallback
> default registers?

The default case is there just to appease the compiler really.
Whatever we do here is most likely going to be wrong anyway so
there's little point in trying to make it polished in any way.

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