[PATCH v4 0/4] Reserve DSPPs based on user request

[PATCH v4 0/4] Reserve DSPPs based on user request

[Kalyan Thota]

This series will enable color features on sc7280 target which has 
primary panel as eDP

The series removes DSPP allocation based on encoder type and allows 
the DSPP reservation based on user request via CTM.

The series will release/reserve the dpu resources whenever there is 
a CTM enable/disable change so that DSPPs are allocated appropriately.

Kalyan Thota (4):
  drm/msm/dpu: clear DSPP reservations in rm release
  drm/msm/dpu: add DSPPs into reservation upon a CTM request
  drm/msm/dpu: avoid unnecessary check in DPU reservations
  drm/msm/dpu: manage DPU resources if CTM is requested

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 38 ++++++++++++-----------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  2 ++
 drivers/gpu/drm/msm/msm_atomic.c            | 18 ++++++++++++++
 drivers/gpu/drm/msm/msm_drv.c               |  2 +-
 drivers/gpu/drm/msm/msm_drv.h               |  1 +
 5 files changed, 38 insertions(+), 23 deletions(-)

-- 
2.7.4

[PATCH v4 1/4] drm/msm/dpu: clear DSPP reservations in rm release

[Kalyan Thota]

Clear DSPP reservations from the global state during
rm release

Fixes: e47616df008b ("drm/msm/dpu: add support for color processing blocks in dpu driver")
Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 73b3442..718ea0a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -572,6 +572,8 @@ void dpu_rm_release(struct dpu_global_state *global_state,
 		ARRAY_SIZE(global_state->ctl_to_enc_id), enc->base.id);
 	_dpu_rm_clear_mapping(global_state->dsc_to_enc_id,
 		ARRAY_SIZE(global_state->dsc_to_enc_id), enc->base.id);
+	_dpu_rm_clear_mapping(global_state->dspp_to_enc_id,
+		ARRAY_SIZE(global_state->dspp_to_enc_id), enc->base.id);
 }
 
 int dpu_rm_reserve(
-- 
2.7.4

[PATCH v4 2/4] drm/msm/dpu: add DSPPs into reservation upon a CTM request

[Kalyan Thota]

Add DSPP blocks into the topology for reservation, if there
is a CTM request for that composition.

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Changes in v1:
- Minor nits (Dmitry)

Changes in v2:
- Populate DSPPs into the reservation only if CTM is requested (Dmitry)
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c6817b..46d2a5c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
 static struct msm_display_topology dpu_encoder_get_topology(
 			struct dpu_encoder_virt *dpu_enc,
 			struct dpu_kms *dpu_kms,
-			struct drm_display_mode *mode)
+			struct drm_display_mode *mode,
+			struct drm_crtc_state *crtc_state)
 {
 	struct msm_display_topology topology = {0};
 	int i, intf_count = 0;
@@ -563,8 +564,7 @@ static struct msm_display_topology dpu_encoder_get_topology(
 	 * 1 LM, 1 INTF
 	 * 2 LM, 1 INTF (stream merge to support high resolution interfaces)
 	 *
-	 * Adding color blocks only to primary interface if available in
-	 * sufficient number
+	 * Add dspps to the reservation requirements if ctm is requested
 	 */
 	if (intf_count == 2)
 		topology.num_lm = 2;
@@ -573,11 +573,8 @@ static struct msm_display_topology dpu_encoder_get_topology(
 	else
 		topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
 
-	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
-		if (dpu_kms->catalog->dspp &&
-			(dpu_kms->catalog->dspp_count >= topology.num_lm))
-			topology.num_dspp = topology.num_lm;
-	}
+	if (crtc_state->ctm)
+		topology.num_dspp = topology.num_lm;
 
 	topology.num_enc = 0;
 	topology.num_intf = intf_count;
@@ -643,7 +640,7 @@ static int dpu_encoder_virt_atomic_check(
 		}
 	}
 
-	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
+	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
 
 	/* Reserve dynamic resources now. */
 	if (!ret) {
-- 
2.7.4

[PATCH v4 3/4] drm/msm/dpu: avoid unnecessary check in DPU reservations

[Kalyan Thota]

Return immediately on failure, this will make dpu reservations
part look cleaner.

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 46d2a5c..3920efd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -636,25 +636,22 @@ static int dpu_encoder_virt_atomic_check(
 		if (ret) {
 			DPU_ERROR_ENC(dpu_enc,
 					"mode unsupported, phys idx %d\n", i);
-			break;
+			return ret;
 		}
 	}
 
 	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
 
-	/* Reserve dynamic resources now. */
-	if (!ret) {
-		/*
-		 * Release and Allocate resources on every modeset
-		 * Dont allocate when active is false.
-		 */
-		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
-			dpu_rm_release(global_state, drm_enc);
+	/*
+	 * Release and Allocate resources on every modeset
+	 * Dont allocate when active is false.
+	 */
+	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
+		dpu_rm_release(global_state, drm_enc);
 
-			if (!crtc_state->active_changed || crtc_state->active)
-				ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
-						drm_enc, crtc_state, topology);
-		}
+		if (!crtc_state->active_changed || crtc_state->active)
+			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
+					drm_enc, crtc_state, topology);
 	}
 
 	trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
