Add DATA_COMPRESS feature flag to DPU INTF block.
In DPU 7.x and later, DSC/DCE enablement registers have been moved from
PINGPONG to INTF.
As core_rev (and related macros) was removed from the dpu_kms struct, the
most straightforward way to indicate the presence of this register would be
to have a feature flag.
Changes in v2:
- Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag
Signed-off-by: Jessica Zhang <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
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 7944481d0a33..c74051906d05 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -104,7 +104,7 @@
#define INTF_SC7180_MASK \
(BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE) | BIT(DPU_INTF_STATUS_SUPPORTED))
-#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
+#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) | BIT(DPU_INTF_DATA_COMPRESS)
#define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
BIT(DPU_WB_UBWC) | \
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 4eda2cc847ef..01c65f940f2a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -185,6 +185,7 @@ enum {
* @DPU_DATA_HCTL_EN Allows data to be transferred at different rate
* than video timing
* @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register
+ * @DPU_INTF_DATA_COMPRESS INTF block has DATA_COMPRESS register
* @DPU_INTF_MAX
*/
enum {
@@ -192,6 +193,7 @@ enum {
DPU_INTF_TE,
DPU_DATA_HCTL_EN,
DPU_INTF_STATUS_SUPPORTED,
+ DPU_INTF_DATA_COMPRESS,
DPU_INTF_MAX
};
--
2.40.1
On 2023-05-05 14:23:50, Jessica Zhang wrote:
> Add DATA_COMPRESS feature flag to DPU INTF block.
>
> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
> PINGPONG to INTF.
>
> As core_rev (and related macros) was removed from the dpu_kms struct, the
> most straightforward way to indicate the presence of this register would be
> to have a feature flag.
Irrelevant. Even though core_rev was still in mainline until recently,
we always hardcoded the features in the catalog and only used core_rev
to select a dpu_mdss_cfg catalog entry. There is no "if version >= X
then enable feature Y" logic, this manually-enabled feature flag is the
only, correct way to do it.
> Changes in v2:
> - Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag
>
> Signed-off-by: Jessica Zhang <[email protected]>
Reviewed-by: Marijn Suijten <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> 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 7944481d0a33..c74051906d05 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -104,7 +104,7 @@
> #define INTF_SC7180_MASK \
> (BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE) | BIT(DPU_INTF_STATUS_SUPPORTED))
>
> -#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
> +#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) | BIT(DPU_INTF_DATA_COMPRESS)
Konrad: Your SM6350/SM6375 series v3 [1] switched from INTF_SC7180_MASK
to INTF_SC7280_MASK to enable HCTL on SM6375, but that will now
erroneously also receive this feature flag and write the new
DATA_COMPESS mask even if it's DPU 6.9 (< 7.x where it got added).
[1]: https://lore.kernel.org/linux-arm-msm/[email protected]/T/#u
Depending on who lands first, this flag should be split.
I still see value in inlining and removing these defines, though that
brings a host of other complexity.
- Marijn
> #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
> BIT(DPU_WB_UBWC) | \
> 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 4eda2cc847ef..01c65f940f2a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -185,6 +185,7 @@ enum {
> * @DPU_DATA_HCTL_EN Allows data to be transferred at different rate
> * than video timing
> * @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register
> + * @DPU_INTF_DATA_COMPRESS INTF block has DATA_COMPRESS register
> * @DPU_INTF_MAX
> */
> enum {
> @@ -192,6 +193,7 @@ enum {
> DPU_INTF_TE,
> DPU_DATA_HCTL_EN,
> DPU_INTF_STATUS_SUPPORTED,
> + DPU_INTF_DATA_COMPRESS,
> DPU_INTF_MAX
> };
>
>
> --
> 2.40.1
>
On 07/05/2023 19:00, Marijn Suijten wrote:
> On 2023-05-05 14:23:50, Jessica Zhang wrote:
>> Add DATA_COMPRESS feature flag to DPU INTF block.
>>
>> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
>> PINGPONG to INTF.
>>
>> As core_rev (and related macros) was removed from the dpu_kms struct, the
>> most straightforward way to indicate the presence of this register would be
>> to have a feature flag.
>
> Irrelevant. Even though core_rev was still in mainline until recently,
> we always hardcoded the features in the catalog and only used core_rev
> to select a dpu_mdss_cfg catalog entry. There is no "if version >= X
> then enable feature Y" logic, this manually-enabled feature flag is the
> only, correct way to do it.
>
>> Changes in v2:
>> - Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag
>>
>> Signed-off-by: Jessica Zhang <[email protected]>
>
> Reviewed-by: Marijn Suijten <[email protected]>
>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> 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 7944481d0a33..c74051906d05 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -104,7 +104,7 @@
>> #define INTF_SC7180_MASK \
>> (BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE) | BIT(DPU_INTF_STATUS_SUPPORTED))
>>
>> -#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
>> +#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) | BIT(DPU_INTF_DATA_COMPRESS)
>
> Konrad: Your SM6350/SM6375 series v3 [1] switched from INTF_SC7180_MASK
> to INTF_SC7280_MASK to enable HCTL on SM6375, but that will now
> erroneously also receive this feature flag and write the new
> DATA_COMPESS mask even if it's DPU 6.9 (< 7.x where it got added).
Yeah, that's why I had the idea of including at least the DPU major in
the mask name.
It looks like we should enable DPU_DATA_HCTL_EN at least for
sm8150/sm8250 (and other DPU 6.x) too. I am not sure if it is present on
sdm845.
>
> [1]: https://lore.kernel.org/linux-arm-msm/[email protected]/T/#u
>
> Depending on who lands first, this flag should be split.
>
> I still see value in inlining and removing these defines, though that
> brings a host of other complexity.
>
> - Marijn
>
>> #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
>> BIT(DPU_WB_UBWC) | \
>> 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 4eda2cc847ef..01c65f940f2a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -185,6 +185,7 @@ enum {
>> * @DPU_DATA_HCTL_EN Allows data to be transferred at different rate
>> * than video timing
>> * @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register
>> + * @DPU_INTF_DATA_COMPRESS INTF block has DATA_COMPRESS register
>> * @DPU_INTF_MAX
>> */
>> enum {
>> @@ -192,6 +193,7 @@ enum {
>> DPU_INTF_TE,
>> DPU_DATA_HCTL_EN,
>> DPU_INTF_STATUS_SUPPORTED,
>> + DPU_INTF_DATA_COMPRESS,
>> DPU_INTF_MAX
>> };
>>
>>
>> --
>> 2.40.1
>>
--
With best wishes
Dmitry
On 7.05.2023 18:00, Marijn Suijten wrote:
> On 2023-05-05 14:23:50, Jessica Zhang wrote:
>> Add DATA_COMPRESS feature flag to DPU INTF block.
>>
>> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
>> PINGPONG to INTF.
>>
>> As core_rev (and related macros) was removed from the dpu_kms struct, the
>> most straightforward way to indicate the presence of this register would be
>> to have a feature flag.
>
> Irrelevant. Even though core_rev was still in mainline until recently,
> we always hardcoded the features in the catalog and only used core_rev
> to select a dpu_mdss_cfg catalog entry. There is no "if version >= X
> then enable feature Y" logic, this manually-enabled feature flag is the
> only, correct way to do it.
>
>> Changes in v2:
>> - Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag
>>
>> Signed-off-by: Jessica Zhang <[email protected]>
>
> Reviewed-by: Marijn Suijten <[email protected]>
>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> 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 7944481d0a33..c74051906d05 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -104,7 +104,7 @@
>> #define INTF_SC7180_MASK \
>> (BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE) | BIT(DPU_INTF_STATUS_SUPPORTED))
>>
>> -#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
>> +#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) | BIT(DPU_INTF_DATA_COMPRESS)
>
> Konrad: Your SM6350/SM6375 series v3 [1] switched from INTF_SC7180_MASK
> to INTF_SC7280_MASK to enable HCTL on SM6375, but that will now
> erroneously also receive this feature flag and write the new
> DATA_COMPESS mask even if it's DPU 6.9 (< 7.x where it got added).
>
> [1]: https://lore.kernel.org/linux-arm-msm/[email protected]/T/#u
>
> Depending on who lands first, this flag should be split.
I'll adapt my patches. Jessica, no changes required on your side.
>
> I still see value in inlining and removing these defines, though that
> brings a host of other complexity.
right, we should totally do it after we settle down from the patch
flurry
Konrad
>
> - Marijn
>
>> #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
>> BIT(DPU_WB_UBWC) | \
>> 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 4eda2cc847ef..01c65f940f2a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -185,6 +185,7 @@ enum {
>> * @DPU_DATA_HCTL_EN Allows data to be transferred at different rate
>> * than video timing
>> * @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register
>> + * @DPU_INTF_DATA_COMPRESS INTF block has DATA_COMPRESS register
>> * @DPU_INTF_MAX
>> */
>> enum {
>> @@ -192,6 +193,7 @@ enum {
>> DPU_INTF_TE,
>> DPU_DATA_HCTL_EN,
>> DPU_INTF_STATUS_SUPPORTED,
>> + DPU_INTF_DATA_COMPRESS,
>> DPU_INTF_MAX
>> };
>>
>>
>> --
>> 2.40.1
>>
On 2023-05-07 22:21:35, Dmitry Baryshkov wrote:
<snip>
> > Konrad: Your SM6350/SM6375 series v3 [1] switched from INTF_SC7180_MASK
> > to INTF_SC7280_MASK to enable HCTL on SM6375, but that will now
> > erroneously also receive this feature flag and write the new
> > DATA_COMPESS mask even if it's DPU 6.9 (< 7.x where it got added).
>
> Yeah, that's why I had the idea of including at least the DPU major in
> the mask name.
Yes please, that would be much more clear. We could even drop the SoC
name altogether.
> It looks like we should enable DPU_DATA_HCTL_EN at least for
> sm8150/sm8250 (and other DPU 6.x) too. I am not sure if it is present on
> sdm845.
Agreed, thanks for sending that patch!
<snip>
- Marijn
On 2023-05-08 14:46:10, Jessica Zhang wrote:
>
>
> On 5/7/2023 9:00 AM, Marijn Suijten wrote:
> > On 2023-05-05 14:23:50, Jessica Zhang wrote:
> >> Add DATA_COMPRESS feature flag to DPU INTF block.
> >>
> >> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
> >> PINGPONG to INTF.
> >>
> >> As core_rev (and related macros) was removed from the dpu_kms struct, the
> >> most straightforward way to indicate the presence of this register would be
> >> to have a feature flag.
> >
> > Irrelevant. Even though core_rev was still in mainline until recently,
> > we always hardcoded the features in the catalog and only used core_rev
> > to select a dpu_mdss_cfg catalog entry. There is no "if version >= X
> > then enable feature Y" logic, this manually-enabled feature flag is the
> > only, correct way to do it.
>
> Hi Marijn,
>
> Understood.
Thanks if you can drop the paragraph.
> FWIW, if we do find more register bit-level differences
> between HW versions in the future, it might make more sense to keep the
> HW catalog small and bring core_rev back, rather than keep adding these
> kinds of small differences to caps.
That is not up to me to decide, but I do agree that DPU is currently
"one big mess" where lots of things are hardcoded in the catalog (which
isn't a bad thing, these things won't change but it does make it harder
on us than if we could dynamically state "every DPU between these two
revisions"), and certain other things are/were read back from hardware
registers.
As well as the sub-block feature flags that pain us :)
- Marijn
> Thanks,
>
> Jessica Zhang
<snip>
On 2023-05-08 10:36:09, Konrad Dybcio wrote:
<snip>
> > Depending on who lands first, this flag should be split.
> I'll adapt my patches. Jessica, no changes required on your side.
Looks like no split is needed after moving HCTL to SC7180. Your SM6375
series can use SC7180 instead of SC7280 now and everything will be right
without a change here.
> > I still see value in inlining and removing these defines, though that
> > brings a host of other complexity.
> right, we should totally do it after we settle down from the patch
> flurry
But we just got a nice velocity, I'd rather keep going :) - we can
totally address these cleanups somewhere along the line though.
- Marijn
On 5/7/2023 9:00 AM, Marijn Suijten wrote:
> On 2023-05-05 14:23:50, Jessica Zhang wrote:
>> Add DATA_COMPRESS feature flag to DPU INTF block.
>>
>> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
>> PINGPONG to INTF.
>>
>> As core_rev (and related macros) was removed from the dpu_kms struct, the
>> most straightforward way to indicate the presence of this register would be
>> to have a feature flag.
>
> Irrelevant. Even though core_rev was still in mainline until recently,
> we always hardcoded the features in the catalog and only used core_rev
> to select a dpu_mdss_cfg catalog entry. There is no "if version >= X
> then enable feature Y" logic, this manually-enabled feature flag is the
> only, correct way to do it.
Hi Marijn,
Understood. FWIW, if we do find more register bit-level differences
between HW versions in the future, it might make more sense to keep the
HW catalog small and bring core_rev back, rather than keep adding these
kinds of small differences to caps.
Thanks,
Jessica Zhang
>
>> Changes in v2:
>> - Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag
>>
>> Signed-off-by: Jessica Zhang <[email protected]>
>
> Reviewed-by: Marijn Suijten <[email protected]>
>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> 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 7944481d0a33..c74051906d05 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -104,7 +104,7 @@
>> #define INTF_SC7180_MASK \
>> (BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE) | BIT(DPU_INTF_STATUS_SUPPORTED))
>>
>> -#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
>> +#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) | BIT(DPU_INTF_DATA_COMPRESS)
>
> Konrad: Your SM6350/SM6375 series v3 [1] switched from INTF_SC7180_MASK
> to INTF_SC7280_MASK to enable HCTL on SM6375, but that will now
> erroneously also receive this feature flag and write the new
> DATA_COMPESS mask even if it's DPU 6.9 (< 7.x where it got added).
>
> [1]: https://lore.kernel.org/linux-arm-msm/[email protected]/T/#u
>
> Depending on who lands first, this flag should be split.
>
> I still see value in inlining and removing these defines, though that
> brings a host of other complexity.
>
> - Marijn
>
>> #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
>> BIT(DPU_WB_UBWC) | \
>> 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 4eda2cc847ef..01c65f940f2a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -185,6 +185,7 @@ enum {
>> * @DPU_DATA_HCTL_EN Allows data to be transferred at different rate
>> * than video timing
>> * @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register
>> + * @DPU_INTF_DATA_COMPRESS INTF block has DATA_COMPRESS register
>> * @DPU_INTF_MAX
>> */
>> enum {
>> @@ -192,6 +193,7 @@ enum {
>> DPU_INTF_TE,
>> DPU_DATA_HCTL_EN,
>> DPU_INTF_STATUS_SUPPORTED,
>> + DPU_INTF_DATA_COMPRESS,
>> DPU_INTF_MAX
>> };
>>
>>
>> --
>> 2.40.1
>>
On 09/05/2023 00:46, Jessica Zhang wrote:
>
>
> On 5/7/2023 9:00 AM, Marijn Suijten wrote:
>> On 2023-05-05 14:23:50, Jessica Zhang wrote:
>>> Add DATA_COMPRESS feature flag to DPU INTF block.
>>>
>>> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
>>> PINGPONG to INTF.
>>>
>>> As core_rev (and related macros) was removed from the dpu_kms struct,
>>> the
>>> most straightforward way to indicate the presence of this register
>>> would be
>>> to have a feature flag.
>>
>> Irrelevant. Even though core_rev was still in mainline until recently,
>> we always hardcoded the features in the catalog and only used core_rev
>> to select a dpu_mdss_cfg catalog entry. There is no "if version >= X
>> then enable feature Y" logic, this manually-enabled feature flag is the
>> only, correct way to do it.
>
> Hi Marijn,
>
> Understood. FWIW, if we do find more register bit-level differences
> between HW versions in the future, it might make more sense to keep the
> HW catalog small and bring core_rev back, rather than keep adding these
> kinds of small differences to caps.
Let's see how it goes. Abhinav suggested that there might be feature
differences inside the DPU generations (and even inside the single DPU
major/minor combo). So I'm not sure what core_rev will bring us.
Let's land the platforms which are ready (or if there is anything close
to be submitted). I'll post the next proposal for the catalog cleanups
close to -rc4, when the dust settles then we can have one or two weaks
for the discussion and polishing.
I'd like to consider:
- inlining foo_BLK macros, if that makes adding new features easier
- reformat of clk_ctrls
- maybe reintroduction of per-generation feature masks instead of
keeping them named after the random SoC
- maybe a rework of mdss_irqs / INTFn_INTR. We already have this info in
hw catalog.
Comments are appreciated.
>
> Thanks,
>
> Jessica Zhang
>
>>
>>> Changes in v2:
>>> - Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag
>>>
>>> Signed-off-by: Jessica Zhang <[email protected]>
>>
>> Reviewed-by: Marijn Suijten <[email protected]>
>>
>>> ---
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> 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 7944481d0a33..c74051906d05 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>> @@ -104,7 +104,7 @@
>>> #define INTF_SC7180_MASK \
>>> (BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE) |
>>> BIT(DPU_INTF_STATUS_SUPPORTED))
>>> -#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
>>> +#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) |
>>> BIT(DPU_INTF_DATA_COMPRESS)
>>
>> Konrad: Your SM6350/SM6375 series v3 [1] switched from INTF_SC7180_MASK
>> to INTF_SC7280_MASK to enable HCTL on SM6375, but that will now
>> erroneously also receive this feature flag and write the new
>> DATA_COMPESS mask even if it's DPU 6.9 (< 7.x where it got added).
>>
>> [1]:
>> https://lore.kernel.org/linux-arm-msm/[email protected]/T/#u
>>
>> Depending on who lands first, this flag should be split.
>>
>> I still see value in inlining and removing these defines, though that
>> brings a host of other complexity.
>>
>> - Marijn
>>
>>> #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
>>> BIT(DPU_WB_UBWC) | \
>>> 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 4eda2cc847ef..01c65f940f2a 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>> @@ -185,6 +185,7 @@ enum {
>>> * @DPU_DATA_HCTL_EN Allows data to be transferred
>>> at different rate
>>> * than video timing
>>> * @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS
>>> register
>>> + * @DPU_INTF_DATA_COMPRESS INTF block has DATA_COMPRESS
>>> register
>>> * @DPU_INTF_MAX
>>> */
>>> enum {
>>> @@ -192,6 +193,7 @@ enum {
>>> DPU_INTF_TE,
>>> DPU_DATA_HCTL_EN,
>>> DPU_INTF_STATUS_SUPPORTED,
>>> + DPU_INTF_DATA_COMPRESS,
>>> DPU_INTF_MAX
>>> };
>>>
>>> --
>>> 2.40.1
>>>
--
With best wishes
Dmitry
On 5/8/2023 4:08 PM, Dmitry Baryshkov wrote:
> On 09/05/2023 00:46, Jessica Zhang wrote:
>>
>>
>> On 5/7/2023 9:00 AM, Marijn Suijten wrote:
>>> On 2023-05-05 14:23:50, Jessica Zhang wrote:
>>>> Add DATA_COMPRESS feature flag to DPU INTF block.
>>>>
>>>> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
>>>> PINGPONG to INTF.
>>>>
>>>> As core_rev (and related macros) was removed from the dpu_kms
>>>> struct, the
>>>> most straightforward way to indicate the presence of this register
>>>> would be
>>>> to have a feature flag.
>>>
>>> Irrelevant. Even though core_rev was still in mainline until recently,
>>> we always hardcoded the features in the catalog and only used core_rev
>>> to select a dpu_mdss_cfg catalog entry. There is no "if version >= X
>>> then enable feature Y" logic, this manually-enabled feature flag is the
>>> only, correct way to do it.
>>
>> Hi Marijn,
>>
>> Understood. FWIW, if we do find more register bit-level differences
>> between HW versions in the future, it might make more sense to keep
>> the HW catalog small and bring core_rev back, rather than keep adding
>> these kinds of small differences to caps.
>
> Let's see how it goes. Abhinav suggested that there might be feature
> differences inside the DPU generations (and even inside the single DPU
> major/minor combo). So I'm not sure what core_rev will bring us.
>
It allows us to have if MDSS_REV() checks which are convenient for some
calculations / bit programming which we dont want to expose in the
catalog as they cannot be classified as a hw cap as such or atleast we
dont want them to be classified as such.
> Let's land the platforms which are ready (or if there is anything close
> to be submitted). I'll post the next proposal for the catalog cleanups
> close to -rc4, when the dust settles then we can have one or two weaks
> for the discussion and polishing.
>
> I'd like to consider:
> - inlining foo_BLK macros, if that makes adding new features easier
> - reformat of clk_ctrls
> - maybe reintroduction of per-generation feature masks instead of
> keeping them named after the random SoC
> - maybe a rework of mdss_irqs / INTFn_INTR. We already have this info in
> hw catalog.
>
> Comments are appreciated.
>
I would say, lets wait for DSC to settle. Atleast the parts already on
the list. Continuous rebase of features already on the list is becoming
time consuming because of overlapping catalog reworks.
>
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>> Changes in v2:
>>>> - Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature
>>>> flag
>>>>
>>>> Signed-off-by: Jessica Zhang <[email protected]>
>>>
>>> Reviewed-by: Marijn Suijten <[email protected]>
>>>
>>>> ---
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> 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 7944481d0a33..c74051906d05 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> @@ -104,7 +104,7 @@
>>>> #define INTF_SC7180_MASK \
>>>> (BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE) |
>>>> BIT(DPU_INTF_STATUS_SUPPORTED))
>>>> -#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
>>>> +#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) |
>>>> BIT(DPU_INTF_DATA_COMPRESS)
>>>
>>> Konrad: Your SM6350/SM6375 series v3 [1] switched from INTF_SC7180_MASK
>>> to INTF_SC7280_MASK to enable HCTL on SM6375, but that will now
>>> erroneously also receive this feature flag and write the new
>>> DATA_COMPESS mask even if it's DPU 6.9 (< 7.x where it got added).
>>>
>>> [1]:
>>> https://lore.kernel.org/linux-arm-msm/[email protected]/T/#u
>>>
>>>
>>> Depending on who lands first, this flag should be split.
>>>
>>> I still see value in inlining and removing these defines, though that
>>> brings a host of other complexity.
>>>
>>> - Marijn
>>>
>>>> #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
>>>> BIT(DPU_WB_UBWC) | \
>>>> 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 4eda2cc847ef..01c65f940f2a 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>> @@ -185,6 +185,7 @@ enum {
>>>> * @DPU_DATA_HCTL_EN Allows data to be transferred
>>>> at different rate
>>>> * than video timing
>>>> * @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS
>>>> register
>>>> + * @DPU_INTF_DATA_COMPRESS INTF block has DATA_COMPRESS
>>>> register
>>>> * @DPU_INTF_MAX
>>>> */
>>>> enum {
>>>> @@ -192,6 +193,7 @@ enum {
>>>> DPU_INTF_TE,
>>>> DPU_DATA_HCTL_EN,
>>>> DPU_INTF_STATUS_SUPPORTED,
>>>> + DPU_INTF_DATA_COMPRESS,
>>>> DPU_INTF_MAX
>>>> };
>>>>
>>>> --
>>>> 2.40.1
>>>>
>
On 09/05/2023 03:28, Abhinav Kumar wrote:
>
>
> On 5/8/2023 4:08 PM, Dmitry Baryshkov wrote:
>> On 09/05/2023 00:46, Jessica Zhang wrote:
>>>
>>>
>>> On 5/7/2023 9:00 AM, Marijn Suijten wrote:
>>>> On 2023-05-05 14:23:50, Jessica Zhang wrote:
>>>>> Add DATA_COMPRESS feature flag to DPU INTF block.
>>>>>
>>>>> In DPU 7.x and later, DSC/DCE enablement registers have been moved
>>>>> from
>>>>> PINGPONG to INTF.
>>>>>
>>>>> As core_rev (and related macros) was removed from the dpu_kms
>>>>> struct, the
>>>>> most straightforward way to indicate the presence of this register
>>>>> would be
>>>>> to have a feature flag.
>>>>
>>>> Irrelevant. Even though core_rev was still in mainline until recently,
>>>> we always hardcoded the features in the catalog and only used core_rev
>>>> to select a dpu_mdss_cfg catalog entry. There is no "if version >= X
>>>> then enable feature Y" logic, this manually-enabled feature flag is the
>>>> only, correct way to do it.
>>>
>>> Hi Marijn,
>>>
>>> Understood. FWIW, if we do find more register bit-level differences
>>> between HW versions in the future, it might make more sense to keep
>>> the HW catalog small and bring core_rev back, rather than keep adding
>>> these kinds of small differences to caps.
>>
>> Let's see how it goes. Abhinav suggested that there might be feature
>> differences inside the DPU generations (and even inside the single DPU
>> major/minor combo). So I'm not sure what core_rev will bring us.
>>
>
> It allows us to have if MDSS_REV() checks which are convenient for some
> calculations / bit programming which we dont want to expose in the
> catalog as they cannot be classified as a hw cap as such or atleast we
> dont want them to be classified as such.
>
>> Let's land the platforms which are ready (or if there is anything
>> close to be submitted). I'll post the next proposal for the catalog
>> cleanups close to -rc4, when the dust settles then we can have one or
>> two weaks for the discussion and polishing.
>>
>> I'd like to consider:
>> - inlining foo_BLK macros, if that makes adding new features easier
>> - reformat of clk_ctrls
>> - maybe reintroduction of per-generation feature masks instead of
>> keeping them named after the random SoC
>> - maybe a rework of mdss_irqs / INTFn_INTR. We already have this info
>> in hw catalog.
>>
>> Comments are appreciated.
>>
>
> I would say, lets wait for DSC to settle. Atleast the parts already on
> the list. Continuous rebase of features already on the list is becoming
> time consuming because of overlapping catalog reworks.
As I wrote, -rc4. Until that time, I'd expect DSC to be settled and
accepted.
--
With best wishes
Dmitry
On 2023-05-09 02:08:52, Dmitry Baryshkov wrote:
> On 09/05/2023 00:46, Jessica Zhang wrote:
> >
> >
> > On 5/7/2023 9:00 AM, Marijn Suijten wrote:
> >> On 2023-05-05 14:23:50, Jessica Zhang wrote:
> >>> Add DATA_COMPRESS feature flag to DPU INTF block.
> >>>
> >>> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
> >>> PINGPONG to INTF.
> >>>
> >>> As core_rev (and related macros) was removed from the dpu_kms struct,
> >>> the
> >>> most straightforward way to indicate the presence of this register
> >>> would be
> >>> to have a feature flag.
> >>
> >> Irrelevant.? Even though core_rev was still in mainline until recently,
> >> we always hardcoded the features in the catalog and only used core_rev
> >> to select a dpu_mdss_cfg catalog entry.? There is no "if version >= X
> >> then enable feature Y" logic, this manually-enabled feature flag is the
> >> only, correct way to do it.
> >
> > Hi Marijn,
> >
> > Understood. FWIW, if we do find more register bit-level differences
> > between HW versions in the future, it might make more sense to keep the
> > HW catalog small and bring core_rev back, rather than keep adding these
> > kinds of small differences to caps.
>
> Let's see how it goes. Abhinav suggested that there might be feature
> differences inside the DPU generations (and even inside the single DPU
> major/minor combo). So I'm not sure what core_rev will bring us.
>
> Let's land the platforms which are ready (or if there is anything close
> to be submitted).
You mean waiting for catalog changes on the list specifically, so the
DSC series as well as SM6350/SM6375? I do intend to send SM6125 now
that the INTF TE series (required for it, as well as for SM63**) seems
to be generally accepted, but have been quite busy with the DSC series
on the list as we're now unblocking many Xperia's to finally have
display!
The catalog addition is "pretty much ready", let me know if you'd like
it to be sent in prior to your cleanup.
> I'll post the next proposal for the catalog cleanups
> close to -rc4, when the dust settles then we can have one or two weaks
> for the discussion and polishing.
>
> I'd like to consider:
> - inlining foo_BLK macros, if that makes adding new features easier
Sounds like a good idea.
> - reformat of clk_ctrls
> - maybe reintroduction of per-generation feature masks instead of
> keeping them named after the random SoC
Yes that would make things more clear. Or we can inline them entirely
though that might accidentally diverge SoCs and revisions.
> - maybe a rework of mdss_irqs / INTFn_INTR. We already have this info in
> hw catalog.
Sounds good as well, that should get rid of some "duplication".
- Marijn
On Tue, 9 May 2023 at 10:00, Marijn Suijten
<[email protected]> wrote:
>
> On 2023-05-09 02:08:52, Dmitry Baryshkov wrote:
> > On 09/05/2023 00:46, Jessica Zhang wrote:
> > >
> > >
> > > On 5/7/2023 9:00 AM, Marijn Suijten wrote:
> > >> On 2023-05-05 14:23:50, Jessica Zhang wrote:
> > >>> Add DATA_COMPRESS feature flag to DPU INTF block.
> > >>>
> > >>> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
> > >>> PINGPONG to INTF.
> > >>>
> > >>> As core_rev (and related macros) was removed from the dpu_kms struct,
> > >>> the
> > >>> most straightforward way to indicate the presence of this register
> > >>> would be
> > >>> to have a feature flag.
> > >>
> > >> Irrelevant. Even though core_rev was still in mainline until recently,
> > >> we always hardcoded the features in the catalog and only used core_rev
> > >> to select a dpu_mdss_cfg catalog entry. There is no "if version >= X
> > >> then enable feature Y" logic, this manually-enabled feature flag is the
> > >> only, correct way to do it.
> > >
> > > Hi Marijn,
> > >
> > > Understood. FWIW, if we do find more register bit-level differences
> > > between HW versions in the future, it might make more sense to keep the
> > > HW catalog small and bring core_rev back, rather than keep adding these
> > > kinds of small differences to caps.
> >
> > Let's see how it goes. Abhinav suggested that there might be feature
> > differences inside the DPU generations (and even inside the single DPU
> > major/minor combo). So I'm not sure what core_rev will bring us.
> >
> > Let's land the platforms which are ready (or if there is anything close
> > to be submitted).
>
> You mean waiting for catalog changes on the list specifically, so the
> DSC series as well as SM6350/SM6375? I do intend to send SM6125 now
> that the INTF TE series (required for it, as well as for SM63**) seems
> to be generally accepted, but have been quite busy with the DSC series
> on the list as we're now unblocking many Xperia's to finally have
> display!
Yes, please send it, as you find time. No time pressure.
>
> The catalog addition is "pretty much ready", let me know if you'd like
> it to be sent in prior to your cleanup.
--
With best wishes
Dmitry