On Mon, Mar 03, 2025 at 11:55:30AM +0200, Murthy, Arun R wrote:
> [...]
> > > +{
> > > + struct intel_encoder *encoder = connector->encoder;
> > > + struct intel_display *display = to_intel_display(encoder);
> > > + const struct drm_display_mode *adjusted_mode =
> > > + &crtc_state->hw.adjusted_mode;
> > > + int symbol_size = intel_dp_is_uhbr(crtc_state) ? 32 : 8;
> > > + int hblank;
> > > +
> > > + if (DISPLAY_VER(display) < 20)
> > > + return;
> > > +
> > > + /* Calculate min Hblank Link Layer Symbol Cycle Count for 8b/10b MST & 128b/132b */
> > > + hblank = DIV_ROUND_UP((DIV_ROUND_UP
> > > + (adjusted_mode->htotal - adjusted_mode->hdisplay, 4) * bpp_x16),
> > > + symbol_size);
> >
> > bpp_x16 is the bpp in .4 fixed point format, so the above seems to be missing a
> > div-by-16 accordingly.
Please also comment on the above.
> > Also the above formula doesn't seem to take DSC into account. Based on the
> > HBLNK_ML_SYM_CYC_CNT formula in DP Standard v2.1 (at 2.6.4.4.1 and
> > 2.7.9) Bspec tells us to use (converting it to LL cycles, which is just assuming 4
> > lanes always), I think we'd need something like:
> >
> > lane_count = 4;
> > hactive = adjusted_mode->hdisplay;
> > htotal = adjusted_mode->htotal;
> > is_mst = true;
> >
> > hactive_sym_cycles = is_dsc ? drm_dp_link_dsc_symbol_cycles(lane_count, hactive, dsc_slices,
> > bpp_x16, symbol_size, is_mst) :
> > drm_dp_link_symbol_cycles(...);
> > htotal_sym_cycles = htotal * hactive_sym_cycles / hactive;
> > hblank_sym_cycles = htotal_sym_cycles - hactive_sym_cycles;
> >
> I assume this is for Htotal LL CYC CNT.
> Min Hblank calculation is included in the VESA spec (https://groups.vesa.org/wg/DP/document/20494)
I meant MIN_HBLNK_LL_SYM_CYC_CNT which Bspec refers to and
defined by the DP Standard as:
MIN_HBLNK_LL_SYM_CYC_CNT = MAX((FLOOR(HTOTAL_WIDTH * EFF_PIX_BPP / (4 * SYMBOL_SIZE)) - HACT_LL_SYM_CYC_CNT),
min_sym_cycles)
where min_sym_cycles is 5 for 8b/10b and 3 for 128b/132b. This is the
same as
MAX(hblank_sym_cycles, min_sym_cycles)
using hblank_sym_cycles from above.
> > > +
> > > + crtc_state->min_hblank = hblank;
> > > +}
> [...]
> > > @@ -1280,6 +1308,29 @@ static void mst_stream_enable(struct intel_atomic_state *state,
> > > TRANS_DP2_VFREQ_PIXEL_CLOCK(crtc_clock_hz & 0xffffff));
> > > }
> > >
> > > + if (DISPLAY_VER(display) >= 20) {
> > > + /*
> > > + * adjust the BlankingStart/BlankingEnd framing control from
> > > + * the calculated value
> > > + */
> >
> > All these adjustments should be done already during computation in
> > intel_dp_mst_compute_min_hblank(), the only thing done here is
> > writing pipe_config->min_hblank to the register.
> >
> As per the comment, the crtc_state should not hold the register
> content, retained the calculated value in crtc_state and register
> alignment done here.
The actual value programmed to the register should be computed already
in intel_dp_mst_compute_min_hblank(). There is no point in doing part of
the computation in intel_dp_mst_compute_min_hblank() and part of it
here. Moving all the computation to intel_dp_mst_compute_min_hblank()
is also what makes sense wrt. the HW/SW state verification.
>
> > HW/SW state verification should be added, as Ville reminded.
> >
> > > + min_hblank = pipe_config->min_hblank - 2;
> >
> > Probably better to avoid getting min_hblank negative already here.
> >
> > > +
> > > + /* Maximum value to be programmed is limited to 0x10 */
> > > + min_hblank = min(0x10, min_hblank);
> >
> > Bspec seems to specify a decimal 10 value.
> >
> Will get this clarified.
>
> > > + /*
> > > + * Minimum hblank accepted for 128b/132b would be 5 and for
> > > + * 8b/10b would be 3 symbol count
This seems to be swapped, based on the DP Standard the minimum should
be 3 for 128b/132b and 5 for 8b/10b.
> > > + */
> >
> > Where is the above 5 and 3 minimum symbol count defined at? I can't see
> > anything related to that at least in Bspec.
> >
> This is mentioned in the HLD and https://groups.vesa.org/wg/DP/document/20494
There should be a comment about this in the code. Based on the DP
Standard, I suppose for 128b/132b the 3 symbols are BS, VB-ID, BE.
> > > + if (intel_dp_is_uhbr(pipe_config))
> > > + min_hblank = max(min_hblank, 5);
> > > + else
> > > + min_hblank = max(min_hblank, 3);
> > > +
> > > + intel_de_write(display, DP_MIN_HBLANK_CTL(trans),
> > > + min_hblank);
> >
> > What is the purpose of programming this? I can't find any explanation for it,
> > but one reason could be to ensure that the audio SDP data can be transmitted
> > during the Hblank period. That would mean it wouldn't need to be programmed
> > if there is no audio being transmitted and that it could be adjusted according to
> > the actual audio BW. Would be good to request a clarification for this from HW
> > people.
> >
> https://groups.vesa.org/wg/DP/document/20494
>
> Address issues for resolutions with high refresh rate that have small
> Hblank, specifically where Hblank is smaller than one MTP.
> Simulations indicate this this will address the jitter issues that
> currently causes BS to be immediately followed by BE which DPRX
> devices are unable to handle.
I can't see this. It would make sense to document this as well in the
code.
> Thanks and Regards,
> Arun R Murthy
> ---------------------
