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

[PATCH v3 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 when ever there is 
a topology change to suit the new requirements.

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: reserve the resources on topology change

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  2 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 58 ++++++++++++++++-------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  2 +
 3 files changed, 37 insertions(+), 25 deletions(-)

-- 
2.7.4

[PATCH v3 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 v3 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 v3 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 v3 4/4] drm/msm/dpu: reserve the resources on topology change

[Kalyan Thota]

Some features like CTM can be enabled dynamically. Release
and reserve the DPU resources whenever a topology change
occurs such that required hw blocks are allocated appropriately.

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
Changes in v1:
- Avoid mode_set call directly (Dmitry)

Changes in v2:
- Minor nits (Dmitry)

Changes in v3:
- avoid unnecessary modeset check call (Dmitry)
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  2 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 +++++++++++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 539b68b..85bd5645 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -204,6 +204,7 @@ struct dpu_crtc {
  * @hw_ctls       : List of active ctl paths
  * @crc_source    : CRC source
  * @crc_frame_skip_count: Number of frames skipped before getting CRC
+ * @ctm_enabled   : Cached color management enablement state
  */
 struct dpu_crtc_state {
 	struct drm_crtc_state base;
@@ -225,6 +226,7 @@ struct dpu_crtc_state {
 
 	enum dpu_crtc_crc_source crc_source;
 	int crc_frame_skip_count;
+	bool ctm_enabled;
 };
 
 #define to_dpu_crtc_state(x) \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 3920efd..038e077 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -217,6 +217,19 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
 	15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
 };
 
+static bool _dpu_enc_is_dspp_changed(struct drm_crtc_state *crtc_state,
+	struct msm_display_topology topology)
+{
+	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
+
+	if ((cstate->ctm_enabled && !topology.num_dspp) ||
+	    (!cstate->ctm_enabled && topology.num_dspp)) {
+		crtc_state->mode_changed = true;
+		return true;
+	}
+
+	return false;
+}
 
 bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc)
 {
@@ -642,14 +655,15 @@ static int dpu_encoder_virt_atomic_check(
 
 	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
 
+	_dpu_enc_is_dspp_changed(crtc_state, topology);
+
 	/*
 	 * 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)
+		if (crtc_state->enable)
 			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
 					drm_enc, crtc_state, topology);
 	}
@@ -1022,7 +1036,7 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 	struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
 	struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL };
 	struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
-	int num_lm, num_ctl, num_pp, num_dsc;
+	int num_lm, num_ctl, num_pp, num_dsc, num_dspp;
 	unsigned int dsc_mask = 0;
 	int i;
 
@@ -1053,7 +1067,7 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 		drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl));
 	num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
 		drm_enc->base.id, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm));
-	dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
+	num_dspp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
 		drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp,
 		ARRAY_SIZE(hw_dspp));
 
@@ -1084,7 +1098,7 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 	}
 
 	cstate->num_mixers = num_lm;
-
+	cstate->ctm_enabled = !!num_dspp;
 	dpu_enc->connector = conn_state->connector;
 
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-- 
2.7.4

Re: [PATCH v3 4/4] drm/msm/dpu: reserve the resources on topology change

[Dmitry Baryshkov]

On 08/02/2023 15:42, Kalyan Thota wrote:
> Some features like CTM can be enabled dynamically. Release
> and reserve the DPU resources whenever a topology change
> occurs such that required hw blocks are allocated appropriately.
> 
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> ---
> Changes in v1:
> - Avoid mode_set call directly (Dmitry)
> 
> Changes in v2:
> - Minor nits (Dmitry)
> 
> Changes in v3:
> - avoid unnecessary modeset check call (Dmitry)
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  2 ++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 +++++++++++++++++++-----
>   2 files changed, 21 insertions(+), 5 deletions(-)

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

-- 
With best wishes
Dmitry

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

[Doug Anderson]

Hi,

On Wed, Feb 8, 2023 at 5:42 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 when ever there is
> a topology change to suit the new requirements.
>
> 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: reserve the resources on topology change
>
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  2 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 58 ++++++++++++++++-------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  2 +
>  3 files changed, 37 insertions(+), 25 deletions(-)

I tried out your changes, but unfortunately it seems like there's
something wrong. :( I did this:

1. Picked your 5 patches to the chromeos-5.15 tree (this series plus [1])

2. Put them on herobrine villager.

3. Booted up with no external display plugged in.

4. Tried to enable night light in the ChromeOS UI.

5. Night light didn't turn on for the internal display.


I also tried applying them to the top of msm-next (had to resolve some
small conflicts). Same thing, night light didn't work.


I thought maybe this was because the Chrome browser hasn't been
updated to properly use atomic_check for testing for night light, so I
hacked my herobrine device tree to not mark "mdss_dp" as "okay". Now
there's _only_ an eDP display. Same thing, night light didn't work.


I could only get night light to work for the internal display if I
plugged and unplugged an external display in.


Is the above the behavior that's expected right now?


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

RE: [PATCH v3 0/4] Reserve DSPPs based on user request

[Kalyan Thota]

Hi Doug,

Have you picked the core change to program dspp's  (below) ? the current series will go on top of it.
https://patchwork.kernel.org/project/linux-arm-msm/patch/1671542719-12655-1-git-send-email-quic_kalyant@quicinc.com/

Thanks,
Kalyan

>-----Original Message-----
>From: Doug Anderson <dianders@chromium.org>
>Sent: Wednesday, February 8, 2023 10:44 PM
>To: 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; swboyd@chromium.org;
>Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>;
>dmitry.baryshkov@linaro.org; Abhinav Kumar (QUIC)
><quic_abhinavk@quicinc.com>; marijn.suijten@somainline.org
>Subject: Re: [PATCH v3 0/4] Reserve DSPPs based on user request
>
>WARNING: This email originated from outside of Qualcomm. Please be wary of
>any links or attachments, and do not enable macros.
>
>Hi,
>
>On Wed, Feb 8, 2023 at 5:42 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 when ever there is a
>> topology change to suit the new requirements.
>>
>> 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: reserve the resources on topology change
>>
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  2 +
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 58 ++++++++++++++++------
>-------
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  2 +
>>  3 files changed, 37 insertions(+), 25 deletions(-)
>
>I tried out your changes, but unfortunately it seems like there's something wrong.
>:( I did this:
>
>1. Picked your 5 patches to the chromeos-5.15 tree (this series plus [1])
>
>2. Put them on herobrine villager.
>
>3. Booted up with no external display plugged in.
>
>4. Tried to enable night light in the ChromeOS UI.
>
>5. Night light didn't turn on for the internal display.
>
>
>I also tried applying them to the top of msm-next (had to resolve some small
>conflicts). Same thing, night light didn't work.
>
>
>I thought maybe this was because the Chrome browser hasn't been updated to
>properly use atomic_check for testing for night light, so I hacked my herobrine
>device tree to not mark "mdss_dp" as "okay". Now there's _only_ an eDP display.
>Same thing, night light didn't work.
>
>
>I could only get night light to work for the internal display if I plugged and
>unplugged an external display in.
>
>
>Is the above the behavior that's expected right now?
>
>
>[1] https://lore.kernel.org/all/1674814487-2112-1-git-send-email-
>quic_kalyant@quicinc.com/

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

[Doug Anderson]

Hi,

On Wed, Feb 8, 2023 at 8:16 PM Kalyan Thota <kalyant@qti.qualcomm.com> wrote:
>
> Hi Doug,
>
> Have you picked the core change to program dspp's  (below) ? the current series will go on top of it.
> https://patchwork.kernel.org/project/linux-arm-msm/patch/1671542719-12655-1-git-send-email-quic_kalyant@quicinc.com/

I didn't pick v11 of it like you link, but I did pick v12 of the same
patch. In my response I said that I picked 5 patches, this series plus
[1] where [1] is:

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


> Thanks,
> Kalyan
>
> >-----Original Message-----
> >From: Doug Anderson <dianders@chromium.org>
> >Sent: Wednesday, February 8, 2023 10:44 PM
> >To: 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; swboyd@chromium.org;
> >Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>;
> >dmitry.baryshkov@linaro.org; Abhinav Kumar (QUIC)
> ><quic_abhinavk@quicinc.com>; marijn.suijten@somainline.org
> >Subject: Re: [PATCH v3 0/4] Reserve DSPPs based on user request
> >
> >WARNING: This email originated from outside of Qualcomm. Please be wary of
> >any links or attachments, and do not enable macros.
> >
> >Hi,
> >
> >On Wed, Feb 8, 2023 at 5:42 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 when ever there is a
> >> topology change to suit the new requirements.
> >>
> >> 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: reserve the resources on topology change
> >>
> >>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  2 +
> >>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 58 ++++++++++++++++------
> >-------
> >>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  2 +
> >>  3 files changed, 37 insertions(+), 25 deletions(-)
> >
> >I tried out your changes, but unfortunately it seems like there's something wrong.
> >:( I did this:
> >
> >1. Picked your 5 patches to the chromeos-5.15 tree (this series plus [1])
> >
> >2. Put them on herobrine villager.
> >
> >3. Booted up with no external display plugged in.
> >
> >4. Tried to enable night light in the ChromeOS UI.
> >
> >5. Night light didn't turn on for the internal display.
> >
> >
> >I also tried applying them to the top of msm-next (had to resolve some small
> >conflicts). Same thing, night light didn't work.
> >
> >
> >I thought maybe this was because the Chrome browser hasn't been updated to
> >properly use atomic_check for testing for night light, so I hacked my herobrine
> >device tree to not mark "mdss_dp" as "okay". Now there's _only_ an eDP display.
> >Same thing, night light didn't work.
> >
> >
> >I could only get night light to work for the internal display if I plugged and
> >unplugged an external display in.
> >
> >
> >Is the above the behavior that's expected right now?
> >
> >
> >[1] https://lore.kernel.org/all/1674814487-2112-1-git-send-email-
> >quic_kalyant@quicinc.com/

RE: [PATCH v3 0/4] Reserve DSPPs based on user request

[Kalyan Thota]

[-- Attachment #1: Type: text/plain, Size: 4369 bytes --]

Kindly ignore my previous email. Sent too early !!

We have tested the changes on top of tip: https://chromium.googlesource.com/chromiumos/third_party/kernel/+log/refs/heads/chromeos-5.15 + 5 CTM patches ( that you have quoted )
We didn’t see the issue that you have reported on herobrine. Night light always came up on primary display as the reservation with dspp was successful.  Are you seeing any reservation failures for primary display ?

Attached a debug patch, can you share the logs when you see the issue.

Thanks,
Kalyan

>-----Original Message-----
>From: Kalyan Thota
>Sent: Thursday, February 9, 2023 9:47 AM
>To: Doug Anderson <dianders@chromium.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; swboyd@chromium.org;
>Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>;
>dmitry.baryshkov@linaro.org; Abhinav Kumar (QUIC)
><quic_abhinavk@quicinc.com>; marijn.suijten@somainline.org
>Subject: RE: [PATCH v3 0/4] Reserve DSPPs based on user request
>
>Hi Doug,
>
>Have you picked the core change to program dspp's  (below) ? the current series
>will go on top of it.
>https://patchwork.kernel.org/project/linux-arm-msm/patch/1671542719-12655-
>1-git-send-email-quic_kalyant@quicinc.com/
>
>Thanks,
>Kalyan
>
>>-----Original Message-----
>>From: Doug Anderson <dianders@chromium.org>
>>Sent: Wednesday, February 8, 2023 10:44 PM
>>To: 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; swboyd@chromium.org;
>>Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>;
>>dmitry.baryshkov@linaro.org; Abhinav Kumar (QUIC)
>><quic_abhinavk@quicinc.com>; marijn.suijten@somainline.org
>>Subject: Re: [PATCH v3 0/4] Reserve DSPPs based on user request
>>
>>WARNING: This email originated from outside of Qualcomm. Please be wary
>>of any links or attachments, and do not enable macros.
>>
>>Hi,
>>
>>On Wed, Feb 8, 2023 at 5:42 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 when ever there is
>>> a topology change to suit the new requirements.
>>>
>>> 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: reserve the resources on topology change
>>>
>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  2 +
>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 58
>>> ++++++++++++++++------
>>-------
>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  2 +
>>>  3 files changed, 37 insertions(+), 25 deletions(-)
>>
>>I tried out your changes, but unfortunately it seems like there's something
>wrong.
>>:( I did this:
>>
>>1. Picked your 5 patches to the chromeos-5.15 tree (this series plus
>>[1])
>>
>>2. Put them on herobrine villager.
>>
>>3. Booted up with no external display plugged in.
>>
>>4. Tried to enable night light in the ChromeOS UI.
>>
>>5. Night light didn't turn on for the internal display.
>>
>>
>>I also tried applying them to the top of msm-next (had to resolve some
>>small conflicts). Same thing, night light didn't work.
>>
>>
>>I thought maybe this was because the Chrome browser hasn't been updated
>>to properly use atomic_check for testing for night light, so I hacked
>>my herobrine device tree to not mark "mdss_dp" as "okay". Now there's _only_
>an eDP display.
>>Same thing, night light didn't work.
>>
>>
>>I could only get night light to work for the internal display if I
>>plugged and unplugged an external display in.
>>
>>
>>Is the above the behavior that's expected right now?
>>
>>
>>[1] https://lore.kernel.org/all/1674814487-2112-1-git-send-email-
>>quic_kalyant@quicinc.com/

[-- Attachment #2: nightlight_debug.patch --]
[-- Type: application/octet-stream, Size: 2945 bytes --]

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 36bdcaa..a247649 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -762,6 +762,7 @@ static void _dpu_crtc_setup_cp_blocks(struct drm_crtc *crtc)
 			dspp->ops.setup_pcc(dspp, NULL);
 		} else {
 			_dpu_crtc_get_pcc_coeff(state, &cfg);
+			pr_err("\n");
 			dspp->ops.setup_pcc(dspp, &cfg);
 		}
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 038e077..4ba7ee8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -221,7 +221,7 @@ static bool _dpu_enc_is_dspp_changed(struct drm_crtc_state *crtc_state,
 	struct msm_display_topology topology)
 {
 	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
-
+	pr_err("cstate ctm enable :%d toplogy dsppnum:%d\n", cstate->ctm_enabled, topology.num_dspp);
 	if ((cstate->ctm_enabled && !topology.num_dspp) ||
 	    (!cstate->ctm_enabled && topology.num_dspp)) {
 		crtc_state->mode_changed = true;
@@ -603,7 +603,7 @@ static struct msm_display_topology dpu_encoder_get_topology(
 		topology.num_intf = 1;
 		topology.num_lm = 2;
 	}
-
+	pr_err("ctm : %p topology num_dspp: %d topology num_lm:%d\n",crtc_state->ctm, topology.num_dspp, topology.num_lm);
 	return topology;
 }
 
@@ -626,7 +626,7 @@ static int dpu_encoder_virt_atomic_check(
 				drm_enc != NULL, crtc_state != NULL, conn_state != NULL);
 		return -EINVAL;
 	}
-
+	pr_err("\n");
 	dpu_enc = to_dpu_encoder_virt(drm_enc);
 	DPU_DEBUG_ENC(dpu_enc, "\n");
 
@@ -657,12 +657,13 @@ static int dpu_encoder_virt_atomic_check(
 
 	_dpu_enc_is_dspp_changed(crtc_state, topology);
 
+	pr_err("crtc state enable:%d needs_modeset: %d\n", crtc_state->enable, drm_atomic_crtc_needs_modeset(crtc_state));
+
 	/*
 	 * Release and Allocate resources on every modeset
 	 */
 	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
 		dpu_rm_release(global_state, drm_enc);
-
 		if (crtc_state->enable)
 			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
 					drm_enc, crtc_state, topology);
@@ -1099,6 +1100,7 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 
 	cstate->num_mixers = num_lm;
 	cstate->ctm_enabled = !!num_dspp;
+	pr_err("ctm_enabled: %d\n", cstate->ctm_enabled);
 	dpu_enc->connector = conn_state->connector;
 
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 718ea0a..a0e2496 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -428,6 +428,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
 
 		trace_dpu_rm_reserve_lms(lm_idx[i] + LM_0, enc_id,
 					 pp_idx[i] + PINGPONG_0);
+		pr_err("encid : %d  enc_to_dsppid : %d\n", enc_id, reqs->topology.num_dspp);
 	}
 
 	return 0;

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

[Doug Anderson]

Hi,

On Thu, Feb 9, 2023 at 3:26 AM Kalyan Thota <kalyant@qti.qualcomm.com> wrote:
>
> Kindly ignore my previous email. Sent too early !!
>
> We have tested the changes on top of tip: https://chromium.googlesource.com/chromiumos/third_party/kernel/+log/refs/heads/chromeos-5.15 + 5 CTM patches ( that you have quoted )
> We didn’t see the issue that you have reported on herobrine. Night light always came up on primary display as the reservation with dspp was successful.  Are you seeing any reservation failures for primary display ?
>
> Attached a debug patch, can you share the logs when you see the issue.

Sounds good. Since officially this hardware is not available to the
public at this time, I have shared the `dmesg` privately to your (and
Abhinav's) Google partner accounts. Yell if you don't see the
notificatioin. I don't have any reason to believe there's anything
secret in the dmesg, but it didn't seem worth the time to fully audit
it.

For that dmesg, I:

1. Made sure that night light was off.

2. Updated the kernel with the 5 patches + the debug patch.

3. Booted up and logged into ChromeOS

4. Tried turning night light off/on several times and saw nothing on
dmesg while I did this (and night light didn't work)

5. Unplugged power and servo (just to make sure they didn't interfere)

6. Echoed "DOUG: plug in external display now" several times to "/dev/kmsg"

7. Plugged in my external display, which is behind a Type C dock

8. Turned night light on/off several times. Night light worked on the
internal display.

In case it matters, my ChromeOS root filesystem is R111-15328.0.0


> >-----Original Message-----
> >From: Kalyan Thota
> >Sent: Thursday, February 9, 2023 9:47 AM
> >To: Doug Anderson <dianders@chromium.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; swboyd@chromium.org;
> >Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>;
> >dmitry.baryshkov@linaro.org; Abhinav Kumar (QUIC)
> ><quic_abhinavk@quicinc.com>; marijn.suijten@somainline.org
> >Subject: RE: [PATCH v3 0/4] Reserve DSPPs based on user request
> >
> >Hi Doug,
> >
> >Have you picked the core change to program dspp's  (below) ? the current series
> >will go on top of it.
> >https://patchwork.kernel.org/project/linux-arm-msm/patch/1671542719-12655-
> >1-git-send-email-quic_kalyant@quicinc.com/
> >
> >Thanks,
> >Kalyan
> >
> >>-----Original Message-----
> >>From: Doug Anderson <dianders@chromium.org>
> >>Sent: Wednesday, February 8, 2023 10:44 PM
> >>To: 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; swboyd@chromium.org;
> >>Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>;
> >>dmitry.baryshkov@linaro.org; Abhinav Kumar (QUIC)
> >><quic_abhinavk@quicinc.com>; marijn.suijten@somainline.org
> >>Subject: Re: [PATCH v3 0/4] Reserve DSPPs based on user request
> >>
> >>WARNING: This email originated from outside of Qualcomm. Please be wary
> >>of any links or attachments, and do not enable macros.
> >>
> >>Hi,
> >>
> >>On Wed, Feb 8, 2023 at 5:42 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 when ever there is
> >>> a topology change to suit the new requirements.
> >>>
> >>> 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: reserve the resources on topology change
> >>>
> >>>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  2 +
> >>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 58
> >>> ++++++++++++++++------
> >>-------
> >>>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  2 +
> >>>  3 files changed, 37 insertions(+), 25 deletions(-)
> >>
> >>I tried out your changes, but unfortunately it seems like there's something
> >wrong.
> >>:( I did this:
> >>
> >>1. Picked your 5 patches to the chromeos-5.15 tree (this series plus
> >>[1])
> >>
> >>2. Put them on herobrine villager.
> >>
> >>3. Booted up with no external display plugged in.
> >>
> >>4. Tried to enable night light in the ChromeOS UI.
> >>
> >>5. Night light didn't turn on for the internal display.
> >>
> >>
> >>I also tried applying them to the top of msm-next (had to resolve some
> >>small conflicts). Same thing, night light didn't work.
> >>
> >>
> >>I thought maybe this was because the Chrome browser hasn't been updated
> >>to properly use atomic_check for testing for night light, so I hacked
> >>my herobrine device tree to not mark "mdss_dp" as "okay". Now there's _only_
> >an eDP display.
> >>Same thing, night light didn't work.
> >>
> >>
> >>I could only get night light to work for the internal display if I
> >>plugged and unplugged an external display in.
> >>
> >>
> >>Is the above the behavior that's expected right now?
> >>
> >>
> >>[1] https://lore.kernel.org/all/1674814487-2112-1-git-send-email-
> >>quic_kalyant@quicinc.com/