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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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