linux-kernel - Re: [PATCH v2] drm/msm/dpu: don't always activate merge

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <098b51a8-0165-4d14-9365-ef56ea2bcfae@quicinc.com>
Date: Mon, 14 Oct 2024 13:23:24 -0700
From: Jessica Zhang <quic_jesszhan@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Abhinav Kumar
	<quic_abhinavk@...cinc.com>
CC: Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
        "Marijn
 Suijten" <marijn.suijten@...ainline.org>,
        David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
        Rob Clark <robdclark@...omium.org>, <linux-arm-msm@...r.kernel.org>,
        <dri-devel@...ts.freedesktop.org>, <freedreno@...ts.freedesktop.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] drm/msm/dpu: don't always activate merge_3d block



On 10/14/2024 12:13 AM, Dmitry Baryshkov wrote:
> On Sun, Oct 13, 2024 at 07:37:20PM -0700, Abhinav Kumar wrote:
>> Hi Dmitry
>>
>> On 10/13/2024 5:20 PM, Dmitry Baryshkov wrote:
>>> On Fri, Oct 11, 2024 at 10:25:13AM -0700, Jessica Zhang wrote:
>>>> Only enable the merge_3d block for the video phys encoder when the 3d
>>>> blend mode is not *_NONE since there is no need to activate the merge_3d
>>>> block for cases where merge_3d is not needed.
>>>>
>>>> Fixes: 3e79527a33a8 ("drm/msm/dpu: enable merge_3d support on sm8150/sm8250")
>>>> Suggested-by: Abhinav Kumar <quic_abhinavk@...cinc.com>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@...cinc.com>
>>>> ---
>>>> Changes in v2:
>>>> - Added more detailed commit message
>>>> - Link to v1: https://lore.kernel.org/r/20241009-merge3d-fix-v1-1-0d0b6f5c244e@quicinc.com
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> LGTM now. Please clarify, is there any dependency between this patch and
>>> [1]
>>>
>>
>> No dependency as such. Both are tackling similar issues though. One for
>> video mode and the other for writeback thats all. Namely:
>>
>> 1) We should not be enabling merge_3d block if two LMs are not being used as
>> that block needs to be enabled only to merge two streams. If its always
>> enabled, its incorrect programming because as per the docs its mentioned "if
>> required". Even if thats not causing issues, I would prefer not to enable it
>> always due to the "if required" clause and also we dont need to enable a
>> hardware sub-block unnecessarily.
>>
>> 2) We should be flushing the merge_3d only if its active like Jessica wrote
>> in the commit message of [1]. Otherwise, the flush bit will never be taken
>> by hardware leading to the false timeout errors.
>>
>> It has been sent as two patches as one is for video mode and the other for
>> writeback and for writeback it includes both (1) and (2) together in the
>> same patch.
> 
> I think it's better to handle (1) in a single patch (both for video and
> WB) and (2) in another patch. This way it becomes more obvious that WB
> had two different independent issues issues.

Hi Dmitry,

Just to clarify, the patches are already being split this way.

Thanks,

Jessica Zhang

> 
>>
>> I thought this separation is fine, if we need to squash it, let me know.
>>
>> Thanks
>>
>> Abhinav
>>
>>> [1] https://lore.kernel.org/dri-devel/20241009-mode3d-fix-v1-1-c0258354fadc@quicinc.com/
>>>
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>> index ba8878d21cf0e1945a393cca806cb64f03b16640..c5e27eeaff0423a69fad98122ffef7e041fbc68e 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>> @@ -302,7 +302,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
>>>>    	intf_cfg.stream_sel = 0; /* Don't care value for video mode */
>>>>    	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>>>    	intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>>> -	if (phys_enc->hw_pp->merge_3d)
>>>> +	if (intf_cfg.mode_3d && phys_enc->hw_pp->merge_3d)
>>>>    		intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
>>>>    	spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
>>>>
>>>> ---
>>>> base-commit: a20a91fb1bfac5d05ec5bcf9afe0c9363f6c8c93
>>>> change-id: 20240828-merge3d-fix-1a8d005e3277
>>>>
>>>> Best regards,
>>>> -- 
>>>> Jessica Zhang <quic_jesszhan@...cinc.com>
>>>>
>>>
> 
> -- 
> With best wishes
> Dmitry