[PATCH 15/20] drm/i915/dp: Read/ack sink count and sink IRQs for SST as it's done for MST

[Luca Coelho]

On Thu, 2025-07-03 at 16:14 +0300, Imre Deak wrote:
> On Thu, Jul 03, 2025 at 04:02:18PM +0300, Luca Coelho wrote:
> > On Thu, 2025-06-26 at 11:20 +0300, Imre Deak wrote:
> > > From: Imre Deak <imre.deak at gmail.com>
> > > 
> > > Read and ack the sink count, sink device and link service IRQs for SST
> > > the same way this is done for MST, the read/ack happening in separate
> > > steps via an ESI (Event Status Indicator) vector.
> > > 
> > > The above way is more efficient, since on newer (DPCD_REV >= 1.2) sinks
> > > the DP_SINK_COUNT_ESI..DP_LINK_SERVICE_IRQ_VECTOR_ESI0 registers can be
> > > read out in one AUX transaction - and the 3 last one written in one
> > > transaction. Also this allows sharing more of the SST and MST IRQ
> > > handling code (done as a follow-up).
> > > 
> > > For now keep the current behavior of always reading the legacy
> > > DP_SINK_COUNT, DP_DEVICE_SERVICE_IRQ_VECTOR registers and not reading
> > > the DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1 register.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak at gmail.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp.c | 132 +++++++++++++-----------
> > >  1 file changed, 73 insertions(+), 59 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 2ba4a810f22c2..2e6ed7d2a64a6 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -4573,6 +4573,70 @@ static bool intel_dp_ack_sink_irq_esi(struct intel_dp *intel_dp, u8 esi[4])
> > >  	return false;
> > >  }
> > >  
> > > +static bool intel_dp_get_sink_irq_esi_sst(struct intel_dp *intel_dp, u8 esi[4])
> > > +{
> > > +	memset(esi, 0, 4);
> > > +
> > > +	/*
> > > +	 * TODO: For DP_DPCD_REV >= 0x12 read
> > > +	 * DP_SINK_COUNT_ESI and DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0.
> > > +	 */
> > > +	if (drm_dp_dpcd_read_data(&intel_dp->aux, DP_SINK_COUNT, esi, 2) != 0)
> > > +		return false;
> > > +
> > > +	if (intel_dp->dpcd[DP_DPCD_REV] < DP_DPCD_REV_12)
> > > +		return true;
> > > +
> > > +	/* TODO: Read DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1 as well */
> > > +	if (drm_dp_dpcd_read_byte(&intel_dp->aux, DP_LINK_SERVICE_IRQ_VECTOR_ESI0, &esi[3]) != 0)
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static bool intel_dp_ack_sink_irq_esi_sst(struct intel_dp *intel_dp, u8 esi[4])
> > > +{
> > > +	/*
> > > +	 * TODO: For DP_DPCD_REV >= 0x12 write
> > > +	 * DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0
> > > +	 */
> > > +	if (drm_dp_dpcd_write_byte(&intel_dp->aux, DP_DEVICE_SERVICE_IRQ_VECTOR, esi[1]) != 0)
> > > +		return false;
> > > +
> > > +	if (intel_dp->dpcd[DP_DPCD_REV] < DP_DPCD_REV_12)
> > > +		return true;
> > > +
> > > +	/* TODO: Read DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1 as well */
> > > +	if (drm_dp_dpcd_write_byte(&intel_dp->aux, DP_LINK_SERVICE_IRQ_VECTOR_ESI0, esi[3]) != 0)
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static bool intel_dp_get_and_ack_sink_irq_esi_sst(struct intel_dp *intel_dp, u8 esi[4])
> > > +{
> > > +	struct intel_display *display = to_intel_display(intel_dp);
> > > +	struct intel_connector *connector = intel_dp->attached_connector;
> > > +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > > +
> > > +	if (!intel_dp_get_sink_irq_esi_sst(intel_dp, esi))
> > > +		return false;
> > > +
> > > +	drm_dbg_kms(display->drm,
> > > +		    "[CONNECTOR:%d:%s][ENCODER:%d:%s] DPRX ESI: %4ph\n",
> > > +		    connector->base.base.id, connector->base.name,
> > > +		    encoder->base.base.id, encoder->base.name,
> > > +		    esi);
> > > +
> > > +	if (mem_is_zero(&esi[1], 3))
> > > +		return true;
> > > +
> > > +	if (!intel_dp_ack_sink_irq_esi_sst(intel_dp, esi))
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > 
> > Again, I think it's better to propagate the error than to swallow it
> > and return a bool.
> 
> I agree. But doing that would make these functions return error in
> different ways than the MST
> 
> intel_dp_get_sink_irq_esi(), intel_dp_ack_sink_irq_esi()
> 
> functions, which return a pass/fail bool. Imo the error return should be
> the same for both the SST and MST variety of functions and converting
> to propagate an error instead of a pass/fail bool should be done for
> both (SST and MST), which is best done as a follow-up. Are you ok with
> that?
> 

Okay, a follow-up patch sounds good to me.

--
Cheers,
Luca.