[PATCH v6 09/10] drm/msm/dpu: merge DPU_SSPP_SCALER_QSEED3, QSEED3LITE, QSEED4
Abhinav Kumar
quic_abhinavk at quicinc.com
Wed Nov 1 21:32:45 UTC 2023
On 11/1/2023 2:23 PM, Dmitry Baryshkov wrote:
> On Wed, 1 Nov 2023 at 21:56, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 11/1/2023 12:39 PM, Dmitry Baryshkov wrote:
>>> On Wed, 1 Nov 2023 at 20:43, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/31/2023 1:19 AM, Dmitry Baryshkov wrote:
>>>>> On Mon, 30 Oct 2023 at 22:24, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 10/6/2023 6:14 AM, Dmitry Baryshkov wrote:
>>>>>>> Three different features, DPU_SSPP_SCALER_QSEED3, QSEED3LITE and QSEED4
>>>>>>> are all related to different versions of the same HW scaling block.
>>>>>>> Corresponding driver parts use scaler_blk.version to identify the
>>>>>>> correct way to program the hardware. In order to simplify the driver
>>>>>>> codepath, merge these three feature bits.
>>>>>>>
>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 4 ++--
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 6 +-----
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 9 ++-------
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 3 +--
>>>>>>> 4 files changed, 6 insertions(+), 16 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>> index 32c396abf877..eb867c8123d7 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>> @@ -31,10 +31,10 @@
>>>>>>> (VIG_SDM845_MASK | BIT(DPU_SSPP_SMART_DMA_V2))
>>>>>>>
>>>>>>> #define VIG_SC7180_MASK \
>>>>>>> - (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | BIT(DPU_SSPP_SCALER_QSEED4))
>>>>>>> + (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | BIT(DPU_SSPP_SCALER_QSEED3))
>>>>>>>
>>>>>>> #define VIG_SM6125_MASK \
>>>>>>> - (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | BIT(DPU_SSPP_SCALER_QSEED3LITE))
>>>>>>> + (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | BIT(DPU_SSPP_SCALER_QSEED3))
>>>>>>>
>>>>>>
>>>>>> This merging is coming at a cost of inaccuracy. We are marking sc7180
>>>>>> and sm6125 as scaler_qseed3. But they are not. Let me know what you
>>>>>> think of below idea instead.
>>>>>>
>>>>>>> #define VIG_SC7180_MASK_SDMA \
>>>>>>> (VIG_SC7180_MASK | BIT(DPU_SSPP_SMART_DMA_V2))
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>> index fc5027b0123a..ba262b3f0bdc 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>> @@ -51,9 +51,7 @@ enum {
>>>>>>> /**
>>>>>>> * SSPP sub-blocks/features
>>>>>>> * @DPU_SSPP_SCALER_QSEED2, QSEED2 algorithm support
>>>>>>> - * @DPU_SSPP_SCALER_QSEED3, QSEED3 alogorithm support
>>>>>>> - * @DPU_SSPP_SCALER_QSEED3LITE, QSEED3 Lite alogorithm support
>>>>>>> - * @DPU_SSPP_SCALER_QSEED4, QSEED4 algorithm support
>>>>>>> + * @DPU_SSPP_SCALER_QSEED3, QSEED3 alogorithm support (also QSEED3LITE and QSEED4)
>>>>>>> * @DPU_SSPP_SCALER_RGB, RGB Scaler, supported by RGB pipes
>>>>>>> * @DPU_SSPP_CSC, Support of Color space converion
>>>>>>> * @DPU_SSPP_CSC_10BIT, Support of 10-bit Color space conversion
>>>>>>> @@ -72,8 +70,6 @@ enum {
>>>>>>> enum {
>>>>>>> DPU_SSPP_SCALER_QSEED2 = 0x1,
>>>>>>> DPU_SSPP_SCALER_QSEED3,
>>>>>>> - DPU_SSPP_SCALER_QSEED3LITE,
>>>>>>> - DPU_SSPP_SCALER_QSEED4,
>>>>>>> DPU_SSPP_SCALER_RGB,
>>>>>>> DPU_SSPP_CSC,
>>>>>>> DPU_SSPP_CSC_10BIT,
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>>>>> index 7e9c87088e17..d1b70cf72eef 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>>>>> @@ -594,9 +594,7 @@ static void _setup_layer_ops(struct dpu_hw_sspp *c,
>>>>>>> test_bit(DPU_SSPP_SMART_DMA_V2, &c->cap->features))
>>>>>>> c->ops.setup_multirect = dpu_hw_sspp_setup_multirect;
>>>>>>>
>>>>>>> - if (test_bit(DPU_SSPP_SCALER_QSEED3, &features) ||
>>>>>>> - test_bit(DPU_SSPP_SCALER_QSEED3LITE, &features) ||
>>>>>>> - test_bit(DPU_SSPP_SCALER_QSEED4, &features))
>>>>>>> + if (test_bit(DPU_SSPP_SCALER_QSEED3, &features))
>>>>>>> c->ops.setup_scaler = _dpu_hw_sspp_setup_scaler3;
>>>>>>
>>>>>> Can we just do sblk->scaler_blk.version >= 0x3000 instead of this
>>>>>> merging? That way you can still drop those enums without inaccuracy.
>>>>>
>>>>> No. QSEED3 from sdm845 has version 1.3, msm8998, sdm660 and sdm630
>>>>> have version 1.2.
>>>>>
>>>>
>>>> Ah got it.
>>>>
>>>> HW versioning is getting harder to generalize with the example you have
>>>> given. In my opinion, in that case lets keep these enums intact because
>>>> we dont have any other way of knowing the Qseed version otherwise and in
>>>> terms of LOC, we are not really saving so much in this change.
>>>>
>>>> In the prev change I agreed because along with the name and the version,
>>>> we could still interpret the version but with compressing the enums into
>>>> just QSEED3, this becomes very confusing. So now, in the future if we
>>>> have QSEED5 HW, we will have to change this anyway as its LUT
>>>> programming can change. So we need this distinction.
>>>
>>> I'm trying to eliminate them, because they cause more confusion than
>>> the bonuses.
>>> Currently we have QSEED3 / 3LITE / 4, which are somewhat compatible.
>>>
>>> We are aiming to support QSEED2 and RGB, which are incompatible with
>>> the QSEED3/3lite/4 family programming sequence. So they get their own
>>> feature bits (DPU_SSPP_SCALER_QSEED2 and DPU_SSPP_SCALER_RGB).
>>>
>>> QSEED5-to-be will either be compatible with QSEED3 (and thus can fall
>>> into the same bucket) or it will be a different kind of scaler (and
>>> will get its own feature).
>>>
>>> I'm not a fan of DPU_SSPP_SCALER_QSEED3LITE/QSEED4 and I'd like to
>>> remove those two bits for the following reasons:
>>> - We already encode this info into the scaler version. How should
>>> driver behave it it has e.g. version 3.1, but DPU_SSPP_SCALER_QSEED3?
>>> Or vice versa: version 1.2 but QSEED4 feature bit? Having a single
>>> QSEED3 removes this issue.
>>> - Adding QSEED5-compatible-with-QSEED3 requires changing several
>>> places which deal with the feature bits and the per-version setup
>>> sequence. This seems like an overkill. It is easy to miss one of the
>>> places and thus end up with the half-supported scaler
>>>
>>> I admit, it might not be ideal to use QSEED3 for all scaler versions.
>>> I'm open to suggestions on the better name for this feature bit. But I
>>> have no doubts that there should be a single feature bit for all
>>> QSEED3/3LITE/4 scalers.
>>>
>>
>> hmmm, the concern i had was that from the version the driver doesnt seem
>> to know which qseed it is as you rightly pointed out in your earlier
>> response with the examples of sdm845, msm8998 etc.
>>
>> It needs this feature bit to know which qseed version it is to use the
>> correct scaler function. If you remove the other two places below, this
>> will be the only one left right?
>>
>> I was thinking of solving this problem with something like
>> QSEED3_3LITE_4 but then this is not scalable if QSEED5 is also a variant
>> of QSEED3.
>>
>> After we remove below 2 places, are there more places where we test
>> these feature bits?
>
> Hmm, true, this is the only place enumerating them.
>
>> One thing we can perhaps do is move all this feature bit handling to one
>> function like dpu_scaler_is_qseed3_compatible() and move these feature
>> bits there. That way you will have only one place to change for all the
>> code.
>
> What about renaming QSEED3 to QSEED3_COMPATIBLE then? This would leave
> us with RGB, QSEED2, QSEED3_COMPATIBLE. The QSEED5-to-be will either
> be added as a new entry (and a new scaler function) or it will fall
> into the QSEED3_COMPATIBLE bucket.
>
> I'd really like to remove any chance of confusion between QSEEDn and
> the scaler_block.version. I think we already had that wrong in several
> catalog entries, so let's not walk twice into the same water.
>
Ok thats fine. Lets go ahead with QSEED3_COMPATIBLE as that aligns with
the expectation that we will use the scaler3_lut for everything thats
compatible with QSEED3.
>>>> The below two changes seem fine and that way we are eliminating the
>>>> usages of the enum and we will end up with only one place using this.
>>>>
>>>>
>>>>>>
>>>>>>>
>>>>>>> if (test_bit(DPU_SSPP_CDP, &features))
>>>>>>> @@ -629,10 +627,7 @@ int _dpu_hw_sspp_init_debugfs(struct dpu_hw_sspp *hw_pipe, struct dpu_kms *kms,
>>>>>>> cfg->len,
>>>>>>> kms);
>>>>>>>
>>>>>>> - if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>>>>>> - cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>>>>>> - cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
>>>>>>> - cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>>>>>> + if (sblk->scaler_blk.len)
>>>>>>
>>>>>> This part seems fine.
>>>>>>
>>>>>>> dpu_debugfs_create_regset32("scaler_blk", 0400,
>>>>>>> debugfs_root,
>>>>>>> sblk->scaler_blk.base + cfg->base,
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>>>> index 43135894263c..ba3ee4ba25b3 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>>>> @@ -438,8 +438,7 @@ static void _dpu_plane_setup_scaler3(struct dpu_hw_sspp *pipe_hw,
>>>>>>> scale_cfg->src_height[i] /= chroma_subsmpl_v;
>>>>>>> }
>>>>>>>
>>>>>>> - if (pipe_hw->cap->features &
>>>>>>> - BIT(DPU_SSPP_SCALER_QSEED4)) {
>>>>>>> + if (pipe_hw->cap->sblk->scaler_blk.version >= 0x3000) {
>>>>>> This is fine too.
>>>>>>> scale_cfg->preload_x[i] = DPU_QSEED4_DEFAULT_PRELOAD_H;
>>>>>>> scale_cfg->preload_y[i] = DPU_QSEED4_DEFAULT_PRELOAD_V;
>>>>>>> } else {
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>
>
>
More information about the dri-devel
mailing list