Re: [Intel-gfx] [PATCH v3 04/22] drm/i915: Introduce <platoform>_hotplug_mask()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 28, 2023 at 08:36:16PM +0200, Jani Nikula wrote:
> On Wed, 22 Feb 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Pair each <platform>_hotplug_enables() function with
> > a corresponding <platform>_hotplug_mask() function so that
> > we can determine right bits to clear on a per hpd_pin basis.
> > We'll need this for turning on HPD sense for a specific
> > encoder rather than just all of them.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 231 ++++++++++++++++++++++----------
> >  1 file changed, 160 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 13ada0916c2a..ecfa6dad145a 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2835,8 +2835,25 @@ static void cherryview_irq_reset(struct drm_i915_private *dev_priv)
> >  		vlv_display_irq_reset(dev_priv);
> >  	spin_unlock_irq(&dev_priv->irq_lock);
> >  }
> >  
> > +static u32 ibx_hotplug_mask(struct drm_i915_private *i915,
> > +			    enum hpd_pin hpd_pin)
> 
> 
> What's the reason for passing i915 to these functions? I see no use in
> this series, am I missing something? Seems like it makes some calls a
> bit convoluted.
> 
> BR,
> Jani.
> 
> > +{
> > +	switch (hpd_pin) {
> > +	case HPD_PORT_A:

I think I initially copied over the LPT-LP check from
ibx_hotplug_enables(), which is what needed i915. But
apparently I got rid of it later, and so looks like 
i915 can go too.

> > +		return PORTA_HOTPLUG_ENABLE;
> > +	case HPD_PORT_B:
> > +		return PORTB_HOTPLUG_ENABLE | PORTB_PULSE_DURATION_MASK;
> > +	case HPD_PORT_C:
> > +		return PORTC_HOTPLUG_ENABLE | PORTC_PULSE_DURATION_MASK;
> > +	case HPD_PORT_D:
> > +		return PORTD_HOTPLUG_ENABLE | PORTD_PULSE_DURATION_MASK;
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> >  static u32 ibx_hotplug_enables(struct intel_encoder *encoder)
> >  {
> >  	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> >  
> > @@ -2869,15 +2886,12 @@ static void ibx_hpd_detection_setup(struct drm_i915_private *dev_priv)
> >  	 * duration to 2ms (which is the minimum in the Display Port spec).
> >  	 * The pulse duration bits are reserved on LPT+.
> >  	 */
> >  	intel_uncore_rmw(&dev_priv->uncore, PCH_PORT_HOTPLUG,
> > -			 PORTA_HOTPLUG_ENABLE |

No LPT-LP check here either, so presumably clearing the bit
unconditionally is fine... Yeah, doesn't look like there was
anything else there, ever.

> > -			 PORTB_HOTPLUG_ENABLE |
> > -			 PORTC_HOTPLUG_ENABLE |
> > -			 PORTD_HOTPLUG_ENABLE |
> > -			 PORTB_PULSE_DURATION_MASK |
> > -			 PORTC_PULSE_DURATION_MASK |
> > -			 PORTD_PULSE_DURATION_MASK,
> > +			 ibx_hotplug_mask(dev_priv, HPD_PORT_A) |
> > +			 ibx_hotplug_mask(dev_priv, HPD_PORT_B) |
> > +			 ibx_hotplug_mask(dev_priv, HPD_PORT_C) |
> > +			 ibx_hotplug_mask(dev_priv, HPD_PORT_D),
> >  			 intel_hpd_hotplug_enables(dev_priv, ibx_hotplug_enables));
> >  }
> >  

-- 
Ville Syrjälä
Intel