[PATCH 1/4] drm/msm/disp/dpu1: pin 1 crtc to 1 encoder

[PATCH 1/4] drm/msm/disp/dpu1: pin 1 crtc to 1 encoder

[Kalyan Thota]

Pin each crtc with one encoder. This arrangement will
disallow crtc switching between encoders and also will
facilitate to advertise certain features on crtc based
on encoder type.

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 7a5fabc..552a89c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -798,19 +798,19 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
 	max_crtc_count = min(max_crtc_count, primary_planes_idx);
 
 	/* Create one CRTC per encoder */
+	encoder = list_first_entry(&(dev)->mode_config.encoder_list,
+		struct drm_encoder, head);
 	for (i = 0; i < max_crtc_count; i++) {
 		crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
-		if (IS_ERR(crtc)) {
+		if (IS_ERR(crtc) || IS_ERR_OR_NULL(encoder)) {
 			ret = PTR_ERR(crtc);
 			return ret;
 		}
 		priv->crtcs[priv->num_crtcs++] = crtc;
+		encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
+		encoder = list_next_entry(encoder, head);
 	}
 
-	/* All CRTCs are compatible with all encoders */
-	drm_for_each_encoder(encoder, dev)
-		encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
-
 	return 0;
 }
 
-- 
2.7.4

[PATCH 2/4] drm/msm/disp/dpu1: populate disp_info if an interface is external

[Kalyan Thota]

DRM encoder type is same for eDP and DP (DRM_MODE_ENCODER_TMDS)
populate is_external information in the disp_info so as to
differentiate between eDP and DP on the DPU encoder side.

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 +++++++++++++++++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 14 +++++++++++---
 drivers/gpu/drm/msm/dp/dp_display.c         |  5 +++++
 drivers/gpu/drm/msm/msm_drv.h               |  7 ++++++-
 4 files changed, 39 insertions(+), 7 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..5d6ad1f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2412,7 +2412,7 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
 	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
 	struct drm_encoder *drm_enc = NULL;
 	struct dpu_encoder_virt *dpu_enc = NULL;
-	int ret = 0;
+	int ret = 0, intf_i;
 
 	dpu_enc = to_dpu_encoder_virt(enc);
 
@@ -2424,13 +2424,16 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
 	timer_setup(&dpu_enc->frame_done_timer,
 			dpu_encoder_frame_done_timeout, 0);
 
+	intf_i = disp_info->h_tile_instance[0];
 	if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
 		timer_setup(&dpu_enc->vsync_event_timer,
 				dpu_encoder_vsync_event_handler,
 				0);
-	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
+	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
 		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
-				priv->dp[disp_info->h_tile_instance[0]]);
+				priv->dp[intf_i]);
+		disp_info->is_external = msm_dp_is_external(priv->dp[intf_i]);
+	}
 
 	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
 			dpu_encoder_off_work);
@@ -2455,6 +2458,17 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
 
 }
 
+bool dpu_encoder_is_external(struct drm_encoder *drm_enc)
+{
+	struct dpu_encoder_virt *dpu_enc;
+
+	if (!drm_enc)
+		return false;
+
+	dpu_enc = to_dpu_encoder_virt(drm_enc);
+	return dpu_enc->disp_info.is_external;
+}
+
 struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
 		int drm_enc_mode)
 {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 9e7236e..43f0d8b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -25,16 +25,18 @@
  * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
  * @h_tile_instance:    Controller instance used per tile. Number of elements is
  *                      based on num_of_h_tiles
- * @is_cmd_mode		Boolean to indicate if the CMD mode is requested
+ * @is_cmd_mode:        Boolean to indicate if the CMD mode is requested
+ * @is_external:        Boolean to indicate if the intf is external
  * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
- *				 used instead of panel TE in cmd mode panels
- * @dsc:		DSC configuration data for DSC-enabled displays
+ *                      used instead of panel TE in cmd mode panels
+ * @dsc:                DSC configuration data for DSC-enabled displays
  */
 struct msm_display_info {
 	int intf_type;
 	uint32_t num_of_h_tiles;
 	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
 	bool is_cmd_mode;
+	bool is_external;
 	bool is_te_using_watchdog_timer;
 	struct drm_dsc_config *dsc;
 };
@@ -128,6 +130,12 @@ enum dpu_intf_mode dpu_encoder_get_intf_mode(struct drm_encoder *encoder);
 void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder);
 
 /**
+ * dpu_encoder_is_external - find if the encoder is of type external
+ * @drm_enc:    Pointer to previously created drm encoder structure
+ */
+bool dpu_encoder_is_external(struct drm_encoder *drm_enc);
+
+/**
  * dpu_encoder_init - initialize virtual encoder object
  * @dev:        Pointer to drm device structure
  * @disp_info:  Pointer to display information structure
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bfd0aef..0bbdcca5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1509,6 +1509,11 @@ bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)
 	return dp->wide_bus_en;
 }
 
+bool msm_dp_is_external(const struct msm_dp *dp_display)
+{
+	return (dp_display->connector_type == DRM_MODE_CONNECTOR_DisplayPort);
+}
+
 void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
 {
 	struct dp_display_private *dp;
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index ea80846..3b9f8d2 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -331,7 +331,7 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_displa
 
 void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
 bool msm_dp_wide_bus_available(const struct msm_dp *dp_display);
-
+bool msm_dp_is_external(const struct msm_dp *dp_display);
 #else
 static inline int __init msm_dp_register(void)
 {
@@ -365,6 +365,11 @@ static inline bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)
 	return false;
 }
 
+static inline bool msm_dp_is_external(const struct msm_dp *dp_display)
+{
+	return false;
+}
+
 #endif
 
 #ifdef CONFIG_DRM_MSM_MDP4
-- 
2.7.4

[PATCH 3/4] drm/msm/disp/dpu1: helper function to determine if encoder is virtual

[Kalyan Thota]

Add a helper function to determine if an encoder is of type
virtual.

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 5d6ad1f..4c56a16 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2469,6 +2469,17 @@ bool dpu_encoder_is_external(struct drm_encoder *drm_enc)
 	return dpu_enc->disp_info.is_external;
 }
 
+bool dpu_encoder_is_virtual(struct drm_encoder *drm_enc)
+{
+	struct dpu_encoder_virt *dpu_enc;
+
+	if (!drm_enc)
+		return false;
+
+	dpu_enc = to_dpu_encoder_virt(drm_enc);
+	return (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_VIRTUAL);
+}
+
 struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
 		int drm_enc_mode)
 {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 43f0d8b..6ae3c62 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -136,6 +136,12 @@ void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder);
 bool dpu_encoder_is_external(struct drm_encoder *drm_enc);
 
 /**
+ * dpu_encoder_is_virtual - find if the encoder is of type virtual.
+ * @drm_enc:    Pointer to previously created drm encoder structure
+ */
+bool dpu_encoder_is_virtual(struct drm_encoder *drm_enc);
+
+/**
  * dpu_encoder_init - initialize virtual encoder object
  * @dev:        Pointer to drm device structure
  * @disp_info:  Pointer to display information structure
-- 
2.7.4

[PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc

[Kalyan Thota]

Add color management support for the crtc provided there are
enough dspps that can be allocated from the catalogue

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 ++++++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 ++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 53 +++++++++++++++++++++++++++++
 4 files changed, 77 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 4170fbe..6bd3a64 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -18,9 +18,11 @@
 #include <drm/drm_flip_work.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_mode.h>
+#include <drm/drm_mode_object.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_rect.h>
 #include <drm/drm_vblank.h>
+#include "../../../drm_crtc_internal.h"
 
 #include "dpu_kms.h"
 #include "dpu_hw_lm.h"
@@ -553,6 +555,17 @@ static void _dpu_crtc_complete_flip(struct drm_crtc *crtc)
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 }
 
+bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc)
+{
+	u32 ctm_id = crtc->dev->mode_config.ctm_property->base.id;
+	u32 gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
+	u32 degamma_id = crtc->dev->mode_config.degamma_lut_property->base.id;
+
+	return !!(drm_mode_obj_find_prop_id(&crtc->base, ctm_id) ||
+		   drm_mode_obj_find_prop_id(&crtc->base, gamma_id) ||
+		   drm_mode_obj_find_prop_id(&crtc->base, degamma_id));
+}
+
 enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
 {
 	struct drm_encoder *encoder;
@@ -1604,8 +1617,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
 
 	drm_crtc_helper_add(crtc, &dpu_crtc_helper_funcs);
 
-	drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
-
 	/* save user friendly CRTC name for later */
 	snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u", crtc->base.id);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 539b68b..8bac395 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -300,4 +300,10 @@ static inline enum dpu_crtc_client_type dpu_crtc_get_client_type(
 	return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
 }
 
+/**
+ * dpu_crtc_has_color_enabled - check if the crtc has color management enabled
+ * @crtc: Pointer to drm crtc object
+ */
+bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc);
+
 #endif /* _DPU_CRTC_H_ */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 4c56a16..ebc3f25 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 *crtc)
 {
 	struct msm_display_topology topology = {0};
 	int i, intf_count = 0;
@@ -573,11 +574,9 @@ 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))
+	if (dpu_crtc_has_color_enabled(crtc) &&
+		(dpu_kms->catalog->dspp_count >= topology.num_lm))
 			topology.num_dspp = topology.num_lm;
-	}
 
 	topology.num_enc = 0;
 	topology.num_intf = intf_count;
@@ -643,7 +642,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->crtc);
 
 	/* Reserve dynamic resources now. */
 	if (!ret) {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 552a89c..47a73fa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -13,6 +13,7 @@
 #include <linux/dma-buf.h>
 #include <linux/of_irq.h>
 #include <linux/pm_opp.h>
+#include <linux/bitops.h>
 
 #include <drm/drm_crtc.h>
 #include <drm/drm_file.h>
@@ -537,6 +538,44 @@ static void dpu_kms_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
 		dpu_kms_wait_for_commit_done(kms, crtc);
 }
 
+/**
+ * _dpu_kms_possible_dspps - Evaluate how many dspps pairs can be facilitated
+                             to enable color features for crtcs.
+ * @dpu_kms:    Pointer to dpu kms structure
+ * Returns:     count of dspp pairs
+ *
+ * Baring single entry, if atleast 2 dspps are available in the catalogue,
+ * then color can be enabled for that crtc
+ */
+static inline u32 _dpu_kms_possible_dspps(struct dpu_kms *dpu_kms)
+{
+
+	u32 num_dspps = dpu_kms->catalog->dspp_count;
+
+	if (num_dspps > 1)
+		num_dspps =
+			!(num_dspps % 2) ? num_dspps / 2 : (num_dspps - 1) / 2;
+
+	return num_dspps;
+}
+
+static u32 _dpu_kms_attach_color(struct drm_device *dev, u32 enc_mask,
+						u32 num_dspps)
+{
+	struct drm_encoder *encoder;
+	struct drm_crtc *crtc;
+
+	drm_for_each_encoder_mask(encoder, dev, enc_mask) {
+		crtc = drm_crtc_from_index(dev, ffs(encoder->possible_crtcs) - 1);
+		if (num_dspps && crtc) {
+			drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
+			num_dspps--;
+		}
+	}
+
+	return num_dspps;
+}
+
 static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 				    struct msm_drm_private *priv,
 				    struct dpu_kms *dpu_kms)
@@ -747,6 +786,8 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
 
 	int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret;
 	int max_crtc_count;
+	u32 num_dspps, primary_enc_mask = 0, external_enc_mask = 0;
+
 	dev = dpu_kms->dev;
 	priv = dev->dev_private;
 	catalog = dpu_kms->catalog;
@@ -796,6 +837,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
 	}
 
 	max_crtc_count = min(max_crtc_count, primary_planes_idx);
+	num_dspps = _dpu_kms_possible_dspps(dpu_kms);
 
 	/* Create one CRTC per encoder */
 	encoder = list_first_entry(&(dev)->mode_config.encoder_list,
@@ -808,9 +850,20 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
 		}
 		priv->crtcs[priv->num_crtcs++] = crtc;
 		encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
+
+		if (!dpu_encoder_is_external(encoder) &&
+			!dpu_encoder_is_virtual(encoder))
+			primary_enc_mask |= drm_encoder_mask(encoder);
+		else if (dpu_encoder_is_external(encoder))
+			external_enc_mask |= drm_encoder_mask(encoder);
+
 		encoder = list_next_entry(encoder, head);
 	}
 
+	/* Prefer Primary encoders in registering for color support */
+	num_dspps = _dpu_kms_attach_color(dev, primary_enc_mask, num_dspps);
+	num_dspps = _dpu_kms_attach_color(dev, external_enc_mask, num_dspps);
+
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH 1/4] drm/msm/disp/dpu1: pin 1 crtc to 1 encoder

[Dmitry Baryshkov]

On 09/11/2022 15:16, Kalyan Thota wrote:
> Pin each crtc with one encoder. This arrangement will
> disallow crtc switching between encoders and also will
> facilitate to advertise certain features on crtc based
> on encoder type.
> 
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 7a5fabc..552a89c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -798,19 +798,19 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>   	max_crtc_count = min(max_crtc_count, primary_planes_idx);
>   
>   	/* Create one CRTC per encoder */
> +	encoder = list_first_entry(&(dev)->mode_config.encoder_list,
> +		struct drm_encoder, head);

Please use drm_for_each_encoder() here.

>   	for (i = 0; i < max_crtc_count; i++) {
>   		crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
> -		if (IS_ERR(crtc)) {
> +		if (IS_ERR(crtc) || IS_ERR_OR_NULL(encoder)) {

Why? Not to mention that the OR_NULL part is quite frequently a mistake.

>   			ret = PTR_ERR(crtc);
>   			return ret;
>   		}
>   		priv->crtcs[priv->num_crtcs++] = crtc;
> +		encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
> +		encoder = list_next_entry(encoder, head);
>   	}
>   
> -	/* All CRTCs are compatible with all encoders */
> -	drm_for_each_encoder(encoder, dev)
> -		encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
> -
>   	return 0;
>   }
>   

-- 
With best wishes
Dmitry

Re: [PATCH 2/4] drm/msm/disp/dpu1: populate disp_info if an interface is external

[Dmitry Baryshkov]

On 09/11/2022 15:16, Kalyan Thota wrote:
> DRM encoder type is same for eDP and DP (DRM_MODE_ENCODER_TMDS)
> populate is_external information in the disp_info so as to
> differentiate between eDP and DP on the DPU encoder side.
> 
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 +++++++++++++++++---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 14 +++++++++++---
>   drivers/gpu/drm/msm/dp/dp_display.c         |  5 +++++
>   drivers/gpu/drm/msm/msm_drv.h               |  7 ++++++-
>   4 files changed, 39 insertions(+), 7 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..5d6ad1f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2412,7 +2412,7 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>   	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
>   	struct drm_encoder *drm_enc = NULL;
>   	struct dpu_encoder_virt *dpu_enc = NULL;
> -	int ret = 0;
> +	int ret = 0, intf_i;
>   
>   	dpu_enc = to_dpu_encoder_virt(enc);
>   
> @@ -2424,13 +2424,16 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>   	timer_setup(&dpu_enc->frame_done_timer,
>   			dpu_encoder_frame_done_timeout, 0);
>   
> +	intf_i = disp_info->h_tile_instance[0];
>   	if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
>   		timer_setup(&dpu_enc->vsync_event_timer,
>   				dpu_encoder_vsync_event_handler,
>   				0);
> -	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
> +	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
>   		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
> -				priv->dp[disp_info->h_tile_instance[0]]);
> +				priv->dp[intf_i]);
> +		disp_info->is_external = msm_dp_is_external(priv->dp[intf_i]);
> +	}

I will quite myself: "And DSI can be pluggable too. Please enumerate 
connector types here rather than doing that in DP driver."
Your s/pluggable/external/ doesn't fix the issue.

