[PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Dmitry Baryshkov]

The downstream uses read-modify-write for updating command mode
compression registers. Let's follow this approach. This also fixes the
following warning:

drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]

Reported-by: kernel test robot <lkp@intel.com>
Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---

Changes since v1:
 - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
   (Abhinav)

---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index c983698d1384..a95d5df52653 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
 		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
 
+		reg_ctrl &= ~0xffff;
 		reg_ctrl |= reg;
+
+		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
 		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
 
-		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
+		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
 		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
 	} else {
 		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
-- 
2.35.1

[PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Dmitry Baryshkov]

The downstream uses read-modify-write for updating command mode
compression registers. Let's follow this approach. This also fixes the
following warning:

drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]

Reported-by: kernel test robot <lkp@intel.com>
Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---

Changes since v1:
 - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
   (Abhinav)

---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index c983698d1384..a95d5df52653 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
 		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
 
+		reg_ctrl &= ~0xffff;
 		reg_ctrl |= reg;
+
+		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
 		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
 
-		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
+		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
 		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
 	} else {
 		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
-- 
2.35.1

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Abhinav Kumar]

On 4/30/2022 10:55 AM, Dmitry Baryshkov wrote:
> The downstream uses read-modify-write for updating command mode
> compression registers. Let's follow this approach. This also fixes the
> following warning:
> 
> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> 
> Changes since v1:
>   - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
>     (Abhinav)
> 
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index c983698d1384..a95d5df52653 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>   		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
>   		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
>   
> +		reg_ctrl &= ~0xffff;
>   		reg_ctrl |= reg;
> +
> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
>   		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
>   
> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
>   		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
>   	} else {
>   		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Abhinav Kumar]

On 4/30/2022 10:55 AM, Dmitry Baryshkov wrote:
> The downstream uses read-modify-write for updating command mode
> compression registers. Let's follow this approach. This also fixes the
> following warning:
> 
> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> 
> Changes since v1:
>   - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
>     (Abhinav)
> 
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index c983698d1384..a95d5df52653 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>   		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
>   		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
>   
> +		reg_ctrl &= ~0xffff;
>   		reg_ctrl |= reg;
> +
> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
>   		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
>   
> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
>   		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
>   	} else {
>   		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Marijn Suijten]

On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
> The downstream uses read-modify-write for updating command mode
> compression registers. Let's follow this approach. This also fixes the
> following warning:
> 
> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

I pointed this out in review multiple times, so you'll obviously get my:

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

(But are you sure there's nothing else to clear in the 1st CTRL
register, only the lowest 16 bits?  That should mean `reg` never
contains anything in 0xffff0000)

However, this seems to indicate that the DSC patch series has been
approved and merged somehow??

> ---
> 
> Changes since v1:
>  - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
>    (Abhinav)
> 
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index c983698d1384..a95d5df52653 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>  		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
>  		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
>  
> +		reg_ctrl &= ~0xffff;
>  		reg_ctrl |= reg;
> +
> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
>  		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
>  
> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
>  		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
>  	} else {
>  		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> -- 
> 2.35.1
> 

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Marijn Suijten]

On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
> The downstream uses read-modify-write for updating command mode
> compression registers. Let's follow this approach. This also fixes the
> following warning:
> 
> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

I pointed this out in review multiple times, so you'll obviously get my:

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

(But are you sure there's nothing else to clear in the 1st CTRL
register, only the lowest 16 bits?  That should mean `reg` never
contains anything in 0xffff0000)

However, this seems to indicate that the DSC patch series has been
approved and merged somehow??

> ---
> 
> Changes since v1:
>  - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
>    (Abhinav)
> 
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index c983698d1384..a95d5df52653 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>  		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
>  		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
>  
> +		reg_ctrl &= ~0xffff;
>  		reg_ctrl |= reg;
> +
> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
>  		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
>  
> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
>  		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
>  	} else {
>  		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> -- 
> 2.35.1
> 

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Abhinav Kumar]

On 4/30/2022 11:58 AM, Marijn Suijten wrote:
> On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
>> The downstream uses read-modify-write for updating command mode
>> compression registers. Let's follow this approach. This also fixes the
>> following warning:
>>
>> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> I pointed this out in review multiple times, so you'll obviously get my:
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> (But are you sure there's nothing else to clear in the 1st CTRL
> register, only the lowest 16 bits?  That should mean `reg` never
> contains anything in 0xffff0000)

The top 16 bits contain information for stream 1.

Stream 1 is unused. And whatever is the reset value we should retain that.

So this patch is correct.

> 
> However, this seems to indicate that the DSC patch series has been
> approved and merged somehow??
> 
>> ---
>>
>> Changes since v1:
>>   - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
>>     (Abhinav)
>>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index c983698d1384..a95d5df52653 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>>   		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
>>   		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
>>   
>> +		reg_ctrl &= ~0xffff;
>>   		reg_ctrl |= reg;
>> +
>> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
>>   		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
>>   
>> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
>> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
>>   		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
>>   	} else {
>>   		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
>> -- 
>> 2.35.1
>>

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Abhinav Kumar]

On 4/30/2022 11:58 AM, Marijn Suijten wrote:
> On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
>> The downstream uses read-modify-write for updating command mode
>> compression registers. Let's follow this approach. This also fixes the
>> following warning:
>>
>> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> I pointed this out in review multiple times, so you'll obviously get my:
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> (But are you sure there's nothing else to clear in the 1st CTRL
> register, only the lowest 16 bits?  That should mean `reg` never
> contains anything in 0xffff0000)

The top 16 bits contain information for stream 1.

Stream 1 is unused. And whatever is the reset value we should retain that.

So this patch is correct.

> 
> However, this seems to indicate that the DSC patch series has been
> approved and merged somehow??
> 
>> ---
>>
>> Changes since v1:
>>   - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
>>     (Abhinav)
>>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index c983698d1384..a95d5df52653 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>>   		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
>>   		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
>>   
>> +		reg_ctrl &= ~0xffff;
>>   		reg_ctrl |= reg;
>> +
>> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
>>   		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
>>   
>> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
>> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
>>   		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
>>   	} else {
>>   		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
>> -- 
>> 2.35.1
>>

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Dmitry Baryshkov]

On 30/04/2022 21:58, Marijn Suijten wrote:
> On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
>> The downstream uses read-modify-write for updating command mode
>> compression registers. Let's follow this approach. This also fixes the
>> following warning:
>>
>> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> I pointed this out in review multiple times, so you'll obviously get my:

I think I might have also pointed this out once (and then forgot to 
check that the issue was fixed by Vinod).

> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> (But are you sure there's nothing else to clear in the 1st CTRL
> register, only the lowest 16 bits?  That should mean `reg` never
> contains anything in 0xffff0000)

Judging from the downstream the upper half conains the same fields, but 
used for other virtual channel. I didn't research what's the difference 
yet. All the dtsi files that I have here at hand use 
'qcom,mdss-dsi-virtual-channel-id = <0>;'

> 
> However, this seems to indicate that the DSC patch series has been
> approved and merged somehow??

Pending inclusion, yes. If Vinod missed or ignored any other review 
points, please excuse Abhinav and me not noticing that.

Can you please take a look at the latest revision posted, if there are 
any other missing points. Let's decide if there are grave issues or we 
can work them through.

> 
>> ---
>>
>> Changes since v1:
>>   - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
>>     (Abhinav)
>>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index c983698d1384..a95d5df52653 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>>   		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
>>   		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
>>   
>> +		reg_ctrl &= ~0xffff;
>>   		reg_ctrl |= reg;
>> +
>> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
>>   		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
>>   
>> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
>> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
>>   		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
>>   	} else {
>>   		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
>> -- 
>> 2.35.1
>>


-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Dmitry Baryshkov]

On 30/04/2022 21:58, Marijn Suijten wrote:
> On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
>> The downstream uses read-modify-write for updating command mode
>> compression registers. Let's follow this approach. This also fixes the
>> following warning:
>>
>> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> I pointed this out in review multiple times, so you'll obviously get my:

I think I might have also pointed this out once (and then forgot to 
check that the issue was fixed by Vinod).

> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> (But are you sure there's nothing else to clear in the 1st CTRL
> register, only the lowest 16 bits?  That should mean `reg` never
> contains anything in 0xffff0000)

Judging from the downstream the upper half conains the same fields, but 
used for other virtual channel. I didn't research what's the difference 
yet. All the dtsi files that I have here at hand use 
'qcom,mdss-dsi-virtual-channel-id = <0>;'

> 
> However, this seems to indicate that the DSC patch series has been
> approved and merged somehow??

Pending inclusion, yes. If Vinod missed or ignored any other review 
points, please excuse Abhinav and me not noticing that.

Can you please take a look at the latest revision posted, if there are 
any other missing points. Let's decide if there are grave issues or we 
can work them through.

> 
>> ---
>>
>> Changes since v1:
>>   - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
>>     (Abhinav)
>>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index c983698d1384..a95d5df52653 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>>   		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
>>   		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
>>   
>> +		reg_ctrl &= ~0xffff;
>>   		reg_ctrl |= reg;
>> +
>> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
>>   		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
>>   
>> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
>> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
>>   		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
>>   	} else {
>>   		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
>> -- 
>> 2.35.1
>>


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Marijn Suijten]

On 2022-04-30 12:25:57, Abhinav Kumar wrote:
> 
> 
> On 4/30/2022 11:58 AM, Marijn Suijten wrote:
> > On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
> >> The downstream uses read-modify-write for updating command mode
> >> compression registers. Let's follow this approach. This also fixes the
> >> following warning:
> >>
> >> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
> >>
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > I pointed this out in review multiple times, so you'll obviously get my:
> > 
> > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > 
> > (But are you sure there's nothing else to clear in the 1st CTRL
> > register, only the lowest 16 bits?  That should mean `reg` never
> > contains anything in 0xffff0000)
> 
> The top 16 bits contain information for stream 1.
> 
> Stream 1 is unused. And whatever is the reset value we should retain that.
> 
> So this patch is correct.

I was not debating the correctness of this patch, quite the contrary:
it's already much better than it was before.

I'm simply asking whether we should prevent `reg` (not `reg_ctrl`!)
from having anything in the top 16 bits, which would overwrite the reset
value for stream 1 which you correctly say should be retained as it is.
It seems unlikely that this happens, but better be safe than sorry?

Looking at the DSI register definition for DSC [1] again, I wonder if
there's support for defining a common bitfield layout and reusing it
thrice: the two channels for CMD mode and a single use for VIDEO.  Don't
think that'd make the kernel code better though if even possible at all.

[1]: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14967/diffs

- Marijn

> > 
> > However, this seems to indicate that the DSC patch series has been
> > approved and merged somehow??
> > 
> >> ---
> >>
> >> Changes since v1:
> >>   - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
> >>     (Abhinav)
> >>
> >> ---
> >>   drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> index c983698d1384..a95d5df52653 100644
> >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
> >>   		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> >>   		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
> >>   
> >> +		reg_ctrl &= ~0xffff;
> >>   		reg_ctrl |= reg;
> >> +
> >> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
> >>   		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
> >>   
> >> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> >> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
> >>   		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
> >>   	} else {
> >>   		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> >> -- 
> >> 2.35.1
> >>

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Marijn Suijten]

On 2022-04-30 12:25:57, Abhinav Kumar wrote:
> 
> 
> On 4/30/2022 11:58 AM, Marijn Suijten wrote:
> > On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
> >> The downstream uses read-modify-write for updating command mode
> >> compression registers. Let's follow this approach. This also fixes the
> >> following warning:
> >>
> >> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
> >>
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > I pointed this out in review multiple times, so you'll obviously get my:
> > 
> > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > 
> > (But are you sure there's nothing else to clear in the 1st CTRL
> > register, only the lowest 16 bits?  That should mean `reg` never
> > contains anything in 0xffff0000)
> 
> The top 16 bits contain information for stream 1.
> 
> Stream 1 is unused. And whatever is the reset value we should retain that.
> 
> So this patch is correct.

I was not debating the correctness of this patch, quite the contrary:
it's already much better than it was before.

I'm simply asking whether we should prevent `reg` (not `reg_ctrl`!)
from having anything in the top 16 bits, which would overwrite the reset
value for stream 1 which you correctly say should be retained as it is.
It seems unlikely that this happens, but better be safe than sorry?

Looking at the DSI register definition for DSC [1] again, I wonder if
there's support for defining a common bitfield layout and reusing it
thrice: the two channels for CMD mode and a single use for VIDEO.  Don't
think that'd make the kernel code better though if even possible at all.

[1]: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14967/diffs

- Marijn

> > 
> > However, this seems to indicate that the DSC patch series has been
> > approved and merged somehow??
> > 
> >> ---
> >>
> >> Changes since v1:
> >>   - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
> >>     (Abhinav)
> >>
> >> ---
> >>   drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> index c983698d1384..a95d5df52653 100644
> >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
> >>   		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> >>   		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
> >>   
> >> +		reg_ctrl &= ~0xffff;
> >>   		reg_ctrl |= reg;
> >> +
> >> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
> >>   		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
> >>   
> >> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> >> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
> >>   		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
> >>   	} else {
> >>   		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> >> -- 
> >> 2.35.1
> >>

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Marijn Suijten]

On 2022-04-30 22:28:42, Dmitry Baryshkov wrote:
> On 30/04/2022 21:58, Marijn Suijten wrote:
> > On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
> >> The downstream uses read-modify-write for updating command mode
> >> compression registers. Let's follow this approach. This also fixes the
> >> following warning:
> >>
> >> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
> >>
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > I pointed this out in review multiple times, so you'll obviously get my:
> 
> I think I might have also pointed this out once (and then forgot to 
> check that the issue was fixed by Vinod).
> 
> > 
> > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > 
> > (But are you sure there's nothing else to clear in the 1st CTRL
> > register, only the lowest 16 bits?  That should mean `reg` never
> > contains anything in 0xffff0000)
> 
> Judging from the downstream the upper half conains the same fields, but 
> used for other virtual channel. I didn't research what's the difference 
> yet. All the dtsi files that I have here at hand use 
> 'qcom,mdss-dsi-virtual-channel-id = <0>;'

