Re: [PATCH 4/4] drm/msm/dpu: Enable compression for command mode

[Jessica Zhang <quic_jesszhan@quicinc.com>]

On 5/3/2023 4:00 PM, Marijn Suijten wrote:
> Hi Jessica,
> 
> On 2023-05-03 12:04:59, Jessica Zhang wrote:
>>
>>
>> On 5/3/2023 12:28 AM, Marijn Suijten wrote:
>>> On 2023-05-02 18:19:15, Jessica Zhang wrote:
>>>> Add a dpu_hw_intf op to enable data compression.
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
>>>>    3 files changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> index 74470d068622..4321a1aba17f 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>
>>> Can we have INTF DCE on video-mode encoders as well?
>>
>> Hi Marijn,
>>
>> Currently, there's no way to validate DSC for video mode as I've only
>> made changes to support DSI for command mode. We are planning to post
>> changes to support DSC over DP, which will include changes for video mode.
> 
> Okay, but then mention so in the patch description (which is rather
> short in this revision).

Acked.

> 
> <snip>
> 
>>>>    #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
>>>>    #define INTF_CFG2_DATA_HCTL_EN	BIT(4)
>>>
>>> These should probably be reindented to match the below... And the rest
>>> of the defines use spaces instead of tabs.
>>
>> Fair point, though I think fixing the whitespace for these 2 macros
>> specifically might be better in a more relevant series.
> 
> Yes, I have many patches to start cleaning these up, as well as all the
> broken kerneldoc comments, but it's an uphill battle.  Not sure if I'll
> get to it any time soon if at all.
> 
>> With that being said, I'll change the spacing of the DATA_COMPRESS bit
>> to spaces instead of tabs.
> 
> Thanks, that seems to be the most common format.
> 
>>>> +#define INTF_CFG2_DCE_DATA_COMPRESS	BIT(12)
>>>>    
>>>>    #define INTF_MISR_CTRL			0x180
>>>>    #define INTF_MISR_SIGNATURE		0x184
>>>
>>> This does not seem to apply on top of:
>>> https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-10-27ce1a5ab5c6@somainline.org/
>>
>> Seems like I'm missing some patches from that series on my working
>> branch. Will rebase on top of the full series for the v2.
> 
> Thanks, but do discuss with Abhinav/Dmitry which series will land first.
> 
>>>> +static inline void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
>>>
>>> Why inline?  This is used as a pointer callback.
>>
>> Acked, will remove the inline.
>>
>>>
>>>> +{
>>>> +	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, INTF_CFG2_DCE_DATA_COMPRESS);
>>>
>>> dpu_hw_intf_setup_timing_engine() also programs INTF_CONFIG2.  Is it
>>> double-buffered, or is that config **always** unused when DSI CMD mode
>>> is used in conjunction with DSC/DCE?  Otherwise this should perhaps OR
>>> the bitflag into the register, or write the whole thing at once in
>>> dpu_hw_intf_setup_timing_engine()?
>>
>> For command mode, INTF_CONFIG2 is unused aside from setting
>> DATA_COMPRESS for DSC.
>>
>> Since setup_timing_engine() is only used for video mode, the
>> corresponding changes will be made in the DSC v1.2 for DP changes.
> 
> Ack, that makes sense.  However, is this a guarantee that nothing else
> will write INTF_CONFIG2 in the future, or will we solve that problem
> when it happens?  I'm afraid more config-bits get added to this register
> in the future and might possibly race/overwrite each other.

That's a fair point. There's no guarantee that nothing else will set 
INTF_CONFIG2 for command mode in the future. I think it would be better 
to add a register read now instead of having to fix that issue in a 
future change.

Thanks,

Jessica Zhang

> 
> - Marijn
> 
> <snip>