Re: [Intel-gfx] [PATCH v3 05/22] drm/i915: Introduce intel_hpd_detection()

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

On Tue, Feb 28, 2023 at 02:40:20PM +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>
> >
> > Add a mechanism by which we can toggle the HPD sense for
> > individual encoders.
> >
> > This will be used during eDP probing to figure out if
> > anything is actually connected. The normal intel_hpd_irq_setup()
> > thing doesn't work since we only do that after probing the
> > outputs, and we only enable HPD sense for encoders that were
> > successfully probed.
> >
> > The other idea that crossed my minds was to just turn on
> > HPD sense for all pins before output probing and let hpd_irq_setup()
> > clean it up afterwards. But that doesn't work for BXT/GLK where
> > the HPD invert information comes from the VBT child device.
> > So looks like this really needs to be per-encoder.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 132 ++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_irq.h |   2 +
> >  2 files changed, 134 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index ecfa6dad145a..c5058d834f99 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2893,8 +2893,17 @@ static void ibx_hpd_detection_setup(struct drm_i915_private *dev_priv)
> >  			 ibx_hotplug_mask(dev_priv, HPD_PORT_D),
> >  			 intel_hpd_hotplug_enables(dev_priv, ibx_hotplug_enables));
> >  }
> >  
> > +static void ibx_hpd_detection(struct intel_encoder *encoder, bool enable)
> 
> Just quickly skimming through the series...
> 
> ...and nitpicking, *_detection() should really be a verb.
> 
> It isn't obvious to me what intel_hpd_detection() does without looking
> at the implementation.

I was origianlly musing about foo_enable_hpd_detection(), but then
passing in a 'bool enable' for an enable() function seems a bit
crap. I guess it could be foo_set_hpd_detection() or something like
that.

-- 
Ville Syrjälä
Intel