[PATCH 09/10] drm/i915/xe3lpd: Add check to see if edp over type c is allowed

[Kandpal, Suraj]

> -----Original Message-----
> From: Atwood, Matthew S <matthew.s.atwood at intel.com>
> Sent: Thursday, October 10, 2024 4:36 AM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>
> Cc: intel-xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org; Kandpal,
> Suraj <suraj.kandpal at intel.com>
> Subject: Re: [PATCH 09/10] drm/i915/xe3lpd: Add check to see if edp over
> type c is allowed
> 
> On Wed, Oct 09, 2024 at 10:53:56AM +0300, Jani Nikula wrote:
> > On Tue, 08 Oct 2024, Matt Atwood <matthew.s.atwood at intel.com> wrote:
> > > From: Suraj Kandpal <suraj.kandpal at intel.com>
> > >
> > > Read PICA register to see if edp over type C is possible and then
> > > add the appropriate tables for it.
> >
> > There's clearly more to be done for the feature than this.

>From what I could see in the spec we just need to read this the rest of the framework
Already seemed to be in place and removing the checks where we didn't allow edp to go ahead when
It was type c

> >
> > >
> > > Bspec: 68846
> > > Signed-off-by: Suraj Kandpal <suraj.kandpal at intel.com>
> > > Signed-off-by: Matt Atwood <matthew.s.atwood at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_cx0_phy.c     |  2 ++
> > >  .../gpu/drm/i915/display/intel_display_types.h   |  1 +
> > >  drivers/gpu/drm/i915/display/intel_dp.c          | 16 ++++++++++++++++
> > >  drivers/gpu/drm/i915/i915_reg.h                  |  3 +++
> > >  4 files changed, 22 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > index 0d6f75ae35f5..1c8c2a2b05e1 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > @@ -2261,6 +2261,8 @@ intel_c20_pll_tables_get(struct intel_crtc_state
> *crtc_state,
> > >  		if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)) {
> > >  			if (DISPLAY_VER_FULL(i915) == IP_VER(14, 1))
> > >  				return xe2hpd_c20_edp_tables;
> > > +			if (DISPLAY_VER(i915) >= 30 && encoder-
> >typec_supp)
> > > +				return xe3lpd_c20_dp_edp_tables;
> > >  		}
> > >
> > >  		if (DISPLAY_VER_FULL(i915) == IP_VER(14, 1)) diff --git
> > > a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 2bb1fa64da2f..e9dc7707fbcd 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -158,6 +158,7 @@ struct intel_encoder {
> > >  	enum port port;
> > >  	u16 cloneable;
> > >  	u8 pipe_mask;
> > > +	bool typec_supp;
> >
> > The register is global, why do we store this per encoder?

Do you think having this in drm_i915_private makes sense wanted to put it there originally

> >
> > Side not, please let's not abbreviate stuff like _supp for the sake of
> > abbreviating stuff.

Sure will fix the naming
Also quick question what would be the rule when abbreviating variables or
When would we want to abbreviate the a variable if we want to

> >
> > >
> > >  	/* Check and recover a bad link state. */
> > >  	struct delayed_work link_check_work; diff --git
> > > a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index fbb096be02ad..917a503cc43b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -5570,6 +5570,20 @@ intel_dp_detect_sdp_caps(struct intel_dp
> *intel_dp)
> > >  		drm_dp_as_sdp_supported(&intel_dp->aux, intel_dp-
> >dpcd);  }
> > >
> > > +static void
> > > +intel_dp_check_edp_typec_supp(struct intel_encoder *encoder)
> >
> > It's not about checking anything, it's about reading, right?

Yes will rename this to intel_dp_read_edp_typec_support

> >
> > > +{
> > > +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > > +	bool is_tc_port = intel_encoder_is_tc(encoder);
> > > +	u32 ret = 0;
> > > +
> > > +	if (encoder->type != INTEL_OUTPUT_EDP || !is_tc_port)
> >
> > Currently we warn at connector init for eDP type-C combo.

That's true we will need to remove that check for DISPLAY_VER > 20
Thanks will add that in this patch

> >
> > > +		return;
> > > +
> > > +	ret = intel_de_read(i915, PICA_PHY_CONFIG_CONTROL);
> > > +	encoder->typec_supp = ret & EDP_ON_TYPEC; }
> > > +
> > >  static int
> > >  intel_dp_detect(struct drm_connector *connector,
> > >  		struct drm_modeset_acquire_ctx *ctx, @@ -5595,6 +5609,8
> @@
> > > intel_dp_detect(struct drm_connector *connector,
> > >  	if (!intel_display_driver_check_access(dev_priv))
> > >  		return connector->status;
> > >
> > > +	intel_dp_check_edp_typec_supp(encoder);
> > > +
> >
> > Isn't this something that should be determined at intel_ddi_init() time?

Or intel_dp_connector_init can add it there what do you think ?

Regards,
Suraj Kandpal
> >
> > BR,
> > Jani.
> Please respond to Jani's comments
> MattA
> >
> >
> > >  	/* Can't disconnect eDP */
> > >  	if (intel_dp_is_edp(intel_dp))
> > >  		status = edp_detect(intel_dp);
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h index da65500cd0c8..5f5a6ade5f8c
> > > 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4583,4 +4583,7 @@ enum skl_power_gate {
> > >
> > >  #define MTL_MEDIA_GSI_BASE		0x380000
> > >
> > > +#define PICA_PHY_CONFIG_CONTROL 	_MMIO(0x16FE68)
> > > +#define   EDP_ON_TYPEC			REG_BIT(31)
> > > +
> > >  #endif /* _I915_REG_H_ */
> >
> > --
> > Jani Nikula, Intel