| Message ID | 20220829095832.21770-5-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add DP MST DSC support to i915 | expand |
Hi Stan, I wonder if it is better if you reorder the 3 and 4 patches in this - move this 4/4 before the 3rd one and modify the 3rd one accordingly. Also, instead of getting rid of limits, keep limits and populate the limits according to dsc or normal dp_mst. What do you think? BR vinod On Mon, 2022-08-29 at 12:58 +0300, Stanislav Lisovskiy wrote: > We are using almost same code to loop through bpps while calling > drm_dp_atomic_find_vcpi_slots - lets remove this duplication by > introducing a new function intel_dp_mst_find_vcpi_slots_for_bpp > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 88 +++++++++++---------- > 1 file changed, 46 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index 94d7e7f84c51..2a524816dbfd 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -44,10 +44,14 @@ > #include "intel_hotplug.h" > #include "skl_scaler.h" > > -static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > - struct intel_crtc_state *crtc_state, > - struct drm_connector_state *conn_state, > - struct link_config_limits *limits) > +static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder, > + struct intel_crtc_state *crtc_state, > + int max_bpp, > + int min_bpp, > + struct link_config_limits *limits, > + struct drm_connector_state *conn_state, > + int step, > + bool dsc) > { > struct drm_atomic_state *state = crtc_state->uapi.state; > struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); > @@ -58,7 +62,6 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > struct drm_i915_private *i915 = to_i915(connector->base.dev); > const struct drm_display_mode *adjusted_mode = > &crtc_state->hw.adjusted_mode; > - bool constant_n = drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_CONSTANT_N); > int bpp, slots = -EINVAL; > int ret = 0; > > @@ -71,19 +74,21 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > // TODO: Handle pbn_div changes by adding a new MST helper > if (!mst_state->pbn_div) { > - mst_state->pbn_div = drm_dp_get_vc_payload_bw(&intel_dp->mst_mgr, > - limits->max_rate, > - limits->max_lane_count); > + mst_state->pbn_div = !dsc ? drm_dp_get_vc_payload_bw(&intel_dp->mst_mgr, > + crtc_state->port_clock, > + crtc_state->lane_count) : 0; > } > > - for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) { > + for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) { > crtc_state->pipe_bpp = bpp; > > crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, > - crtc_state->pipe_bpp, > - false); > + dsc ? bpp << 4 : crtc_state->pipe_bpp, > + dsc); > + > slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr, > - connector->port, crtc_state->pbn); > + connector->port, > + crtc_state->pbn); > if (slots == -EDEADLK) > return slots; > if (slots >= 0) { > @@ -101,11 +106,32 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > if (ret && slots >= 0) > slots = ret; > > - if (slots < 0) { > + if (slots < 0) > drm_dbg_kms(&i915->drm, "failed finding vcpi slots:%d\n", > slots); > + > + return slots; > +} > + > + > +static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > + struct intel_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + struct link_config_limits *limits) > +{ > + const struct drm_display_mode *adjusted_mode = > + &crtc_state->hw.adjusted_mode; > + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); > + struct intel_dp *intel_dp = &intel_mst->primary->dp; > + bool constant_n = drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_CONSTANT_N); > + int slots = -EINVAL; > + > + slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state, limits->max_bpp, > + limits->min_bpp, limits, > + conn_state, 2 * 3, false); > + > + if (slots < 0) > return slots; > - } > > intel_link_compute_m_n(crtc_state->pipe_bpp, > crtc_state->lane_count, > @@ -123,25 +149,21 @@ static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder > *encoder, > struct drm_connector_state *conn_state, > struct link_config_limits *limits) > { > - struct drm_atomic_state *state = crtc_state->uapi.state; > - struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); > - struct intel_dp *intel_dp = &intel_mst->primary->dp; > struct intel_connector *connector = > to_intel_connector(conn_state->connector); > + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); > + struct intel_dp *intel_dp = &intel_mst->primary->dp; > struct drm_i915_private *i915 = to_i915(connector->base.dev); > const struct drm_display_mode *adjusted_mode = > &crtc_state->hw.adjusted_mode; > bool constant_n = drm_dp_has_quirk(&intel_dp->desc, > DP_DPCD_QUIRK_CONSTANT_N); > - int bpp, slots = -EINVAL; > + int slots = -EINVAL; > int i, num_bpc; > u8 dsc_bpc[3] = {0}; > int min_bpp, max_bpp; > u8 dsc_max_bpc; > > - crtc_state->lane_count = limits->max_lane_count; > - crtc_state->port_clock = limits->max_rate; > - > /* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */ > if (DISPLAY_VER(i915) >= 12) > dsc_max_bpc = min_t(u8, 12, conn_state->max_requested_bpc); > @@ -162,29 +184,11 @@ static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder > *encoder, > drm_dbg_kms(&i915->drm, "DSC Sink supported min bpp %d max bpp %d\n", > min_bpp, max_bpp); > > - for (bpp = max_bpp; bpp >= min_bpp; bpp -= 2 * 3) { > - crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, > - bpp << 4, > - true); > + slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state, max_bpp, min_bpp, > + limits, conn_state, 2 * 3, true); > > - slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr, > - connector->port, > - crtc_state->pbn, 0); > - > - drm_dbg_kms(&i915->drm, "Trying bpp %d got %d pbn %d slots\n", > - bpp, crtc_state->pbn, slots); > - > - if (slots == -EDEADLK) > - return slots; > - if (slots >= 0) > - break; > - } > - > - if (slots < 0) { > - drm_dbg_kms(&i915->drm, "failed finding vcpi slots:%d\n", > - slots); > + if (slots < 0) > return slots; > - } > > intel_link_compute_m_n(crtc_state->pipe_bpp, > crtc_state->lane_count,
On Mon, Aug 29, 2022 at 05:43:19PM +0300, Govindapillai, Vinod wrote: > Hi Stan, > > I wonder if it is better if you reorder the 3 and 4 patches in this - move this 4/4 before the 3rd > one and modify the 3rd one accordingly. Was thiking about that, but decided to first introduce a new function, using same code, so that we don't mix introduction of the new functionality with code optimization, also it then becomes obvious why we need to remove that duplicate code. But.. may be you are right - I could first extract that function and introduce new DSC functionality using it. > > Also, instead of getting rid of limits, keep limits and populate the limits according to dsc or > normal dp_mst. What do you think? Yeah, was wondering if someone asks this, problem is that in non DSC case limits structure contains exactly min, max bpps which we need, so can be passed on nicely, however in DSC case those are not the same: max_bpp = min_t(u8, dsc_max_bpc * 3, limits->max_bpp); min_bpp = limits->min_bpp; So we would need to create some other limits struct, which we will populate with those(I guess we shouldn't replace the ones, which were calculated for non-DSC case in current limits), so I didn't see much benefit in using it as an argument, if we can't pass it rightaway in both cases. Stan > > BR > vinod > > > On Mon, 2022-08-29 at 12:58 +0300, Stanislav Lisovskiy wrote: > > We are using almost same code to loop through bpps while calling > > drm_dp_atomic_find_vcpi_slots - lets remove this duplication by > > introducing a new function intel_dp_mst_find_vcpi_slots_for_bpp > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 88 +++++++++++---------- > > 1 file changed, 46 insertions(+), 42 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > index 94d7e7f84c51..2a524816dbfd 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > @@ -44,10 +44,14 @@ > > #include "intel_hotplug.h" > > #include "skl_scaler.h" > > > > -static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > - struct intel_crtc_state *crtc_state, > > - struct drm_connector_state *conn_state, > > - struct link_config_limits *limits) > > +static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder, > > + struct intel_crtc_state *crtc_state, > > + int max_bpp, > > + int min_bpp, > > + struct link_config_limits *limits, > > + struct drm_connector_state *conn_state, > > + int step, > > + bool dsc) > > { > > struct drm_atomic_state *state = crtc_state->uapi.state; > > struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); > > @@ -58,7 +62,6 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > struct drm_i915_private *i915 = to_i915(connector->base.dev); > > const struct drm_display_mode *adjusted_mode = > > &crtc_state->hw.adjusted_mode; > > - bool constant_n = drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_CONSTANT_N); > > int bpp, slots = -EINVAL; > > int ret = 0; > > > > @@ -71,19 +74,21 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > > > // TODO: Handle pbn_div changes by adding a new MST helper > > if (!mst_state->pbn_div) { > > - mst_state->pbn_div = drm_dp_get_vc_payload_bw(&intel_dp->mst_mgr, > > - limits->max_rate, > > - limits->max_lane_count); > > + mst_state->pbn_div = !dsc ? drm_dp_get_vc_payload_bw(&intel_dp->mst_mgr, > > + crtc_state->port_clock, > > + crtc_state->lane_count) : 0; > > } > > > > - for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) { > > + for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) { > > crtc_state->pipe_bpp = bpp; > > > > crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, > > - crtc_state->pipe_bpp, > > - false); > > + dsc ? bpp << 4 : crtc_state->pipe_bpp, > > + dsc); > > + > > slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr, > > - connector->port, crtc_state->pbn); > > + connector->port, > > + crtc_state->pbn); > > if (slots == -EDEADLK) > > return slots; > > if (slots >= 0) { > > @@ -101,11 +106,32 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > if (ret && slots >= 0) > > slots = ret; > > > > - if (slots < 0) { > > + if (slots < 0) > > drm_dbg_kms(&i915->drm, "failed finding vcpi slots:%d\n", > > slots); > > + > > + return slots; > > +} > > + > > + > > +static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > + struct intel_crtc_state *crtc_state, > > + struct drm_connector_state *conn_state, > > + struct link_config_limits *limits) > > +{ > > + const struct drm_display_mode *adjusted_mode = > > + &crtc_state->hw.adjusted_mode; > > + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); > > + struct intel_dp *intel_dp = &intel_mst->primary->dp; > > + bool constant_n = drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_CONSTANT_N); > > + int slots = -EINVAL; > > + > > + slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state, limits->max_bpp, > > + limits->min_bpp, limits, > > + conn_state, 2 * 3, false); > > + > > + if (slots < 0) > > return slots; > > - } > > > > intel_link_compute_m_n(crtc_state->pipe_bpp, > > crtc_state->lane_count, > > @@ -123,25 +149,21 @@ static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder > > *encoder, > > struct drm_connector_state *conn_state, > > struct link_config_limits *limits) > > { > > - struct drm_atomic_state *state = crtc_state->uapi.state; > > - struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); > > - struct intel_dp *intel_dp = &intel_mst->primary->dp; > > struct intel_connector *connector = > > to_intel_connector(conn_state->connector); > > + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); > > + struct intel_dp *intel_dp = &intel_mst->primary->dp; > > struct drm_i915_private *i915 = to_i915(connector->base.dev); > > const struct drm_display_mode *adjusted_mode = > > &crtc_state->hw.adjusted_mode; > > bool constant_n = drm_dp_has_quirk(&intel_dp->desc, > > DP_DPCD_QUIRK_CONSTANT_N); > > - int bpp, slots = -EINVAL; > > + int slots = -EINVAL; > > int i, num_bpc; > > u8 dsc_bpc[3] = {0}; > > int min_bpp, max_bpp; > > u8 dsc_max_bpc; > > > > - crtc_state->lane_count = limits->max_lane_count; > > - crtc_state->port_clock = limits->max_rate; > > - > > /* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */ > > if (DISPLAY_VER(i915) >= 12) > > dsc_max_bpc = min_t(u8, 12, conn_state->max_requested_bpc); > > @@ -162,29 +184,11 @@ static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder > > *encoder, > > drm_dbg_kms(&i915->drm, "DSC Sink supported min bpp %d max bpp %d\n", > > min_bpp, max_bpp); > > > > - for (bpp = max_bpp; bpp >= min_bpp; bpp -= 2 * 3) { > > - crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, > > - bpp << 4, > > - true); > > + slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state, max_bpp, min_bpp, > > + limits, conn_state, 2 * 3, true); > > > > - slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr, > > - connector->port, > > - crtc_state->pbn, 0); > > - > > - drm_dbg_kms(&i915->drm, "Trying bpp %d got %d pbn %d slots\n", > > - bpp, crtc_state->pbn, slots); > > - > > - if (slots == -EDEADLK) > > - return slots; > > - if (slots >= 0) > > - break; > > - } > > - > > - if (slots < 0) { > > - drm_dbg_kms(&i915->drm, "failed finding vcpi slots:%d\n", > > - slots); > > + if (slots < 0) > > return slots; > > - } > > > > intel_link_compute_m_n(crtc_state->pipe_bpp, > > crtc_state->lane_count, >
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index 94d7e7f84c51..2a524816dbfd 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -44,10 +44,14 @@ #include "intel_hotplug.h" #include "skl_scaler.h" -static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, - struct intel_crtc_state *crtc_state, - struct drm_connector_state *conn_state, - struct link_config_limits *limits) +static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder, + struct intel_crtc_state *crtc_state, + int max_bpp, + int min_bpp, + struct link_config_limits *limits, + struct drm_connector_state *conn_state, + int step, + bool dsc) { struct drm_atomic_state *state = crtc_state->uapi.state; struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); @@ -58,7 +62,6 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, struct drm_i915_private *i915 = to_i915(connector->base.dev); const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; - bool constant_n = drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_CONSTANT_N); int bpp, slots = -EINVAL; int ret = 0; @@ -71,19 +74,21 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, // TODO: Handle pbn_div changes by adding a new MST helper if (!mst_state->pbn_div) { - mst_state->pbn_div = drm_dp_get_vc_payload_bw(&intel_dp->mst_mgr, - limits->max_rate, - limits->max_lane_count); + mst_state->pbn_div = !dsc ? drm_dp_get_vc_payload_bw(&intel_dp->mst_mgr, + crtc_state->port_clock, + crtc_state->lane_count) : 0; } - for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) { + for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) { crtc_state->pipe_bpp = bpp; crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, - crtc_state->pipe_bpp, - false); + dsc ? bpp << 4 : crtc_state->pipe_bpp, + dsc); + slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr, - connector->port, crtc_state->pbn); + connector->port, + crtc_state->pbn); if (slots == -EDEADLK) return slots; if (slots >= 0) { @@ -101,11 +106,32 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, if (ret && slots >= 0) slots = ret; - if (slots < 0) { + if (slots < 0) drm_dbg_kms(&i915->drm, "failed finding vcpi slots:%d\n", slots); + + return slots; +} + + +static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, + struct intel_crtc_state *crtc_state, + struct drm_connector_state *conn_state, + struct link_config_limits *limits) +{ + const struct drm_display_mode *adjusted_mode = + &crtc_state->hw.adjusted_mode; + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); + struct intel_dp *intel_dp = &intel_mst->primary->dp; + bool constant_n = drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_CONSTANT_N); + int slots = -EINVAL; + + slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state, limits->max_bpp, + limits->min_bpp, limits, + conn_state, 2 * 3, false); + + if (slots < 0) return slots; - } intel_link_compute_m_n(crtc_state->pipe_bpp, crtc_state->lane_count, @@ -123,25 +149,21 @@ static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder *encoder, struct drm_connector_state *conn_state, struct link_config_limits *limits) { - struct drm_atomic_state *state = crtc_state->uapi.state; - struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); - struct intel_dp *intel_dp = &intel_mst->primary->dp; struct intel_connector *connector = to_intel_connector(conn_state->connector); + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); + struct intel_dp *intel_dp = &intel_mst->primary->dp; struct drm_i915_private *i915 = to_i915(connector->base.dev); const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; bool constant_n = drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_CONSTANT_N); - int bpp, slots = -EINVAL; + int slots = -EINVAL; int i, num_bpc; u8 dsc_bpc[3] = {0}; int min_bpp, max_bpp; u8 dsc_max_bpc; - crtc_state->lane_count = limits->max_lane_count; - crtc_state->port_clock = limits->max_rate; - /* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */ if (DISPLAY_VER(i915) >= 12) dsc_max_bpc = min_t(u8, 12, conn_state->max_requested_bpc); @@ -162,29 +184,11 @@ static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder *encoder, drm_dbg_kms(&i915->drm, "DSC Sink supported min bpp %d max bpp %d\n", min_bpp, max_bpp); - for (bpp = max_bpp; bpp >= min_bpp; bpp -= 2 * 3) { - crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, - bpp << 4, - true); + slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state, max_bpp, min_bpp, + limits, conn_state, 2 * 3, true); - slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr, - connector->port, - crtc_state->pbn, 0); - - drm_dbg_kms(&i915->drm, "Trying bpp %d got %d pbn %d slots\n", - bpp, crtc_state->pbn, slots); - - if (slots == -EDEADLK) - return slots; - if (slots >= 0) - break; - } - - if (slots < 0) { - drm_dbg_kms(&i915->drm, "failed finding vcpi slots:%d\n", - slots); + if (slots < 0) return slots; - } intel_link_compute_m_n(crtc_state->pipe_bpp, crtc_state->lane_count,
We are using almost same code to loop through bpps while calling drm_dp_atomic_find_vcpi_slots - lets remove this duplication by introducing a new function intel_dp_mst_find_vcpi_slots_for_bpp Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_dp_mst.c | 88 +++++++++++---------- 1 file changed, 46 insertions(+), 42 deletions(-)