>   
>   	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>   			dpu_encoder_off_work);
> @@ -2455,6 +2458,17 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>   
>   }
>   
> +bool dpu_encoder_is_external(struct drm_encoder *drm_enc)
> +{
> +	struct dpu_encoder_virt *dpu_enc;
> +
> +	if (!drm_enc)
> +		return false;
> +
> +	dpu_enc = to_dpu_encoder_virt(drm_enc);
> +	return dpu_enc->disp_info.is_external;
> +}
> +
>   struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>   		int drm_enc_mode)
>   {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 9e7236e..43f0d8b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -25,16 +25,18 @@
>    * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
>    * @h_tile_instance:    Controller instance used per tile. Number of elements is
>    *                      based on num_of_h_tiles
> - * @is_cmd_mode		Boolean to indicate if the CMD mode is requested
> + * @is_cmd_mode:        Boolean to indicate if the CMD mode is requested
> + * @is_external:        Boolean to indicate if the intf is external
>    * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> - *				 used instead of panel TE in cmd mode panels
> - * @dsc:		DSC configuration data for DSC-enabled displays
> + *                      used instead of panel TE in cmd mode panels
> + * @dsc:                DSC configuration data for DSC-enabled displays
>    */
>   struct msm_display_info {
>   	int intf_type;
>   	uint32_t num_of_h_tiles;
>   	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
>   	bool is_cmd_mode;
> +	bool is_external;
>   	bool is_te_using_watchdog_timer;
>   	struct drm_dsc_config *dsc;
>   };
> @@ -128,6 +130,12 @@ enum dpu_intf_mode dpu_encoder_get_intf_mode(struct drm_encoder *encoder);
>   void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder);
>   
>   /**
> + * dpu_encoder_is_external - find if the encoder is of type external
> + * @drm_enc:    Pointer to previously created drm encoder structure
> + */
> +bool dpu_encoder_is_external(struct drm_encoder *drm_enc);
> +
> +/**
>    * dpu_encoder_init - initialize virtual encoder object
>    * @dev:        Pointer to drm device structure
>    * @disp_info:  Pointer to display information structure
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index bfd0aef..0bbdcca5 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1509,6 +1509,11 @@ bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)
>   	return dp->wide_bus_en;
>   }
>   
> +bool msm_dp_is_external(const struct msm_dp *dp_display)
> +{
> +	return (dp_display->connector_type == DRM_MODE_CONNECTOR_DisplayPort);
> +}
> +
>   void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
>   {
>   	struct dp_display_private *dp;
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index ea80846..3b9f8d2 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -331,7 +331,7 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_displa
>   
>   void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
>   bool msm_dp_wide_bus_available(const struct msm_dp *dp_display);
> -
> +bool msm_dp_is_external(const struct msm_dp *dp_display);
>   #else
>   static inline int __init msm_dp_register(void)
>   {
> @@ -365,6 +365,11 @@ static inline bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)
>   	return false;
>   }
>   
> +static inline bool msm_dp_is_external(const struct msm_dp *dp_display)
> +{
> +	return false;
> +}
> +
>   #endif
>   
>   #ifdef CONFIG_DRM_MSM_MDP4

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] drm/msm/disp/dpu1: helper function to determine if encoder is virtual

[Dmitry Baryshkov]

On 09/11/2022 15:16, Kalyan Thota wrote:
> Add a helper function to determine if an encoder is of type
> virtual.

Please use commit messages to describe why you are doing something, not 
what you are doing. In this case I can predict, why do you need this API 
without going to patch 4.

> 
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++++++++++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++++++
>   2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 5d6ad1f..4c56a16 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2469,6 +2469,17 @@ bool dpu_encoder_is_external(struct drm_encoder *drm_enc)
>   	return dpu_enc->disp_info.is_external;
>   }
>   
> +bool dpu_encoder_is_virtual(struct drm_encoder *drm_enc)
> +{
> +	struct dpu_encoder_virt *dpu_enc;
> +
> +	if (!drm_enc)
> +		return false;
> +
> +	dpu_enc = to_dpu_encoder_virt(drm_enc);
> +	return (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_VIRTUAL);
> +}
> +
>   struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>   		int drm_enc_mode)
>   {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 43f0d8b..6ae3c62 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -136,6 +136,12 @@ void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder);
>   bool dpu_encoder_is_external(struct drm_encoder *drm_enc);
>   
>   /**
> + * dpu_encoder_is_virtual - find if the encoder is of type virtual.
> + * @drm_enc:    Pointer to previously created drm encoder structure
> + */
> +bool dpu_encoder_is_virtual(struct drm_encoder *drm_enc);
> +
> +/**
>    * dpu_encoder_init - initialize virtual encoder object
>    * @dev:        Pointer to drm device structure
>    * @disp_info:  Pointer to display information structure

-- 
With best wishes
Dmitry

Re: [PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc

[Dmitry Baryshkov]

On 09/11/2022 15:16, Kalyan Thota wrote:
> Add color management support for the crtc provided there are
> enough dspps that can be allocated from the catalogue
> 
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 ++++++--
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 ++++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 53 +++++++++++++++++++++++++++++
>   4 files changed, 77 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 4170fbe..6bd3a64 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -18,9 +18,11 @@
>   #include <drm/drm_flip_work.h>
>   #include <drm/drm_framebuffer.h>
>   #include <drm/drm_mode.h>
> +#include <drm/drm_mode_object.h>
>   #include <drm/drm_probe_helper.h>
>   #include <drm/drm_rect.h>
>   #include <drm/drm_vblank.h>
> +#include "../../../drm_crtc_internal.h"

If it's internal, it is not supposed to be used by other parties, 
including the msm drm. At least a comment why you are including this 
file would be helpful.

>   
>   #include "dpu_kms.h"
>   #include "dpu_hw_lm.h"
> @@ -553,6 +555,17 @@ static void _dpu_crtc_complete_flip(struct drm_crtc *crtc)
>   	spin_unlock_irqrestore(&dev->event_lock, flags);
>   }
>   
> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc)
> +{
> +	u32 ctm_id = crtc->dev->mode_config.ctm_property->base.id;
> +	u32 gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
> +	u32 degamma_id = crtc->dev->mode_config.degamma_lut_property->base.id;
> +
> +	return !!(drm_mode_obj_find_prop_id(&crtc->base, ctm_id) ||
> +		   drm_mode_obj_find_prop_id(&crtc->base, gamma_id) ||
> +		   drm_mode_obj_find_prop_id(&crtc->base, degamma_id));
> +}
> +
>   enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
>   {
>   	struct drm_encoder *encoder;
> @@ -1604,8 +1617,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
>   
>   	drm_crtc_helper_add(crtc, &dpu_crtc_helper_funcs);
>   
> -	drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
> -
>   	/* save user friendly CRTC name for later */
>   	snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u", crtc->base.id);
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 539b68b..8bac395 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -300,4 +300,10 @@ static inline enum dpu_crtc_client_type dpu_crtc_get_client_type(
>   	return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
>   }
>   
> +/**
> + * dpu_crtc_has_color_enabled - check if the crtc has color management enabled
> + * @crtc: Pointer to drm crtc object
> + */
> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc);
> +
>   #endif /* _DPU_CRTC_H_ */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 4c56a16..ebc3f25 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 *crtc)
>   {
>   	struct msm_display_topology topology = {0};
>   	int i, intf_count = 0;
> @@ -573,11 +574,9 @@ 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))
> +	if (dpu_crtc_has_color_enabled(crtc) &&
> +		(dpu_kms->catalog->dspp_count >= topology.num_lm))

See the comment to the previous patch. It still applies here.

>   			topology.num_dspp = topology.num_lm;
> -	}
>   
>   	topology.num_enc = 0;
>   	topology.num_intf = intf_count;
> @@ -643,7 +642,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->crtc);
>   
>   	/* Reserve dynamic resources now. */
>   	if (!ret) {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 552a89c..47a73fa 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -13,6 +13,7 @@
>   #include <linux/dma-buf.h>
>   #include <linux/of_irq.h>
>   #include <linux/pm_opp.h>
> +#include <linux/bitops.h>
>   
>   #include <drm/drm_crtc.h>
>   #include <drm/drm_file.h>
> @@ -537,6 +538,44 @@ static void dpu_kms_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
>   		dpu_kms_wait_for_commit_done(kms, crtc);
>   }
>   
> +/**
> + * _dpu_kms_possible_dspps - Evaluate how many dspps pairs can be facilitated
> +                             to enable color features for crtcs.
> + * @dpu_kms:    Pointer to dpu kms structure
> + * Returns:     count of dspp pairs
> + *
> + * Baring single entry, if atleast 2 dspps are available in the catalogue,
> + * then color can be enabled for that crtc
> + */
> +static inline u32 _dpu_kms_possible_dspps(struct dpu_kms *dpu_kms)
> +{
> +
> +	u32 num_dspps = dpu_kms->catalog->dspp_count;
> +
> +	if (num_dspps > 1)
> +		num_dspps =
> +			!(num_dspps % 2) ? num_dspps / 2 : (num_dspps - 1) / 2;

Ugh. No. Please spell this clearly rather than using nice math and 
ternary operators:

if (num_dspps <= 1)
   return num_dspps;
else
   return num_dspps / 2;

You see, if num_dspps %2 ! =0, then num_dspps / 2 == (num_dspps_2 - 1) / 2.


> +
> +	return num_dspps;
> +}
> +
> +static u32 _dpu_kms_attach_color(struct drm_device *dev, u32 enc_mask,
> +						u32 num_dspps)
> +{
> +	struct drm_encoder *encoder;
> +	struct drm_crtc *crtc;
> +
> +	drm_for_each_encoder_mask(encoder, dev, enc_mask) {
> +		crtc = drm_crtc_from_index(dev, ffs(encoder->possible_crtcs) - 1);
> +		if (num_dspps && crtc) {
> +			drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
> +			num_dspps--;

Please.  You can do this at the time you create the crtc. It would be 
much simpler.

> +		}
> +	}
> +
> +	return num_dspps;
> +}
> +
>   static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>   				    struct msm_drm_private *priv,
>   				    struct dpu_kms *dpu_kms)
> @@ -747,6 +786,8 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>   
>   	int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret;
>   	int max_crtc_count;
> +	u32 num_dspps, primary_enc_mask = 0, external_enc_mask = 0;
> +
>   	dev = dpu_kms->dev;
>   	priv = dev->dev_private;
>   	catalog = dpu_kms->catalog;
> @@ -796,6 +837,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>   	}
>   
>   	max_crtc_count = min(max_crtc_count, primary_planes_idx);
> +	num_dspps = _dpu_kms_possible_dspps(dpu_kms);
>   
>   	/* Create one CRTC per encoder */
>   	encoder = list_first_entry(&(dev)->mode_config.encoder_list,
> @@ -808,9 +850,20 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>   		}
>   		priv->crtcs[priv->num_crtcs++] = crtc;
>   		encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
> +
> +		if (!dpu_encoder_is_external(encoder) &&
> +			!dpu_encoder_is_virtual(encoder))

if (dpu_encoder_internal_output(encoder))

> +			primary_enc_mask |= drm_encoder_mask(encoder);
> +		else if (dpu_encoder_is_external(encoder))
> +			external_enc_mask |= drm_encoder_mask(encoder);
> +
>   		encoder = list_next_entry(encoder, head);
>   	}
>   
> +	/* Prefer Primary encoders in registering for color support */
> +	num_dspps = _dpu_kms_attach_color(dev, primary_enc_mask, num_dspps);
> +	num_dspps = _dpu_kms_attach_color(dev, external_enc_mask, num_dspps);
> +
>   	return 0;
>   }
>   

