On Fri, Jan 26, 2018 at 09:27:41PM +0000, Pandiyan, Dhinakaran wrote:
>
> 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@xxxxxxxxxxxxxxx>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Reviewed-by: David Weinehall <david.weinehall@xxxxxxxxxxxxxxx>
> > ---
> > 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
I missed those, but you are right.
yet another v comming without this...
>
> <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.
agree. Shouldn't be different.
I will fix.
>
>
>
> > @@ -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?
The other matters now. That's how we are removing from the list
for platforms without port_f... array_size - 1. (-2 on the next that adds ddi one)
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
