Re: [PATCH 04/20] drm/i915/dp: Handle a tunneling IRQ after acking it

[Imre Deak <imre.deak@intel.com>]

On Tue, Jul 01, 2025 at 11:02:30AM +0300, Luca Coelho wrote:
> On Thu, 2025-06-26 at 11:20 +0300, Imre Deak wrote:
> > From: Imre Deak <imre.deak@gmail.com>
> > 
> > HPD IRQs in general should be handled after acking them. The
> > 
> > 1. Read IRQ register (read DP_DEVICE_SERVICE_IRQ_VECTOR,
> >    DP_LINK_SERVICE_IRQ_VECTOR_ESI0)
> > 2. Handle IRQ
> > 3. Ack IRQ (write DP_DEVICE_SERVICE_IRQ_VECTOR,
> >    DP_LINK_SERVICE_IRQ_VECTOR_ESI0)
> > 
> > sequence would miss a new interrupt triggered after 2. and before 3.,
> > since the flag set in the IRQ register for this interrupt would be
> > cleared in step 3.
> > 
> > Fix the above by handling the IRQ after acking it.
> > 
> > Signed-off-by: Imre Deak <imre.deak@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 25 +++++++++++--------------
> >  1 file changed, 11 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 285cd9a5d4a7b..453416b9e9bec 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -5099,17 +5099,10 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >  
> >  		drm_dbg_kms(display->drm, "DPRX ESI: %4ph\n", esi);
> >  
> > -		ack[3] |= esi[3] & LINK_STATUS_CHANGED;
> > +		ack[3] |= esi[3] & (LINK_STATUS_CHANGED | DP_TUNNELING_IRQ);
> >  
> >  		intel_dp_mst_hpd_irq(intel_dp, esi, ack);
> >  
> > -		if (esi[3] & DP_TUNNELING_IRQ) {
> > -			if (drm_dp_tunnel_handle_irq(display->dp_tunnel_mgr,
> > -						     &intel_dp->aux))
> > -				reprobe_needed = true;
> > -			ack[3] |= DP_TUNNELING_IRQ;
> > -		}
> > -
> >  		if (mem_is_zero(ack, sizeof(ack)))
> >  			break;
> 
> What happens if we ack the interrupt, but end up not handling it, e.g.
> if mem_is_zero() returns true here?

This is an optimization in case of ack[] having no bits set (and also to
break from the loop acking->handling IRQs). I.e. if ack[] is zero the
AUX write to ack IRQs in intel_dp_ack_sink_irq_esi() can be skipped,
since no IRQ needs to be acked and then also no IRQ needs to be handled.

> >  
> > @@ -5121,6 +5114,10 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >  
> >  		if ((ack[3] & LINK_STATUS_CHANGED) || intel_dp->link.force_retrain)
> >  			intel_dp_check_link_state(intel_dp);
> > +
> > +		if ((ack[3] & DP_TUNNELING_IRQ) &&
> > +		    drm_dp_tunnel_handle_irq(display->dp_tunnel_mgr, &intel_dp->aux))
> > +			reprobe_needed = true;
> >  	}
> >  
> >  	return !reprobe_needed;
> > @@ -5423,18 +5420,18 @@ static bool intel_dp_check_link_service_irq(struct intel_dp *intel_dp)
> >  			      DP_LINK_SERVICE_IRQ_VECTOR_ESI0, &val) != 1 || !val)
> >  		return false;
> >  
> > -	if ((val & DP_TUNNELING_IRQ) &&
> > -	    drm_dp_tunnel_handle_irq(display->dp_tunnel_mgr,
> > -				     &intel_dp->aux))
> > -		reprobe_needed = true;
> > -
> >  	if (drm_dp_dpcd_writeb(&intel_dp->aux,
> >  			       DP_LINK_SERVICE_IRQ_VECTOR_ESI0, val) != 1)
> > -		return reprobe_needed;
> > +		return false;
> >  
> >  	if (val & HDMI_LINK_STATUS_CHANGED)
> >  		intel_dp_handle_hdmi_link_status_change(intel_dp);
> >  
> > +	if ((val & DP_TUNNELING_IRQ) &&
> > +	    drm_dp_tunnel_handle_irq(display->dp_tunnel_mgr,
> > +				     &intel_dp->aux))
> > +		reprobe_needed = true;
> > +
> >  	return reprobe_needed;
> >  }
> >  
> 
> --
> Cheers,
> Luca.