Re: [PATCH 02/10] drm/i915/cnl: Add AUX-F support

[David Weinehall <david.weinehall@linux.intel.com>]

On Thu, Jan 25, 2018 at 02:03:22PM -0800, Rodrigo Vivi wrote:
> On some Cannonlake SKUs we have a dedicated Aux for port F,
> that is only the full split between port A and port E.
> 
> There is still no Aux E for Port E, as in previous platforms,
> because port_E still means shared lanes with port A.
> 

Looks good to me, except the dual sets of review comments and
signed-offs by, which seems kind of odd--did you squash two
patches into one?

Anyway, the code looks clean & documented, and the registers
seem to match documentation, so:

Reviewed-by: David Weinehall <david.weinehall@linux.intel.com>

> v2: Rebase.
> v3: Add couple missed PORT_F cases on intel_dp.
> v4: Rebase and fix commit message.
> v5: Squash Imre's "drm/i915: Add missing AUX_F power well string"
> v6: Rebase on top of display headers rework.
> v7: s/IS_CANNONLAKE/IS_CNL_WITH_PORT_F (DK)
> v8: Fix Aux bits for Port F (DK)
> v9: Fix VBT definition of Port F (DK).
> v10: Squash power well addition to this patch to avoid
>      warns as pointed by DK.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> drm/i915/cnl: Don't try to manage Port F power wells on all CNL.
> 
> SKUs that lacks on the full port F split will just time out
> when touching this power well bits, causing a noisy warn.
> 
> v2: Suggested-by: Imre. Temporarily remove the aux pw id after setting
>     it instead of duplicating and redefining everything.
> v3: Simplify even more the logic, using one that don't mix the
>     array size with the pw bits. Also add a comment.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  1 +
>  drivers/gpu/drm/i915/i915_irq.c         |  6 ++++++
>  drivers/gpu/drm/i915/i915_reg.h         |  9 +++++++++
>  drivers/gpu/drm/i915/intel_display.h    |  1 +
>  drivers/gpu/drm/i915/intel_dp.c         |  8 ++++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 21 +++++++++++++++++++++
>  6 files changed, 46 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5702ebf17974..f89a1a0a25c8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1255,6 +1255,7 @@ enum modeset_restore {
>  #define DP_AUX_B 0x10
>  #define DP_AUX_C 0x20
>  #define DP_AUX_D 0x30
> +#define DP_AUX_F 0x60
>  
>  #define DDC_PIN_B  0x05
>  #define DDC_PIN_C  0x04
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index f643d5ad7ff7..4d84b1c41a94 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2585,6 +2585,9 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
>  					    GEN9_AUX_CHANNEL_C |
>  					    GEN9_AUX_CHANNEL_D;
>  
> +			if (IS_CNL_WITH_PORT_F(dev_priv))
> +				tmp_mask |= CNL_AUX_CHANNEL_F;
> +
>  			if (iir & tmp_mask) {
>  				dp_aux_irq_handler(dev_priv);
>  				found = true;
> @@ -3623,6 +3626,9 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>  		de_pipe_masked |= GEN8_DE_PIPE_IRQ_FAULT_ERRORS;
>  	}
>  
> +	if (IS_CNL_WITH_PORT_F(dev_priv))
> +		de_port_masked |= CNL_AUX_CHANNEL_F;
> +
>  	de_pipe_enables = de_pipe_masked | GEN8_PIPE_VBLANK |
>  					   GEN8_PIPE_FIFO_UNDERRUN;
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 64933fd74cb6..44f46593172f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1312,6 +1312,7 @@ enum i915_power_well_id {
>  	CNL_DISP_PW_AUX_B = GLK_DISP_PW_AUX_B,
>  	CNL_DISP_PW_AUX_C = GLK_DISP_PW_AUX_C,
>  	CNL_DISP_PW_AUX_D,
> +	CNL_DISP_PW_AUX_F,
>  
>  	SKL_DISP_PW_1 = 14,
>  	SKL_DISP_PW_2,
> @@ -5284,6 +5285,13 @@ enum {
>  #define _DPD_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64320)
>  #define _DPD_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64324)
>  
> +#define _DPF_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64510)
> +#define _DPF_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64514)
> +#define _DPF_AUX_CH_DATA2	(dev_priv->info.display_mmio_offset + 0x64518)
> +#define _DPF_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6451c)
> +#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 */
>  
> @@ -6939,6 +6947,7 @@ enum {
>  #define GEN8_DE_PORT_IMR _MMIO(0x44444)
>  #define GEN8_DE_PORT_IIR _MMIO(0x44448)
>  #define GEN8_DE_PORT_IER _MMIO(0x4444c)
> +#define  CNL_AUX_CHANNEL_F		(1 << 28)
>  #define  GEN9_AUX_CHANNEL_D		(1 << 27)
>  #define  GEN9_AUX_CHANNEL_C		(1 << 26)
>  #define  GEN9_AUX_CHANNEL_B		(1 << 25)
> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> index e47638931b51..30fa2041a45f 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -172,6 +172,7 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_AUX_B,
>  	POWER_DOMAIN_AUX_C,
>  	POWER_DOMAIN_AUX_D,
> +	POWER_DOMAIN_AUX_F,
>  	POWER_DOMAIN_GMBUS,
>  	POWER_DOMAIN_MODESET,
>  	POWER_DOMAIN_GT_IRQ,
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a2e887999915..ae3b0b030177 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1323,6 +1323,9 @@ static enum port intel_aux_port(struct drm_i915_private *dev_priv,
>  	case DP_AUX_D:
>  		aux_port = PORT_D;
>  		break;
> +	case DP_AUX_F:
> +		aux_port = PORT_F;
> +		break;
>  	default:
>  		MISSING_CASE(info->alternate_aux_channel);
>  		aux_port = PORT_A;
> @@ -1342,6 +1345,7 @@ static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
>  	case PORT_B:
>  	case PORT_C:
>  	case PORT_D:
> +	case PORT_F:
>  		return DP_AUX_CH_CTL(port);
>  	default:
>  		MISSING_CASE(port);
> @@ -1356,6 +1360,7 @@ static i915_reg_t g4x_aux_data_reg(struct drm_i915_private *dev_priv,
>  	case PORT_B:
>  	case PORT_C:
>  	case PORT_D:
> +	case PORT_F:
>  		return DP_AUX_CH_DATA(port, index);
>  	default:
>  		MISSING_CASE(port);
> @@ -6224,6 +6229,9 @@ intel_dp_init_connector_port_info(struct intel_digital_port *intel_dig_port)
>  		/* FIXME: Check VBT for actual wiring of PORT E */
>  		intel_dp->aux_power_domain = POWER_DOMAIN_AUX_D;
>  		break;
> +	case PORT_F:
> +		intel_dp->aux_power_domain = POWER_DOMAIN_AUX_F;
> +		break;
>  	default:
>  		MISSING_CASE(encoder->port);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 5b1aa4b9c72c..93cc07998cc0 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -124,6 +124,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
>  		return "AUX_C";
>  	case POWER_DOMAIN_AUX_D:
>  		return "AUX_D";
> +	case POWER_DOMAIN_AUX_F:
> +		return "AUX_F";
>  	case POWER_DOMAIN_GMBUS:
>  		return "GMBUS";
>  	case POWER_DOMAIN_INIT:
> @@ -1855,6 +1857,9 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  #define CNL_DISPLAY_AUX_D_POWER_DOMAINS (		\
>  	BIT_ULL(POWER_DOMAIN_AUX_D) |			\
>  	BIT_ULL(POWER_DOMAIN_INIT))
> +#define CNL_DISPLAY_AUX_F_POWER_DOMAINS (		\
> +	BIT_ULL(POWER_DOMAIN_AUX_F) |			\
> +	BIT_ULL(POWER_DOMAIN_INIT))
>  #define CNL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
>  	CNL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
>  	BIT_ULL(POWER_DOMAIN_GT_IRQ) |			\
> @@ -2405,6 +2410,12 @@ static struct i915_power_well cnl_power_wells[] = {
>  		.ops = &hsw_power_well_ops,
>  		.id = SKL_DISP_PW_DDI_D,
>  	},
> +	{
> +		.name = "AUX F",
> +		.domains = CNL_DISPLAY_AUX_F_POWER_DOMAINS,
> +		.ops = &hsw_power_well_ops,
> +		.id = CNL_DISP_PW_AUX_F,
> +	},
>  };
>  
>  static int
> @@ -2520,6 +2531,16 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
>  		set_power_wells(power_domains, skl_power_wells);
>  	} else if (IS_CANNONLAKE(dev_priv)) {
>  		set_power_wells(power_domains, cnl_power_wells);
> +
> +		/*
> +		 * Aux IO is getting enabled for all ports
> +		 * regardless the presence or use. So, in order to avoid
> +		 * timeouts, lets remove it from the list
> +		 * for the SKUs without port F.
> +		 */
> +		if (!IS_CNL_WITH_PORT_F(dev_priv))
> +			power_domains->power_well_count -= 1;
> +
>  	} else if (IS_BROXTON(dev_priv)) {
>  		set_power_wells(power_domains, bxt_power_wells);
>  	} else if (IS_GEMINILAKE(dev_priv)) {
> -- 
> 2.13.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx