On 3/17/2023 6:30 AM, Ville Syrjälä wrote:
On Tue, Mar 14, 2023 at 04:34:10PM +0530, Ankit Nautiyal wrote:
During modevalid step, the pipe bpp is computed assuming RGB output
format. When checking with DSC, consider the output_format and compute
the input bpp for DSC appropriately.
v2: For DP-MST we currently use RGB output format only, so continue
using RGB while computing dsc_bpp for MST case.
Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
---
drivers/gpu/drm/i915/display/intel_dp.c | 28 ++++++++++++++++-----
drivers/gpu/drm/i915/display/intel_dp.h | 4 ++-
drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 +-
3 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index dcb3c2519041..499390c519ca 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1110,11 +1110,21 @@ intel_dp_mode_valid(struct drm_connector *_connector,
if (HAS_DSC(dev_priv) &&
drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) {
+ int pipe_bpp;
+ enum intel_output_format output_format, sink_format;
+ const struct drm_display_info *info = &connector->base.display_info;
+
+ if ( (info, mode))
+ sink_format = INTEL_OUTPUT_FORMAT_YCBCR420;
+ else
+ sink_format = INTEL_OUTPUT_FORMAT_RGB;
I think I saw this same code duplicated somewhere else already.
Time for a intel_dp_sink_format()?
Yes this can be made as a common function, also used in
intel_dp_mode_min_output_bpp.
Will add the helper intel_dp_sink_format for this.
+
+ output_format = intel_dp_output_format(connector, sink_format);
/*
* TBD pass the connector BPC,
* for now U8_MAX so that max BPC on that platform would be picked
*/
- int pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, U8_MAX);
+ pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, output_format, U8_MAX);
/*
* Output bpp is stored in 6.4 format so right shift by 4 to get the
@@ -1454,12 +1464,15 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
return -EINVAL;
}
-int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 max_req_bpc)
+int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp,
+ enum intel_output_format output_format,
+ u8 max_req_bpc)
{
struct drm_i915_private *i915 = dp_to_i915(intel_dp);
int i, num_bpc;
u8 dsc_bpc[3] = {0};
u8 dsc_max_bpc;
+ int pipe_bpp = 0;
/* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */
if (DISPLAY_VER(i915) >= 12)
@@ -1470,11 +1483,13 @@ int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 max_req_bpc)
num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd,
dsc_bpc);
for (i = 0; i < num_bpc; i++) {
- if (dsc_max_bpc >= dsc_bpc[i])
- return dsc_bpc[i] * 3;
+ if (dsc_max_bpc >= dsc_bpc[i]) {
+ pipe_bpp = dsc_bpc[i] * 3;
+ break;
+ }
}
- return 0;
+ return intel_dp_output_bpp(output_format, pipe_bpp);
The pipe_bpp vs. output_bpp terms seem a bit confused now in the dsc
code.
In the non-compressed cases pipe_bpp does not include any
subsampling, output_bpp is the subsampled version.
What this dsc code seems to want is an intermediate value which
is the subsampled pipe_bpp that is the input to dsc compressor?
And output_bpp/dsc.compressed_bpp is then the final bpp coming
out of the compressor.
Yes I now realize, this is wrong, it should have been pipe_bpp not the
subsampled pipe_bpp.
As I understand, we are using subsampled pipe_bpp only to compute the
bandwidth/clock checks.
You are right, we need use pipe_bpp for programming, and the same is
input to DSC, and not the subsampled pipe_bpp.
I Will remove this line from the function.
I think we should invent a consistent set of names for each so that
it's clear which value the code is concerned with.
Yes you are right output_bpp is used as compressed_bpp in some places,
functions.
We can go with something like this perhaps:
pipe_bpp : the actual bits_per_pixel, that we program
output_bpp : In case of YcbCr420,422 output format bytes_per_pixel will
be half the number of bytes of RGB pixel.
compressed_bpp : (Only for DSC case) the target BPP for the DSC encoder,
after compression.
Does this make sense? I can try to have a separate patch for this.
}
static int intel_dp_source_dsc_version_minor(struct intel_dp *intel_dp)
@@ -1588,7 +1603,8 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
return -EINVAL;
if (compute_pipe_bpp)
- pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, conn_state->max_requested_bpc);
+ pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, pipe_config->output_format,
+ conn_state->max_requested_bpc);
This pipe_bpp gets plugged back into crtc_state->pipe_bpp later and
then it'll be the subsampled version. I don't think that is what we want
eg. for dithering setup and whatnot.
You are right, this is incorrect. Removing the line as mentioned above
will fix this.
Thanks for pointing out this issue. I will fix this in next version of
the patch.
Regards,
Ankit
else
pipe_bpp = pipe_config->pipe_bpp;
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index ef39e4f7a329..2f4136e43f38 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -102,7 +102,9 @@ void intel_read_dp_sdp(struct intel_encoder *encoder,
struct intel_crtc_state *crtc_state,
unsigned int type);
bool intel_digital_port_connected(struct intel_encoder *encoder);
-int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
+int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp,
+ enum intel_output_format output_format,
+ u8 dsc_max_bpc);
u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
u32 link_clock, u32 lane_count,
u32 mode_clock, u32 mode_hdisplay,
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index ff0b821a901a..bdc5c53ccd75 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -917,7 +917,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
* TBD pass the connector BPC,
* for now U8_MAX so that max BPC on that platform would be picked
*/
- int pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, U8_MAX);
+ int pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, INTEL_OUTPUT_FORMAT_RGB, U8_MAX);
if (drm_dp_sink_supports_fec(intel_dp->fec_capable)) {
dsc_max_output_bpp =
--
2.25.1