-- 
With best wishes
Dmitry

RE: [PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc

[Kalyan Thota]

>-----Original Message-----
>From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>Sent: Wednesday, November 9, 2022 6:02 PM
>To: Kalyan Thota (QUIC) <quic_kalyant@quicinc.com>; dri-
>devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org;
>freedreno@lists.freedesktop.org; devicetree@vger.kernel.org
>Cc: linux-kernel@vger.kernel.org; robdclark@chromium.org;
>dianders@chromium.org; swboyd@chromium.org; Vinod Polimera (QUIC)
><quic_vpolimer@quicinc.com>; Abhinav Kumar (QUIC)
><quic_abhinavk@quicinc.com>
>Subject: Re: [PATCH 4/4] drm/msm/disp/dpu1: add color management support
>for the crtc
>
>WARNING: This email originated from outside of Qualcomm. Please be wary of
>any links or attachments, and do not enable macros.
>
>On 09/11/2022 15:16, Kalyan Thota wrote:
>> Add color management support for the crtc provided there are enough
>> dspps that can be allocated from the catalogue
>>
>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 ++++++--
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 ++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 53
>+++++++++++++++++++++++++++++
>>   4 files changed, 77 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 4170fbe..6bd3a64 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -18,9 +18,11 @@
>>   #include <drm/drm_flip_work.h>
>>   #include <drm/drm_framebuffer.h>
>>   #include <drm/drm_mode.h>
>> +#include <drm/drm_mode_object.h>
>>   #include <drm/drm_probe_helper.h>
>>   #include <drm/drm_rect.h>
>>   #include <drm/drm_vblank.h>
>> +#include "../../../drm_crtc_internal.h"
>
>If it's internal, it is not supposed to be used by other parties, including the msm
>drm. At least a comment why you are including this file would be helpful.
>
This header file was included to make use of " drm_mode_obj_find_prop_id" function from DRM framework.
Should I add a comment near function definition ?
>>
>>   #include "dpu_kms.h"
>>   #include "dpu_hw_lm.h"
>> @@ -553,6 +555,17 @@ static void _dpu_crtc_complete_flip(struct drm_crtc
>*crtc)
>>       spin_unlock_irqrestore(&dev->event_lock, flags);
>>   }
>>
>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc) {
>> +     u32 ctm_id = crtc->dev->mode_config.ctm_property->base.id;
>> +     u32 gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
>> +     u32 degamma_id =
>> +crtc->dev->mode_config.degamma_lut_property->base.id;
>> +
>> +     return !!(drm_mode_obj_find_prop_id(&crtc->base, ctm_id) ||
>> +                drm_mode_obj_find_prop_id(&crtc->base, gamma_id) ||
>> +                drm_mode_obj_find_prop_id(&crtc->base, degamma_id));
>> +}
>> +
>>   enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
>>   {
>>       struct drm_encoder *encoder;
>> @@ -1604,8 +1617,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device
>> *dev, struct drm_plane *plane,
>>
>>       drm_crtc_helper_add(crtc, &dpu_crtc_helper_funcs);
>>
>> -     drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>> -
>>       /* save user friendly CRTC name for later */
>>       snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u",
>> crtc->base.id);
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index 539b68b..8bac395 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> @@ -300,4 +300,10 @@ static inline enum dpu_crtc_client_type
>dpu_crtc_get_client_type(
>>       return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
>>   }
>>
>> +/**
>> + * dpu_crtc_has_color_enabled - check if the crtc has color
>> +management enabled
>> + * @crtc: Pointer to drm crtc object
>> + */
>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc);
>> +
>>   #endif /* _DPU_CRTC_H_ */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 4c56a16..ebc3f25 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 *crtc)
>>   {
>>       struct msm_display_topology topology = {0};
>>       int i, intf_count = 0;
>> @@ -573,11 +574,9 @@ 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))
>> +     if (dpu_crtc_has_color_enabled(crtc) &&
>> +             (dpu_kms->catalog->dspp_count >= topology.num_lm))
>
>See the comment to the previous patch. It still applies here.
>
>>                       topology.num_dspp = topology.num_lm;
>> -     }
>>
>>       topology.num_enc = 0;
>>       topology.num_intf = intf_count;
>> @@ -643,7 +642,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->crtc);
>>
>>       /* Reserve dynamic resources now. */
>>       if (!ret) {
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 552a89c..47a73fa 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -13,6 +13,7 @@
>>   #include <linux/dma-buf.h>
>>   #include <linux/of_irq.h>
>>   #include <linux/pm_opp.h>
>> +#include <linux/bitops.h>
>>
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_file.h>
>> @@ -537,6 +538,44 @@ static void dpu_kms_wait_flush(struct msm_kms
>*kms, unsigned crtc_mask)
>>               dpu_kms_wait_for_commit_done(kms, crtc);
>>   }
>>
>> +/**
>> + * _dpu_kms_possible_dspps - Evaluate how many dspps pairs can be
>facilitated
>> +                             to enable color features for crtcs.
>> + * @dpu_kms:    Pointer to dpu kms structure
>> + * Returns:     count of dspp pairs
>> + *
>> + * Baring single entry, if atleast 2 dspps are available in the
>> +catalogue,
>> + * then color can be enabled for that crtc  */ static inline u32
>> +_dpu_kms_possible_dspps(struct dpu_kms *dpu_kms) {
>> +
>> +     u32 num_dspps = dpu_kms->catalog->dspp_count;
>> +
>> +     if (num_dspps > 1)
>> +             num_dspps =
>> +                     !(num_dspps % 2) ? num_dspps / 2 : (num_dspps -
>> + 1) / 2;
>
>Ugh. No. Please spell this clearly rather than using nice math and ternary
>operators:
>
>if (num_dspps <= 1)
>   return num_dspps;
>else
>   return num_dspps / 2;
>
>You see, if num_dspps %2 ! =0, then num_dspps / 2 == (num_dspps_2 - 1) / 2.
>
>
>> +
>> +     return num_dspps;
>> +}
>> +
>> +static u32 _dpu_kms_attach_color(struct drm_device *dev, u32 enc_mask,
>> +                                             u32 num_dspps) {
>> +     struct drm_encoder *encoder;
>> +     struct drm_crtc *crtc;
>> +
>> +     drm_for_each_encoder_mask(encoder, dev, enc_mask) {
>> +             crtc = drm_crtc_from_index(dev, ffs(encoder->possible_crtcs) - 1);
>> +             if (num_dspps && crtc) {
>> +                     drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>> +                     num_dspps--;
>
>Please.  You can do this at the time you create the crtc. It would be much simpler.
>
>> +             }
>> +     }
>> +
>> +     return num_dspps;
>> +}
>> +
>>   static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>>                                   struct msm_drm_private *priv,
>>                                   struct dpu_kms *dpu_kms) @@ -747,6
>> +786,8 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>>
>>       int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret;
>>       int max_crtc_count;
>> +     u32 num_dspps, primary_enc_mask = 0, external_enc_mask = 0;
>> +
>>       dev = dpu_kms->dev;
>>       priv = dev->dev_private;
>>       catalog = dpu_kms->catalog;
>> @@ -796,6 +837,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
>*dpu_kms)
>>       }
>>
>>       max_crtc_count = min(max_crtc_count, primary_planes_idx);
>> +     num_dspps = _dpu_kms_possible_dspps(dpu_kms);
>>
>>       /* Create one CRTC per encoder */
>>       encoder = list_first_entry(&(dev)->mode_config.encoder_list,
>> @@ -808,9 +850,20 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
>*dpu_kms)
>>               }
>>               priv->crtcs[priv->num_crtcs++] = crtc;
>>               encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
>> +
>> +             if (!dpu_encoder_is_external(encoder) &&
>> +                     !dpu_encoder_is_virtual(encoder))
>
>if (dpu_encoder_internal_output(encoder))
>
>> +                     primary_enc_mask |= drm_encoder_mask(encoder);
>> +             else if (dpu_encoder_is_external(encoder))
>> +                     external_enc_mask |= drm_encoder_mask(encoder);
>> +
>>               encoder = list_next_entry(encoder, head);
>>       }
>>
>> +     /* Prefer Primary encoders in registering for color support */
>> +     num_dspps = _dpu_kms_attach_color(dev, primary_enc_mask,
>num_dspps);
>> +     num_dspps = _dpu_kms_attach_color(dev, external_enc_mask,
>> + num_dspps);
>> +
>>       return 0;
>>   }
>>
>
>--
>With best wishes
>Dmitry

Re: [PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc

[Dmitry Baryshkov]

On 09/11/2022 15:39, Kalyan Thota wrote:
> 
> 
>> -----Original Message-----
>> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Sent: Wednesday, November 9, 2022 6:02 PM
>> To: Kalyan Thota (QUIC) <quic_kalyant@quicinc.com>; dri-
>> devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org;
>> freedreno@lists.freedesktop.org; devicetree@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org; robdclark@chromium.org;
>> dianders@chromium.org; swboyd@chromium.org; Vinod Polimera (QUIC)
>> <quic_vpolimer@quicinc.com>; Abhinav Kumar (QUIC)
>> <quic_abhinavk@quicinc.com>
>> Subject: Re: [PATCH 4/4] drm/msm/disp/dpu1: add color management support
>> for the crtc
>>
>> WARNING: This email originated from outside of Qualcomm. Please be wary of
>> any links or attachments, and do not enable macros.
>>
>> On 09/11/2022 15:16, Kalyan Thota wrote:
>>> Add color management support for the crtc provided there are enough
>>> dspps that can be allocated from the catalogue
>>>
>>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 ++++++--
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 ++++
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 53
>> +++++++++++++++++++++++++++++
>>>    4 files changed, 77 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> index 4170fbe..6bd3a64 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> @@ -18,9 +18,11 @@
>>>    #include <drm/drm_flip_work.h>
>>>    #include <drm/drm_framebuffer.h>
>>>    #include <drm/drm_mode.h>
>>> +#include <drm/drm_mode_object.h>
>>>    #include <drm/drm_probe_helper.h>
>>>    #include <drm/drm_rect.h>
>>>    #include <drm/drm_vblank.h>
>>> +#include "../../../drm_crtc_internal.h"
>>
>> If it's internal, it is not supposed to be used by other parties, including the msm
>> drm. At least a comment why you are including this file would be helpful.
>>
> This header file was included to make use of " drm_mode_obj_find_prop_id" function from DRM framework.
> Should I add a comment near function definition ?

No, it would have been better to add a comment near the #include 
directive. However if this is the only user, you don't need it at all. 
You see, you know whether the CRTC has color management propreties at 
the time of creation. And this can not change. So, you just add a 
boolean to dpu_crtc (or dpu_crtc_state, whichever suits better).

>>>
>>>    #include "dpu_kms.h"
>>>    #include "dpu_hw_lm.h"
>>> @@ -553,6 +555,17 @@ static void _dpu_crtc_complete_flip(struct drm_crtc
>> *crtc)
>>>        spin_unlock_irqrestore(&dev->event_lock, flags);
>>>    }
>>>
>>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc) {
>>> +     u32 ctm_id = crtc->dev->mode_config.ctm_property->base.id;
>>> +     u32 gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
>>> +     u32 degamma_id =
>>> +crtc->dev->mode_config.degamma_lut_property->base.id;
>>> +
>>> +     return !!(drm_mode_obj_find_prop_id(&crtc->base, ctm_id) ||
>>> +                drm_mode_obj_find_prop_id(&crtc->base, gamma_id) ||
>>> +                drm_mode_obj_find_prop_id(&crtc->base, degamma_id));
>>> +}
>>> +
>>>    enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
>>>    {
>>>        struct drm_encoder *encoder;
>>> @@ -1604,8 +1617,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device
>>> *dev, struct drm_plane *plane,
>>>
>>>        drm_crtc_helper_add(crtc, &dpu_crtc_helper_funcs);
>>>
>>> -     drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>>> -
>>>        /* save user friendly CRTC name for later */
>>>        snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u",
>>> crtc->base.id);
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> index 539b68b..8bac395 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> @@ -300,4 +300,10 @@ static inline enum dpu_crtc_client_type
>> dpu_crtc_get_client_type(
>>>        return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
>>>    }
>>>
>>> +/**
>>> + * dpu_crtc_has_color_enabled - check if the crtc has color
>>> +management enabled
>>> + * @crtc: Pointer to drm crtc object
>>> + */
>>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc);
>>> +
>>>    #endif /* _DPU_CRTC_H_ */
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 4c56a16..ebc3f25 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 *crtc)
>>>    {
>>>        struct msm_display_topology topology = {0};
>>>        int i, intf_count = 0;
>>> @@ -573,11 +574,9 @@ 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))
>>> +     if (dpu_crtc_has_color_enabled(crtc) &&
>>> +             (dpu_kms->catalog->dspp_count >= topology.num_lm))
>>
>> See the comment to the previous patch. It still applies here.
>>
>>>                        topology.num_dspp = topology.num_lm;
>>> -     }
>>>
>>>        topology.num_enc = 0;
>>>        topology.num_intf = intf_count;
>>> @@ -643,7 +642,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->crtc);
>>>
>>>        /* Reserve dynamic resources now. */
>>>        if (!ret) {
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 552a89c..47a73fa 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -13,6 +13,7 @@
>>>    #include <linux/dma-buf.h>
>>>    #include <linux/of_irq.h>
>>>    #include <linux/pm_opp.h>
>>> +#include <linux/bitops.h>
>>>
>>>    #include <drm/drm_crtc.h>
>>>    #include <drm/drm_file.h>
>>> @@ -537,6 +538,44 @@ static void dpu_kms_wait_flush(struct msm_kms
>> *kms, unsigned crtc_mask)
>>>                dpu_kms_wait_for_commit_done(kms, crtc);
>>>    }
>>>
>>> +/**
>>> + * _dpu_kms_possible_dspps - Evaluate how many dspps pairs can be
>> facilitated
>>> +                             to enable color features for crtcs.
>>> + * @dpu_kms:    Pointer to dpu kms structure
>>> + * Returns:     count of dspp pairs
>>> + *
>>> + * Baring single entry, if atleast 2 dspps are available in the
>>> +catalogue,
>>> + * then color can be enabled for that crtc  */ static inline u32
>>> +_dpu_kms_possible_dspps(struct dpu_kms *dpu_kms) {
>>> +
>>> +     u32 num_dspps = dpu_kms->catalog->dspp_count;
>>> +
>>> +     if (num_dspps > 1)
>>> +             num_dspps =
>>> +                     !(num_dspps % 2) ? num_dspps / 2 : (num_dspps -
>>> + 1) / 2;
>>
>> Ugh. No. Please spell this clearly rather than using nice math and ternary
>> operators:
>>
>> if (num_dspps <= 1)
>>    return num_dspps;
>> else
>>    return num_dspps / 2;
>>
>> You see, if num_dspps %2 ! =0, then num_dspps / 2 == (num_dspps_2 - 1) / 2.
>>
>>
>>> +
>>> +     return num_dspps;
>>> +}
>>> +
>>> +static u32 _dpu_kms_attach_color(struct drm_device *dev, u32 enc_mask,
>>> +                                             u32 num_dspps) {
>>> +     struct drm_encoder *encoder;
>>> +     struct drm_crtc *crtc;
>>> +
>>> +     drm_for_each_encoder_mask(encoder, dev, enc_mask) {
>>> +             crtc = drm_crtc_from_index(dev, ffs(encoder->possible_crtcs) - 1);
>>> +             if (num_dspps && crtc) {
>>> +                     drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>>> +                     num_dspps--;
>>
>> Please.  You can do this at the time you create the crtc. It would be much simpler.
>>
>>> +             }
>>> +     }
>>> +
>>> +     return num_dspps;
>>> +}
>>> +
>>>    static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>>>                                    struct msm_drm_private *priv,
>>>                                    struct dpu_kms *dpu_kms) @@ -747,6
>>> +786,8 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>>>
>>>        int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret;
>>>        int max_crtc_count;
>>> +     u32 num_dspps, primary_enc_mask = 0, external_enc_mask = 0;
>>> +
>>>        dev = dpu_kms->dev;
>>>        priv = dev->dev_private;
>>>        catalog = dpu_kms->catalog;
>>> @@ -796,6 +837,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
>> *dpu_kms)
>>>        }
>>>
>>>        max_crtc_count = min(max_crtc_count, primary_planes_idx);
>>> +     num_dspps = _dpu_kms_possible_dspps(dpu_kms);
>>>
>>>        /* Create one CRTC per encoder */
>>>        encoder = list_first_entry(&(dev)->mode_config.encoder_list,
>>> @@ -808,9 +850,20 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
>> *dpu_kms)
>>>                }
>>>                priv->crtcs[priv->num_crtcs++] = crtc;
>>>                encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
>>> +
>>> +             if (!dpu_encoder_is_external(encoder) &&
>>> +                     !dpu_encoder_is_virtual(encoder))
>>
>> if (dpu_encoder_internal_output(encoder))
>>
>>> +                     primary_enc_mask |= drm_encoder_mask(encoder);
>>> +             else if (dpu_encoder_is_external(encoder))
>>> +                     external_enc_mask |= drm_encoder_mask(encoder);
>>> +
>>>                encoder = list_next_entry(encoder, head);
>>>        }
>>>
>>> +     /* Prefer Primary encoders in registering for color support */
>>> +     num_dspps = _dpu_kms_attach_color(dev, primary_enc_mask,
>> num_dspps);
>>> +     num_dspps = _dpu_kms_attach_color(dev, external_enc_mask,
>>> + num_dspps);
>>> +
>>>        return 0;
>>>    }
>>>
>>
>> --
>> With best wishes
>> Dmitry
> 

-- 
With best wishes
Dmitry