-- 
2.7.4

[PATCH v4 4/4] drm/msm/dpu: manage DPU resources if CTM is requested

[Kalyan Thota]

Allow modeset to be triggered during CTM enable/disable.
In the modeset callbacks, DPU resources required for the
CTM feature are managed appropriately.

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
 drivers/gpu/drm/msm/msm_atomic.c | 18 ++++++++++++++++++
 drivers/gpu/drm/msm/msm_drv.c    |  2 +-
 drivers/gpu/drm/msm/msm_drv.h    |  1 +
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 1686fbb..e3e607c 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -179,6 +179,24 @@ static unsigned get_crtc_mask(struct drm_atomic_state *state)
 	return mask;
 }
 
+int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	struct drm_crtc *crtc;
+	int i;
+
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
+				      new_crtc_state, i) {
+		if ((old_crtc_state->ctm && !new_crtc_state->ctm) ||
+		    (!old_crtc_state->ctm && new_crtc_state->ctm)) {
+			new_crtc_state->mode_changed = true;
+			state->allow_modeset = true;
+		}
+	}
+
+	return drm_atomic_helper_check(dev, state);
+}
+
 void msm_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 0759e2d..3221284 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -52,7 +52,7 @@
 static const struct drm_mode_config_funcs mode_config_funcs = {
 	.fb_create = msm_framebuffer_create,
 	.output_poll_changed = drm_fb_helper_output_poll_changed,
-	.atomic_check = drm_atomic_helper_check,
+	.atomic_check = msm_atomic_check,
 	.atomic_commit = drm_atomic_helper_commit,
 };
 
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index ea80846..7d0243a 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -209,6 +209,7 @@ int msm_atomic_init_pending_timer(struct msm_pending_timer *timer,
 		struct msm_kms *kms, int crtc_idx);
 void msm_atomic_destroy_pending_timer(struct msm_pending_timer *timer);
 void msm_atomic_commit_tail(struct drm_atomic_state *state);
+int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state);
 struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev);
 void msm_atomic_state_clear(struct drm_atomic_state *state);
 void msm_atomic_state_free(struct drm_atomic_state *state);
-- 
2.7.4

Re: [PATCH v4 0/4] Reserve DSPPs based on user request

[Doug Anderson]

Hi,

On Mon, Feb 13, 2023 at 3:11 AM Kalyan Thota <quic_kalyant@quicinc.com> wrote:
>
> This series will enable color features on sc7280 target which has
> primary panel as eDP
>
> The series removes DSPP allocation based on encoder type and allows
> the DSPP reservation based on user request via CTM.
>
> The series will release/reserve the dpu resources whenever there is
> a CTM enable/disable change so that DSPPs are allocated appropriately.
>
> Kalyan Thota (4):
>   drm/msm/dpu: clear DSPP reservations in rm release
>   drm/msm/dpu: add DSPPs into reservation upon a CTM request
>   drm/msm/dpu: avoid unnecessary check in DPU reservations
>   drm/msm/dpu: manage DPU resources if CTM is requested
>
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 38 ++++++++++++-----------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  2 ++
>  drivers/gpu/drm/msm/msm_atomic.c            | 18 ++++++++++++++
>  drivers/gpu/drm/msm/msm_drv.c               |  2 +-
>  drivers/gpu/drm/msm/msm_drv.h               |  1 +
>  5 files changed, 38 insertions(+), 23 deletions(-)

