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

  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(-)
  

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;
>   }
>
  

>-----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
  

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
>
  

>-----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
  

>-----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
>
  

>-----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
  

Hi Kalyan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.1-rc4 next-20221111]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kalyan-Thota/drm-msm-disp-dpu1-pin-1-crtc-to-1-encoder/20221109-201925
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/1667996206-4153-4-git-send-email-quic_kalyant%40quicinc.com
patch subject: [PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc
config: arc-buildonly-randconfig-r005-20221111
compiler: arceb-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/e3a68199ddde8a052bc837e4a7cdaed7278c5528
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kalyan-Thota/drm-msm-disp-dpu1-pin-1-crtc-to-1-encoder/20221109-201925
        git checkout e3a68199ddde8a052bc837e4a7cdaed7278c5528
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "drm_mode_obj_find_prop_id" [drivers/gpu/drm/msm/msm.ko] undefined!