As replied to Abhinav I'm simply asking whether we should be strict
and add `reg & 0xffff` to prevent accidentally writing the top 16 bits,
which are stream 1.  It doesn't seem like the current code can hit that
though, with all the macros using masks internally already; but it's
still a little scary since we're assuming the registers for VIDEO are
identical to CMD (as mentioned in the reply to Abhinav: I wonder if it's
possible to declare a a pair of bitfields as a single layout in the XML,
and reuse that across CMD's stream 0/1 and the VIDEO register).

> > However, this seems to indicate that the DSC patch series has been
> > approved and merged somehow??
> 
> Pending inclusion, yes. If Vinod missed or ignored any other review 
> points, please excuse Abhinav and me not noticing that.

Vinod replied to most of the comments so I'll double-check if they were
applied in the way requested.  Note that I don't always post a full
review up-front if it gets too noisy: I'll simply start out with the
most glaring issues and go in more detail in further revisions to
prevent drowning everyone in comments.

> Can you please take a look at the latest revision posted, if there are 
> any other missing points. Let's decide if there are grave issues or we 
> can work them through.

Thanks, I'll queue that up this week.  One of my thus-far-unaddressed
issues with the patches which can't be addressed in hindsight is the
relatively lackluster commit messages: most happen to be repeating the
title in a slightly different way without any additional clarification,
which doesn't fit the upstream spirit at all.
I understand that the reference manuals can't be quoted nor am I asking
to, but a little more insight in the process and details of each patch
goes a very long way.  Explain how certain calculations work or came to
be, link to public sources detailing the protocol, explain design
decisions or document how to use/test the feature and describe possible
limitations.
I usually link contributors to [1], but it's a bit of an odd read at
times.

[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

In any case, given that you've already sent this patch and another three
patches [2] fixing/cleaning up the series tells me it's far from ready.
Most of this should just be handled - or have been handled - in review
and amended?

[2]: https://lore.kernel.org/linux-arm-msm/20220501151220.3999164-1-dmitry.baryshkov@linaro.org/T/#t

I'll look through v14 again this week and let you know.

- Marijn

> > 
> >> ---
> >>
> >> Changes since v1:
> >>   - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
> >>     (Abhinav)
> >>
> >> ---
> >>   drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> index c983698d1384..a95d5df52653 100644
> >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
> >>   		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> >>   		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
> >>   
> >> +		reg_ctrl &= ~0xffff;
> >>   		reg_ctrl |= reg;
> >> +
> >> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
> >>   		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
> >>   
> >> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> >> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
> >>   		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
> >>   	} else {
> >>   		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> >> -- 
> >> 2.35.1
> >>
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Marijn Suijten]

On 2022-04-30 22:28:42, Dmitry Baryshkov wrote:
> On 30/04/2022 21:58, Marijn Suijten wrote:
> > On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
> >> The downstream uses read-modify-write for updating command mode
> >> compression registers. Let's follow this approach. This also fixes the
> >> following warning:
> >>
> >> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
> >>
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > I pointed this out in review multiple times, so you'll obviously get my:
> 
> I think I might have also pointed this out once (and then forgot to 
> check that the issue was fixed by Vinod).
> 
> > 
> > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > 
> > (But are you sure there's nothing else to clear in the 1st CTRL
> > register, only the lowest 16 bits?  That should mean `reg` never
> > contains anything in 0xffff0000)
> 
> Judging from the downstream the upper half conains the same fields, but 
> used for other virtual channel. I didn't research what's the difference 
> yet. All the dtsi files that I have here at hand use 
> 'qcom,mdss-dsi-virtual-channel-id = <0>;'

As replied to Abhinav I'm simply asking whether we should be strict
and add `reg & 0xffff` to prevent accidentally writing the top 16 bits,
which are stream 1.  It doesn't seem like the current code can hit that
though, with all the macros using masks internally already; but it's
still a little scary since we're assuming the registers for VIDEO are
identical to CMD (as mentioned in the reply to Abhinav: I wonder if it's
possible to declare a a pair of bitfields as a single layout in the XML,
and reuse that across CMD's stream 0/1 and the VIDEO register).

> > However, this seems to indicate that the DSC patch series has been
> > approved and merged somehow??
> 
> Pending inclusion, yes. If Vinod missed or ignored any other review 
> points, please excuse Abhinav and me not noticing that.

Vinod replied to most of the comments so I'll double-check if they were
applied in the way requested.  Note that I don't always post a full
review up-front if it gets too noisy: I'll simply start out with the
most glaring issues and go in more detail in further revisions to
prevent drowning everyone in comments.

> Can you please take a look at the latest revision posted, if there are 
> any other missing points. Let's decide if there are grave issues or we 
> can work them through.

Thanks, I'll queue that up this week.  One of my thus-far-unaddressed
issues with the patches which can't be addressed in hindsight is the
relatively lackluster commit messages: most happen to be repeating the
title in a slightly different way without any additional clarification,
which doesn't fit the upstream spirit at all.
I understand that the reference manuals can't be quoted nor am I asking
to, but a little more insight in the process and details of each patch
goes a very long way.  Explain how certain calculations work or came to
be, link to public sources detailing the protocol, explain design
decisions or document how to use/test the feature and describe possible
limitations.
I usually link contributors to [1], but it's a bit of an odd read at
times.

[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

In any case, given that you've already sent this patch and another three
patches [2] fixing/cleaning up the series tells me it's far from ready.
Most of this should just be handled - or have been handled - in review
and amended?

[2]: https://lore.kernel.org/linux-arm-msm/20220501151220.3999164-1-dmitry.baryshkov@linaro.org/T/#t

I'll look through v14 again this week and let you know.

- Marijn

> > 
> >> ---
> >>
> >> Changes since v1:
> >>   - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
> >>     (Abhinav)
> >>
> >> ---
> >>   drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> index c983698d1384..a95d5df52653 100644
> >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
> >>   		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> >>   		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
> >>   
> >> +		reg_ctrl &= ~0xffff;
> >>   		reg_ctrl |= reg;
> >> +
> >> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
> >>   		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
> >>   
> >> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> >> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
> >>   		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
> >>   	} else {
> >>   		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> >> -- 
> >> 2.35.1
> >>
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Dmitry Baryshkov]

On 01/05/2022 23:41, Marijn Suijten wrote:
> On 2022-04-30 22:28:42, Dmitry Baryshkov wrote:
>> On 30/04/2022 21:58, Marijn Suijten wrote:
>>> On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
>>>> The downstream uses read-modify-write for updating command mode
>>>> compression registers. Let's follow this approach. This also fixes the
>>>> following warning:
>>>>
>>>> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
>>>>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>
>>> I pointed this out in review multiple times, so you'll obviously get my:
>>
>> I think I might have also pointed this out once (and then forgot to
>> check that the issue was fixed by Vinod).
>>
>>>
>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>
>>> (But are you sure there's nothing else to clear in the 1st CTRL
>>> register, only the lowest 16 bits?  That should mean `reg` never
>>> contains anything in 0xffff0000)
>>
>> Judging from the downstream the upper half conains the same fields, but
>> used for other virtual channel. I didn't research what's the difference
>> yet. All the dtsi files that I have here at hand use
>> 'qcom,mdss-dsi-virtual-channel-id = <0>;'
> 
> As replied to Abhinav I'm simply asking whether we should be strict
> and add `reg & 0xffff` to prevent accidentally writing the top 16 bits,
> which are stream 1.  It doesn't seem like the current code can hit that
> though, with all the macros using masks internally already; but it's
> still a little scary since we're assuming the registers for VIDEO are
> identical to CMD (as mentioned in the reply to Abhinav: I wonder if it's
> possible to declare a a pair of bitfields as a single layout in the XML,
> and reuse that across CMD's stream 0/1 and the VIDEO register).
> 
>>> However, this seems to indicate that the DSC patch series has been
>>> approved and merged somehow??
>>
>> Pending inclusion, yes. If Vinod missed or ignored any other review
>> points, please excuse Abhinav and me not noticing that.
> 
> Vinod replied to most of the comments so I'll double-check if they were
> applied in the way requested.  Note that I don't always post a full
> review up-front if it gets too noisy: I'll simply start out with the
> most glaring issues and go in more detail in further revisions to
> prevent drowning everyone in comments.
> 
>> Can you please take a look at the latest revision posted, if there are
>> any other missing points. Let's decide if there are grave issues or we
>> can work them through.
> 
> Thanks, I'll queue that up this week.  One of my thus-far-unaddressed
> issues with the patches which can't be addressed in hindsight is the
> relatively lackluster commit messages: most happen to be repeating the
> title in a slightly different way without any additional clarification,
> which doesn't fit the upstream spirit at all.
> I understand that the reference manuals can't be quoted nor am I asking
> to, but a little more insight in the process and details of each patch
> goes a very long way.  Explain how certain calculations work or came to
> be, link to public sources detailing the protocol, explain design
> decisions or document how to use/test the feature and describe possible
> limitations.
> I usually link contributors to [1], but it's a bit of an odd read at
> times.
> 
> [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> 
> In any case, given that you've already sent this patch and another three
> patches [2] fixing/cleaning up the series tells me it's far from ready.
> Most of this should just be handled - or have been handled - in review
> and amended?

During the review time we agreed that [2] would come as a separate 
change It is an API change that would make using panel-bridge easier, 
but isn't otherwise required.

I have been working towards more logical drm_bridge/drm_bridge_connector 
chains employing panel-bridge and display-connector where required, [2] 
is a part of that effort (as well as few other patches that hit 
dri-devel in the last few days).

> 
> [2]: https://lore.kernel.org/linux-arm-msm/20220501151220.3999164-1-dmitry.baryshkov@linaro.org/T/#t
> 
> I'll look through v14 again this week and let you know.
> 
> - Marijn
> 
>>>
>>>> ---
>>>>
>>>> Changes since v1:
>>>>    - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
>>>>      (Abhinav)
>>>>
>>>> ---
>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> index c983698d1384..a95d5df52653 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>>>>    		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
>>>>    		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
>>>>    
>>>> +		reg_ctrl &= ~0xffff;
>>>>    		reg_ctrl |= reg;
>>>> +
>>>> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
>>>>    		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
>>>>    
>>>> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
>>>> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
>>>>    		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
>>>>    	} else {
>>>>    		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
>>>> -- 
>>>> 2.35.1
>>>>
>>
>>
>> -- 
>> With best wishes
>> Dmitry


-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Dmitry Baryshkov]

On 01/05/2022 23:41, Marijn Suijten wrote:
> On 2022-04-30 22:28:42, Dmitry Baryshkov wrote:
>> On 30/04/2022 21:58, Marijn Suijten wrote:
>>> On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
>>>> The downstream uses read-modify-write for updating command mode
>>>> compression registers. Let's follow this approach. This also fixes the
>>>> following warning:
>>>>
>>>> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
>>>>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>
>>> I pointed this out in review multiple times, so you'll obviously get my:
>>
>> I think I might have also pointed this out once (and then forgot to
>> check that the issue was fixed by Vinod).
>>
>>>
>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>
>>> (But are you sure there's nothing else to clear in the 1st CTRL
>>> register, only the lowest 16 bits?  That should mean `reg` never
>>> contains anything in 0xffff0000)
>>
>> Judging from the downstream the upper half conains the same fields, but
>> used for other virtual channel. I didn't research what's the difference
>> yet. All the dtsi files that I have here at hand use
>> 'qcom,mdss-dsi-virtual-channel-id = <0>;'
> 
> As replied to Abhinav I'm simply asking whether we should be strict
> and add `reg & 0xffff` to prevent accidentally writing the top 16 bits,
> which are stream 1.  It doesn't seem like the current code can hit that
> though, with all the macros using masks internally already; but it's
> still a little scary since we're assuming the registers for VIDEO are
> identical to CMD (as mentioned in the reply to Abhinav: I wonder if it's
> possible to declare a a pair of bitfields as a single layout in the XML,
> and reuse that across CMD's stream 0/1 and the VIDEO register).
> 
>>> However, this seems to indicate that the DSC patch series has been
>>> approved and merged somehow??
>>
>> Pending inclusion, yes. If Vinod missed or ignored any other review
>> points, please excuse Abhinav and me not noticing that.
> 
> Vinod replied to most of the comments so I'll double-check if they were
> applied in the way requested.  Note that I don't always post a full
> review up-front if it gets too noisy: I'll simply start out with the
> most glaring issues and go in more detail in further revisions to
> prevent drowning everyone in comments.
> 
>> Can you please take a look at the latest revision posted, if there are
>> any other missing points. Let's decide if there are grave issues or we
>> can work them through.
> 
> Thanks, I'll queue that up this week.  One of my thus-far-unaddressed
> issues with the patches which can't be addressed in hindsight is the
> relatively lackluster commit messages: most happen to be repeating the
> title in a slightly different way without any additional clarification,
> which doesn't fit the upstream spirit at all.
> I understand that the reference manuals can't be quoted nor am I asking
> to, but a little more insight in the process and details of each patch
> goes a very long way.  Explain how certain calculations work or came to
> be, link to public sources detailing the protocol, explain design
> decisions or document how to use/test the feature and describe possible
> limitations.
> I usually link contributors to [1], but it's a bit of an odd read at
> times.
> 
> [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> 
> In any case, given that you've already sent this patch and another three
> patches [2] fixing/cleaning up the series tells me it's far from ready.
> Most of this should just be handled - or have been handled - in review
> and amended?

During the review time we agreed that [2] would come as a separate 
change It is an API change that would make using panel-bridge easier, 
but isn't otherwise required.

I have been working towards more logical drm_bridge/drm_bridge_connector 
chains employing panel-bridge and display-connector where required, [2] 
is a part of that effort (as well as few other patches that hit 
dri-devel in the last few days).

> 
> [2]: https://lore.kernel.org/linux-arm-msm/20220501151220.3999164-1-dmitry.baryshkov@linaro.org/T/#t
> 
> I'll look through v14 again this week and let you know.
> 
> - Marijn
> 
>>>
>>>> ---
>>>>
>>>> Changes since v1:
>>>>    - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
>>>>      (Abhinav)
>>>>
>>>> ---
>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> index c983698d1384..a95d5df52653 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>>>>    		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
>>>>    		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
>>>>    
>>>> +		reg_ctrl &= ~0xffff;
>>>>    		reg_ctrl |= reg;
>>>> +
>>>> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
>>>>    		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
>>>>    
>>>> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
>>>> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
>>>>    		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
>>>>    	} else {
>>>>    		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
>>>> -- 
>>>> 2.35.1
>>>>
>>
>>
>> -- 
>> With best wishes
>> Dmitry


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Abhinav Kumar]

On 5/1/2022 1:06 PM, Marijn Suijten wrote:
> On 2022-04-30 12:25:57, Abhinav Kumar wrote:
>>
>>
>> On 4/30/2022 11:58 AM, Marijn Suijten wrote:
>>> On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
>>>> The downstream uses read-modify-write for updating command mode
>>>> compression registers. Let's follow this approach. This also fixes the
>>>> following warning:
>>>>
>>>> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
>>>>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>
>>> I pointed this out in review multiple times, so you'll obviously get my:
>>>
>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>
>>> (But are you sure there's nothing else to clear in the 1st CTRL
>>> register, only the lowest 16 bits?  That should mean `reg` never
>>> contains anything in 0xffff0000)
>>
>> The top 16 bits contain information for stream 1.
>>
>> Stream 1 is unused. And whatever is the reset value we should retain that.
>>
>> So this patch is correct.
> 
> I was not debating the correctness of this patch, quite the contrary:
> it's already much better than it was before.
> 
> I'm simply asking whether we should prevent `reg` (not `reg_ctrl`!)
> from having anything in the top 16 bits, which would overwrite the reset
> value for stream 1 which you correctly say should be retained as it is.
> It seems unlikely that this happens, but better be safe than sorry?

Wouln't this macro already make sure that 'reg' doesnt have anything in 
the top 16 bits? Its doing a & with 0x00003f00

#define DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK 
0x00003f00
#define DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT   8
static inline uint32_t 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(uint32_t val)
{
     return ((val) << 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT) & 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
}

Did you have anything else in mind? If so, please suggest.

> 
> Looking at the DSI register definition for DSC [1] again, I wonder if
> there's support for defining a common bitfield layout and reusing it
> thrice: the two channels for CMD mode and a single use for VIDEO.  Don't
> think that'd make the kernel code better though if even possible at all.
> 

We can have a common bitfield layout for the two channels for command mode.

So we can do something like below for common fields:

-static inline uint32_t 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(uint32_t val)
+static inline uint32_t 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM_DATATYPE(uint32_t val, uint32_t 
stream_id)
  {
-       return ((val) << 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT) & 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
+       return ((val) << 
(DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT + (stream_id 
* 16)) & DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
  }

Video mode can also use all of these except for WC as that field is not 
present for cmd modes.

This can go as a separate change .

I can push this and perhaps get a Tested-by from Vinod as I dont have a 
setup to re-validate this.

Thanks

Abhinav
> [1]: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14967/diffs
> 
> - Marijn
> 
>>>
>>> However, this seems to indicate that the DSC patch series has been
>>> approved and merged somehow??
>>>
>>>> ---
>>>>
>>>> Changes since v1:
>>>>    - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
>>>>      (Abhinav)
>>>>
>>>> ---
>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> index c983698d1384..a95d5df52653 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>>>>    		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
>>>>    		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
>>>>    
>>>> +		reg_ctrl &= ~0xffff;
>>>>    		reg_ctrl |= reg;
>>>> +
>>>> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
>>>>    		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
>>>>    
>>>> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
>>>> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
>>>>    		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
>>>>    	} else {
>>>>    		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
>>>> -- 
>>>> 2.35.1
>>>>

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Abhinav Kumar]

On 5/1/2022 1:06 PM, Marijn Suijten wrote:
> On 2022-04-30 12:25:57, Abhinav Kumar wrote:
>>
>>
>> On 4/30/2022 11:58 AM, Marijn Suijten wrote:
>>> On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
>>>> The downstream uses read-modify-write for updating command mode
>>>> compression registers. Let's follow this approach. This also fixes the
>>>> following warning:
>>>>
>>>> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
>>>>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>
>>> I pointed this out in review multiple times, so you'll obviously get my:
>>>
>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>
>>> (But are you sure there's nothing else to clear in the 1st CTRL
>>> register, only the lowest 16 bits?  That should mean `reg` never
>>> contains anything in 0xffff0000)
>>
>> The top 16 bits contain information for stream 1.
>>
>> Stream 1 is unused. And whatever is the reset value we should retain that.
>>
>> So this patch is correct.
> 
> I was not debating the correctness of this patch, quite the contrary:
> it's already much better than it was before.
> 
> I'm simply asking whether we should prevent `reg` (not `reg_ctrl`!)
> from having anything in the top 16 bits, which would overwrite the reset
> value for stream 1 which you correctly say should be retained as it is.
> It seems unlikely that this happens, but better be safe than sorry?

Wouln't this macro already make sure that 'reg' doesnt have anything in 
the top 16 bits? Its doing a & with 0x00003f00

#define DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK 
0x00003f00
#define DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT   8
static inline uint32_t 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(uint32_t val)
{
     return ((val) << 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT) & 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
}

Did you have anything else in mind? If so, please suggest.

> 
> Looking at the DSI register definition for DSC [1] again, I wonder if
> there's support for defining a common bitfield layout and reusing it
> thrice: the two channels for CMD mode and a single use for VIDEO.  Don't
> think that'd make the kernel code better though if even possible at all.
> 

We can have a common bitfield layout for the two channels for command mode.

So we can do something like below for common fields:

-static inline uint32_t 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(uint32_t val)
+static inline uint32_t 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM_DATATYPE(uint32_t val, uint32_t 
stream_id)
  {
-       return ((val) << 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT) & 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
+       return ((val) << 
(DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT + (stream_id 
* 16)) & DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
  }

Video mode can also use all of these except for WC as that field is not 
present for cmd modes.

This can go as a separate change .

I can push this and perhaps get a Tested-by from Vinod as I dont have a 
setup to re-validate this.

Thanks

Abhinav
> [1]: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14967/diffs
> 
> - Marijn
> 
>>>
>>> However, this seems to indicate that the DSC patch series has been
>>> approved and merged somehow??
>>>
>>>> ---
>>>>
>>>> Changes since v1:
>>>>    - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
>>>>      (Abhinav)
>>>>
>>>> ---
>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> index c983698d1384..a95d5df52653 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>>>>    		reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
>>>>    		reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
>>>>    
>>>> +		reg_ctrl &= ~0xffff;
>>>>    		reg_ctrl |= reg;
>>>> +
>>>> +		reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
>>>>    		reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
>>>>    
>>>> -		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
>>>> +		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
>>>>    		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
>>>>    	} else {
>>>>    		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
>>>> -- 
>>>> 2.35.1
>>>>

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Dmitry Baryshkov]

On Mon, 2 May 2022 at 02:56, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 5/1/2022 1:06 PM, Marijn Suijten wrote:
> > On 2022-04-30 12:25:57, Abhinav Kumar wrote:
> >>
> >>
> >> On 4/30/2022 11:58 AM, Marijn Suijten wrote:
> >>> On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
> >>>> The downstream uses read-modify-write for updating command mode
> >>>> compression registers. Let's follow this approach. This also fixes the
> >>>> following warning:
> >>>>
> >>>> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
> >>>>
> >>>> Reported-by: kernel test robot <lkp@intel.com>
> >>>> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>
> >>> I pointed this out in review multiple times, so you'll obviously get my:
> >>>
> >>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> >>>
> >>> (But are you sure there's nothing else to clear in the 1st CTRL
> >>> register, only the lowest 16 bits?  That should mean `reg` never
> >>> contains anything in 0xffff0000)
> >>
> >> The top 16 bits contain information for stream 1.
> >>
> >> Stream 1 is unused. And whatever is the reset value we should retain that.
> >>
> >> So this patch is correct.
> >
> > I was not debating the correctness of this patch, quite the contrary:
> > it's already much better than it was before.
> >
> > I'm simply asking whether we should prevent `reg` (not `reg_ctrl`!)
> > from having anything in the top 16 bits, which would overwrite the reset
> > value for stream 1 which you correctly say should be retained as it is.
> > It seems unlikely that this happens, but better be safe than sorry?
>
> Wouln't this macro already make sure that 'reg' doesnt have anything in
> the top 16 bits? Its doing a & with 0x00003f00
>
> #define DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK
> 0x00003f00
> #define DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT   8
> static inline uint32_t
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(uint32_t val)
> {
>      return ((val) <<
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT) &
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
> }
>
> Did you have anything else in mind? If so, please suggest.
>
> >
> > Looking at the DSI register definition for DSC [1] again, I wonder if
> > there's support for defining a common bitfield layout and reusing it
> > thrice: the two channels for CMD mode and a single use for VIDEO.  Don't
> > think that'd make the kernel code better though if even possible at all.
> >
>
> We can have a common bitfield layout for the two channels for command mode.
>
> So we can do something like below for common fields:
>
> -static inline uint32_t
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(uint32_t val)
> +static inline uint32_t
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM_DATATYPE(uint32_t val, uint32_t
> stream_id)
>   {
> -       return ((val) <<
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT) &
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
> +       return ((val) <<
> (DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT + (stream_id
> * 16)) & DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
>   }

NAK. Please express this in the xml source. This file is autogenerated.

>
> Video mode can also use all of these except for WC as that field is not
> present for cmd modes.
>
> This can go as a separate change .
>
> I can push this and perhaps get a Tested-by from Vinod as I dont have a
> setup to re-validate this.
>
> Thanks
>
> Abhinav
> > [1]: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14967/diffs
> >
> > - Marijn
> >
> >>>
> >>> However, this seems to indicate that the DSC patch series has been
> >>> approved and merged somehow??
> >>>
> >>>> ---
> >>>>
> >>>> Changes since v1:
> >>>>    - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
> >>>>      (Abhinav)
> >>>>
> >>>> ---
> >>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
> >>>>    1 file changed, 4 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >>>> index c983698d1384..a95d5df52653 100644
> >>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >>>> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
> >>>>                    reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> >>>>                    reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
> >>>>
> >>>> +          reg_ctrl &= ~0xffff;
> >>>>                    reg_ctrl |= reg;
> >>>> +
> >>>> +          reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
> >>>>                    reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
> >>>>
> >>>> -          dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> >>>> +          dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
> >>>>                    dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
> >>>>            } else {
> >>>>                    dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> >>>> --
> >>>> 2.35.1
> >>>>



-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Dmitry Baryshkov]

On Mon, 2 May 2022 at 02:56, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 5/1/2022 1:06 PM, Marijn Suijten wrote:
> > On 2022-04-30 12:25:57, Abhinav Kumar wrote:
> >>
> >>
> >> On 4/30/2022 11:58 AM, Marijn Suijten wrote:
> >>> On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
> >>>> The downstream uses read-modify-write for updating command mode
> >>>> compression registers. Let's follow this approach. This also fixes the
> >>>> following warning:
> >>>>
> >>>> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
> >>>>
> >>>> Reported-by: kernel test robot <lkp@intel.com>
> >>>> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>
> >>> I pointed this out in review multiple times, so you'll obviously get my:
> >>>
> >>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> >>>
> >>> (But are you sure there's nothing else to clear in the 1st CTRL
> >>> register, only the lowest 16 bits?  That should mean `reg` never
> >>> contains anything in 0xffff0000)
> >>
> >> The top 16 bits contain information for stream 1.
> >>
> >> Stream 1 is unused. And whatever is the reset value we should retain that.
> >>
> >> So this patch is correct.
> >
> > I was not debating the correctness of this patch, quite the contrary:
> > it's already much better than it was before.
> >
> > I'm simply asking whether we should prevent `reg` (not `reg_ctrl`!)
> > from having anything in the top 16 bits, which would overwrite the reset
> > value for stream 1 which you correctly say should be retained as it is.
> > It seems unlikely that this happens, but better be safe than sorry?
>
> Wouln't this macro already make sure that 'reg' doesnt have anything in
> the top 16 bits? Its doing a & with 0x00003f00
>
> #define DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK
> 0x00003f00
> #define DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT   8
> static inline uint32_t
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(uint32_t val)
> {
>      return ((val) <<
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT) &
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
> }
>
> Did you have anything else in mind? If so, please suggest.
>
> >
> > Looking at the DSI register definition for DSC [1] again, I wonder if
> > there's support for defining a common bitfield layout and reusing it
> > thrice: the two channels for CMD mode and a single use for VIDEO.  Don't
> > think that'd make the kernel code better though if even possible at all.
> >
>
> We can have a common bitfield layout for the two channels for command mode.
>
> So we can do something like below for common fields:
>
> -static inline uint32_t
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(uint32_t val)
> +static inline uint32_t
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM_DATATYPE(uint32_t val, uint32_t
> stream_id)
>   {
> -       return ((val) <<
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT) &
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
> +       return ((val) <<
> (DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT + (stream_id
> * 16)) & DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
>   }

NAK. Please express this in the xml source. This file is autogenerated.

>
> Video mode can also use all of these except for WC as that field is not
> present for cmd modes.
>
> This can go as a separate change .
>
> I can push this and perhaps get a Tested-by from Vinod as I dont have a
> setup to re-validate this.
>
> Thanks
>
> Abhinav
> > [1]: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14967/diffs
> >
> > - Marijn
> >
> >>>
> >>> However, this seems to indicate that the DSC patch series has been
> >>> approved and merged somehow??
> >>>
> >>>> ---
> >>>>
> >>>> Changes since v1:
> >>>>    - Fix c&p error and apply mask clear to reg_ctrl2 instead of reg_ctrl
> >>>>      (Abhinav)
> >>>>
> >>>> ---
> >>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 5 ++++-
> >>>>    1 file changed, 4 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >>>> index c983698d1384..a95d5df52653 100644
> >>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >>>> @@ -961,10 +961,13 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
> >>>>                    reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> >>>>                    reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
> >>>>
> >>>> +          reg_ctrl &= ~0xffff;
> >>>>                    reg_ctrl |= reg;
> >>>> +
> >>>> +          reg_ctrl2 &= ~DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH__MASK;
> >>>>                    reg_ctrl2 |= DSI_COMMAND_COMPRESSION_MODE_CTRL2_STREAM0_SLICE_WIDTH(bytes_in_slice);
> >>>>
> >>>> -          dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> >>>> +          dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
> >>>>                    dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
> >>>>            } else {
> >>>>                    dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> >>>> --
> >>>> 2.35.1
> >>>>



-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Marijn Suijten]

On 2022-05-01 16:56:45, Abhinav Kumar wrote:
> [snip]
> Wouln't this macro already make sure that 'reg' doesnt have anything in 
> the top 16 bits? Its doing a & with 0x00003f00

Like I said, it is unlikely that this happens, only if someone starts
changing the code that assigns to `reg` which is unlikely to pass review
anyway.

> [snip]
> We can have a common bitfield layout for the two channels for command mode.
> 
> So we can do something like below for common fields:
> 
> -static inline uint32_t 
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(uint32_t val)
> +static inline uint32_t 
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM_DATATYPE(uint32_t val, uint32_t 
> stream_id)
>   {
> -       return ((val) << 
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT) & 
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
> +       return ((val) << 
> (DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT + (stream_id 
> * 16)) & DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
>   }
> 
> Video mode can also use all of these except for WC as that field is not 
> present for cmd modes.
> 
> This can go as a separate change .
> 
> I can push this and perhaps get a Tested-by from Vinod as I dont have a 
> setup to re-validate this.

How would you represent this in XML?  I was hoping for a method that
allows to construct the value in a generic way, without register names,
and then simply have a "register macro" that moves (and perhaps masks)
the preconstructed value into the right place.  A bit like how `enum`s
are currently set up in XML, but with bit ranges for the values and
macros to set a value.

I think I've _partially_ found what I was looking for: a `<bitset>`.
However, I don't know if we can utilize this multiple times within a
single `reg32`, once with an offset for stream1.  Alas, it's just
bikeshedding at this point.

- Marijn

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Marijn Suijten]

On 2022-05-01 16:56:45, Abhinav Kumar wrote:
> [snip]
> Wouln't this macro already make sure that 'reg' doesnt have anything in 
> the top 16 bits? Its doing a & with 0x00003f00

Like I said, it is unlikely that this happens, only if someone starts
changing the code that assigns to `reg` which is unlikely to pass review
anyway.

> [snip]
> We can have a common bitfield layout for the two channels for command mode.
> 
> So we can do something like below for common fields:
> 
> -static inline uint32_t 
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(uint32_t val)
> +static inline uint32_t 
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM_DATATYPE(uint32_t val, uint32_t 
> stream_id)
>   {
> -       return ((val) << 
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT) & 
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
> +       return ((val) << 
> (DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT + (stream_id 
> * 16)) & DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
>   }
> 
> Video mode can also use all of these except for WC as that field is not 
> present for cmd modes.
> 
> This can go as a separate change .
> 
> I can push this and perhaps get a Tested-by from Vinod as I dont have a 
> setup to re-validate this.

How would you represent this in XML?  I was hoping for a method that
allows to construct the value in a generic way, without register names,
and then simply have a "register macro" that moves (and perhaps masks)
the preconstructed value into the right place.  A bit like how `enum`s
are currently set up in XML, but with bit ranges for the values and
macros to set a value.

I think I've _partially_ found what I was looking for: a `<bitset>`.
However, I don't know if we can utilize this multiple times within a
single `reg32`, once with an offset for stream1.  Alas, it's just
bikeshedding at this point.

- Marijn

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Marijn Suijten]

On 2022-05-02 01:44:20, Dmitry Baryshkov wrote:
> [sni[
> > In any case, given that you've already sent this patch and another three
> > patches [2] fixing/cleaning up the series tells me it's far from ready.
> > Most of this should just be handled - or have been handled - in review
> > and amended?
> 
> During the review time we agreed that [2] would come as a separate 
> change It is an API change that would make using panel-bridge easier, 
> but isn't otherwise required.
> 
> I have been working towards more logical drm_bridge/drm_bridge_connector 
> chains employing panel-bridge and display-connector where required, [2] 
> is a part of that effort (as well as few other patches that hit 
> dri-devel in the last few days).

I understand what is going on now.  Since the DSC patches have already
been queued up in the 5.19 pull I won't hurry to review them; rather
will go over them when time allows me to play with the many phones here
that require DSC for the screen to work.  I've been told the series
didn't result in positive screen output way back in its infancy, but
I'll re-evaluate and send fixes or improvements if/when necessary.

- Marijn

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Marijn Suijten]

On 2022-05-02 01:44:20, Dmitry Baryshkov wrote:
> [sni[
> > In any case, given that you've already sent this patch and another three
> > patches [2] fixing/cleaning up the series tells me it's far from ready.
> > Most of this should just be handled - or have been handled - in review
> > and amended?
> 
> During the review time we agreed that [2] would come as a separate 
> change It is an API change that would make using panel-bridge easier, 
> but isn't otherwise required.
> 
> I have been working towards more logical drm_bridge/drm_bridge_connector 
> chains employing panel-bridge and display-connector where required, [2] 
> is a part of that effort (as well as few other patches that hit 
> dri-devel in the last few days).

I understand what is going on now.  Since the DSC patches have already
been queued up in the 5.19 pull I won't hurry to review them; rather
will go over them when time allows me to play with the many phones here
that require DSC for the screen to work.  I've been told the series
didn't result in positive screen output way back in its infancy, but
I'll re-evaluate and send fixes or improvements if/when necessary.

- Marijn

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Dmitry Baryshkov]

On 02/05/2022 11:43, Marijn Suijten wrote:
> On 2022-05-02 01:44:20, Dmitry Baryshkov wrote:
>> [sni[
>>> In any case, given that you've already sent this patch and another three
>>> patches [2] fixing/cleaning up the series tells me it's far from ready.
>>> Most of this should just be handled - or have been handled - in review
>>> and amended?
>>
>> During the review time we agreed that [2] would come as a separate
>> change It is an API change that would make using panel-bridge easier,
>> but isn't otherwise required.
>>
>> I have been working towards more logical drm_bridge/drm_bridge_connector
>> chains employing panel-bridge and display-connector where required, [2]
>> is a part of that effort (as well as few other patches that hit
>> dri-devel in the last few days).
> 
> I understand what is going on now.  Since the DSC patches have already
> been queued up in the 5.19 pull I won't hurry to review them; rather
> will go over them when time allows me to play with the many phones here
> that require DSC for the screen to work.  I've been told the series
> didn't result in positive screen output way back in its infancy, but
> I'll re-evaluate and send fixes or improvements if/when necessary.

Sure, thank you!

They work on Pixel3 (sdm845, non-active CTLs, no ping-pong binding to 
intf). I still didn't have time to test them on P4 (sm8150, active CTLs, 
PPs bound to the intf in runtime).

-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Dmitry Baryshkov]

On 02/05/2022 11:43, Marijn Suijten wrote:
> On 2022-05-02 01:44:20, Dmitry Baryshkov wrote:
>> [sni[
>>> In any case, given that you've already sent this patch and another three
>>> patches [2] fixing/cleaning up the series tells me it's far from ready.
>>> Most of this should just be handled - or have been handled - in review
>>> and amended?
>>
>> During the review time we agreed that [2] would come as a separate
>> change It is an API change that would make using panel-bridge easier,
>> but isn't otherwise required.
>>
>> I have been working towards more logical drm_bridge/drm_bridge_connector
>> chains employing panel-bridge and display-connector where required, [2]
>> is a part of that effort (as well as few other patches that hit
>> dri-devel in the last few days).
> 
> I understand what is going on now.  Since the DSC patches have already
> been queued up in the 5.19 pull I won't hurry to review them; rather
> will go over them when time allows me to play with the many phones here
> that require DSC for the screen to work.  I've been told the series
> didn't result in positive screen output way back in its infancy, but
> I'll re-evaluate and send fixes or improvements if/when necessary.

Sure, thank you!

They work on Pixel3 (sdm845, non-active CTLs, no ping-pong binding to 
intf). I still didn't have time to test them on P4 (sm8150, active CTLs, 
PPs bound to the intf in runtime).

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Dmitry Baryshkov]

On 02/05/2022 11:34, Marijn Suijten wrote:
> On 2022-05-01 16:56:45, Abhinav Kumar wrote:
>> [snip]
>> Wouln't this macro already make sure that 'reg' doesnt have anything in
>> the top 16 bits? Its doing a & with 0x00003f00
> 
> Like I said, it is unlikely that this happens, only if someone starts
> changing the code that assigns to `reg` which is unlikely to pass review
> anyway.
> 
>> [snip]
>> We can have a common bitfield layout for the two channels for command mode.
>>
>> So we can do something like below for common fields:
>>
>> -static inline uint32_t
>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(uint32_t val)
>> +static inline uint32_t
>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM_DATATYPE(uint32_t val, uint32_t
>> stream_id)
>>    {
>> -       return ((val) <<
>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT) &
>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
>> +       return ((val) <<
>> (DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT + (stream_id
>> * 16)) & DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
>>    }
>>
>> Video mode can also use all of these except for WC as that field is not
>> present for cmd modes.
>>
>> This can go as a separate change .
>>
>> I can push this and perhaps get a Tested-by from Vinod as I dont have a
>> setup to re-validate this.
> 
> How would you represent this in XML?  I was hoping for a method that
> allows to construct the value in a generic way, without register names,
> and then simply have a "register macro" that moves (and perhaps masks)
> the preconstructed value into the right place.  A bit like how `enum`s
> are currently set up in XML, but with bit ranges for the values and
> macros to set a value.
> 
> I think I've _partially_ found what I was looking for: a `<bitset>`.
> However, I don't know if we can utilize this multiple times within a
> single `reg32`, once with an offset for stream1.  Alas, it's just
> bikeshedding at this point.

Unfortunately the following naïve patch doesn't work, stream1 bits are 
still defined in the 0:15 bit space. One would have to modify rnn tool 
to make sure that it takes into account the low/high parts of the 
bitfield when generating offsets/masks.

diff --git a/src/freedreno/registers/dsi/dsi.xml 
b/src/freedreno/registers/dsi/dsi.xml
index f2eef4ff41ae..b0166628ad0a 100644
--- a/src/freedreno/registers/dsi/dsi.xml
+++ b/src/freedreno/registers/dsi/dsi.xml
@@ -361,22 +361,19 @@ 
xsi:schemaLocation="http://nouveau.freedesktop.org/ rules-ng.xsd">
                 <bitfield name="MAJOR" low="24" high="31" type="uint"/>
         </reg32>
         <reg32 offset="0x002d4" name="CPHY_MODE_CTRL"/>
-       <reg32 offset="0x0029c" name="VIDEO_COMPRESSION_MODE_CTRL">
-               <bitfield name="WC" low="16" high="31" type="uint"/>
+       <bitset name="COMPRESSION_MODE_CTRL" inline="true">
                 <bitfield name="DATATYPE" low="8" high="13" type="uint"/>
                 <bitfield name="PKT_PER_LINE" low="6" high="7" 
type="uint"/>
                 <bitfield name="EOL_BYTE_NUM" low="4" high="5" 
type="uint"/>
                 <bitfield name="EN" pos="0" type="boolean"/>
+       </bitset>
+       <reg32 offset="0x0029c" name="VIDEO_COMPRESSION_MODE_CTRL">
+               <bitfield name="WC" low="16" high="31" type="uint"/>
+               <bitfield name="STREAM0" low="0" high="15" 
type="COMPRESSION_MODE_CTRL"/>
         </reg32>
         <reg32 offset="0x002a4" name="COMMAND_COMPRESSION_MODE_CTRL">
-               <bitfield name="STREAM1_DATATYPE" low="24" high="29" 
type="uint"/>
-               <bitfield name="STREAM1_PKT_PER_LINE" low="22" high="23" 
type="uint"/>
-               <bitfield name="STREAM1_EOL_BYTE_NUM" low="20" high="21" 
type="uint"/>
-               <bitfield name="STREAM1_EN" pos="16" type="boolean"/>
-               <bitfield name="STREAM0_DATATYPE" low="8" high="13" 
type="uint"/>
-               <bitfield name="STREAM0_PKT_PER_LINE" low="6" high="7" 
type="uint"/>
-               <bitfield name="STREAM0_EOL_BYTE_NUM" low="4" high="5" 
type="uint"/>
-               <bitfield name="STREAM0_EN" pos="0" type="boolean"/>
+               <bitfield name="STREAM1" low="16" high="31" 
type="COMPRESSION_MODE_CTRL"/>
+               <bitfield name="STREAM0" low="0" high="15" 
type="COMPRESSION_MODE_CTRL"/>
         </reg32>
         <reg32 offset="0x002a8" name="COMMAND_COMPRESSION_MODE_CTRL2">
                 <bitfield name="STREAM1_SLICE_WIDTH" low="16" high="31" 
type="uint"/>


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Dmitry Baryshkov]

On 02/05/2022 11:34, Marijn Suijten wrote:
> On 2022-05-01 16:56:45, Abhinav Kumar wrote:
>> [snip]
>> Wouln't this macro already make sure that 'reg' doesnt have anything in
>> the top 16 bits? Its doing a & with 0x00003f00
> 
> Like I said, it is unlikely that this happens, only if someone starts
> changing the code that assigns to `reg` which is unlikely to pass review
> anyway.
> 
>> [snip]
>> We can have a common bitfield layout for the two channels for command mode.
>>
>> So we can do something like below for common fields:
>>
>> -static inline uint32_t
>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(uint32_t val)
>> +static inline uint32_t
>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM_DATATYPE(uint32_t val, uint32_t
>> stream_id)
>>    {
>> -       return ((val) <<
>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT) &
>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
>> +       return ((val) <<
>> (DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__SHIFT + (stream_id
>> * 16)) & DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE__MASK;
>>    }
>>
>> Video mode can also use all of these except for WC as that field is not
>> present for cmd modes.
>>
>> This can go as a separate change .
>>
>> I can push this and perhaps get a Tested-by from Vinod as I dont have a
>> setup to re-validate this.
> 
> How would you represent this in XML?  I was hoping for a method that
> allows to construct the value in a generic way, without register names,
> and then simply have a "register macro" that moves (and perhaps masks)
> the preconstructed value into the right place.  A bit like how `enum`s
> are currently set up in XML, but with bit ranges for the values and
> macros to set a value.
> 
> I think I've _partially_ found what I was looking for: a `<bitset>`.
> However, I don't know if we can utilize this multiple times within a
> single `reg32`, once with an offset for stream1.  Alas, it's just
> bikeshedding at this point.

Unfortunately the following naïve patch doesn't work, stream1 bits are 
still defined in the 0:15 bit space. One would have to modify rnn tool 
to make sure that it takes into account the low/high parts of the 
bitfield when generating offsets/masks.

diff --git a/src/freedreno/registers/dsi/dsi.xml 
b/src/freedreno/registers/dsi/dsi.xml
index f2eef4ff41ae..b0166628ad0a 100644
--- a/src/freedreno/registers/dsi/dsi.xml
+++ b/src/freedreno/registers/dsi/dsi.xml
@@ -361,22 +361,19 @@ 
xsi:schemaLocation="http://nouveau.freedesktop.org/ rules-ng.xsd">
                 <bitfield name="MAJOR" low="24" high="31" type="uint"/>
         </reg32>
         <reg32 offset="0x002d4" name="CPHY_MODE_CTRL"/>
-       <reg32 offset="0x0029c" name="VIDEO_COMPRESSION_MODE_CTRL">
-               <bitfield name="WC" low="16" high="31" type="uint"/>
+       <bitset name="COMPRESSION_MODE_CTRL" inline="true">
                 <bitfield name="DATATYPE" low="8" high="13" type="uint"/>
                 <bitfield name="PKT_PER_LINE" low="6" high="7" 
type="uint"/>
                 <bitfield name="EOL_BYTE_NUM" low="4" high="5" 
type="uint"/>
                 <bitfield name="EN" pos="0" type="boolean"/>
+       </bitset>
+       <reg32 offset="0x0029c" name="VIDEO_COMPRESSION_MODE_CTRL">
+               <bitfield name="WC" low="16" high="31" type="uint"/>
+               <bitfield name="STREAM0" low="0" high="15" 
type="COMPRESSION_MODE_CTRL"/>
         </reg32>
         <reg32 offset="0x002a4" name="COMMAND_COMPRESSION_MODE_CTRL">
-               <bitfield name="STREAM1_DATATYPE" low="24" high="29" 
type="uint"/>
-               <bitfield name="STREAM1_PKT_PER_LINE" low="22" high="23" 
type="uint"/>
-               <bitfield name="STREAM1_EOL_BYTE_NUM" low="20" high="21" 
type="uint"/>
-               <bitfield name="STREAM1_EN" pos="16" type="boolean"/>
-               <bitfield name="STREAM0_DATATYPE" low="8" high="13" 
type="uint"/>
-               <bitfield name="STREAM0_PKT_PER_LINE" low="6" high="7" 
type="uint"/>
-               <bitfield name="STREAM0_EOL_BYTE_NUM" low="4" high="5" 
type="uint"/>
-               <bitfield name="STREAM0_EN" pos="0" type="boolean"/>
+               <bitfield name="STREAM1" low="16" high="31" 
type="COMPRESSION_MODE_CTRL"/>
+               <bitfield name="STREAM0" low="0" high="15" 
type="COMPRESSION_MODE_CTRL"/>
         </reg32>
         <reg32 offset="0x002a8" name="COMMAND_COMPRESSION_MODE_CTRL2">
                 <bitfield name="STREAM1_SLICE_WIDTH" low="16" high="31" 
type="uint"/>


-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Marijn Suijten]

On 2022-05-02 12:41:37, Dmitry Baryshkov wrote:
> On 02/05/2022 11:43, Marijn Suijten wrote:
> > On 2022-05-02 01:44:20, Dmitry Baryshkov wrote:
> >> [sni[
> >>> In any case, given that you've already sent this patch and another three
> >>> patches [2] fixing/cleaning up the series tells me it's far from ready.
> >>> Most of this should just be handled - or have been handled - in review
> >>> and amended?
> >>
> >> During the review time we agreed that [2] would come as a separate
> >> change It is an API change that would make using panel-bridge easier,
> >> but isn't otherwise required.
> >>
> >> I have been working towards more logical drm_bridge/drm_bridge_connector
> >> chains employing panel-bridge and display-connector where required, [2]
> >> is a part of that effort (as well as few other patches that hit
> >> dri-devel in the last few days).
> >
> > I understand what is going on now.  Since the DSC patches have already
> > been queued up in the 5.19 pull I won't hurry to review them; rather
> > will go over them when time allows me to play with the many phones here
> > that require DSC for the screen to work.  I've been told the series
> > didn't result in positive screen output way back in its infancy, but
> > I'll re-evaluate and send fixes or improvements if/when necessary.
>
> Sure, thank you!
>
> They work on Pixel3 (sdm845, non-active CTLs, no ping-pong binding to
> intf). I still didn't have time to test them on P4 (sm8150, active CTLs,
> PPs bound to the intf in runtime).

The devices mentioned above were all recent SoCs with active CTLs.  My
ping-pong binding to intf patch recently "fixed" sm6125 (non-DSC) but
I have been told it didn't make a difference on the more powerful SoCs
(sm8[123]50) with DSC panels.  There might indeed be a problem with
either active CTLs and CMDmode in general (we still have patches in the
works that move PP features to the INTF block) or DSC + actice CTL, or
both.  To be continued...

- Marijn

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Marijn Suijten]

On 2022-05-02 12:41:37, Dmitry Baryshkov wrote:
> On 02/05/2022 11:43, Marijn Suijten wrote:
> > On 2022-05-02 01:44:20, Dmitry Baryshkov wrote:
> >> [sni[
> >>> In any case, given that you've already sent this patch and another three
> >>> patches [2] fixing/cleaning up the series tells me it's far from ready.
> >>> Most of this should just be handled - or have been handled - in review
> >>> and amended?
> >>
> >> During the review time we agreed that [2] would come as a separate
> >> change It is an API change that would make using panel-bridge easier,
> >> but isn't otherwise required.
> >>
> >> I have been working towards more logical drm_bridge/drm_bridge_connector
> >> chains employing panel-bridge and display-connector where required, [2]
> >> is a part of that effort (as well as few other patches that hit
> >> dri-devel in the last few days).
> >
> > I understand what is going on now.  Since the DSC patches have already
> > been queued up in the 5.19 pull I won't hurry to review them; rather
> > will go over them when time allows me to play with the many phones here
> > that require DSC for the screen to work.  I've been told the series
> > didn't result in positive screen output way back in its infancy, but
> > I'll re-evaluate and send fixes or improvements if/when necessary.
>
> Sure, thank you!
>
> They work on Pixel3 (sdm845, non-active CTLs, no ping-pong binding to
> intf). I still didn't have time to test them on P4 (sm8150, active CTLs,
> PPs bound to the intf in runtime).

The devices mentioned above were all recent SoCs with active CTLs.  My
ping-pong binding to intf patch recently "fixed" sm6125 (non-DSC) but
I have been told it didn't make a difference on the more powerful SoCs
(sm8[123]50) with DSC panels.  There might indeed be a problem with
either active CTLs and CMDmode in general (we still have patches in the
works that move PP features to the INTF block) or DSC + actice CTL, or
both.  To be continued...

- Marijn

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Marijn Suijten]

On 2022-05-02 13:02:09, Dmitry Baryshkov wrote:
> [snip]
> > How would you represent this in XML?  I was hoping for a method that
> > allows to construct the value in a generic way, without register names,
> > and then simply have a "register macro" that moves (and perhaps masks)
> > the preconstructed value into the right place.  A bit like how `enum`s
> > are currently set up in XML, but with bit ranges for the values and
> > macros to set a value.
> > 
> > I think I've _partially_ found what I was looking for: a `<bitset>`.
> > However, I don't know if we can utilize this multiple times within a
> > single `reg32`, once with an offset for stream1.  Alas, it's just
> > bikeshedding at this point.
> 
> Unfortunately the following naïve patch doesn't work, stream1 bits are 
> still defined in the 0:15 bit space. One would have to modify rnn tool 
> to make sure that it takes into account the low/high parts of the 
> bitfield when generating offsets/masks.
> 
> diff --git a/src/freedreno/registers/dsi/dsi.xml 
> b/src/freedreno/registers/dsi/dsi.xml
> index f2eef4ff41ae..b0166628ad0a 100644
> --- a/src/freedreno/registers/dsi/dsi.xml
> +++ b/src/freedreno/registers/dsi/dsi.xml
> @@ -361,22 +361,19 @@ 
> xsi:schemaLocation="http://nouveau.freedesktop.org/ rules-ng.xsd">
>                  <bitfield name="MAJOR" low="24" high="31" type="uint"/>
>          </reg32>
>          <reg32 offset="0x002d4" name="CPHY_MODE_CTRL"/>
> -       <reg32 offset="0x0029c" name="VIDEO_COMPRESSION_MODE_CTRL">
> -               <bitfield name="WC" low="16" high="31" type="uint"/>
> +       <bitset name="COMPRESSION_MODE_CTRL" inline="true">
>                  <bitfield name="DATATYPE" low="8" high="13" type="uint"/>
>                  <bitfield name="PKT_PER_LINE" low="6" high="7" 
> type="uint"/>
>                  <bitfield name="EOL_BYTE_NUM" low="4" high="5" 
> type="uint"/>
>                  <bitfield name="EN" pos="0" type="boolean"/>
> +       </bitset>
> +       <reg32 offset="0x0029c" name="VIDEO_COMPRESSION_MODE_CTRL">
> +               <bitfield name="WC" low="16" high="31" type="uint"/>
> +               <bitfield name="STREAM0" low="0" high="15" 
> type="COMPRESSION_MODE_CTRL"/>
>          </reg32>
>          <reg32 offset="0x002a4" name="COMMAND_COMPRESSION_MODE_CTRL">
> -               <bitfield name="STREAM1_DATATYPE" low="24" high="29" 
> type="uint"/>
> -               <bitfield name="STREAM1_PKT_PER_LINE" low="22" high="23" 
> type="uint"/>
> -               <bitfield name="STREAM1_EOL_BYTE_NUM" low="20" high="21" 
> type="uint"/>
> -               <bitfield name="STREAM1_EN" pos="16" type="boolean"/>
> -               <bitfield name="STREAM0_DATATYPE" low="8" high="13" 
> type="uint"/>
> -               <bitfield name="STREAM0_PKT_PER_LINE" low="6" high="7" 
> type="uint"/>
> -               <bitfield name="STREAM0_EOL_BYTE_NUM" low="4" high="5" 
> type="uint"/>
> -               <bitfield name="STREAM0_EN" pos="0" type="boolean"/>
> +               <bitfield name="STREAM1" low="16" high="31" 
> type="COMPRESSION_MODE_CTRL"/>
> +               <bitfield name="STREAM0" low="0" high="15" 
> type="COMPRESSION_MODE_CTRL"/>
>          </reg32>
>          <reg32 offset="0x002a8" name="COMMAND_COMPRESSION_MODE_CTRL2">
>                  <bitfield name="STREAM1_SLICE_WIDTH" low="16" high="31" 
> type="uint"/>

This is approximately what I was aiming for.  `inline="true"` does
"inline" all the individual bitfields so that they're prefixed with the
reg32+bitfield name again, right?  That's what I wanted to avoid :)

- Marijn

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Marijn Suijten]

On 2022-05-02 13:02:09, Dmitry Baryshkov wrote:
> [snip]
> > How would you represent this in XML?  I was hoping for a method that
> > allows to construct the value in a generic way, without register names,
> > and then simply have a "register macro" that moves (and perhaps masks)
> > the preconstructed value into the right place.  A bit like how `enum`s
> > are currently set up in XML, but with bit ranges for the values and
> > macros to set a value.
> > 
> > I think I've _partially_ found what I was looking for: a `<bitset>`.
> > However, I don't know if we can utilize this multiple times within a
> > single `reg32`, once with an offset for stream1.  Alas, it's just
> > bikeshedding at this point.
> 
> Unfortunately the following naïve patch doesn't work, stream1 bits are 
> still defined in the 0:15 bit space. One would have to modify rnn tool 
> to make sure that it takes into account the low/high parts of the 
> bitfield when generating offsets/masks.
> 
> diff --git a/src/freedreno/registers/dsi/dsi.xml 
> b/src/freedreno/registers/dsi/dsi.xml
> index f2eef4ff41ae..b0166628ad0a 100644
> --- a/src/freedreno/registers/dsi/dsi.xml
> +++ b/src/freedreno/registers/dsi/dsi.xml
> @@ -361,22 +361,19 @@ 
> xsi:schemaLocation="http://nouveau.freedesktop.org/ rules-ng.xsd">
>                  <bitfield name="MAJOR" low="24" high="31" type="uint"/>
>          </reg32>
>          <reg32 offset="0x002d4" name="CPHY_MODE_CTRL"/>
> -       <reg32 offset="0x0029c" name="VIDEO_COMPRESSION_MODE_CTRL">
> -               <bitfield name="WC" low="16" high="31" type="uint"/>
> +       <bitset name="COMPRESSION_MODE_CTRL" inline="true">
>                  <bitfield name="DATATYPE" low="8" high="13" type="uint"/>
>                  <bitfield name="PKT_PER_LINE" low="6" high="7" 
> type="uint"/>
>                  <bitfield name="EOL_BYTE_NUM" low="4" high="5" 
> type="uint"/>
>                  <bitfield name="EN" pos="0" type="boolean"/>
> +       </bitset>
> +       <reg32 offset="0x0029c" name="VIDEO_COMPRESSION_MODE_CTRL">
> +               <bitfield name="WC" low="16" high="31" type="uint"/>
> +               <bitfield name="STREAM0" low="0" high="15" 
> type="COMPRESSION_MODE_CTRL"/>
>          </reg32>
>          <reg32 offset="0x002a4" name="COMMAND_COMPRESSION_MODE_CTRL">
> -               <bitfield name="STREAM1_DATATYPE" low="24" high="29" 
> type="uint"/>
> -               <bitfield name="STREAM1_PKT_PER_LINE" low="22" high="23" 
> type="uint"/>
> -               <bitfield name="STREAM1_EOL_BYTE_NUM" low="20" high="21" 
> type="uint"/>
> -               <bitfield name="STREAM1_EN" pos="16" type="boolean"/>
> -               <bitfield name="STREAM0_DATATYPE" low="8" high="13" 
> type="uint"/>
> -               <bitfield name="STREAM0_PKT_PER_LINE" low="6" high="7" 
> type="uint"/>
> -               <bitfield name="STREAM0_EOL_BYTE_NUM" low="4" high="5" 
> type="uint"/>
> -               <bitfield name="STREAM0_EN" pos="0" type="boolean"/>
> +               <bitfield name="STREAM1" low="16" high="31" 
> type="COMPRESSION_MODE_CTRL"/>
> +               <bitfield name="STREAM0" low="0" high="15" 
> type="COMPRESSION_MODE_CTRL"/>
>          </reg32>
>          <reg32 offset="0x002a8" name="COMMAND_COMPRESSION_MODE_CTRL2">
>                  <bitfield name="STREAM1_SLICE_WIDTH" low="16" high="31" 
> type="uint"/>

This is approximately what I was aiming for.  `inline="true"` does
"inline" all the individual bitfields so that they're prefixed with the
reg32+bitfield name again, right?  That's what I wanted to avoid :)

- Marijn

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Vinod Koul]

On 30-04-22, 20:55, Dmitry Baryshkov wrote:
> The downstream uses read-modify-write for updating command mode
> compression registers. Let's follow this approach. This also fixes the
> following warning:
> 
> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]

Reviewed-by: Vinod Koul <vkoul@kernel.org>

Tested on pixel3:
Tested-by: Vinod Koul <vkoul@kernel.org>

-- 
~Vinod

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Vinod Koul]

On 30-04-22, 20:55, Dmitry Baryshkov wrote:
> The downstream uses read-modify-write for updating command mode
> compression registers. Let's follow this approach. This also fixes the
> following warning:
> 
> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]

Reviewed-by: Vinod Koul <vkoul@kernel.org>

Tested on pixel3:
Tested-by: Vinod Koul <vkoul@kernel.org>

-- 
~Vinod

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Vinod Koul]

