[Intel-gfx] [PATCH] drm/i915/cnl: Add AUX-F support

[Pandiyan, Dhinakaran]

On Fri, 2018-01-26 at 09:15 -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.
> 
> 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.
> v11: Clean up squashed commit message. (David)
> 
> Cc: David Weinehall <david.weinehall at linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Manasi Navare <manasi.d.navare at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Reviewed-by: David Weinehall <david.weinehall at linux.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(+)

<snip>

> @@ -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);

I pointed this out in the last review, but it must have got lost among
other comments and code.
Why are these hunks needed? skl_aux_data_reg and skl_aux_ctl_reg already
have the necessary changes and the gfx_ variants won't get called for
INTEL_GEN >= 9

<snip>

> @@ -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) |			\


Why is BIT_ULL(POWER_DOMAIN_AUX_F) not included in
CNL_DISPLAY_POWERWELL_2_POWER_DOMAINS, while POWER_DOMAIN_AUX_B,
POWER_DOMAIN_AUX_C and POWER_DOMAIN_AUX_D are?

If I understand this correctly, power_get(AUX_B) would
enable both AUX_B powerwell and powerwell 2. 

But, power_get(AUX_F) would just enable AUX_F.

I don't see anything in the spec justifies why AUX_F should be treated
differently.



> @@ -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,
> +	},

I wonder if placing AUX_F after dc_off and power well 2 has any impact.
Is there an expected order that the hardware requires us to power these
wells? For e.g, is it okay to enable power well 2 before enabling
AUX_F?