> Subject: RE: [PATCH v7 5/8] drm/i915: override Snps's VS/PE when requested
>
> > Subject: RE: [PATCH v7 5/8] drm/i915: override Snps's VS/PE when
> > requested
> >
> >
> > > Subject: [PATCH v7 5/8] drm/i915: override Snps's VS/PE when
> > > requested
> > >
> > > Add accessor function for Snps to read requested table from VBT #57.
> > > Parse the requested table and transform data into port's buffer.
> > >
> > > Choose appropriate accessor function in intel_ddi_buf_trans_get()
> > > basing on
> >
> > * based
> >
> > > display version and PHY type.
> > >
> > > For C20, use 6th table if encoder supports DP 2.0 or higher.
> > > Otherwise use 5th table for DP.
> > >
> > > For C20, tables 1-4 are not used at all and are most likely to be
> > > zeroed. 5th table is used for any mode below DP 2.0 (exclusive). 6th
> > > table is used for any mode above DP 2.0 (inclusive).
> > >
> > > For C10, use 2nd table for external DP if encoder supports any mode
> > > beyond or including HBR2. Use 1st table if external DP encoder
> > > supports anything lower than HBR2. For eDP, use 4th table if encoder
> > > supports HBR3. Otherwise use 3rd table for eDP.
> > >
> > > For C10, 1st table is used for external DP with modes below HBR2
> > (exclusive).
> > > 1st table is also used as a fallback for non-DPs. 2nd table is used
> > > for external DP with modes higher than HBR2 (inclusive).
> > > 3rd table is used for eDP with modes lower than HBR3 (exclusive).
> > > 4th table is used for eDP with modes higher than HBR3 (inclusive).
> > >
> > > Indices for other tables have not yet been observed to be used as of now.
> > >
> > > There are no changes to intel_ddi_dp_level() since selection of
> > > correct row of intel_ddi_buf_trans_entry is same as when no override
> > > request has been done.
> > >
> > > v6->v7
> > > - handle VS/PE-O's VBT details in intel_bios_* functions (Jani)
> > > - remove vspeo's cast to (void *) (Jani)
> > > - check devdata->vspeo if VS/PE-O was requested
> > > - call encoder->get_buf_trans() once (Jani)
> > > - return NULL from intel_bios_get_* when using default (Jani)
> > > - validate VS/PE-O in intel_bios.c (Jani)
> > > - inline mtl_{c10,c20}_get_vspeo_buf_trans()
> > > - remove temporarily LT
> > >
> > > v4->v5
> > > - blend index computation with table parsing
> > > - remove enums entirely
> > > - change funcs prefix from snps_ to mtl_ (Suraj)
> > > - add spaces around operators (Suraj)
> > > - remove spaces after type casting (Suraj)
> > > - remove INTEL_DISPLAY_STATE_WARN (Suraj)
> > >
> > > v3->v4
> > > - stick to solely changing VBT data into current structures (Jani)
> > > - move iterator declaration to declaration block (Suraj)
> > >
> > > v2->v3
> > > - remove unnecessary braces from if block (Suraj)
> > > - return -EINVAL instead of -1 (Suraj)
> > >
> > > Signed-off-by: Michał Grzelak <[email protected]>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_bios.c | 100 ++++++++++++++++++
> > > drivers/gpu/drm/i915/display/intel_bios.h | 7 ++
> > > .../drm/i915/display/intel_ddi_buf_trans.c | 21 ++++
> > > 3 files changed, 128 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
> > > b/drivers/gpu/drm/i915/display/intel_bios.c
> > > index 3d8864374cac..59ffb5bc8848 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > > @@ -3861,6 +3861,106 @@ bool intel_bios_encoder_supports_tbt(const
> > > struct intel_bios_encoder_data *devda
> > > return devdata->display->vbt.version >= 209 && devdata->child.tbt;
> > > }
> > >
> > > +static bool
> > > +validate_vspeo(const struct intel_bios_encoder_data *devdata, bool
> > > +has_dp) {
> >
> > Ahh ohkay so you deal with that allocation problem this way. This
> > seems good too.
> >
> >
> > > + struct intel_ddi_buf_trans *vspeo;
> > > +
> > > + if (!devdata)
> > > + return false;
> > > +
> > > + /* if vspeo is allocated then VS/PE-O was requested */
So you can remove these comments not needed
> > > + vspeo = devdata->vspeo;
> > > + if (!vspeo)
> > > + return false;
> > > +
> > > + /* if crtc_state has eDP it also has DP */
Same here.
> > > + if (!has_dp)
> > > + return false;
> > > +
> > > + return true;
> > > +}
> > > +
> > > +const struct intel_ddi_buf_trans *
> > > +intel_bios_get_c20_vspeo(const struct intel_bios_encoder_data
> *devdata,
> > > + bool has_dp, bool is_uhbr)
> > > +{
> > > + struct intel_display *display;
> > > + union intel_ddi_buf_trans_entry *entries;
> > > + int num_columns, num_rows, level, idx;
> > > + struct intel_ddi_buf_trans *vspeo;
> > > + const u32 *tables;
> > > + size_t offset = 0;
> > > +
> > > + if (!validate_vspeo(devdata, has_dp))
> > > + return NULL;
> > > +
> > > + display = devdata->display;
> > > + entries = (void *)vspeo->entries;
> > > + tables = display->vbt.vspeo.tables;
> > > + num_columns = display->vbt.vspeo.num_columns;
> > > + num_rows = display->vbt.vspeo.num_rows;
> > > + idx = is_uhbr ? 5 : 4;
> > > +
> > > + offset += idx * num_rows * num_columns;
> > > +
> > > + for (level = 0; level < num_rows; level++) {
> > > + u32 vswing = tables[offset];
> > > + u32 pre_cursor = tables[offset + 1];
> > > + u32 post_cursor = tables[offset + 2];
> > > +
> > > + entries[level].snps.vswing = vswing;
> > > + entries[level].snps.pre_cursor = pre_cursor;
> > > + entries[level].snps.post_cursor = post_cursor;
> > > +
> > > + offset += num_columns;
> > > + }
> > > +
> > > + return vspeo;
> > > +}
> > > +
> > > +const struct intel_ddi_buf_trans *
> > > +intel_bios_get_c10_vspeo(const struct intel_bios_encoder_data
> *devdata,
> > > + bool has_dp, int port_clock, bool has_edp) {
> > > + struct intel_display *display;
> > > + union intel_ddi_buf_trans_entry *entries;
> > > + int num_columns, num_rows, level, idx;
> > > + struct intel_ddi_buf_trans *vspeo;
> > > + const u32 *tables;
> > > + size_t offset = 0;
> > > +
> > > + if (!validate_vspeo(devdata, has_dp))
> > > + return NULL;
> > > +
> > > + display = devdata->display;
> > > + vspeo = devdata->vspeo;
> > > + entries = (void *)vspeo->entries;
> > > + tables = display->vbt.vspeo.tables;
> > > + num_columns = display->vbt.vspeo.num_columns;
> > > + num_rows = display->vbt.vspeo.num_rows;
> > > +
> > > + idx = port_clock > 270000 ? 1 : 0;
> > > + if (has_edp)
> > > + idx = port_clock > 540000 ? 3 : 2;
> > > +
> > > + offset += idx * num_rows * num_columns;
> > > +
> > > + for (level = 0; level < num_rows; level++) {
> > > + u32 vswing = tables[offset];
> > > + u32 pre_cursor = tables[offset + 1];
> > > + u32 post_cursor = tables[offset + 2];
> > > +
> > > + entries[level].snps.vswing = vswing;
> > > + entries[level].snps.pre_cursor = pre_cursor;
> > > + entries[level].snps.post_cursor = post_cursor;
> > > +
> > > + offset += num_columns;
> > > + }
> > > +
> > > + return vspeo;
> > > +}
> > > +
> > > bool intel_bios_encoder_is_dedicated_external(const struct
> > > intel_bios_encoder_data *devdata) {
> > > return devdata->display->vbt.version >= 264 && diff --git
> > > a/drivers/gpu/drm/i915/display/intel_bios.h
> > > b/drivers/gpu/drm/i915/display/intel_bios.h
> > > index 7a50a272cd27..49acf8c405e2 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bios.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> > > @@ -73,6 +73,13 @@ bool intel_bios_get_dsc_params(struct
> > > intel_encoder *encoder, const struct intel_bios_encoder_data *
> > > intel_bios_encoder_data_lookup(struct intel_display *display, enum
> > > port port);
> > >
> > > +const struct intel_ddi_buf_trans *
> > > +intel_bios_get_c20_vspeo(const struct intel_bios_encoder_data
> *devdata,
> > > + bool has_dp, bool is_uhbr);
> > > +const struct intel_ddi_buf_trans *
> > > +intel_bios_get_c10_vspeo(const struct intel_bios_encoder_data
> *devdata,
> > > + bool has_dp, int port_clock, bool has_edp);
> > > +
> > > bool intel_bios_encoder_requests_vspeo(const struct
> > > intel_bios_encoder_data *devdata); bool
> > > intel_bios_encoder_supports_dvi(const struct intel_bios_encoder_data
> > > *devdata); bool intel_bios_encoder_supports_hdmi(const struct
> > > intel_bios_encoder_data *devdata); diff --git
> > > a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> > > index 4cd1e4d76c7a..b4120b9c49b2 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> > > @@ -1857,5 +1857,26 @@ const struct intel_ddi_buf_trans
> > > *intel_ddi_buf_trans_get(struct intel_encoder *
> > > const struct
> > > intel_crtc_state *crtc_state,
> > > int *n_entries)
> > > {
> > > + struct intel_display *display = to_intel_display(encoder);
> > > + const struct intel_bios_encoder_data *devdata = encoder->devdata;
> > > + const struct intel_ddi_buf_trans *buf_trans = NULL;
> > > + bool has_edp, has_dp, is_uhbr;
> > > + int port_clock;
> > > +
> > > +
> > > + is_uhbr = intel_dp_is_uhbr(crtc_state);
> > > + port_clock = crtc_state->port_clock;
> >
> > We don't need a separate port_clock variable its just called at one
> > place just directly use crtc_state->port_clock.
> >
> > > +
> > > + if (DISPLAY_VER(display) >= 14) {
> >
> > This needs to use >= 14 && < 35 so that this does not spill into LT
> > PHY territory
So this need to be done like this just so that we don’t break bisectbility
Anyways you later add LT PHY check.
I think you can also remove the line where you say you remove LT PHY as it
still exists in series.
With above fixed.
LGTM,
Reviewed-by: Suraj Kandpal <[email protected]>
> > > + if (intel_encoder_is_c10phy(encoder))
> > > + buf_trans = intel_bios_get_c10_vspeo(devdata,
> > > has_dp, port_clock, has_edp);
> > > + else
> > > + buf_trans = intel_bios_get_c20_vspeo(devdata,
> > > has_dp, is_uhbr);
> > > + }
> > > +
> > > + if (buf_trans)
> > > + return intel_get_buf_trans(buf_trans, n_entries);
> >
> > I think this code block belongs inside the above if block guarded by
> > DISPLAY_VER check
>
> Or maybe not since this later becomes a else if ladder you can ignore this
> comment.
>
> Regards,
> Suraj Kandpal
>
> >
> > Regards,
> > Suraj Kandpal
> >
> > > +
> > > return encoder->get_buf_trans(encoder, crtc_state, n_entries); }
> > > --
> > > 2.45.2