On 30-04-22, 22:28, Dmitry Baryshkov wrote:
> On 30/04/2022 21:58, Marijn Suijten wrote:
> > On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
> > > The downstream uses read-modify-write for updating command mode
> > > compression registers. Let's follow this approach. This also fixes the
> > > following warning:
> > > 
> > > drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
> > > 
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > I pointed this out in review multiple times, so you'll obviously get my:
> 
> I think I might have also pointed this out once (and then forgot to check
> that the issue was fixed by Vinod).

I think i have tried to reply to all comments, if anything was missed
that would be my mistake..

> > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > 
> > (But are you sure there's nothing else to clear in the 1st CTRL
> > register, only the lowest 16 bits?  That should mean `reg` never
> > contains anything in 0xffff0000)
> 
> Judging from the downstream the upper half conains the same fields, but used
> for other virtual channel. I didn't research what's the difference yet. All
> the dtsi files that I have here at hand use
> 'qcom,mdss-dsi-virtual-channel-id = <0>;'

Yes the register description is for STREAM1 in documentation, it is
unclear to me when that can be used

-- 
~Vinod

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Vinod Koul]

On 30-04-22, 22:28, Dmitry Baryshkov wrote:
> On 30/04/2022 21:58, Marijn Suijten wrote:
> > On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
> > > The downstream uses read-modify-write for updating command mode
> > > compression registers. Let's follow this approach. This also fixes the
> > > following warning:
> > > 
> > > drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
> > > 
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > I pointed this out in review multiple times, so you'll obviously get my:
> 
> I think I might have also pointed this out once (and then forgot to check
> that the issue was fixed by Vinod).