For whatever reason when I use "b4 shazam" on your series it yells:

Patch failed at 0002 drm/msm/dpu: add DSPPs into reservation upon a CTM request
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
error: patch failed: drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:573
error: drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch

...but then I can apply it with `git am --show-current-patch=diff |
patch -p1` and it just applies with fuzz. Presumably Abhniav / Dmitry
can do the same but it would be nice if your patch applied cleanly to
msm-next.

In any case, I tried this patch on both a herobrine (sc7280) and
trogdor (sc7180) based board running msm-next (including the sc7280
patch [1]). In both cases the night light on the internal display
worked fine when using ChromeOS. External night light didn't work on
either of them (as expected) because we don't have the compositor
support yet.

I'm happy enough with:

Tested-by: Douglas Anderson <dianders@chromium.org>


[1] https://lore.kernel.org/r/1674814487-2112-1-git-send-email-quic_kalyant@quicinc.com/

Re: [PATCH v4 0/4] Reserve DSPPs based on user request

[Dmitry Baryshkov]

On Mon, 13 Feb 2023 at 20:12, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Feb 13, 2023 at 3:11 AM Kalyan Thota <quic_kalyant@quicinc.com> wrote:
> >
> > This series will enable color features on sc7280 target which has
> > primary panel as eDP
> >
> > The series removes DSPP allocation based on encoder type and allows
> > the DSPP reservation based on user request via CTM.
> >
> > The series will release/reserve the dpu resources whenever there is
> > a CTM enable/disable change so that DSPPs are allocated appropriately.
> >
> > Kalyan Thota (4):
> >   drm/msm/dpu: clear DSPP reservations in rm release
> >   drm/msm/dpu: add DSPPs into reservation upon a CTM request
> >   drm/msm/dpu: avoid unnecessary check in DPU reservations
> >   drm/msm/dpu: manage DPU resources if CTM is requested
> >
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 38 ++++++++++++-----------------
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  2 ++
> >  drivers/gpu/drm/msm/msm_atomic.c            | 18 ++++++++++++++
> >  drivers/gpu/drm/msm/msm_drv.c               |  2 +-
> >  drivers/gpu/drm/msm/msm_drv.h               |  1 +
> >  5 files changed, 38 insertions(+), 23 deletions(-)
>
> For whatever reason when I use "b4 shazam" on your series it yells:
>
> Patch failed at 0002 drm/msm/dpu: add DSPPs into reservation upon a CTM request
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> error: patch failed: drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:573
> error: drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c: patch does not apply
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
>
> ...but then I can apply it with `git am --show-current-patch=diff |
> patch -p1` and it just applies with fuzz. Presumably Abhniav / Dmitry
> can do the same but it would be nice if your patch applied cleanly to
> msm-next.

Hopefully it is not some 'tested on 5.15' story.

> In any case, I tried this patch on both a herobrine (sc7280) and
> trogdor (sc7180) based board running msm-next (including the sc7280
> patch [1]). In both cases the night light on the internal display
> worked fine when using ChromeOS. External night light didn't work on
> either of them (as expected) because we don't have the compositor
> support yet.
>
> I'm happy enough with:
>
> Tested-by: Douglas Anderson <dianders@chromium.org>

Thanks for the testing!


-- 
With best wishes
Dmitry

Re: [PATCH v4 4/4] drm/msm/dpu: manage DPU resources if CTM is requested

[Dmitry Baryshkov]

On 13/02/2023 13:11, Kalyan Thota wrote:
> Allow modeset to be triggered during CTM enable/disable.
> In the modeset callbacks, DPU resources required for the
> CTM feature are managed appropriately.
> 
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>   drivers/gpu/drm/msm/msm_atomic.c | 18 ++++++++++++++++++
>   drivers/gpu/drm/msm/msm_drv.c    |  2 +-
>   drivers/gpu/drm/msm/msm_drv.h    |  1 +
>   3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index 1686fbb..e3e607c 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -179,6 +179,24 @@ static unsigned get_crtc_mask(struct drm_atomic_state *state)
>   	return mask;
>   }
>   
> +int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	struct drm_crtc *crtc;
> +	int i;
> +

I hope this can be gone for good if at some point we have CRTC resource 
allocation split from encoder resource alloc.

> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> +				      new_crtc_state, i) {
> +		if ((old_crtc_state->ctm && !new_crtc_state->ctm) ||
> +		    (!old_crtc_state->ctm && new_crtc_state->ctm)) {
> +			new_crtc_state->mode_changed = true;
> +			state->allow_modeset = true;
> +		}
> +	}
> +
> +	return drm_atomic_helper_check(dev, state);
> +}
> +
>   void msm_atomic_commit_tail(struct drm_atomic_state *state)
>   {
>   	struct drm_device *dev = state->dev;
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 0759e2d..3221284 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -52,7 +52,7 @@
>   static const struct drm_mode_config_funcs mode_config_funcs = {
>   	.fb_create = msm_framebuffer_create,
>   	.output_poll_changed = drm_fb_helper_output_poll_changed,
> -	.atomic_check = drm_atomic_helper_check,
> +	.atomic_check = msm_atomic_check,
>   	.atomic_commit = drm_atomic_helper_commit,
>   };
>   
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index ea80846..7d0243a 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -209,6 +209,7 @@ int msm_atomic_init_pending_timer(struct msm_pending_timer *timer,
>   		struct msm_kms *kms, int crtc_idx);
>   void msm_atomic_destroy_pending_timer(struct msm_pending_timer *timer);
>   void msm_atomic_commit_tail(struct drm_atomic_state *state);
> +int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state);
>   struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev);
>   void msm_atomic_state_clear(struct drm_atomic_state *state);
>   void msm_atomic_state_free(struct drm_atomic_state *state);

-- 
With best wishes
Dmitry

Re: [PATCH v4 2/4] drm/msm/dpu: add DSPPs into reservation upon a CTM request

[Marijn Suijten]

On 2023-02-13 03:11:42, Kalyan Thota wrote:
> Add DSPP blocks into the topology for reservation, if there
> is a CTM request for that composition.
> 
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
> Changes in v1:
> - Minor nits (Dmitry)
> 
> Changes in v2:
> - Populate DSPPs into the reservation only if CTM is requested (Dmitry)
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 9c6817b..46d2a5c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
>  static struct msm_display_topology dpu_encoder_get_topology(
>  			struct dpu_encoder_virt *dpu_enc,
>  			struct dpu_kms *dpu_kms,
> -			struct drm_display_mode *mode)
> +			struct drm_display_mode *mode,
> +			struct drm_crtc_state *crtc_state)
>  {
>  	struct msm_display_topology topology = {0};
>  	int i, intf_count = 0;
> @@ -563,8 +564,7 @@ static struct msm_display_topology dpu_encoder_get_topology(
>  	 * 1 LM, 1 INTF
>  	 * 2 LM, 1 INTF (stream merge to support high resolution interfaces)
>  	 *
> -	 * Adding color blocks only to primary interface if available in
> -	 * sufficient number
> +	 * Add dspps to the reservation requirements if ctm is requested
>  	 */
>  	if (intf_count == 2)
>  		topology.num_lm = 2;
> @@ -573,11 +573,8 @@ static struct msm_display_topology dpu_encoder_get_topology(
>  	else
>  		topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
>  
> -	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
> -		if (dpu_kms->catalog->dspp &&
> -			(dpu_kms->catalog->dspp_count >= topology.num_lm))
> -			topology.num_dspp = topology.num_lm;
> -	}
> +	if (crtc_state->ctm)
> +		topology.num_dspp = topology.num_lm;
>  
>  	topology.num_enc = 0;
>  	topology.num_intf = intf_count;
> @@ -643,7 +640,7 @@ static int dpu_encoder_virt_atomic_check(
>  		}
>  	}
>  
> -	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
> +	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
>  
>  	/* Reserve dynamic resources now. */
>  	if (!ret) {
> -- 
> 2.7.4
> 

Re: [PATCH v4 3/4] drm/msm/dpu: avoid unnecessary check in DPU reservations

[Marijn Suijten]

On 2023-02-13 03:11:43, Kalyan Thota wrote:
> Return immediately on failure, this will make dpu reservations
> part look cleaner.
> 
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 46d2a5c..3920efd 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -636,25 +636,22 @@ static int dpu_encoder_virt_atomic_check(
>  		if (ret) {
>  			DPU_ERROR_ENC(dpu_enc,
>  					"mode unsupported, phys idx %d\n", i);
> -			break;
> +			return ret;
>  		}
>  	}
>  
>  	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
>  
> -	/* Reserve dynamic resources now. */
> -	if (!ret) {
> -		/*
> -		 * Release and Allocate resources on every modeset
> -		 * Dont allocate when active is false.
> -		 */
> -		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> -			dpu_rm_release(global_state, drm_enc);
> +	/*
> +	 * Release and Allocate resources on every modeset
> +	 * Dont allocate when active is false.
> +	 */
> +	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> +		dpu_rm_release(global_state, drm_enc);
>  
> -			if (!crtc_state->active_changed || crtc_state->active)
> -				ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> -						drm_enc, crtc_state, topology);
> -		}
> +		if (!crtc_state->active_changed || crtc_state->active)
> +			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> +					drm_enc, crtc_state, topology);
>  	}
>  
>  	trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
> -- 
> 2.7.4
> 

Re: (subset) [PATCH v4 1/4] drm/msm/dpu: clear DSPP reservations in rm release

[Abhinav Kumar]

On Mon, 13 Feb 2023 03:11:41 -0800, Kalyan Thota wrote:
> Clear DSPP reservations from the global state during
> rm release
> 
> 

Applied, thanks!

[1/4] drm/msm/dpu: clear DSPP reservations in rm release
      https://gitlab.freedesktop.org/drm/msm/-/commit/5ec498ba8655

Best regards,
-- 
Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH v4 0/4] Reserve DSPPs based on user request

[Dmitry Baryshkov]

On Mon, 13 Feb 2023 03:11:40 -0800, Kalyan Thota wrote:
> This series will enable color features on sc7280 target which has
> primary panel as eDP
> 
> The series removes DSPP allocation based on encoder type and allows
> the DSPP reservation based on user request via CTM.
> 
> The series will release/reserve the dpu resources whenever there is
> a CTM enable/disable change so that DSPPs are allocated appropriately.
> 
> [...]

Applied, thanks!

[2/4] drm/msm/dpu: add DSPPs into reservation upon a CTM request
      https://gitlab.freedesktop.org/lumag/msm/-/commit/1a9c3512fbd4
[3/4] drm/msm/dpu: avoid unnecessary check in DPU reservations
      https://gitlab.freedesktop.org/lumag/msm/-/commit/8b1ed0088e21
[4/4] drm/msm/dpu: manage DPU resources if CTM is requested
      https://gitlab.freedesktop.org/lumag/msm/-/commit/34c74e76a6a5

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Re: [PATCH v4 2/4] drm/msm/dpu: add DSPPs into reservation upon a CTM request

[Yongqin Liu]

Hi, Kalyan Thota

It seems this change caused some drm problems with the Android build.
I tested with one Android build that based on the ACK android-mainline
kernel[1], and there are the message printed like
    01-01 06:51:50.541   504   504 E         :
[drm:_dpu_rm_check_lm_and_get_connected_blks] [dpu error]failed to get
dspp on lm 0
    01-01 06:51:50.551   504   504 E         :
[drm:_dpu_rm_check_lm_and_get_connected_blks] [dpu error]failed to get
dspp on lm 0
    01-01 06:51:50.560   504   504 E         :
[drm:_dpu_rm_check_lm_and_get_connected_blks] [dpu error]failed to get
dspp on lm 0
    01-01 06:51:50.570   504   504 E         :
[drm:_dpu_rm_check_lm_and_get_connected_blks] [dpu error]failed to get
dspp on lm 0
    01-01 06:51:50.579   504   504 E         :
[drm:_dpu_rm_make_reservation] [dpu error]unable to find appropriate
mixers
    01-01 06:51:50.588   504   504 E         : [drm:dpu_rm_reserve]
[dpu error]failed to reserve hw resources: -119
For details, please check the link here: https://termbin.com/31lc

If I revert this commit, then the problem will be gone.
Could you please help check if there is any problem with this commit?

[1]: https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline

Thanks,
Yongqin Liu

On Wed, 1 Mar 2023 at 16:24, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2023-02-13 03:11:42, Kalyan Thota wrote:
> > Add DSPP blocks into the topology for reservation, if there
> > is a CTM request for that composition.
> >
> > Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>
> > ---
> > Changes in v1:
> > - Minor nits (Dmitry)
> >
> > Changes in v2:
> > - Populate DSPPs into the reservation only if CTM is requested (Dmitry)
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 9c6817b..46d2a5c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
> >  static struct msm_display_topology dpu_encoder_get_topology(
> >                       struct dpu_encoder_virt *dpu_enc,
> >                       struct dpu_kms *dpu_kms,
> > -                     struct drm_display_mode *mode)
> > +                     struct drm_display_mode *mode,
> > +                     struct drm_crtc_state *crtc_state)
> >  {
> >       struct msm_display_topology topology = {0};
> >       int i, intf_count = 0;
> > @@ -563,8 +564,7 @@ static struct msm_display_topology dpu_encoder_get_topology(
> >        * 1 LM, 1 INTF
> >        * 2 LM, 1 INTF (stream merge to support high resolution interfaces)
> >        *
> > -      * Adding color blocks only to primary interface if available in
> > -      * sufficient number
> > +      * Add dspps to the reservation requirements if ctm is requested
> >        */
> >       if (intf_count == 2)
> >               topology.num_lm = 2;
> > @@ -573,11 +573,8 @@ static struct msm_display_topology dpu_encoder_get_topology(
> >       else
> >               topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
> >
> > -     if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
> > -             if (dpu_kms->catalog->dspp &&
> > -                     (dpu_kms->catalog->dspp_count >= topology.num_lm))
> > -                     topology.num_dspp = topology.num_lm;
> > -     }
> > +     if (crtc_state->ctm)
> > +             topology.num_dspp = topology.num_lm;
> >
> >       topology.num_enc = 0;
> >       topology.num_intf = intf_count;
> > @@ -643,7 +640,7 @@ static int dpu_encoder_virt_atomic_check(
> >               }
> >       }
> >
> > -     topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
> > +     topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
> >
> >       /* Reserve dynamic resources now. */
> >       if (!ret) {
> > --
> > 2.7.4
> >



-- 
Best Regards,
Yongqin Liu
---------------------------------------------------------------
#mailing list
linaro-android@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-android

RE: [PATCH v4 2/4] drm/msm/dpu: add DSPPs into reservation upon a CTM request

[Kalyan Thota]

Thanks for your email. Can you help share some details.
1) https://termbin.com/31lc , I am unable to open the link in my office network, can you copy the logs in a text file and share with me.
2) Which DPU version are you trying from the catalogue, can you share the version ?
3) The patch will fail the atomic_check, if the commit is requesting for a CTM, and there are no dspp HW blocks to reserve. Can you check in your catalogue entry that you have sufficient dspp resources to LM 

Thanks,
Kalyan

>-----Original Message-----
>From: Yongqin Liu <yongqin.liu@linaro.org>
>Sent: Saturday, June 10, 2023 3:57 PM
>To: Marijn Suijten <marijn.suijten@somainline.org>; Kalyan Thota (QUIC)
><quic_kalyant@quicinc.com>
>Cc: dri-devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org;
>freedreno@lists.freedesktop.org; devicetree@vger.kernel.org; linux-
>kernel@vger.kernel.org; robdclark@chromium.org; dianders@chromium.org;
>swboyd@chromium.org; Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>;
>dmitry.baryshkov@linaro.org; Abhinav Kumar (QUIC)
><quic_abhinavk@quicinc.com>; Amit Pundir <amit.pundir@linaro.org>; Sumit
>Semwal <sumit.semwal@linaro.org>; John Stultz <jstultz@google.com>
>Subject: Re: [PATCH v4 2/4] drm/msm/dpu: add DSPPs into reservation upon a
>CTM request
>
>WARNING: This email originated from outside of Qualcomm. Please be wary of any
>links or attachments, and do not enable macros.
>
>Hi, Kalyan Thota
>
>It seems this change caused some drm problems with the Android build.
>I tested with one Android build that based on the ACK android-mainline kernel[1],
>and there are the message printed like
>    01-01 06:51:50.541   504   504 E         :
>[drm:_dpu_rm_check_lm_and_get_connected_blks] [dpu error]failed to get dspp
>on lm 0
>    01-01 06:51:50.551   504   504 E         :
>[drm:_dpu_rm_check_lm_and_get_connected_blks] [dpu error]failed to get dspp
>on lm 0
>    01-01 06:51:50.560   504   504 E         :
>[drm:_dpu_rm_check_lm_and_get_connected_blks] [dpu error]failed to get dspp
>on lm 0
>    01-01 06:51:50.570   504   504 E         :
>[drm:_dpu_rm_check_lm_and_get_connected_blks] [dpu error]failed to get dspp
>on lm 0
>    01-01 06:51:50.579   504   504 E         :
>[drm:_dpu_rm_make_reservation] [dpu error]unable to find appropriate mixers
>    01-01 06:51:50.588   504   504 E         : [drm:dpu_rm_reserve]
>[dpu error]failed to reserve hw resources: -119 For details, please check the link
>here: https://termbin.com/31lc
>
>If I revert this commit, then the problem will be gone.
>Could you please help check if there is any problem with this commit?
>
>[1]: https://android.googlesource.com/kernel/common/+/refs/heads/android-
>mainline
>
>Thanks,
>Yongqin Liu
>
>On Wed, 1 Mar 2023 at 16:24, Marijn Suijten <marijn.suijten@somainline.org>
>wrote:
>>
>> On 2023-02-13 03:11:42, Kalyan Thota wrote:
>> > Add DSPP blocks into the topology for reservation, if there is a CTM
>> > request for that composition.
>> >
>> > Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>
>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>
>> > ---
>> > Changes in v1:
>> > - Minor nits (Dmitry)
>> >
>> > Changes in v2:
>> > - Populate DSPPs into the reservation only if CTM is requested
>> > (Dmitry)
>> > ---
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 15 ++++++---------
>> >  1 file changed, 6 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > index 9c6817b..46d2a5c 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct
>> > drm_encoder *drm_enc)  static struct msm_display_topology
>dpu_encoder_get_topology(
>> >                       struct dpu_encoder_virt *dpu_enc,
>> >                       struct dpu_kms *dpu_kms,
>> > -                     struct drm_display_mode *mode)
>> > +                     struct drm_display_mode *mode,
>> > +                     struct drm_crtc_state *crtc_state)
>> >  {
>> >       struct msm_display_topology topology = {0};
>> >       int i, intf_count = 0;
>> > @@ -563,8 +564,7 @@ static struct msm_display_topology
>dpu_encoder_get_topology(
>> >        * 1 LM, 1 INTF
>> >        * 2 LM, 1 INTF (stream merge to support high resolution interfaces)
>> >        *
>> > -      * Adding color blocks only to primary interface if available in
>> > -      * sufficient number
>> > +      * Add dspps to the reservation requirements if ctm is
>> > + requested
>> >        */
>> >       if (intf_count == 2)
>> >               topology.num_lm = 2;
>> > @@ -573,11 +573,8 @@ static struct msm_display_topology
>dpu_encoder_get_topology(
>> >       else
>> >               topology.num_lm = (mode->hdisplay >
>> > MAX_HDISPLAY_SPLIT) ? 2 : 1;
>> >
>> > -     if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
>> > -             if (dpu_kms->catalog->dspp &&
>> > -                     (dpu_kms->catalog->dspp_count >= topology.num_lm))
>> > -                     topology.num_dspp = topology.num_lm;
>> > -     }
>> > +     if (crtc_state->ctm)
>> > +             topology.num_dspp = topology.num_lm;
>> >
>> >       topology.num_enc = 0;
>> >       topology.num_intf = intf_count; @@ -643,7 +640,7 @@ static int
>> > dpu_encoder_virt_atomic_check(
>> >               }
>> >       }
>> >
>> > -     topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
>> > +     topology = dpu_encoder_get_topology(dpu_enc, dpu_kms,
>> > + adj_mode, crtc_state);
>> >
>> >       /* Reserve dynamic resources now. */
>> >       if (!ret) {
>> > --
>> > 2.7.4
>> >
>
>
>
>--
>Best Regards,
>Yongqin Liu
>---------------------------------------------------------------
>#mailing list
>linaro-android@lists.linaro.org
>http://lists.linaro.org/mailman/listinfo/linaro-android