Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b039a202-5415-29a5-6c27-ff22ecefb79e@quicinc.com>
Date:   Wed, 3 May 2023 17:09:47 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [PATCH 4/4] drm/msm/dpu: Enable compression for command mode
Content-Language: en-US
To:     Marijn Suijten <marijn.suijten@somainline.org>
CC:     Rob Clark <robdclark@gmail.com>,
        Abhinav Kumar <quic_abhinavk@quicinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Sean Paul <sean@poorly.run>, David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        <linux-arm-msm@vger.kernel.org>, <dri-devel@lists.freedesktop.org>,
        <freedreno@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>
References: <20230405-add-dsc-support-v1-0-6bc6f03ae735@quicinc.com>
 <20230405-add-dsc-support-v1-4-6bc6f03ae735@quicinc.com>
 <hxqxnfcydzyfrlvihmil3gecan6p6xyjw44gielu63ltgtqul7@xwvoprzofq6g>
 <d4b7a747-77a0-95eb-1201-c8b1c80defe3@quicinc.com>
 <rg6nlm4knw4hgtnwtvr4a5wqusyvvsetygiwxscp6bs6pun7ii@wzz6rd2y3vcn>
From:   Jessica Zhang <quic_jesszhan@quicinc.com>
In-Reply-To: <rg6nlm4knw4hgtnwtvr4a5wqusyvvsetygiwxscp6bs6pun7ii@wzz6rd2y3vcn>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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>