| Message ID | 20241021123414.3993899-3-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for 3 VDSC engines 12 slices | expand |
On Mon, 21 Oct 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> 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. > > v2: Avoid new enum for dsc split. (Suraj) > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > --- > 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? > 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,
On 10/21/2024 6:16 PM, Jani Nikula wrote: > On Mon, 21 Oct 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> 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 <ankit.k.nautiyal@intel.com> >> --- >> 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). 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,
On 10/22/2024 9:23 AM, Nautiyal, Ankit K wrote: > > On 10/21/2024 6:16 PM, Jani Nikula wrote: >> On Mon, 21 Oct 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> 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 <ankit.k.nautiyal@intel.com> >>> --- >>> 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). Suraj, You were actually pointing to the same thing in last version, sorry to misunderstand you. I did remove the complicated enum but some how missed this. Will change this as you had originally suggested. Thanks & Regards, Ankit > > > 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,
On Tue, 22 Oct 2024, "Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com> wrote: > On 10/21/2024 6:16 PM, Jani Nikula wrote: >> On Mon, 21 Oct 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> 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 <ankit.k.nautiyal@intel.com> >>> --- >>> 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,
On 10/22/2024 1:21 PM, Jani Nikula wrote: > On Tue, 22 Oct 2024, "Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com> wrote: >> On 10/21/2024 6:16 PM, Jani Nikula wrote: >>> On Mon, 21 Oct 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> 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 <ankit.k.nautiyal@intel.com> >>>> --- >>>> 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. I agree, I am working with dsc.num_streams, something like : https://patchwork.freedesktop.org/patch/620816/?series=134992&rev=6 Regards, Ankit > > 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,
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; } 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)) + 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", 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,
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. v2: Avoid new enum for dsc split. (Suraj) Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> --- 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(-)