[PATCH v2 10/16] drm/i915/ddi: write payload for 128b/132b SST

[Imre Deak]

On Thu, Jan 02, 2025 at 12:52:38PM +0200, Jani Nikula wrote:
> On Tue, 31 Dec 2024, Imre Deak <imre.deak at intel.com> wrote:
> > On Thu, Dec 19, 2024 at 11:33:59PM +0200, Jani Nikula wrote:
> >> Write the payload allocation table for 128b/132b SST. Use VCPID 1 and
> >> start from slot 0, with dp_m_n.tu slots.
> >> 
> >> This is preparation for enabling 128b/132b SST. This path is not
> >> reachable yet. Indeed, we don't yet compute TU for 128b/132b SST.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_ddi.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> >> index 6f813bf85b23..64528ff8856e 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> >> @@ -2669,6 +2669,12 @@ static void mtl_ddi_pre_enable_dp(struct intel_atomic_state *state,
> >>  	/* 6.o Configure and enable FEC if needed */
> >>  	intel_ddi_enable_fec(encoder, crtc_state);
> >>  
> >> +	/* 7.a 128b/132b SST. */
> >> +	if (!is_mst && intel_dp_is_uhbr(crtc_state)) {
> >> +		/* VCPID 1, start slot 0 for 128b/132b, tu slots */
> >> +		drm_dp_dpcd_write_payload(&intel_dp->aux, 1, 0, crtc_state->dp_m_n.tu);
> >
> > I would handle the error by sending a modeset retry uevent. Either way:
> 
> Mmmh. The MST code calls intel_dp_queue_modeset_retry_for_link() in
> response to drm_dp_add_payload_part{1,2}() failures, but nothing
> else. As in, we don't do anything else (we just plunge on with the
> enable), and we don't even do that much in response to any other errors.
> 
> Our options are limited anyway.

The right thing to do is to report all these errors to user space, so it
can recover from it.

> Arguably the MST path payload update has more failure modes than SST,
> but you'd like to do this:
> 
> 	ret = drm_dp_dpcd_write_payload(&intel_dp->aux, 1, 0, crtc_state->dp_m_n.tu);
> 	if (ret < 0)
> 		intel_dp_queue_modeset_retry_for_link(state, encoder, crtc_state);
> 
> in mtl_ddi_pre_enable_dp() and tgl_ddi_pre_enable_dp()?

Yes.

> Overall I'm wondering if this too needs an overhaul. Instead of queueing
> the retry all over the place, maybe we should just flag it in
> crtc_state, and have intel_atomic_cleanup_work() do it once afterwards?

Atm it's queued once after detecting the first error, not sure if
deferring that would be better.

> BR,
> Jani.
> 
> >
> > Reviewed-by: Imre Deak <imre.deak at intel.com>
> >
> >> +	}
> >> +
> >>  	if (!is_mst)
> >>  		intel_dsc_dp_pps_write(encoder, crtc_state);
> >>  }
> >> @@ -2808,6 +2814,11 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
> >>  	/* 7.l Configure and enable FEC if needed */
> >>  	intel_ddi_enable_fec(encoder, crtc_state);
> >>  
> >> +	if (!is_mst && intel_dp_is_uhbr(crtc_state)) {
> >> +		/* VCPID 1, start slot 0 for 128b/132b, tu slots */
> >> +		drm_dp_dpcd_write_payload(&intel_dp->aux, 1, 0, crtc_state->dp_m_n.tu);
> >> +	}
> >> +
> >>  	if (!is_mst)
> >>  		intel_dsc_dp_pps_write(encoder, crtc_state);
> >>  }
> >> -- 
> >> 2.39.5
> >> 
> 
> -- 
> Jani Nikula, Intel