[Intel-gfx] [PATCH 3/8] drm/i915/icl: store the port type for TC ports

[Paulo Zanoni]

Em Qui, 2018-07-12 às 23:14 -0700, Rodrigo Vivi escreveu:
> On Wed, Jul 11, 2018 at 02:59:04PM -0700, Paulo Zanoni wrote:
> > The type is detected based on the interrupt ISR bit. Once detected,
> > it's not supposed to be changed, so we have some sanity checks for
> > that.
> > 
> > Cc: Animesh Manna <animesh.manna at intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.h |  7 +++++++
> >  drivers/gpu/drm/i915/intel_dp.c      | 36
> > +++++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >  3 files changed, 43 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.h
> > b/drivers/gpu/drm/i915/intel_display.h
> > index ca5a10f3400d..18ed9835335c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -137,6 +137,13 @@ enum tc_port {
> >  	I915_MAX_TC_PORTS
> >  };
> >  
> > +enum tc_port_type {
> > +	TC_PORT_UNKNOWN = 0,
> > +	TC_PORT_TYPEC,
> > +	TC_PORT_TBT,
> > +	TC_PORT_LEGACY,
> > +};
> > +
> >  enum dpio_channel {
> >  	DPIO_CH0,
> >  	DPIO_CH1
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index f2197dea02d0..486b879cdef7 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4752,6 +4752,38 @@ static bool icl_combo_port_connected(struct
> > drm_i915_private *dev_priv,
> >  	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
> >  }
> >  
> > +static void icl_update_tc_port_type(struct drm_i915_private
> > *dev_priv,
> > +				    struct intel_digital_port
> > *intel_dig_port,
> > +				    bool is_legacy, bool is_typec,
> > bool is_tbt)
> > +{
> > +	enum port port = intel_dig_port->base.port;
> > +	enum tc_port_type old_type = intel_dig_port->tc_type;
> > +	const char *type_str;
> > +
> > +	WARN_ON(is_legacy + is_typec + is_tbt != 1);
> > +
> > +	if (is_legacy) {
> > +		intel_dig_port->tc_type = TC_PORT_LEGACY;
> > +		type_str = "legacy";
> > +	} else if (is_typec) {
> > +		intel_dig_port->tc_type = TC_PORT_TYPEC;
> > +		type_str = "typec";
> > +	} else if (is_tbt) {
> > +		intel_dig_port->tc_type = TC_PORT_TBT;
> > +		type_str = "tbt";
> > +	} else {
> > +		return;
> > +	}
> > +
> > +	/* Types are not supposed to be changed at runtime. */
> > +	WARN_ON(old_type != TC_PORT_UNKNOWN &&
> > +		old_type != intel_dig_port->tc_type);
> > +
> > +	if (old_type != intel_dig_port->tc_type)
> > +		DRM_DEBUG_KMS("Port %c has TC type %s\n",
> > port_name(port),
> > +			      type_str);
> > +}
> > +
> >  static bool icl_tc_port_connected(struct drm_i915_private
> > *dev_priv,
> >  				  struct intel_digital_port
> > *intel_dig_port)
> >  {
> > @@ -4772,10 +4804,12 @@ static bool icl_tc_port_connected(struct
> > drm_i915_private *dev_priv,
> >  	if (cpu_isr & tbt_bit)
> >  		is_tbt = true;
> >  
> > -	WARN_ON(is_legacy + is_typec + is_tbt > 1);
> 
> oh, I know see that I got it wrong on my previous suggestion, but
> since
> we are removing maybe it is better to not add at all at first place?!

We are moving it. It was in the correct place in the last patch, it is
in the correct place now.

> 
> >  	if (!is_legacy && !is_typec && !is_tbt)
> >  		return false;
> >  
> > +	icl_update_tc_port_type(dev_priv, intel_dig_port,
> > is_legacy, is_typec,
> > +				is_tbt);
> 
> for a while I thougth this was the start of the chain that I didn't
> like,
> but I was wrong, the type I believe it can/need to be here indeed.
> 
> but for everything else I believe that you need to handle on
> long_pulse.

I really need better explanations instead of "I don't like" and "I feel
this is wrong". I can't even start to think on how to implement an
acceptable solution if you don't try to elaborate a few more problems
on what exactly is wrong with the series, in technical explanations
with reasons instead of feelings.

The way I imagine a solution with code moved out is that every call to
intel_digital_port_connected() will need to have a following call to
the function you're telling me to extract, which doesn't make sense to
me. Or maybe we could wrap the all the call pairs under
intel_digital_port_really_connected() :).

> 
> if it is connected and tc_type != known than you do all the rest
> of the opperation instead of making a huge chain from this point.

What exactly is "huge" about it?

> 
> (for the lspcon wa path I don't believe we need that at all, but if
> we
> need we should also handle there)

Please explain with technical terms the "believe" part.


> 
> For this patch:
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> 
> > +
> >  	return true;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 61e715ddd0d5..8b3c3dc76810 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1162,6 +1162,7 @@ struct intel_digital_port {
> >  	bool release_cl2_override;
> >  	uint8_t max_lanes;
> >  	enum intel_display_power_domain ddi_io_power_domain;
> > +	enum tc_port_type tc_type;
> >  
> >  	void (*write_infoframe)(struct drm_encoder *encoder,
> >  				const struct intel_crtc_state
> > *crtc_state,
> > -- 
> > 2.14.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx