diff mbox series

[v2,14/16] drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output

Message ID 20231208050641.32582-15-quic_abhinavk@quicinc.com
State New
Headers
Series [v2,01/16] drm/msm/dpu: add formats check for writeback encoder |

Commit Message

Abhinav Kumar Dec. 8, 2023, 5:06 a.m. UTC 
  Reserve CDM blocks for writeback if the format of the output fb
is YUV. At the moment, the reservation is done only for writeback
but can easily be extended by relaxing the checks once other
interfaces are ready to output YUV.

changes in v2:
	- use needs_cdm from topology struct
	- drop fb related checks from atomic_mode_set()

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Dmitry Baryshkov Dec. 8, 2023, 11:54 a.m. UTC | #1 
On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Reserve CDM blocks for writeback if the format of the output fb
> is YUV. At the moment, the reservation is done only for writeback
> but can easily be extended by relaxing the checks once other
> interfaces are ready to output YUV.
>
> changes in v2:
>         - use needs_cdm from topology struct
>         - drop fb related checks from atomic_mode_set()

It looks like this should be squashed with the patch 11. The 'unbind
CDM' doesn't really make sense without this patch. We need to allocate
it first,  before touching it.

>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 862912727925..a576e3e62429 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -16,6 +16,7 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_probe_helper.h>
> +#include <drm/drm_framebuffer.h>
>
>  #include "msm_drv.h"
>  #include "dpu_kms.h"
> @@ -583,6 +584,7 @@ static int dpu_encoder_virt_atomic_check(
>         struct drm_display_mode *adj_mode;
>         struct msm_display_topology topology;
>         struct dpu_global_state *global_state;
> +       struct drm_framebuffer *fb;
>         struct drm_dsc_config *dsc;
>         int i = 0;
>         int ret = 0;
> @@ -623,6 +625,22 @@ static int dpu_encoder_virt_atomic_check(
>
>         topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, dsc);
>
> +       /*
> +        * Use CDM only for writeback at the moment as other interfaces cannot handle it.
> +        * if writeback itself cannot handle cdm for some reason it will fail in its atomic_check()
> +        * earlier.
> +        */
> +       if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
> +               fb = conn_state->writeback_job->fb;
> +
> +               if (fb && DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(fb))))
> +                       topology.needs_cdm = true;
> +               if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
> +                       crtc_state->mode_changed = true;
> +               else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> +                       crtc_state->mode_changed = true;
> +       }
> +
>         /*
>          * Release and Allocate resources on every modeset
>          * Dont allocate when active is false.
> @@ -1063,6 +1081,15 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>
>         dpu_enc->dsc_mask = dsc_mask;
>
> +       if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
> +               struct dpu_hw_blk *hw_cdm = NULL;
> +
> +               dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> +                                             drm_enc->base.id, DPU_HW_BLK_CDM,
> +                                             &hw_cdm, 1);
> +               dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL;
> +       }
> +
>         cstate = to_dpu_crtc_state(crtc_state);
>
>         for (i = 0; i < num_lm; i++) {
> --
> 2.40.1
>


--
With best wishes

Dmitry
Abhinav Kumar Dec. 8, 2023, 4:33 p.m. UTC | #2 
On 12/8/2023 3:54 AM, Dmitry Baryshkov wrote:
> On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Reserve CDM blocks for writeback if the format of the output fb
>> is YUV. At the moment, the reservation is done only for writeback
>> but can easily be extended by relaxing the checks once other
>> interfaces are ready to output YUV.
>>
>> changes in v2:
>>          - use needs_cdm from topology struct
>>          - drop fb related checks from atomic_mode_set()
> 
> It looks like this should be squashed with the patch 11. The 'unbind
> CDM' doesn't really make sense without this patch. We need to allocate
> it first,  before touching it.
> 

The way I was thinking was that patch just completes the 
dpu_encoder_phys_cleanup() and yes it was intentionally kept ahead 
because that will not kick in till hw_cdm is assigned.

Then, this patch only handles reserving/assignment of hw_cdm when needed.

That was the motivation behind this split.

>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 862912727925..a576e3e62429 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -16,6 +16,7 @@
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_file.h>
>>   #include <drm/drm_probe_helper.h>
>> +#include <drm/drm_framebuffer.h>
>>
>>   #include "msm_drv.h"
>>   #include "dpu_kms.h"
>> @@ -583,6 +584,7 @@ static int dpu_encoder_virt_atomic_check(
>>          struct drm_display_mode *adj_mode;
>>          struct msm_display_topology topology;
>>          struct dpu_global_state *global_state;
>> +       struct drm_framebuffer *fb;
>>          struct drm_dsc_config *dsc;
>>          int i = 0;
>>          int ret = 0;
>> @@ -623,6 +625,22 @@ static int dpu_encoder_virt_atomic_check(
>>
>>          topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, dsc);
>>
>> +       /*
>> +        * Use CDM only for writeback at the moment as other interfaces cannot handle it.
>> +        * if writeback itself cannot handle cdm for some reason it will fail in its atomic_check()
>> +        * earlier.
>> +        */
>> +       if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
>> +               fb = conn_state->writeback_job->fb;
>> +
>> +               if (fb && DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(fb))))
>> +                       topology.needs_cdm = true;
>> +               if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
>> +                       crtc_state->mode_changed = true;
>> +               else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
>> +                       crtc_state->mode_changed = true;
>> +       }
>> +
>>          /*
>>           * Release and Allocate resources on every modeset
>>           * Dont allocate when active is false.
>> @@ -1063,6 +1081,15 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>
>>          dpu_enc->dsc_mask = dsc_mask;
>>
>> +       if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
>> +               struct dpu_hw_blk *hw_cdm = NULL;
>> +
>> +               dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
>> +                                             drm_enc->base.id, DPU_HW_BLK_CDM,
>> +                                             &hw_cdm, 1);
>> +               dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL;
>> +       }
>> +
>>          cstate = to_dpu_crtc_state(crtc_state);
>>
>>          for (i = 0; i < num_lm; i++) {
>> --
>> 2.40.1
>>
> 
> 
> --
> With best wishes
> 
> Dmitry
Dmitry Baryshkov Dec. 8, 2023, 4:38 p.m. UTC | #3 
On Fri, 8 Dec 2023 at 18:34, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 12/8/2023 3:54 AM, Dmitry Baryshkov wrote:
> > On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >> Reserve CDM blocks for writeback if the format of the output fb
> >> is YUV. At the moment, the reservation is done only for writeback
> >> but can easily be extended by relaxing the checks once other
> >> interfaces are ready to output YUV.
> >>
> >> changes in v2:
> >>          - use needs_cdm from topology struct
> >>          - drop fb related checks from atomic_mode_set()
> >
> > It looks like this should be squashed with the patch 11. The 'unbind
> > CDM' doesn't really make sense without this patch. We need to allocate
> > it first,  before touching it.
> >
>
> The way I was thinking was that patch just completes the
> dpu_encoder_phys_cleanup() and yes it was intentionally kept ahead
> because that will not kick in till hw_cdm is assigned.
>
> Then, this patch only handles reserving/assignment of hw_cdm when needed.
>
> That was the motivation behind this split.

It leaves a leaf code that is not used at all. There is no need to
cleanup anything if it was not allocated. Please remove the split and
squash it with allocation.

>
> >>
> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++++++++++++++++++
> >>   1 file changed, 27 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> index 862912727925..a576e3e62429 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> @@ -16,6 +16,7 @@
> >>   #include <drm/drm_crtc.h>
> >>   #include <drm/drm_file.h>
> >>   #include <drm/drm_probe_helper.h>
> >> +#include <drm/drm_framebuffer.h>
> >>
> >>   #include "msm_drv.h"
> >>   #include "dpu_kms.h"
> >> @@ -583,6 +584,7 @@ static int dpu_encoder_virt_atomic_check(
> >>          struct drm_display_mode *adj_mode;
> >>          struct msm_display_topology topology;
> >>          struct dpu_global_state *global_state;
> >> +       struct drm_framebuffer *fb;
> >>          struct drm_dsc_config *dsc;
> >>          int i = 0;
> >>          int ret = 0;
> >> @@ -623,6 +625,22 @@ static int dpu_encoder_virt_atomic_check(
> >>
> >>          topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, dsc);
> >>
> >> +       /*
> >> +        * Use CDM only for writeback at the moment as other interfaces cannot handle it.
> >> +        * if writeback itself cannot handle cdm for some reason it will fail in its atomic_check()
> >> +        * earlier.
> >> +        */
> >> +       if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
> >> +               fb = conn_state->writeback_job->fb;
> >> +
> >> +               if (fb && DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(fb))))
> >> +                       topology.needs_cdm = true;
> >> +               if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
> >> +                       crtc_state->mode_changed = true;
> >> +               else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> >> +                       crtc_state->mode_changed = true;
> >> +       }
> >> +
> >>          /*
> >>           * Release and Allocate resources on every modeset
> >>           * Dont allocate when active is false.
> >> @@ -1063,6 +1081,15 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >>
> >>          dpu_enc->dsc_mask = dsc_mask;
> >>
> >> +       if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
> >> +               struct dpu_hw_blk *hw_cdm = NULL;
> >> +
> >> +               dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> >> +                                             drm_enc->base.id, DPU_HW_BLK_CDM,
> >> +                                             &hw_cdm, 1);
> >> +               dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL;
> >> +       }
> >> +
> >>          cstate = to_dpu_crtc_state(crtc_state);
> >>
> >>          for (i = 0; i < num_lm; i++) {
> >> --
> >> 2.40.1
> >>
> >
> >
> > --
> > With best wishes
> >
> > Dmitry
Abhinav Kumar Dec. 8, 2023, 4:50 p.m. UTC | #4 
On 12/8/2023 8:38 AM, Dmitry Baryshkov wrote:
> On Fri, 8 Dec 2023 at 18:34, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 12/8/2023 3:54 AM, Dmitry Baryshkov wrote:
>>> On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>> Reserve CDM blocks for writeback if the format of the output fb
>>>> is YUV. At the moment, the reservation is done only for writeback
>>>> but can easily be extended by relaxing the checks once other
>>>> interfaces are ready to output YUV.
>>>>
>>>> changes in v2:
>>>>           - use needs_cdm from topology struct
>>>>           - drop fb related checks from atomic_mode_set()
>>>
>>> It looks like this should be squashed with the patch 11. The 'unbind
>>> CDM' doesn't really make sense without this patch. We need to allocate
>>> it first,  before touching it.
>>>
>>
>> The way I was thinking was that patch just completes the
>> dpu_encoder_phys_cleanup() and yes it was intentionally kept ahead
>> because that will not kick in till hw_cdm is assigned.
>>
>> Then, this patch only handles reserving/assignment of hw_cdm when needed.
>>
>> That was the motivation behind this split.
> 
> It leaves a leaf code that is not used at all. There is no need to
> cleanup anything if it was not allocated. Please remove the split and
> squash it with allocation.
> 

Ack. No concerns with squashing them.

>>
>>>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++++++++++++++++++
>>>>    1 file changed, 27 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 862912727925..a576e3e62429 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -16,6 +16,7 @@
>>>>    #include <drm/drm_crtc.h>
>>>>    #include <drm/drm_file.h>
>>>>    #include <drm/drm_probe_helper.h>
>>>> +#include <drm/drm_framebuffer.h>
>>>>
>>>>    #include "msm_drv.h"
>>>>    #include "dpu_kms.h"
>>>> @@ -583,6 +584,7 @@ static int dpu_encoder_virt_atomic_check(
>>>>           struct drm_display_mode *adj_mode;
>>>>           struct msm_display_topology topology;
>>>>           struct dpu_global_state *global_state;
>>>> +       struct drm_framebuffer *fb;
>>>>           struct drm_dsc_config *dsc;
>>>>           int i = 0;
>>>>           int ret = 0;
>>>> @@ -623,6 +625,22 @@ static int dpu_encoder_virt_atomic_check(
>>>>
>>>>           topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, dsc);
>>>>
>>>> +       /*
>>>> +        * Use CDM only for writeback at the moment as other interfaces cannot handle it.
>>>> +        * if writeback itself cannot handle cdm for some reason it will fail in its atomic_check()
>>>> +        * earlier.
>>>> +        */
>>>> +       if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
>>>> +               fb = conn_state->writeback_job->fb;
>>>> +
>>>> +               if (fb && DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(fb))))
>>>> +                       topology.needs_cdm = true;
>>>> +               if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
>>>> +                       crtc_state->mode_changed = true;
>>>> +               else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
>>>> +                       crtc_state->mode_changed = true;
>>>> +       }
>>>> +
>>>>           /*
>>>>            * Release and Allocate resources on every modeset
>>>>            * Dont allocate when active is false.
>>>> @@ -1063,6 +1081,15 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>>>
>>>>           dpu_enc->dsc_mask = dsc_mask;
>>>>
>>>> +       if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
>>>> +               struct dpu_hw_blk *hw_cdm = NULL;
>>>> +
>>>> +               dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
>>>> +                                             drm_enc->base.id, DPU_HW_BLK_CDM,
>>>> +                                             &hw_cdm, 1);
>>>> +               dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL;
>>>> +       }
>>>> +
>>>>           cstate = to_dpu_crtc_state(crtc_state);
>>>>
>>>>           for (i = 0; i < num_lm; i++) {
>>>> --
>>>> 2.40.1
>>>>
>>>
>>>
>>> --
>>> With best wishes
>>>
>>> Dmitry
> 
> 
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 862912727925..a576e3e62429 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -16,6 +16,7 @@ 
 #include <drm/drm_crtc.h>
 #include <drm/drm_file.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/drm_framebuffer.h>
 
 #include "msm_drv.h"
 #include "dpu_kms.h"
@@ -583,6 +584,7 @@  static int dpu_encoder_virt_atomic_check(
 	struct drm_display_mode *adj_mode;
 	struct msm_display_topology topology;
 	struct dpu_global_state *global_state;
+	struct drm_framebuffer *fb;
 	struct drm_dsc_config *dsc;
 	int i = 0;
 	int ret = 0;
@@ -623,6 +625,22 @@  static int dpu_encoder_virt_atomic_check(
 
 	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, dsc);
 
+	/*
+	 * Use CDM only for writeback at the moment as other interfaces cannot handle it.
+	 * if writeback itself cannot handle cdm for some reason it will fail in its atomic_check()
+	 * earlier.
+	 */
+	if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
+		fb = conn_state->writeback_job->fb;
+
+		if (fb && DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(fb))))
+			topology.needs_cdm = true;
+		if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
+			crtc_state->mode_changed = true;
+		else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
+			crtc_state->mode_changed = true;
+	}
+
 	/*
 	 * Release and Allocate resources on every modeset
 	 * Dont allocate when active is false.
@@ -1063,6 +1081,15 @@  static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 
 	dpu_enc->dsc_mask = dsc_mask;
 
+	if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
+		struct dpu_hw_blk *hw_cdm = NULL;
+
+		dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
+					      drm_enc->base.id, DPU_HW_BLK_CDM,
+					      &hw_cdm, 1);
+		dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL;
+	}
+
 	cstate = to_dpu_crtc_state(crtc_state);
 
 	for (i = 0; i < num_lm; i++) {