[PATCH 1/2] drm/msm/dpu: fix DSC 1.2 block lengths

[Abhinav Kumar]

On 6/23/2023 12:40 PM, Dmitry Baryshkov wrote:
> On 23/06/2023 22:37, Abhinav Kumar wrote:
>>
>>
>> On 6/23/2023 4:37 AM, Dmitry Baryshkov wrote:
>>> On 23/06/2023 09:54, Marijn Suijten wrote:
>>>> On 2023-06-22 22:47:04, Abhinav Kumar wrote:
>>>>> On 6/22/2023 6:37 PM, Dmitry Baryshkov wrote:
>>>>>> All DSC_BLK_1_2 declarations incorrectly pass 0x29c as the block 
>>>>>> length.
>>>>>> This includes the common block itself, enc subblocks and some empty
>>>>>> space around. Change that to pass 0x4 instead, the length of common
>>>>>> register block itself.
>>>>>>
>>>>>> Fixes: 0d1b10c63346 ("drm/msm/dpu: add DSC 1.2 hw blocks for 
>>>>>> relevant chipsets")
>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>>
>>>>> There is no need of a fixes tag for this.
>>>>>
>>>>> This is not a bug but was intentional.
>>>>>
>>>>> Till we added sub-block parsing support we had to dump the whole 
>>>>> block.
>>>>>
>>>>> And hence I would suggest this change should be merged after the
>>>>> sub-block parsing change otherwise we wont have full register dumps 
>>>>> for DSC.
>>>>
>>>> This was indeed intentional, we discussed it in [1].
>>>>
>>>> In fact I asked to make it 0xf00 + 0x10 or 0xf80 + 0x10 to also cover
>>>> the CTL registers, but that change didn't make it through.  0x29c is an
>>>> arbitrary number that I have no clue what it was based on.
>>>
>>> This should have been NAKed. or at least TODOed.
>>>
>>
>> Its is not an arbitrary number. Thats an incorrect comment.
>>
>> Its 4 more than the encoder's last offset which is 0x298 + 0x4 = 0x29c.
>>
>> There was nothing to NAK or TODO here.
> 
> We do not include sub-blocks in the main block area. The SSPP's SRC 
> blocks were cleaned up for this reason. The ENC registers are definitely 
> a sub-block (and are described this way). There should have been a 
> "TODO: reduce block length to 0x4 after adding sub-blocks to dump" comment.
> 

iirc the sub-block dump idea came to me in some other patchset not this 
one. But ack that a comment could have been left.

>>
>>>>
>>>> [1]: 
>>>> https://lore.kernel.org/linux-arm-msm/y2whfntyo2rbrg3taazjdw5sijle6k6swzl4uutcxm6tmuayh4@uxdur74uasua/
>>>>
>>>> - Marijn
>>>
>