On Tue, 22 Oct 2024, "Nautiyal, Ankit K" <[email protected]> wrote: > On 10/21/2024 6:16 PM, Jani Nikula wrote: >> On Mon, 21 Oct 2024, Ankit Nautiyal <[email protected]> wrote: >>> At the moment dsc_split represents that dsc splitter is used or not. >>> With 3 DSC engines, the splitter can split into two streams or three >>> streams. Make the member dsc_split as int and set that to 2 when dsc >>> splitter splits to 2 stream. >> Maybe also name it accordingly? "dsc split" sounds like a boolean, not >> like an int. >> >> Moreover, when you change the meaning of a member, it's often good code >> hygiene to rename the member to catch any incorrect uses and to ensure >> you changed them all. > > Noted. Will change the name accordingly. > >> >>> v2: Avoid new enum for dsc split. (Suraj) >>> >>> Signed-off-by: Ankit Nautiyal <[email protected]> >>> --- >>> drivers/gpu/drm/i915/display/icl_dsi.c | 2 +- >>> drivers/gpu/drm/i915/display/intel_display.c | 2 +- >>> .../drm/i915/display/intel_display_types.h | 2 +- >>> drivers/gpu/drm/i915/display/intel_dp.c | 2 +- >>> drivers/gpu/drm/i915/display/intel_vdsc.c | 20 ++++++++++++++----- >>> 5 files changed, 19 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c >>> b/drivers/gpu/drm/i915/display/icl_dsi.c >>> index 87a27d91d15d..553e75cf118c 100644 >>> --- a/drivers/gpu/drm/i915/display/icl_dsi.c >>> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c >>> @@ -1595,7 +1595,7 @@ static int gen11_dsi_dsc_compute_config(struct >>> intel_encoder *encoder, >>> >>> /* FIXME: split only when necessary */ >>> if (crtc_state->dsc.slice_count > 1) >>> - crtc_state->dsc.dsc_split = true; >>> + crtc_state->dsc.dsc_split = 2; >>> >>> /* FIXME: initialize from VBT */ >>> vdsc_cfg->rc_model_size = DSC_RC_MODEL_SIZE_CONST; >>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c >>> b/drivers/gpu/drm/i915/display/intel_display.c >>> index ef1436146325..9e2f0fd0558f 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_display.c >>> +++ b/drivers/gpu/drm/i915/display/intel_display.c >>> @@ -5741,7 +5741,7 @@ intel_pipe_config_compare(const struct >>> intel_crtc_state *current_config, >>> PIPE_CONF_CHECK_I(dsc.config.nsl_bpg_offset); >>> >>> PIPE_CONF_CHECK_BOOL(dsc.compression_enable); >>> - PIPE_CONF_CHECK_BOOL(dsc.dsc_split); >>> + PIPE_CONF_CHECK_I(dsc.dsc_split); >>> PIPE_CONF_CHECK_I(dsc.compressed_bpp_x16); >>> >>> PIPE_CONF_CHECK_BOOL(splitter.enable); >>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h >>> b/drivers/gpu/drm/i915/display/intel_display_types.h >>> index 2bb1fa64da2f..58f510909f4d 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h >>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h >>> @@ -1235,7 +1235,7 @@ struct intel_crtc_state { >>> /* Display Stream compression state */ >>> struct { >>> bool compression_enable; >>> - bool dsc_split; >>> + int dsc_split; >>> /* Compressed Bpp in U6.4 format (first 4 bits for fractional >>> part) */ >>> u16 compressed_bpp_x16; >>> u8 slice_count; >>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c >>> b/drivers/gpu/drm/i915/display/intel_dp.c >>> index 286b272aa98c..c1867c883b73 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_dp.c >>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c >>> @@ -2409,7 +2409,7 @@ int intel_dp_dsc_compute_config(struct intel_dp >>> *intel_dp, >>> * then we need to use 2 VDSC instances. >>> */ >>> if (pipe_config->joiner_pipes || pipe_config->dsc.slice_count > 1) >>> - pipe_config->dsc.dsc_split = true; >>> + pipe_config->dsc.dsc_split = 2; >>> >>> ret = intel_dp_dsc_compute_params(connector, pipe_config); >>> if (ret < 0) { >>> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c >>> b/drivers/gpu/drm/i915/display/intel_vdsc.c >>> index 40525f5c4c42..3bce13c21756 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c >>> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c >>> @@ -379,7 +379,14 @@ intel_dsc_power_domain(struct intel_crtc *crtc, enum >>> transcoder cpu_transcoder) >>> >>> static int intel_dsc_get_vdsc_per_pipe(const struct intel_crtc_state >>> *crtc_state) >>> { >>> - return crtc_state->dsc.dsc_split ? 2 : 1; >>> + switch (crtc_state->dsc.dsc_split) { >>> + case 2: >>> + return 2; >>> + case 0: >>> + default: >>> + break; >>> + } >>> + return 1; >> Seems overly complicated. >> >>> } >>> >>> int intel_dsc_get_num_vdsc_instances(const struct intel_crtc_state >>> *crtc_state) >>> @@ -976,8 +983,11 @@ void intel_dsc_get_config(struct intel_crtc_state >>> *crtc_state) >>> if (!crtc_state->dsc.compression_enable) >>> goto out; >>> >>> - crtc_state->dsc.dsc_split = (dss_ctl2 & RIGHT_BRANCH_VDSC_ENABLE) && >>> - (dss_ctl1 & JOINER_ENABLE); >>> + if ((dss_ctl1 & JOINER_ENABLE) && >>> + (dss_ctl2 & RIGHT_BRANCH_VDSC_ENABLE)) >> The extra parens are unnecessary. >> >>> + crtc_state->dsc.dsc_split = 2; >>> + else >>> + crtc_state->dsc.dsc_split = 0; >>> >>> intel_dsc_get_pps_config(crtc_state); >>> out: >>> @@ -988,10 +998,10 @@ static void intel_vdsc_dump_state(struct drm_printer >>> *p, int indent, >>> const struct intel_crtc_state *crtc_state) >>> { >>> drm_printf_indent(p, indent, >>> - "dsc-dss: compressed-bpp:" FXP_Q4_FMT ", slice-count: >>> %d, split: %s\n", >>> + "dsc-dss: compressed-bpp:" FXP_Q4_FMT ", slice-count: >>> %d, split: %d\n", >> So what does the reader think when they see "split: 1" in the logs? >> Split enabled? > > I was meaning to capture the DSC split state as originally intended, and > extend it to have splitting to 3, 2, or None. > > With that we can never have split: 1, but can have either 3, 2, or 0. > > I realize, split:0 is a bit ambiguous, so I am thinking about: > > -change the dsc_split to dsc_streams: to capture number of DSC streams > per pipe, instead of DSC splitter operation. > > -dsc_streams can be 1, 2 and extended to 3. > > -Splitter state will then be implicit, 1 DSC Stream => No Splitter, 2 > DSC Streams => Splitter used to split 2 DSC streams and so on. > > With that, deriving number of DSC engine will also be straight forward > (avoiding the switch case above).
Maybe be even more explicit, and call it num_streams or stream_count or something like that. Also, the crtc_state->dsc.dsc_something is a tautology, the dsc_ prefix is unnecessary when it's already in a dsc substruct. BR, Jani. > > > Thanks & Regards, > > Ankit > > > >> >>> FXP_Q4_ARGS(crtc_state->dsc.compressed_bpp_x16), >>> crtc_state->dsc.slice_count, >>> - str_yes_no(crtc_state->dsc.dsc_split)); >>> + crtc_state->dsc.dsc_split); >>> } >>> >>> void intel_vdsc_state_dump(struct drm_printer *p, int indent, -- Jani Nikula, Intel
