Add the cwb_enabled flag to msm_display topology and adjust the toplogy
to account for concurrent writeback
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.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
+ */
+ 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.1On 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?
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.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?
> + 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
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.
>
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.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.
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
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).
>
> >
> > >
> > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.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?
>
> 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
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@quicinc.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
On Thu, Jan 09, 2025 at 02:34:44PM -0800, Jessica Zhang wrote:
>
>
> 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@quicinc.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.
But pre-5.x platforms didn't have ACTIVE_CTL, so they should be using
separte CTL for each of the physical encoders.
>
> >
> > >
> > > 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
>
--
With best wishes
Dmitry
On 1/9/2025 4:00 PM, Dmitry Baryshkov wrote:
> On Thu, Jan 09, 2025 at 02:34:44PM -0800, Jessica Zhang wrote:
>>
>>
>> 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@quicinc.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.
>
> But pre-5.x platforms didn't have ACTIVE_CTL, so they should be using
> separte CTL for each of the physical encoders.
For pre-DPU 5.x, enabling CWB would mean configuring the registers under
both the WB and MODE_SEL_* cases here [1]
[1]
https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c#L588
>
>>
>>>
>>>>
>>>> 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
>>
>
> --
> With best wishes
> Dmitry
On Fri, 10 Jan 2025 at 02:30, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>
>
> On 1/9/2025 4:00 PM, Dmitry Baryshkov wrote:
> > On Thu, Jan 09, 2025 at 02:34:44PM -0800, Jessica Zhang wrote:
> >>
> >>
> >> 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@quicinc.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.
> >
> > But pre-5.x platforms didn't have ACTIVE_CTL, so they should be using
> > separte CTL for each of the physical encoders.
>
> For pre-DPU 5.x, enabling CWB would mean configuring the registers under
> both the WB and MODE_SEL_* cases here [1]
But do we still have to use a single CTL or would we use two different
CTLs, one for the main output and one for WB?
>
> [1]
> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c#L588
>
> >
> >>
> >>>
> >>>>
> >>>> 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
> >>
> >
> > --
> > With best wishes
> > Dmitry
>
--
With best wishes
Dmitry
On 1/9/2025 5:42 PM, Dmitry Baryshkov wrote:
> On Fri, 10 Jan 2025 at 02:30, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>>
>>
>> On 1/9/2025 4:00 PM, Dmitry Baryshkov wrote:
>>> On Thu, Jan 09, 2025 at 02:34:44PM -0800, Jessica Zhang wrote:
>>>>
>>>>
>>>> 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@quicinc.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.
>>>
>>> But pre-5.x platforms didn't have ACTIVE_CTL, so they should be using
>>> separte CTL for each of the physical encoders.
>>
>> For pre-DPU 5.x, enabling CWB would mean configuring the registers under
>> both the WB and MODE_SEL_* cases here [1]
>
> But do we still have to use a single CTL or would we use two different
> CTLs, one for the main output and one for WB?
We would have to enable both WB and the real-time output on the same CTL
>
>>
>> [1]
>> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c#L588
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> 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
>>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>>
>
>
> --
> With best wishes
> Dmitry
On Thu, Jan 09, 2025 at 05:50:16PM -0800, Jessica Zhang wrote:
>
>
> On 1/9/2025 5:42 PM, Dmitry Baryshkov wrote:
> > On Fri, 10 Jan 2025 at 02:30, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> > >
> > >
> > >
> > > On 1/9/2025 4:00 PM, Dmitry Baryshkov wrote:
> > > > On Thu, Jan 09, 2025 at 02:34:44PM -0800, Jessica Zhang wrote:
> > > > >
> > > > >
> > > > > 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@quicinc.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.
> > > >
> > > > But pre-5.x platforms didn't have ACTIVE_CTL, so they should be using
> > > > separte CTL for each of the physical encoders.
> > >
> > > For pre-DPU 5.x, enabling CWB would mean configuring the registers under
> > > both the WB and MODE_SEL_* cases here [1]
> >
> > But do we still have to use a single CTL or would we use two different
> > CTLs, one for the main output and one for WB?
>
> We would have to enable both WB and the real-time output on the same CTL
Thanks for the confirmation. Then the text your wrote above should be
mostly okay. Please drop the first ("Add support...") sentence and s/can
be driven by/must be driven by/ .
>
> >
> > >
> > > [1]
> > > https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c#L588
> > >
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > 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
> > > > >
> > > >
> > > > --
> > > > With best wishes
> > > > Dmitry
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry
>
--
With best wishes
Dmitry
On 1/9/2025 6:10 PM, Dmitry Baryshkov wrote:
> On Thu, Jan 09, 2025 at 05:50:16PM -0800, Jessica Zhang wrote:
>>
>>
>> On 1/9/2025 5:42 PM, Dmitry Baryshkov wrote:
>>> On Fri, 10 Jan 2025 at 02:30, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 1/9/2025 4:00 PM, Dmitry Baryshkov wrote:
>>>>> On Thu, Jan 09, 2025 at 02:34:44PM -0800, Jessica Zhang wrote:
>>>>>>
>>>>>>
>>>>>> 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@quicinc.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.
>>>>>
>>>>> But pre-5.x platforms didn't have ACTIVE_CTL, so they should be using
>>>>> separte CTL for each of the physical encoders.
>>>>
>>>> For pre-DPU 5.x, enabling CWB would mean configuring the registers under
>>>> both the WB and MODE_SEL_* cases here [1]
>>>
>>> But do we still have to use a single CTL or would we use two different
>>> CTLs, one for the main output and one for WB?
>>
>> We would have to enable both WB and the real-time output on the same CTL
>
> Thanks for the confirmation. Then the text your wrote above should be
> mostly okay. Please drop the first ("Add support...") sentence and s/can
> be driven by/must be driven by/ .
Ack, sounds good
>
>>
>>>
>>>>
>>>> [1]
>>>> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c#L588
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> 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
>>>>>>
>>>>>
>>>>> --
>>>>> With best wishes
>>>>> Dmitry
>>>>
>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>>
>
> --
> With best wishes
> Dmitry
© 2016 - 2025 Red Hat, Inc.