[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 at intel.com>
>>>> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>
>>> I pointed this out in review multiple times, so you'll obviously get my:
>>>
>>> Reviewed-by: Marijn Suijten <marijn.suijten at 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
>>>>