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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Jul 16, 2018 at 03:04:01PM -0700, Paulo Zanoni wrote:
> 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@intel.com>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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?

ok... huge was just my wrong impression... it is not huge indeed and
not such a chain because status function is not called inside the tc_port_type...

> 
> > 
> > (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.

But it is not just a matter of belief and I thought I had already explained
my concerns on that very first email where you asked for options, anyways:

According to our current documentation

"
* intel_digital_port_connected - is the specified port connected?
* Return %true if port is connected, %false otherwise.
"

while behavior of intel_digital_port_connected for TC on ICL
after these patches is something like this:

If port is connected, set the TC type, set the safe bit status and
return true if all of these succeeded, false otherwise.

(now I'm avoiding the word "believe") in my honest view the architecture
here could be better in a way that we keep intel_digital_port_connect with
its only meaning and move the safe bit setup anywhere else.

After all spec also indicates that we need to set it if using the
main link or aux. imho the hotplug part is not the best path to
determine that... maybe somewhere around the modeset or power-well
or in the worst case, right after intel_digital_port_connected call,
but outside of intel_digital_port_connect itself...

> 
> 
> > 
> > For this patch:
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx