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 <[email protected]>
> ---
> 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
>-----Original Message-----
>From: Dmitry Baryshkov <[email protected]>
>Sent: Wednesday, November 9, 2022 6:02 PM
>To: Kalyan Thota (QUIC) <[email protected]>; dri-
>[email protected]; [email protected];
>[email protected]; [email protected]
>Cc: [email protected]; [email protected];
>[email protected]; [email protected]; Vinod Polimera (QUIC)
><[email protected]>; Abhinav Kumar (QUIC)
><[email protected]>
>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 <[email protected]>
>> ---
>> 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 <[email protected]>
>> Sent: Wednesday, November 9, 2022 6:02 PM
>> To: Kalyan Thota (QUIC) <[email protected]>; dri-
>> [email protected]; [email protected];
>> [email protected]; [email protected]
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected]; Vinod Polimera (QUIC)
>> <[email protected]>; Abhinav Kumar (QUIC)
>> <[email protected]>
>> 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 <[email protected]>
>>> ---
>>> 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
>-----Original Message-----
>From: Dmitry Baryshkov <[email protected]>
>Sent: Wednesday, November 9, 2022 6:02 PM
>To: Kalyan Thota (QUIC) <[email protected]>; dri-
>[email protected]; [email protected];
>[email protected]; [email protected]
>Cc: [email protected]; [email protected];
>[email protected]; [email protected]; Vinod Polimera (QUIC)
><[email protected]>; Abhinav Kumar (QUIC)
><[email protected]>
>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 <[email protected]>
>> ---
>> 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 <[email protected]>
>Sent: Wednesday, November 9, 2022 6:53 PM
>To: [email protected]; Kalyan Thota (QUIC)
><[email protected]>; [email protected]; linux-arm-
>[email protected]; [email protected];
>[email protected]
>Cc: [email protected]; [email protected];
>[email protected]; [email protected]; Vinod Polimera (QUIC)
><[email protected]>; Abhinav Kumar (QUIC)
><[email protected]>
>Subject: RE: [PATCH 4/4] drm/msm/disp/dpu1: add color management support
>for the crtc
>
>
>
>>-----Original Message-----
>>From: Dmitry Baryshkov <[email protected]>
>>Sent: Wednesday, November 9, 2022 6:02 PM
>>To: Kalyan Thota (QUIC) <[email protected]>; dri-
>>[email protected]; [email protected];
>>[email protected]; [email protected]
>>Cc: [email protected]; [email protected];
>>[email protected]; [email protected]; Vinod Polimera (QUIC)
>><[email protected]>; Abhinav Kumar (QUIC)
>><[email protected]>
>>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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>Sent: Wednesday, November 9, 2022 6:02 PM
>To: Kalyan Thota (QUIC) <[email protected]>; dri-
>[email protected]; [email protected];
>[email protected]; [email protected]
>Cc: [email protected]; [email protected];
>[email protected]; [email protected]; Vinod Polimera (QUIC)
><[email protected]>; Abhinav Kumar (QUIC)
><[email protected]>
>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 <[email protected]>
>> ---
>> 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