Re: [PATCH 19/20] drm/i915/dp: Ack only the handled device service IRQs

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

On Thu, Jul 03, 2025 at 04:18:21PM +0300, Imre Deak wrote:
> On Thu, Jul 03, 2025 at 04:14:51PM +0300, Luca Coelho wrote:
> > On Thu, 2025-06-26 at 11:20 +0300, Imre Deak wrote:
> > > From: Imre Deak <imre.deak@gmail.com>
> > > 
> > > Only those IRQs should be acked that are handled, however for SST all
> > > IRQs triggered by the sink are acked. This can be a problem for flags
> > > that are reserved/reading zero at a given moment, but become used for
> > > some purpose - with a side-effect if set - in a future DPCD revision.
> > > 
> > > Fix the above by acking only those device service IRQs that will be
> > > handled. While at it add asserts that only the known/acked device
> > > service IRQs are handled both in the MST and SST case.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@gmail.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 52249fa5c8a6d..6f67fac9724e1 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -4537,6 +4537,14 @@ intel_dp_mst_disconnect(struct intel_dp *intel_dp)
> > >  	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst.mgr, intel_dp->is_mst);
> > >  }
> > >  
> > > +#define INTEL_DP_DEVICE_SERVICE_IRQ_MASK_SST	(DP_AUTOMATED_TEST_REQUEST | \
> > > +						 DP_CP_IRQ | \
> > > +						 DP_SINK_SPECIFIC_IRQ)
> > > +
> > > +#define INTEL_DP_DEVICE_SERVICE_IRQ_MASK_MST	(DP_CP_IRQ | \
> > > +						 DP_DOWN_REP_MSG_RDY | \
> > > +						 DP_UP_REQ_MSG_RDY)
> > > +
> > >  static bool
> > >  intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *esi)
> > >  {
> > > @@ -4628,6 +4636,8 @@ static bool intel_dp_get_and_ack_sink_irq_esi_sst(struct intel_dp *intel_dp, u8
> > >  		    encoder->base.base.id, encoder->base.name,
> > >  		    esi);
> > >  
> > > +	esi[1] &= INTEL_DP_DEVICE_SERVICE_IRQ_MASK_SST;
> > > +
> > >  	if (mem_is_zero(&esi[1], 3))
> > >  		return true;
> > >  
> > > @@ -5172,6 +5182,8 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> > >  		if (mem_is_zero(ack, sizeof(ack)))
> > >  			break;
> > >  
> > > +		drm_WARN_ON(display->drm, ack[1] & ~INTEL_DP_DEVICE_SERVICE_IRQ_MASK_MST);
> > > +
> > >  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
> > >  			drm_dbg_kms(display->drm, "Failed to ack ESI\n");
> > >  
> > > @@ -5456,6 +5468,8 @@ static void intel_dp_handle_device_service_irq(struct intel_dp *intel_dp, u8 irq
> > >  {
> > >  	struct intel_display *display = to_intel_display(intel_dp);
> > >  
> > > +	drm_WARN_ON(display->drm, irq_mask & ~INTEL_DP_DEVICE_SERVICE_IRQ_MASK_SST);
> > > +
> > >  	if (irq_mask & DP_AUTOMATED_TEST_REQUEST)
> > >  		intel_dp_test_request(intel_dp);
> > >  
> > 
> > Evidently, this function is specific to SST irq.  Wouldn't it be better
> > to have _sst in the name like in "intel_dp_check_mst_status()"? This
> > function is probably in an earlier patch, though, so if handled it
> > should be obviously be done there.
> 
> Yes, it used to be SST specific, but patch 18 reused it for MST as well.

Ah, sorry, yes this function is still only used for SST. However the
device service IRQs handled here are not SST specific and imo the
function should be used for MST as well. But until that I can rename it
to intel_dp_handle_device_service_irq_sst().

> > Otherwise:
> > 
> > Reviewed-by: Luca Coelho <luciano.coelho@intel.com>
> > 
> > --
> > Cheers,
> > Luca.