RE: [PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc

[Kalyan Thota]

>-----Original Message-----
>From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>Sent: Wednesday, November 9, 2022 6:02 PM
>To: Kalyan Thota (QUIC) <quic_kalyant@quicinc.com>; dri-
>devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org;
>freedreno@lists.freedesktop.org; devicetree@vger.kernel.org
>Cc: linux-kernel@vger.kernel.org; robdclark@chromium.org;
>dianders@chromium.org; swboyd@chromium.org; Vinod Polimera (QUIC)
><quic_vpolimer@quicinc.com>; Abhinav Kumar (QUIC)
><quic_abhinavk@quicinc.com>
>Subject: Re: [PATCH 4/4] drm/msm/disp/dpu1: add color management support
>for the crtc
>
>WARNING: This email originated from outside of Qualcomm. Please be wary of
>any links or attachments, and do not enable macros.
>
>On 09/11/2022 15:16, Kalyan Thota wrote:
>> Add color management support for the crtc provided there are enough
>> dspps that can be allocated from the catalogue
>>
>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 ++++++--
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 ++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 53
>+++++++++++++++++++++++++++++
>>   4 files changed, 77 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 4170fbe..6bd3a64 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -18,9 +18,11 @@
>>   #include <drm/drm_flip_work.h>
>>   #include <drm/drm_framebuffer.h>
>>   #include <drm/drm_mode.h>
>> +#include <drm/drm_mode_object.h>
>>   #include <drm/drm_probe_helper.h>
>>   #include <drm/drm_rect.h>
>>   #include <drm/drm_vblank.h>
>> +#include "../../../drm_crtc_internal.h"
>
>If it's internal, it is not supposed to be used by other parties, including the msm
>drm. At least a comment why you are including this file would be helpful.
>
>>
>>   #include "dpu_kms.h"
>>   #include "dpu_hw_lm.h"
>> @@ -553,6 +555,17 @@ static void _dpu_crtc_complete_flip(struct drm_crtc
>*crtc)
>>       spin_unlock_irqrestore(&dev->event_lock, flags);
>>   }
>>
>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc) {
>> +     u32 ctm_id = crtc->dev->mode_config.ctm_property->base.id;
>> +     u32 gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
>> +     u32 degamma_id =
>> +crtc->dev->mode_config.degamma_lut_property->base.id;
>> +
>> +     return !!(drm_mode_obj_find_prop_id(&crtc->base, ctm_id) ||
>> +                drm_mode_obj_find_prop_id(&crtc->base, gamma_id) ||
>> +                drm_mode_obj_find_prop_id(&crtc->base, degamma_id));
>> +}
>> +
>>   enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
>>   {
>>       struct drm_encoder *encoder;
>> @@ -1604,8 +1617,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device
>> *dev, struct drm_plane *plane,
>>
>>       drm_crtc_helper_add(crtc, &dpu_crtc_helper_funcs);
>>
>> -     drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>> -
>>       /* save user friendly CRTC name for later */
>>       snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u",
>> crtc->base.id);
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index 539b68b..8bac395 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> @@ -300,4 +300,10 @@ static inline enum dpu_crtc_client_type
>dpu_crtc_get_client_type(
>>       return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
>>   }
>>
>> +/**
>> + * dpu_crtc_has_color_enabled - check if the crtc has color
>> +management enabled
>> + * @crtc: Pointer to drm crtc object
>> + */
>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc);
>> +
>>   #endif /* _DPU_CRTC_H_ */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 4c56a16..ebc3f25 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 *crtc)
>>   {
>>       struct msm_display_topology topology = {0};
>>       int i, intf_count = 0;
>> @@ -573,11 +574,9 @@ 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))
>> +     if (dpu_crtc_has_color_enabled(crtc) &&
>> +             (dpu_kms->catalog->dspp_count >= topology.num_lm))
>
>See the comment to the previous patch. It still applies here.
>
>>                       topology.num_dspp = topology.num_lm;
>> -     }
>>
>>       topology.num_enc = 0;
>>       topology.num_intf = intf_count;
>> @@ -643,7 +642,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->crtc);
>>
>>       /* Reserve dynamic resources now. */
>>       if (!ret) {
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 552a89c..47a73fa 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -13,6 +13,7 @@
>>   #include <linux/dma-buf.h>
>>   #include <linux/of_irq.h>
>>   #include <linux/pm_opp.h>
>> +#include <linux/bitops.h>
>>
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_file.h>
>> @@ -537,6 +538,44 @@ static void dpu_kms_wait_flush(struct msm_kms
>*kms, unsigned crtc_mask)
>>               dpu_kms_wait_for_commit_done(kms, crtc);
>>   }
>>
>> +/**
>> + * _dpu_kms_possible_dspps - Evaluate how many dspps pairs can be
>facilitated
>> +                             to enable color features for crtcs.
>> + * @dpu_kms:    Pointer to dpu kms structure
>> + * Returns:     count of dspp pairs
>> + *
>> + * Baring single entry, if atleast 2 dspps are available in the
>> +catalogue,
>> + * then color can be enabled for that crtc  */ static inline u32
>> +_dpu_kms_possible_dspps(struct dpu_kms *dpu_kms) {
>> +
>> +     u32 num_dspps = dpu_kms->catalog->dspp_count;
>> +
>> +     if (num_dspps > 1)
>> +             num_dspps =
>> +                     !(num_dspps % 2) ? num_dspps / 2 : (num_dspps -
>> + 1) / 2;
>
>Ugh. No. Please spell this clearly rather than using nice math and ternary
>operators:
>
>if (num_dspps <= 1)
>   return num_dspps;
>else
>   return num_dspps / 2;
>
>You see, if num_dspps %2 ! =0, then num_dspps / 2 == (num_dspps_2 - 1) / 2.
>
>
>> +
>> +     return num_dspps;
>> +}
>> +
>> +static u32 _dpu_kms_attach_color(struct drm_device *dev, u32 enc_mask,
>> +                                             u32 num_dspps) {
>> +     struct drm_encoder *encoder;
>> +     struct drm_crtc *crtc;
>> +
>> +     drm_for_each_encoder_mask(encoder, dev, enc_mask) {
>> +             crtc = drm_crtc_from_index(dev, ffs(encoder->possible_crtcs) - 1);
>> +             if (num_dspps && crtc) {
>> +                     drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>> +                     num_dspps--;
>
>Please.  You can do this at the time you create the crtc. It would be much simpler.
>
Following are the cases that I have considered.
1) DP can be first among the encoder list
2) we can have more than 1 primary display.

Since encoder list operation is iterative, we cannot go back and set the color, if we end up having unused dspp's
Secondly, it will be a bit complex logic to save the dspps for primary assuming DSI/eDP will be later in the list, and comeback to assign to DP as well if there are enough dspps (run through the loop twice)

I have used 2 encoder masks for primary and external. And i have preferred all the primary displays to get a first allocation and then move to external.

Let me know if your thoughts.


>> +             }
>> +     }
>> +
>> +     return num_dspps;
>> +}
>> +
>>   static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>>                                   struct msm_drm_private *priv,
>>                                   struct dpu_kms *dpu_kms) @@ -747,6
>> +786,8 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>>
>>       int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret;
>>       int max_crtc_count;
>> +     u32 num_dspps, primary_enc_mask = 0, external_enc_mask = 0;
>> +
>>       dev = dpu_kms->dev;
>>       priv = dev->dev_private;
>>       catalog = dpu_kms->catalog;
>> @@ -796,6 +837,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
>*dpu_kms)
>>       }
>>
>>       max_crtc_count = min(max_crtc_count, primary_planes_idx);
>> +     num_dspps = _dpu_kms_possible_dspps(dpu_kms);
>>
>>       /* Create one CRTC per encoder */
>>       encoder = list_first_entry(&(dev)->mode_config.encoder_list,
>> @@ -808,9 +850,20 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
>*dpu_kms)
>>               }
>>               priv->crtcs[priv->num_crtcs++] = crtc;
>>               encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
>> +
>> +             if (!dpu_encoder_is_external(encoder) &&
>> +                     !dpu_encoder_is_virtual(encoder))
>
>if (dpu_encoder_internal_output(encoder))
>
>> +                     primary_enc_mask |= drm_encoder_mask(encoder);
>> +             else if (dpu_encoder_is_external(encoder))
>> +                     external_enc_mask |= drm_encoder_mask(encoder);
>> +
>>               encoder = list_next_entry(encoder, head);
>>       }
>>
>> +     /* Prefer Primary encoders in registering for color support */
>> +     num_dspps = _dpu_kms_attach_color(dev, primary_enc_mask,
>num_dspps);
>> +     num_dspps = _dpu_kms_attach_color(dev, external_enc_mask,
>> + num_dspps);
>> +
>>       return 0;
>>   }
>>
>
>--
>With best wishes
>Dmitry

RE: [PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc

[Kalyan Thota]

>-----Original Message-----
>From: Kalyan Thota <kalyant@qti.qualcomm.com>
>Sent: Wednesday, November 9, 2022 6:53 PM
>To: dmitry.baryshkov@linaro.org; Kalyan Thota (QUIC)
><quic_kalyant@quicinc.com>; dri-devel@lists.freedesktop.org; linux-arm-
>msm@vger.kernel.org; freedreno@lists.freedesktop.org;
>devicetree@vger.kernel.org
>Cc: linux-kernel@vger.kernel.org; robdclark@chromium.org;
>dianders@chromium.org; swboyd@chromium.org; Vinod Polimera (QUIC)
><quic_vpolimer@quicinc.com>; Abhinav Kumar (QUIC)
><quic_abhinavk@quicinc.com>
>Subject: RE: [PATCH 4/4] drm/msm/disp/dpu1: add color management support
>for the crtc
>
>
>
>>-----Original Message-----
>>From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>Sent: Wednesday, November 9, 2022 6:02 PM
>>To: Kalyan Thota (QUIC) <quic_kalyant@quicinc.com>; dri-
>>devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org;
>>freedreno@lists.freedesktop.org; devicetree@vger.kernel.org
>>Cc: linux-kernel@vger.kernel.org; robdclark@chromium.org;
>>dianders@chromium.org; swboyd@chromium.org; Vinod Polimera (QUIC)
>><quic_vpolimer@quicinc.com>; Abhinav Kumar (QUIC)
>><quic_abhinavk@quicinc.com>
>>Subject: Re: [PATCH 4/4] drm/msm/disp/dpu1: add color management
>>support for the crtc
>>
>>WARNING: This email originated from outside of Qualcomm. Please be wary
>>of any links or attachments, and do not enable macros.
>>
>>On 09/11/2022 15:16, Kalyan Thota wrote:
>>> Add color management support for the crtc provided there are enough
>>> dspps that can be allocated from the catalogue
>>>
>>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 ++++++--
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 ++++
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 53
>>+++++++++++++++++++++++++++++
>>>   4 files changed, 77 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> index 4170fbe..6bd3a64 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> @@ -18,9 +18,11 @@
>>>   #include <drm/drm_flip_work.h>
>>>   #include <drm/drm_framebuffer.h>
>>>   #include <drm/drm_mode.h>
>>> +#include <drm/drm_mode_object.h>
>>>   #include <drm/drm_probe_helper.h>
>>>   #include <drm/drm_rect.h>
>>>   #include <drm/drm_vblank.h>
>>> +#include "../../../drm_crtc_internal.h"
>>
>>If it's internal, it is not supposed to be used by other parties,
>>including the msm drm. At least a comment why you are including this file would
>be helpful.
>>
>>>
>>>   #include "dpu_kms.h"
>>>   #include "dpu_hw_lm.h"
>>> @@ -553,6 +555,17 @@ static void _dpu_crtc_complete_flip(struct
>>> drm_crtc
>>*crtc)
>>>       spin_unlock_irqrestore(&dev->event_lock, flags);
>>>   }
>>>
>>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc) {
>>> +     u32 ctm_id = crtc->dev->mode_config.ctm_property->base.id;
>>> +     u32 gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
>>> +     u32 degamma_id =
>>> +crtc->dev->mode_config.degamma_lut_property->base.id;
>>> +
>>> +     return !!(drm_mode_obj_find_prop_id(&crtc->base, ctm_id) ||
>>> +                drm_mode_obj_find_prop_id(&crtc->base, gamma_id) ||
>>> +                drm_mode_obj_find_prop_id(&crtc->base, degamma_id));
>>> +}
>>> +
>>>   enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
>>>   {
>>>       struct drm_encoder *encoder;
>>> @@ -1604,8 +1617,6 @@ struct drm_crtc *dpu_crtc_init(struct
>>> drm_device *dev, struct drm_plane *plane,
>>>
>>>       drm_crtc_helper_add(crtc, &dpu_crtc_helper_funcs);
>>>
>>> -     drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>>> -
>>>       /* save user friendly CRTC name for later */
>>>       snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u",
>>> crtc->base.id);
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> index 539b68b..8bac395 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> @@ -300,4 +300,10 @@ static inline enum dpu_crtc_client_type
>>dpu_crtc_get_client_type(
>>>       return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
>>>   }
>>>
>>> +/**
>>> + * dpu_crtc_has_color_enabled - check if the crtc has color
>>> +management enabled
>>> + * @crtc: Pointer to drm crtc object  */ bool
>>> +dpu_crtc_has_color_enabled(struct drm_crtc *crtc);
>>> +
>>>   #endif /* _DPU_CRTC_H_ */
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 4c56a16..ebc3f25 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 *crtc)
>>>   {
>>>       struct msm_display_topology topology = {0};
>>>       int i, intf_count = 0;
>>> @@ -573,11 +574,9 @@ 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))
>>> +     if (dpu_crtc_has_color_enabled(crtc) &&
>>> +             (dpu_kms->catalog->dspp_count >= topology.num_lm))
>>
>>See the comment to the previous patch. It still applies here.
>>
>>>                       topology.num_dspp = topology.num_lm;
>>> -     }
>>>
>>>       topology.num_enc = 0;
>>>       topology.num_intf = intf_count; @@ -643,7 +642,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->crtc);
>>>
>>>       /* Reserve dynamic resources now. */
>>>       if (!ret) {
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 552a89c..47a73fa 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -13,6 +13,7 @@
>>>   #include <linux/dma-buf.h>
>>>   #include <linux/of_irq.h>
>>>   #include <linux/pm_opp.h>
>>> +#include <linux/bitops.h>
>>>
>>>   #include <drm/drm_crtc.h>
>>>   #include <drm/drm_file.h>
>>> @@ -537,6 +538,44 @@ static void dpu_kms_wait_flush(struct msm_kms
>>*kms, unsigned crtc_mask)
>>>               dpu_kms_wait_for_commit_done(kms, crtc);
>>>   }
>>>
>>> +/**
>>> + * _dpu_kms_possible_dspps - Evaluate how many dspps pairs can be
>>facilitated
>>> +                             to enable color features for crtcs.
>>> + * @dpu_kms:    Pointer to dpu kms structure
>>> + * Returns:     count of dspp pairs
>>> + *
>>> + * Baring single entry, if atleast 2 dspps are available in the
>>> +catalogue,
>>> + * then color can be enabled for that crtc  */ static inline u32
>>> +_dpu_kms_possible_dspps(struct dpu_kms *dpu_kms) {
>>> +
>>> +     u32 num_dspps = dpu_kms->catalog->dspp_count;
>>> +
>>> +     if (num_dspps > 1)
>>> +             num_dspps =
>>> +                     !(num_dspps % 2) ? num_dspps / 2 : (num_dspps -
>>> + 1) / 2;
>>
>>Ugh. No. Please spell this clearly rather than using nice math and
>>ternary
>>operators:
>>
>>if (num_dspps <= 1)
>>   return num_dspps;
>>else
>>   return num_dspps / 2;
>>
>>You see, if num_dspps %2 ! =0, then num_dspps / 2 == (num_dspps_2 - 1) / 2.
>>
>>
>>> +
>>> +     return num_dspps;
>>> +}
>>> +
>>> +static u32 _dpu_kms_attach_color(struct drm_device *dev, u32 enc_mask,
>>> +                                             u32 num_dspps) {
>>> +     struct drm_encoder *encoder;
>>> +     struct drm_crtc *crtc;
>>> +
>>> +     drm_for_each_encoder_mask(encoder, dev, enc_mask) {
>>> +             crtc = drm_crtc_from_index(dev, ffs(encoder->possible_crtcs) - 1);
>>> +             if (num_dspps && crtc) {
>>> +                     drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>>> +                     num_dspps--;
>>
>>Please.  You can do this at the time you create the crtc. It would be much
>simpler.
>>
>Following are the cases that I have considered.
>1) DP can be first among the encoder list
>2) we can have more than 1 primary display.
>
>Since encoder list operation is iterative, we cannot go back and set the color, if
>we end up having unused dspp's Secondly, it will be a bit complex logic to save
>the dspps for primary assuming DSI/eDP will be later in the list, and comeback to
>assign to DP as well if there are enough dspps (run through the loop twice)
>
>I have used 2 encoder masks for primary and external. And i have preferred all the
>primary displays to get a first allocation and then move to external.
>
>Let me know if your thoughts.
>
On second thought, with the addition of connector_type, logic can be simplified, let me add those changes
>
>>> +             }
>>> +     }
>>> +
>>> +     return num_dspps;
>>> +}
>>> +
>>>   static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>>>                                   struct msm_drm_private *priv,
>>>                                   struct dpu_kms *dpu_kms) @@ -747,6
>>> +786,8 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>>>
>>>       int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret;
>>>       int max_crtc_count;
>>> +     u32 num_dspps, primary_enc_mask = 0, external_enc_mask = 0;
>>> +
>>>       dev = dpu_kms->dev;
>>>       priv = dev->dev_private;
>>>       catalog = dpu_kms->catalog;
>>> @@ -796,6 +837,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
>>*dpu_kms)
>>>       }
>>>
>>>       max_crtc_count = min(max_crtc_count, primary_planes_idx);
>>> +     num_dspps = _dpu_kms_possible_dspps(dpu_kms);
>>>
>>>       /* Create one CRTC per encoder */
>>>       encoder = list_first_entry(&(dev)->mode_config.encoder_list,
>>> @@ -808,9 +850,20 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
>>*dpu_kms)
>>>               }
>>>               priv->crtcs[priv->num_crtcs++] = crtc;
>>>               encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
>>> +
>>> +             if (!dpu_encoder_is_external(encoder) &&
>>> +                     !dpu_encoder_is_virtual(encoder))
>>
>>if (dpu_encoder_internal_output(encoder))
>>
>>> +                     primary_enc_mask |= drm_encoder_mask(encoder);
>>> +             else if (dpu_encoder_is_external(encoder))
>>> +                     external_enc_mask |= drm_encoder_mask(encoder);
>>> +
>>>               encoder = list_next_entry(encoder, head);
>>>       }
>>>
>>> +     /* Prefer Primary encoders in registering for color support */
>>> +     num_dspps = _dpu_kms_attach_color(dev, primary_enc_mask,
>>num_dspps);
>>> +     num_dspps = _dpu_kms_attach_color(dev, external_enc_mask,
>>> + num_dspps);
>>> +
>>>       return 0;
>>>   }
>>>
>>
>>--
>>With best wishes
>>Dmitry
>

