[PATCH 02/16] drm/i915/display: Prepare for dsc 3 stream splitter
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Tue Oct 22 10:04:26 UTC 2024
On 10/22/2024 1:21 PM, Jani Nikula wrote:
> On Tue, 22 Oct 2024, "Nautiyal, Ankit K" <ankit.k.nautiyal at intel.com> wrote:
>> On 10/21/2024 6:16 PM, Jani Nikula wrote:
>>> On Mon, 21 Oct 2024, Ankit Nautiyal <ankit.k.nautiyal at 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 at 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,
More information about the Intel-gfx
mailing list