In current code, the dsc active bits are set only if the cfg->dsc is set.
However, for displays which are hot-pluggable, there can be a use-case
of disconnecting a DSC supported sink and connecting a non-DSC sink.
For those cases we need to clear DSC active bits during teardown.
As discuss at [1], clear DSC active bit will handled at reset_intf_cfg()
Signed-off-by: Kuogee Hsieh <[email protected]>
Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl")
Reviewed-by: Abhinav Kumar <[email protected]>
Reviewed-by: Marijn Suijten <[email protected]>
[1] https://lore.kernel.org/linux-arm-msm/[email protected]/
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index bbdc95c..88e4efe 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
if (cfg->merge_3d)
DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
BIT(cfg->merge_3d - MERGE_3D_0));
- if (cfg->dsc) {
- DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
- DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
- }
+
+ DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
+ DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
}
static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Fri, 14 Apr 2023 at 19:46, Kuogee Hsieh <[email protected]> wrote:
>
> In current code, the dsc active bits are set only if the cfg->dsc is set.
> However, for displays which are hot-pluggable, there can be a use-case
> of disconnecting a DSC supported sink and connecting a non-DSC sink.
>
> For those cases we need to clear DSC active bits during teardown.
>
> As discuss at [1], clear DSC active bit will handled at reset_intf_cfg()
nit: discussed
>
> Signed-off-by: Kuogee Hsieh <[email protected]>
> Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl")
> Reviewed-by: Abhinav Kumar <[email protected]>
> Reviewed-by: Marijn Suijten <[email protected]>
>
> [1] https://lore.kernel.org/linux-arm-msm/[email protected]/
> ---
Changelog? This is v3 already, but it has no changes described.
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> index bbdc95c..88e4efe 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
> if (cfg->merge_3d)
> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
> BIT(cfg->merge_3d - MERGE_3D_0));
> - if (cfg->dsc) {
> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
> - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
> - }
> +
And the comment got dropped. Please restore it in some form.
> + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
> + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
> }
>
> static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
With best wishes
Dmitry
On 2023-04-14 09:46:17, Kuogee Hsieh wrote:
> In current code, the dsc active bits are set only if the cfg->dsc is set.
This is the old sentence from v1 again, did you accidentally send the
wrong patch as the improvements from v2 are missing?
> However, for displays which are hot-pluggable, there can be a use-case
> of disconnecting a DSC supported sink and connecting a non-DSC sink.
>
> For those cases we need to clear DSC active bits during teardown.
At least teardown is one word again, v2 had "tear down" which is wrong.
> As discuss at [1], clear DSC active bit will handled at reset_intf_cfg()
discussed* as pointed out by Dmitry, and make it clear that this is
about clearing CTL_DSC_ACTIVE (and CTL_DSC_FLUSH?) specifically. Once
that is moved to reset_intf_cfg(), this patch should be reverted as
there is no need to write the registers once again when cfg->dsc equals
0.
- Marijn
> Signed-off-by: Kuogee Hsieh <[email protected]>
> Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl")
> Reviewed-by: Abhinav Kumar <[email protected]>
> Reviewed-by: Marijn Suijten <[email protected]>
>
> [1] https://lore.kernel.org/linux-arm-msm/[email protected]/
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> index bbdc95c..88e4efe 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
> if (cfg->merge_3d)
> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
> BIT(cfg->merge_3d - MERGE_3D_0));
> - if (cfg->dsc) {
> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
> - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
> - }
> +
> + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
> + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
> }
>
> static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
On 15/04/2023 02:02, Marijn Suijten wrote:
> On 2023-04-14 09:46:17, Kuogee Hsieh wrote:
>> In current code, the dsc active bits are set only if the cfg->dsc is set.
>
> This is the old sentence from v1 again, did you accidentally send the
> wrong patch as the improvements from v2 are missing?
>
>> However, for displays which are hot-pluggable, there can be a use-case
>> of disconnecting a DSC supported sink and connecting a non-DSC sink.
>>
>> For those cases we need to clear DSC active bits during teardown.
>
> At least teardown is one word again, v2 had "tear down" which is wrong.
>
>> As discuss at [1], clear DSC active bit will handled at reset_intf_cfg()
>
> discussed* as pointed out by Dmitry, and make it clear that this is
> about clearing CTL_DSC_ACTIVE (and CTL_DSC_FLUSH?) specifically. Once
> that is moved to reset_intf_cfg(), this patch should be reverted as
> there is no need to write the registers once again when cfg->dsc equals
> 0.
Kuogee, can we please get a proper v4? With all the relevant changes
from v2, with the changelog, etc.
Otherwise the present Reviewed-by tags are just incorrect.
>
> - Marijn
>
>> Signed-off-by: Kuogee Hsieh <[email protected]>
>> Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl")
>> Reviewed-by: Abhinav Kumar <[email protected]>
>> Reviewed-by: Marijn Suijten <[email protected]>
>>
>> [1] https://lore.kernel.org/linux-arm-msm/[email protected]/
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> index bbdc95c..88e4efe 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
>> if (cfg->merge_3d)
>> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
>> BIT(cfg->merge_3d - MERGE_3D_0));
>> - if (cfg->dsc) {
>> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
>> - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
>> - }
>> +
>> + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
>> + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
>> }
>>
>> static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
--
With best wishes
Dmitry
Hi Dmitry / Marijn
On 4/20/2023 7:03 AM, Dmitry Baryshkov wrote:
> On 15/04/2023 02:02, Marijn Suijten wrote:
>> On 2023-04-14 09:46:17, Kuogee Hsieh wrote:
>>> In current code, the dsc active bits are set only if the cfg->dsc is
>>> set.
>>
>> This is the old sentence from v1 again, did you accidentally send the
>> wrong patch as the improvements from v2 are missing?
>>
>>> However, for displays which are hot-pluggable, there can be a use-case
>>> of disconnecting a DSC supported sink and connecting a non-DSC sink.
>>>
>>> For those cases we need to clear DSC active bits during teardown.
>>
>> At least teardown is one word again, v2 had "tear down" which is wrong.
>>
>>> As discuss at [1], clear DSC active bit will handled at reset_intf_cfg()
>>
>> discussed* as pointed out by Dmitry, and make it clear that this is
>> about clearing CTL_DSC_ACTIVE (and CTL_DSC_FLUSH?) specifically. Once
>> that is moved to reset_intf_cfg(), this patch should be reverted as
>> there is no need to write the registers once again when cfg->dsc equals
>> 0.
>
> Kuogee, can we please get a proper v4? With all the relevant changes
> from v2, with the changelog, etc.
>
> Otherwise the present Reviewed-by tags are just incorrect.
>
After looking into the DPU DSC changes internally which will be posted
today/tomm, that piece of code is again touching this block, so I am now
also not convinced this change should be made right now because it was
again touching flush programming, so that again leaves only the active
bits which as Marijn mentioned will be applicable for a use-case only
with hot-pluggable display which we dont have till we land DP DSC.
I think this is what we will do:
-> post DPU DSC changes
-> post the patch to improve reset_intf_cfg() to handle DSC
-> post rest of DP DSC changes
-> post rest of DSI DSC changes
I think that way, we all are aligned.
Apologies for posting this patch a bit ahead of time but if i had
foreseen that DPU DSC changes will again touch the flush code, i would
have held this patch back too.
>>
>> - Marijn
>>
>>> Signed-off-by: Kuogee Hsieh <[email protected]>
>>> Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl")
>>> Reviewed-by: Abhinav Kumar <[email protected]>
>>> Reviewed-by: Marijn Suijten <[email protected]>
>>>
>>> [1]
>>> https://lore.kernel.org/linux-arm-msm/[email protected]/
>>>
>>> ---
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++----
>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>>> index bbdc95c..88e4efe 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>>> @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct
>>> dpu_hw_ctl *ctx,
>>> if (cfg->merge_3d)
>>> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
>>> BIT(cfg->merge_3d - MERGE_3D_0));
>>> - if (cfg->dsc) {
>>> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
>>> - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
>>> - }
>>> +
>>> + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
>>> + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
>>> }
>>> static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>> Forum,
>>> a Linux Foundation Collaborative Project
>>>
>
On 20/04/2023 19:40, Abhinav Kumar wrote:
> Hi Dmitry / Marijn
>
> On 4/20/2023 7:03 AM, Dmitry Baryshkov wrote:
>> On 15/04/2023 02:02, Marijn Suijten wrote:
>>> On 2023-04-14 09:46:17, Kuogee Hsieh wrote:
>>>> In current code, the dsc active bits are set only if the cfg->dsc is
>>>> set.
>>>
>>> This is the old sentence from v1 again, did you accidentally send the
>>> wrong patch as the improvements from v2 are missing?
>>>
>>>> However, for displays which are hot-pluggable, there can be a use-case
>>>> of disconnecting a DSC supported sink and connecting a non-DSC sink.
>>>>
>>>> For those cases we need to clear DSC active bits during teardown.
>>>
>>> At least teardown is one word again, v2 had "tear down" which is wrong.
>>>
>>>> As discuss at [1], clear DSC active bit will handled at
>>>> reset_intf_cfg()
>>>
>>> discussed* as pointed out by Dmitry, and make it clear that this is
>>> about clearing CTL_DSC_ACTIVE (and CTL_DSC_FLUSH?) specifically. Once
>>> that is moved to reset_intf_cfg(), this patch should be reverted as
>>> there is no need to write the registers once again when cfg->dsc equals
>>> 0.
>>
>> Kuogee, can we please get a proper v4? With all the relevant changes
>> from v2, with the changelog, etc.
>>
>> Otherwise the present Reviewed-by tags are just incorrect.
>>
>
> After looking into the DPU DSC changes internally which will be posted
> today/tomm, that piece of code is again touching this block, so I am now
> also not convinced this change should be made right now because it was
> again touching flush programming, so that again leaves only the active
> bits which as Marijn mentioned will be applicable for a use-case only
> with hot-pluggable display which we dont have till we land DP DSC.
>
> I think this is what we will do:
>
> -> post DPU DSC changes
> -> post the patch to improve reset_intf_cfg() to handle DSC
> -> post rest of DP DSC changes
> -> post rest of DSI DSC changes
>
> I think that way, we all are aligned.
>
> Apologies for posting this patch a bit ahead of time but if i had
> foreseen that DPU DSC changes will again touch the flush code, i would
> have held this patch back too.
Ack, sounds good. Thank you.
>
>>>
>>> - Marijn
>>>
>>>> Signed-off-by: Kuogee Hsieh <[email protected]>
>>>> Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl")
>>>> Reviewed-by: Abhinav Kumar <[email protected]>
>>>> Reviewed-by: Marijn Suijten <[email protected]>
>>>>
>>>> [1]
>>>> https://lore.kernel.org/linux-arm-msm/[email protected]/
>>>> ---
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++----
>>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>>>> index bbdc95c..88e4efe 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>>>> @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct
>>>> dpu_hw_ctl *ctx,
>>>> if (cfg->merge_3d)
>>>> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
>>>> BIT(cfg->merge_3d - MERGE_3D_0));
>>>> - if (cfg->dsc) {
>>>> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
>>>> - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
>>>> - }
>>>> +
>>>> + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
>>>> + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
>>>> }
>>>> static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
>>>> --
>>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>>> Forum,
>>>> a Linux Foundation Collaborative Project
>>>>
>>
--
With best wishes
Dmitry