diff mbox series

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

Message ID 20230830224910.8091-14-quic_abhinavk@quicinc.com (mailing list archive)
State Not Applicable
Headers show
Series [01/16] drm/msm/dpu: fix writeback programming for YUV cases | expand

Commit Message

Abhinav Kumar Aug. 30, 2023, 10:49 p.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.

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

Comments

Dmitry Baryshkov Aug. 31, 2023, 12:23 a.m. UTC | #1 
On Thu, 31 Aug 2023 at 01:50, 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.
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 35 ++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1b1e07292a9e..7a3d179bdfba 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"
> @@ -615,9 +616,11 @@ 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;
> +       bool needs_cdm = false;
>
>         if (!drm_enc || !crtc_state || !conn_state) {
>                 DPU_ERROR("invalid arg(s), drm_enc %d, crtc/conn state %d/%d\n",
> @@ -655,6 +658,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))))
> +                       needs_cdm = true;
> +               if (needs_cdm && !dpu_enc->cur_master->hw_cdm)
> +                       crtc_state->mode_changed = true;
> +               else if (!needs_cdm && dpu_enc->cur_master->hw_cdm)
> +                       crtc_state->mode_changed = true;
> +       }

What would be the (estimated) check for DP?

> +
>         /*
>          * Release and Allocate resources on every modeset
>          * Dont allocate when active is false.
> @@ -664,7 +683,7 @@ static int dpu_encoder_virt_atomic_check(
>
>                 if (!crtc_state->active_changed || crtc_state->enable)
>                         ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> -                                       drm_enc, crtc_state, topology, false);
> +                                       drm_enc, crtc_state, topology, needs_cdm);
>         }
>
>         trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
> @@ -1126,6 +1145,20 @@ 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;
> +               struct drm_framebuffer *fb;
> +
> +               fb = conn_state->writeback_job->fb;
> +
> +               if (fb && DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(fb)))) {

You can drop all fb-related conditions here. If we have CDM, we know
that we asked for it. If we do not, it's because we do not need it.

> +                       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
>
Abhinav Kumar Dec. 1, 2023, 12:45 a.m. UTC | #2 
On 8/30/2023 5:23 PM, Dmitry Baryshkov wrote:
> On Thu, 31 Aug 2023 at 01:50, 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.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 35 ++++++++++++++++++++-
>>   1 file changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 1b1e07292a9e..7a3d179bdfba 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"
>> @@ -615,9 +616,11 @@ 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;
>> +       bool needs_cdm = false;
>>
>>          if (!drm_enc || !crtc_state || !conn_state) {
>>                  DPU_ERROR("invalid arg(s), drm_enc %d, crtc/conn state %d/%d\n",
>> @@ -655,6 +658,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))))
>> +                       needs_cdm = true;
>> +               if (needs_cdm && !dpu_enc->cur_master->hw_cdm)
>> +                       crtc_state->mode_changed = true;
>> +               else if (!needs_cdm && dpu_enc->cur_master->hw_cdm)
>> +                       crtc_state->mode_changed = true;
>> +       }
> 
> What would be the (estimated) check for DP?
> 

Originally we were planning a lot more but now we are going to start 
with the mode being drm_mode_is_420_only and use CDM for that.

>> +
>>          /*
>>           * Release and Allocate resources on every modeset
>>           * Dont allocate when active is false.
>> @@ -664,7 +683,7 @@ static int dpu_encoder_virt_atomic_check(
>>
>>                  if (!crtc_state->active_changed || crtc_state->enable)
>>                          ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
>> -                                       drm_enc, crtc_state, topology, false);
>> +                                       drm_enc, crtc_state, topology, needs_cdm);
>>          }
>>
>>          trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
>> @@ -1126,6 +1145,20 @@ 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;
>> +               struct drm_framebuffer *fb;
>> +
>> +               fb = conn_state->writeback_job->fb;
>> +
>> +               if (fb && DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(fb)))) {
> 
> You can drop all fb-related conditions here. If we have CDM, we know
> that we asked for it. If we do not, it's because we do not need it.
> 

hmmm .... let me do some testing with this and if it works I will make 
this change as well. Thanks.

>> +                       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
>>
> 
>
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 1b1e07292a9e..7a3d179bdfba 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"
@@ -615,9 +616,11 @@  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;
+	bool needs_cdm = false;
 
 	if (!drm_enc || !crtc_state || !conn_state) {
 		DPU_ERROR("invalid arg(s), drm_enc %d, crtc/conn state %d/%d\n",
@@ -655,6 +658,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))))
+			needs_cdm = true;
+		if (needs_cdm && !dpu_enc->cur_master->hw_cdm)
+			crtc_state->mode_changed = true;
+		else if (!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.
@@ -664,7 +683,7 @@  static int dpu_encoder_virt_atomic_check(
 
 		if (!crtc_state->active_changed || crtc_state->enable)
 			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
-					drm_enc, crtc_state, topology, false);
+					drm_enc, crtc_state, topology, needs_cdm);
 	}
 
 	trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
@@ -1126,6 +1145,20 @@  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;
+		struct drm_framebuffer *fb;
+
+		fb = conn_state->writeback_job->fb;
+
+		if (fb && DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(fb)))) {
+			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++) {