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.d...@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.d...@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.