I think i have tried to reply to all comments, if anything was missed
that would be my mistake..

> > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > 
> > (But are you sure there's nothing else to clear in the 1st CTRL
> > register, only the lowest 16 bits?  That should mean `reg` never
> > contains anything in 0xffff0000)
> 
> Judging from the downstream the upper half conains the same fields, but used
> for other virtual channel. I didn't research what's the difference yet. All
> the dtsi files that I have here at hand use
> 'qcom,mdss-dsi-virtual-channel-id = <0>;'

Yes the register description is for STREAM1 in documentation, it is
unclear to me when that can be used

-- 
~Vinod

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Vinod Koul]

On 01-05-22, 22:41, Marijn Suijten wrote:
> On 2022-04-30 22:28:42, Dmitry Baryshkov wrote:
> > On 30/04/2022 21:58, Marijn Suijten wrote:
> > > On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
> > >> The downstream uses read-modify-write for updating command mode
> > >> compression registers. Let's follow this approach. This also fixes the
> > >> following warning:
> > >>
> > >> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
> > >>
> > >> Reported-by: kernel test robot <lkp@intel.com>
> > >> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > 
> > > I pointed this out in review multiple times, so you'll obviously get my:
> > 
> > I think I might have also pointed this out once (and then forgot to 
> > check that the issue was fixed by Vinod).
> > 
> > > 
> > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > > 
> > > (But are you sure there's nothing else to clear in the 1st CTRL
> > > register, only the lowest 16 bits?  That should mean `reg` never
> > > contains anything in 0xffff0000)
> > 
> > Judging from the downstream the upper half conains the same fields, but 
> > used for other virtual channel. I didn't research what's the difference 
> > yet. All the dtsi files that I have here at hand use 
> > 'qcom,mdss-dsi-virtual-channel-id = <0>;'
> 
> As replied to Abhinav I'm simply asking whether we should be strict
> and add `reg & 0xffff` to prevent accidentally writing the top 16 bits,
> which are stream 1.  It doesn't seem like the current code can hit that
> though, with all the macros using masks internally already; but it's

