linux-kernel - Re: [PATCH v4 15/25] drm/msm/dpu: Add CWB to msm_display

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: <e47b33e6-142d-42d6-8ae9-f2d2c7793d18@quicinc.com>
Date: Thu, 9 Jan 2025 14:34:44 -0800
From: Jessica Zhang <quic_jesszhan@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Rob Clark <robdclark@...il.com>, <quic_abhinavk@...cinc.com>,
        Sean Paul
	<sean@...rly.run>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        "David
 Airlie" <airlied@...il.com>,
        Maarten Lankhorst
	<maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Simona Vetter <simona@...ll.ch>,
        Simona Vetter <simona.vetter@...ll.ch>, <quic_ebharadw@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
        <freedreno@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
        Rob Clark
	<robdclark@...omium.org>,
        Ville Syrjälä
	<ville.syrjala@...ux.intel.com>
Subject: Re: [PATCH v4 15/25] drm/msm/dpu: Add CWB to msm_display_topology



On 1/3/2025 10:16 AM, Dmitry Baryshkov wrote:
> On Fri, Jan 03, 2025 at 10:03:35AM -0800, Jessica Zhang wrote:
>>
>>
>> On 12/19/2024 9:03 PM, Dmitry Baryshkov wrote:
>>> On Mon, Dec 16, 2024 at 04:43:26PM -0800, Jessica Zhang wrote:
>>>> Add the cwb_enabled flag to msm_display topology and adjust the toplogy
>>>> to account for concurrent writeback
>>>
>>> Why?
>>
>> Hi Dmitry,
>>
>> This flag is necessary to specify that CWB mux(es) need to be assigned for
>> the given reqeusted topology.
> 
> Why is necessary? Please rephrase your statement (we need foo bar, so do
> baz).

Ack, what do you think of rephrasing the commit msg to this:

```
Add support for adjusting topology based on if concurrent writeback is 
enabled.

Currently, the topology is calculated based on the assumption that the 
user cannot request real-time and writeback simultaneously. For example, 
the number of LMs and CTLs are currently based off the number of phys 
encoders under the assumption there will be at least 1 LM/CTL per phys 
encoder.

This will not hold true for concurrent writeback as 2 phys encoders (1 
real-time and 1 writeback) can be driven by 1 LM/CTL when concurrent 
writeback is enabled.

To account for this, add a cwb_enabled flag and only adjust the number 
of CTL/LMs needed by a given topology based on the number of phys 
encoders only if CWB is not enabled.

```

> 
>>
>>>
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@...cinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 11 ++++++++++-
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c   | 10 ++++++++--
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h   |  2 ++
>>>>    3 files changed, 20 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> index b4bfded3d53025853cee112ca598533ece290318..b063c8fe4c0594772d84401fa56c9c21afc0ad18 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> @@ -1198,6 +1198,8 @@ static struct msm_display_topology dpu_crtc_get_topology(
>>>>    		dpu_encoder_update_topology(drm_enc, &topology, crtc_state->state,
>>>>    					    &crtc_state->adjusted_mode);
>>>> +	topology.cwb_enabled = drm_crtc_in_clone_mode(crtc_state);
>>>> +
>>>>    	/*
>>>>    	 * Datapath topology selection
>>>>    	 *
>>>> @@ -1209,9 +1211,16 @@ static struct msm_display_topology dpu_crtc_get_topology(
>>>>    	 * 2 LM, 1 INTF (stream merge to support high resolution interfaces)
>>>>    	 *
>>>>    	 * Add dspps to the reservation requirements if ctm is requested
>>>> +	 *
>>>> +	 * Only hardcode num_lm to 2 for cases where num_intf == 2 and CWB is not
>>>> +	 * enabled. This is because in cases where CWB is enabled, num_intf will
>>>> +	 * count both the WB and real-time phys encoders.
>>>> +	 *
>>>> +	 * For non-DSC CWB usecases, have the num_lm be decided by the
>>>> +	 * (mode->hdisplay > MAX_HDISPLAY_SPLIT) check.
>>>>    	 */
>>>> -	if (topology.num_intf == 2)
>>>> +	if (topology.num_intf == 2 && !topology.cwb_enabled)
>>>>    		topology.num_lm = 2;
>>>>    	else if (topology.num_dsc == 2)
>>>>    		topology.num_lm = 2;
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> index b763ef19f4c60ae8a35df6a6ffb19e8411bc63f8..85adaf256b2c705d2d7df378b6ffc0e578f52bc3 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> @@ -382,8 +382,14 @@ static int _dpu_rm_reserve_ctls(
>>>>    	int i = 0, j, num_ctls;
>>>>    	bool needs_split_display;
>>>> -	/* each hw_intf needs its own hw_ctrl to program its control path */
>>>> -	num_ctls = top->num_intf;
>>>> +	/*
>>>> +	 * For non-CWB mode, each hw_intf needs its own hw_ctl to program its
>>>> +	 * control path. Hardcode num_ctls to 1 if CWB is enabled
>>>> +	 */
>>>
>>> Why?
>>
>> This is because num_intf is based on the number of phys_encs. Since in the
>> CWB case, the WB and real-time encoders will be driven by the same CTL. I
>> can add this to the comment doc.
> 
> Why are they driven by the same CTL? Is it also the case for platforms
> before DPU 5.x?

This is because the WB and real-time path for a given topology would be 
driven by the same data path so the same CTL should enable the real-time 
and WB active bits.

This is the same for pre-DPU 5.x.

> 
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>> +	if (top->cwb_enabled)
>>>> +		num_ctls = 1;
>>>> +	else
>>>> +		num_ctls = top->num_intf;
>>>>    	needs_split_display = _dpu_rm_needs_split_display(top);
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> index b061dfdab52e04ab7d777e912a30173273cb3db7..12db21a2403ec6930894c36a58e898c5d94c2568 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> @@ -46,6 +46,7 @@ struct dpu_rm {
>>>>     * @num_dspp:     number of dspp blocks used
>>>>     * @num_dsc:      number of Display Stream Compression (DSC) blocks used
>>>>     * @needs_cdm:    indicates whether cdm block is needed for this display topology
>>>> + * @cwb_enabled:  indicates whether CWB is enabled for this display topology
>>>>     */
>>>>    struct msm_display_topology {
>>>>    	u32 num_lm;
>>>> @@ -53,6 +54,7 @@ struct msm_display_topology {
>>>>    	u32 num_dspp;
>>>>    	u32 num_dsc;
>>>>    	bool needs_cdm;
>>>> +	bool cwb_enabled;
>>>>    };
>>>>    int dpu_rm_init(struct drm_device *dev,
>>>>
>>>> -- 
>>>> 2.34.1
>>>>
>>>
>>> -- 
>>> With best wishes
>>> Dmitry
>>
> 
> -- 
> With best wishes
> Dmitry