RE: [PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc

[Kalyan Thota]

>-----Original Message-----
>From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>Sent: Wednesday, November 9, 2022 6:02 PM
>To: Kalyan Thota (QUIC) <quic_kalyant@quicinc.com>; dri-
>devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org;
>freedreno@lists.freedesktop.org; devicetree@vger.kernel.org
>Cc: linux-kernel@vger.kernel.org; robdclark@chromium.org;
>dianders@chromium.org; swboyd@chromium.org; Vinod Polimera (QUIC)
><quic_vpolimer@quicinc.com>; Abhinav Kumar (QUIC)
><quic_abhinavk@quicinc.com>
>Subject: Re: [PATCH 4/4] drm/msm/disp/dpu1: add color management support
>for the crtc
>
>WARNING: This email originated from outside of Qualcomm. Please be wary of
>any links or attachments, and do not enable macros.
>
>On 09/11/2022 15:16, Kalyan Thota wrote:
>> Add color management support for the crtc provided there are enough
>> dspps that can be allocated from the catalogue
>>
>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 ++++++--
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 ++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 53
>+++++++++++++++++++++++++++++
>>   4 files changed, 77 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 4170fbe..6bd3a64 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -18,9 +18,11 @@
>>   #include <drm/drm_flip_work.h>
>>   #include <drm/drm_framebuffer.h>
>>   #include <drm/drm_mode.h>
>> +#include <drm/drm_mode_object.h>
>>   #include <drm/drm_probe_helper.h>
>>   #include <drm/drm_rect.h>
>>   #include <drm/drm_vblank.h>
>> +#include "../../../drm_crtc_internal.h"
>
>If it's internal, it is not supposed to be used by other parties, including the msm
>drm. At least a comment why you are including this file would be helpful.
>
>>
>>   #include "dpu_kms.h"
>>   #include "dpu_hw_lm.h"
>> @@ -553,6 +555,17 @@ static void _dpu_crtc_complete_flip(struct drm_crtc
>*crtc)
>>       spin_unlock_irqrestore(&dev->event_lock, flags);
>>   }
>>
>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc) {
>> +     u32 ctm_id = crtc->dev->mode_config.ctm_property->base.id;
>> +     u32 gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
>> +     u32 degamma_id =
>> +crtc->dev->mode_config.degamma_lut_property->base.id;
>> +
>> +     return !!(drm_mode_obj_find_prop_id(&crtc->base, ctm_id) ||
>> +                drm_mode_obj_find_prop_id(&crtc->base, gamma_id) ||
>> +                drm_mode_obj_find_prop_id(&crtc->base, degamma_id));
>> +}
>> +
>>   enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
>>   {
>>       struct drm_encoder *encoder;
>> @@ -1604,8 +1617,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device
>> *dev, struct drm_plane *plane,
>>
>>       drm_crtc_helper_add(crtc, &dpu_crtc_helper_funcs);
>>
>> -     drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>> -
>>       /* save user friendly CRTC name for later */
>>       snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u",
>> crtc->base.id);
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index 539b68b..8bac395 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> @@ -300,4 +300,10 @@ static inline enum dpu_crtc_client_type
>dpu_crtc_get_client_type(
>>       return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
>>   }
>>
>> +/**
>> + * dpu_crtc_has_color_enabled - check if the crtc has color
>> +management enabled
>> + * @crtc: Pointer to drm crtc object
>> + */
>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc);
>> +
>>   #endif /* _DPU_CRTC_H_ */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 4c56a16..ebc3f25 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 *crtc)
>>   {
>>       struct msm_display_topology topology = {0};
>>       int i, intf_count = 0;
>> @@ -573,11 +574,9 @@ 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))
>> +     if (dpu_crtc_has_color_enabled(crtc) &&
>> +             (dpu_kms->catalog->dspp_count >= topology.num_lm))
>
>See the comment to the previous patch. It still applies here.
>
This function will only populate the requirements for a given topology based on mode. If there are not enough resources as per requirements, dpu_rm_reserve will fail the modeset.

>>                       topology.num_dspp = topology.num_lm;
>> -     }
>>
>>       topology.num_enc = 0;
>>       topology.num_intf = intf_count;
>> @@ -643,7 +642,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->crtc);
>>
>>       /* Reserve dynamic resources now. */
>>       if (!ret) {
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 552a89c..47a73fa 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -13,6 +13,7 @@
>>   #include <linux/dma-buf.h>
>>   #include <linux/of_irq.h>
>>   #include <linux/pm_opp.h>
>> +#include <linux/bitops.h>
>>
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_file.h>
>> @@ -537,6 +538,44 @@ static void dpu_kms_wait_flush(struct msm_kms
>*kms, unsigned crtc_mask)
>>               dpu_kms_wait_for_commit_done(kms, crtc);
>>   }
>>
>> +/**
>> + * _dpu_kms_possible_dspps - Evaluate how many dspps pairs can be
>facilitated
>> +                             to enable color features for crtcs.
>> + * @dpu_kms:    Pointer to dpu kms structure
>> + * Returns:     count of dspp pairs
>> + *
>> + * Baring single entry, if atleast 2 dspps are available in the
>> +catalogue,
>> + * then color can be enabled for that crtc  */ static inline u32
>> +_dpu_kms_possible_dspps(struct dpu_kms *dpu_kms) {
>> +
>> +     u32 num_dspps = dpu_kms->catalog->dspp_count;
>> +
>> +     if (num_dspps > 1)
>> +             num_dspps =
>> +                     !(num_dspps % 2) ? num_dspps / 2 : (num_dspps -
>> + 1) / 2;
>
>Ugh. No. Please spell this clearly rather than using nice math and ternary
>operators:
>
>if (num_dspps <= 1)
>   return num_dspps;
>else
>   return num_dspps / 2;
>
>You see, if num_dspps %2 ! =0, then num_dspps / 2 == (num_dspps_2 - 1) / 2.
>
>
>> +
>> +     return num_dspps;
>> +}
>> +
>> +static u32 _dpu_kms_attach_color(struct drm_device *dev, u32 enc_mask,
>> +                                             u32 num_dspps) {
>> +     struct drm_encoder *encoder;
>> +     struct drm_crtc *crtc;
>> +
>> +     drm_for_each_encoder_mask(encoder, dev, enc_mask) {
>> +             crtc = drm_crtc_from_index(dev, ffs(encoder->possible_crtcs) - 1);
>> +             if (num_dspps && crtc) {
>> +                     drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>> +                     num_dspps--;
>
>Please.  You can do this at the time you create the crtc. It would be much simpler.
>
>> +             }
>> +     }
>> +
>> +     return num_dspps;
>> +}
>> +
>>   static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>>                                   struct msm_drm_private *priv,
>>                                   struct dpu_kms *dpu_kms) @@ -747,6
>> +786,8 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>>
>>       int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret;
>>       int max_crtc_count;
>> +     u32 num_dspps, primary_enc_mask = 0, external_enc_mask = 0;
>> +
>>       dev = dpu_kms->dev;
>>       priv = dev->dev_private;
>>       catalog = dpu_kms->catalog;
>> @@ -796,6 +837,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
>*dpu_kms)
>>       }
>>
>>       max_crtc_count = min(max_crtc_count, primary_planes_idx);
>> +     num_dspps = _dpu_kms_possible_dspps(dpu_kms);
>>
>>       /* Create one CRTC per encoder */
>>       encoder = list_first_entry(&(dev)->mode_config.encoder_list,
>> @@ -808,9 +850,20 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
>*dpu_kms)
>>               }
>>               priv->crtcs[priv->num_crtcs++] = crtc;
>>               encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
>> +
>> +             if (!dpu_encoder_is_external(encoder) &&
>> +                     !dpu_encoder_is_virtual(encoder))
>
>if (dpu_encoder_internal_output(encoder))
>
>> +                     primary_enc_mask |= drm_encoder_mask(encoder);
>> +             else if (dpu_encoder_is_external(encoder))
>> +                     external_enc_mask |= drm_encoder_mask(encoder);
>> +
>>               encoder = list_next_entry(encoder, head);
>>       }
>>
>> +     /* Prefer Primary encoders in registering for color support */
>> +     num_dspps = _dpu_kms_attach_color(dev, primary_enc_mask,
>num_dspps);
>> +     num_dspps = _dpu_kms_attach_color(dev, external_enc_mask,
>> + num_dspps);
>> +
>>       return 0;
>>   }
>>
>
>--
>With best wishes
>Dmitry