Since the macros were used I skipped setting that up explictly...

> still a little scary since we're assuming the registers for VIDEO are
> identical to CMD (as mentioned in the reply to Abhinav: I wonder if it's

The documentation seems to indicate they are similar and that is the
reason, I merged the code paths and set different registers required for
video and cmd modes

-- 
~Vinod

Re: [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Vinod Koul]

On 01-05-22, 22:41, Marijn Suijten wrote:
> On 2022-04-30 22:28:42, Dmitry Baryshkov wrote:
> > On 30/04/2022 21:58, Marijn Suijten wrote:
> > > On 2022-04-30 20:55:33, Dmitry Baryshkov wrote:
> > >> The downstream uses read-modify-write for updating command mode
> > >> compression registers. Let's follow this approach. This also fixes the
> > >> following warning:
> > >>
> > >> drivers/gpu/drm/msm/dsi/dsi_host.c:918:23: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
> > >>
> > >> Reported-by: kernel test robot <lkp@intel.com>
> > >> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > 
> > > I pointed this out in review multiple times, so you'll obviously get my:
> > 
> > I think I might have also pointed this out once (and then forgot to 
> > check that the issue was fixed by Vinod).
> > 
> > > 
> > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > > 
> > > (But are you sure there's nothing else to clear in the 1st CTRL
> > > register, only the lowest 16 bits?  That should mean `reg` never
> > > contains anything in 0xffff0000)
> > 
> > Judging from the downstream the upper half conains the same fields, but 
> > used for other virtual channel. I didn't research what's the difference 
> > yet. All the dtsi files that I have here at hand use 
> > 'qcom,mdss-dsi-virtual-channel-id = <0>;'
> 
> As replied to Abhinav I'm simply asking whether we should be strict
> and add `reg & 0xffff` to prevent accidentally writing the top 16 bits,
> which are stream 1.  It doesn't seem like the current code can hit that
> though, with all the macros using masks internally already; but it's

Since the macros were used I skipped setting that up explictly...

> still a little scary since we're assuming the registers for VIDEO are
> identical to CMD (as mentioned in the reply to Abhinav: I wonder if it's

The documentation seems to indicate they are similar and that is the
reason, I merged the code paths and set different registers required for
video and cmd modes

-- 
~Vinod

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Vinod Koul]

On 02-05-22, 10:43, Marijn Suijten wrote:
> On 2022-05-02 01:44:20, Dmitry Baryshkov wrote:
> that require DSC for the screen to work.  I've been told the series
> didn't result in positive screen output way back in its infancy, but

I would be intrested to hear about that. I have only pixel3 at my
disposal so tested on that. I would be willing to help with more testing
efforts.

Also, to get DSC to work, the panel needs to be set as well...

> I'll re-evaluate and send fixes or improvements if/when necessary.

That would be nice

-- 
~Vinod

Re: [Freedreno] [PATCH v2] drm/msm/dsi: use RMW cycles in dsi_update_dsc_timing

[Vinod Koul]

On 02-05-22, 10:43, Marijn Suijten wrote:
> On 2022-05-02 01:44:20, Dmitry Baryshkov wrote:
> that require DSC for the screen to work.  I've been told the series
> didn't result in positive screen output way back in its infancy, but

I would be intrested to hear about that. I have only pixel3 at my
disposal so tested on that. I would be willing to help with more testing
efforts.

Also, to get DSC to work, the panel needs to be set as well...

> I'll re-evaluate and send fixes or improvements if/when necessary.

That would be nice

-- 
~Vinod