Add destination side post processing hw block support in sc7280.
This hwblock enablement is necessary to support color features
like CT Matix (Ex: Night Light feature)
Change-Id: Iba7d5e1693b06cede2891f5b998466070a77c6ef
Signed-off-by: Kalyan Thota <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index a4fe77c..021eb2f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -928,7 +928,7 @@ static const struct dpu_lm_cfg sm8150_lm[] = {
static const struct dpu_lm_cfg sc7280_lm[] = {
LM_BLK("lm_0", LM_0, 0x44000, MIXER_SC7180_MASK,
- &sc7180_lm_sblk, PINGPONG_0, 0, 0),
+ &sc7180_lm_sblk, PINGPONG_0, 0, DSPP_0),
LM_BLK("lm_2", LM_2, 0x46000, MIXER_SC7180_MASK,
&sc7180_lm_sblk, PINGPONG_2, LM_3, 0),
LM_BLK("lm_3", LM_3, 0x47000, MIXER_SC7180_MASK,
@@ -1792,6 +1792,8 @@ static void sc7280_cfg_init(struct dpu_mdss_cfg *dpu_cfg)
.ctl = sc7280_ctl,
.sspp_count = ARRAY_SIZE(sc7280_sspp),
.sspp = sc7280_sspp,
+ .dspp_count = ARRAY_SIZE(sc7180_dspp),
+ .dspp = sc7180_dspp,
.mixer_count = ARRAY_SIZE(sc7280_lm),
.mixer = sc7280_lm,
.pingpong_count = ARRAY_SIZE(sc7280_pp),
--
2.7.4
Crtc color management needs to be registered only for the crtc which has the
capability to handle it. Since topology decides which encoder will get the
dspp hw block, tie up the crtc and the encoder together (encoder->possible_crtcs)
Change-Id: If5a0f33547b6f527ca4b8fbb78424b141dbbd711
Signed-off-by: Kalyan Thota <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 8 ++++++--
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 ++++++++++++++++----
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 5 +++++
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 22 ++++++++++++++++++----
5 files changed, 46 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 7763558..2913acb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1511,7 +1511,7 @@ static const struct drm_crtc_helper_funcs dpu_crtc_helper_funcs = {
/* initialize crtc */
struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
- struct drm_plane *cursor)
+ struct drm_plane *cursor, unsigned int enc_mask)
{
struct drm_crtc *crtc = NULL;
struct dpu_crtc *dpu_crtc = NULL;
@@ -1544,7 +1544,11 @@ 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);
+ /* Register crtc color management if the encoder has dspp, use the
+ * crtc to mark it as possible_crtcs for that encoder.
+ */
+ if(BIT(crtc->index) & enc_mask)
+ 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 b8785c3..0a6458e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -269,7 +269,7 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc);
* @Return: new crtc object or error
*/
struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
- struct drm_plane *cursor);
+ struct drm_plane *cursor, unsigned int enc_mask);
/**
* dpu_crtc_register_custom_event - api for enabling/disabling crtc event
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index f2cb497..893ce68 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -13,6 +13,8 @@
#include <drm/drm_crtc.h>
#include <drm/drm_file.h>
#include <drm/drm_probe_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
#include "msm_drv.h"
#include "dpu_kms.h"
@@ -511,13 +513,18 @@ void dpu_encoder_helper_split_config(
}
}
-static struct msm_display_topology dpu_encoder_get_topology(
- struct dpu_encoder_virt *dpu_enc,
+struct msm_display_topology dpu_encoder_get_topology(
+ struct drm_encoder *drm_enc,
struct dpu_kms *dpu_kms,
struct drm_display_mode *mode)
{
struct msm_display_topology topology = {0};
+ struct dpu_encoder_virt *dpu_enc;
+ struct drm_bridge *bridge;
int i, intf_count = 0;
+ bool primary_display = false;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
for (i = 0; i < MAX_PHYS_ENCODERS_PER_VIRTUAL; i++)
if (dpu_enc->phys_encs[i])
@@ -542,7 +549,12 @@ 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) {
+ drm_for_each_bridge_in_chain(drm_enc, bridge) {
+ if (bridge->type != DRM_MODE_CONNECTOR_DisplayPort)
+ primary_display = true;
+ }
+
+ if (primary_display) {
if (dpu_kms->catalog->dspp &&
(dpu_kms->catalog->dspp_count >= topology.num_lm))
topology.num_dspp = topology.num_lm;
@@ -601,7 +613,7 @@ static int dpu_encoder_virt_atomic_check(
}
}
- topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
+ topology = dpu_encoder_get_topology(drm_enc, dpu_kms, adj_mode);
/* Reserve dynamic resources now. */
if (!ret) {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 1f39327..c4daf7c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -172,4 +172,9 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc);
bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc);
+struct msm_display_topology dpu_encoder_get_topology(
+ struct drm_encoder *drm_enc,
+ struct dpu_kms *dpu_kms,
+ struct drm_display_mode *mode);
+
#endif /* __DPU_ENCODER_H__ */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 3a4da0d..486ff9d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -687,9 +687,12 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
unsigned cursor_idx = 0;
unsigned primary_idx = 0;
bool pin_overlays;
+ unsigned int max_dspp_count = 0;
+ unsigned int enc_mask = 0;
struct msm_drm_private *priv;
struct dpu_mdss_cfg *catalog;
+ struct msm_display_topology topology = {0};
int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret;
int max_crtc_count;
@@ -754,10 +757,19 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
}
max_crtc_count = min(max_crtc_count, primary_planes_idx);
+ max_dspp_count = catalog->dspp_count;
+
+ drm_for_each_encoder(encoder, dev) {
+ topology = dpu_encoder_get_topology(encoder, dpu_kms, NULL);
+ if (topology.num_dspp > 0 && (topology.num_dspp <= max_dspp_count)) {
+ enc_mask |= BIT(encoder->index);
+ max_dspp_count -= topology.num_dspp;
+ }
+ }
/* Create one CRTC per encoder */
for (i = 0; i < max_crtc_count; i++) {
- crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
+ crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i], enc_mask);
if (IS_ERR(crtc)) {
ret = PTR_ERR(crtc);
return ret;
@@ -765,9 +777,11 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
priv->crtcs[priv->num_crtcs++] = crtc;
}
- /* All CRTCs are compatible with all encoders */
- drm_for_each_encoder(encoder, dev)
- encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
+ /* Attach CRTC's to compatiable encoders */
+ drm_for_each_encoder(encoder, dev) {
+ encoder->possible_crtcs = (enc_mask & BIT(encoder->index)) ?
+ BIT(encoder->index) : (((1 << priv->num_crtcs) - 1) & ~enc_mask);
+ }
return 0;
}
--
2.7.4
On Tue, 21 Jun 2022 at 12:06, Kalyan Thota <[email protected]> wrote:
>
> Add destination side post processing hw block support in sc7280.
>
> This hwblock enablement is necessary to support color features
> like CT Matix (Ex: Night Light feature)
>
> Change-Id: Iba7d5e1693b06cede2891f5b998466070a77c6ef
> Signed-off-by: Kalyan Thota <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index a4fe77c..021eb2f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -928,7 +928,7 @@ static const struct dpu_lm_cfg sm8150_lm[] = {
>
> static const struct dpu_lm_cfg sc7280_lm[] = {
> LM_BLK("lm_0", LM_0, 0x44000, MIXER_SC7180_MASK,
> - &sc7180_lm_sblk, PINGPONG_0, 0, 0),
> + &sc7180_lm_sblk, PINGPONG_0, 0, DSPP_0),
> LM_BLK("lm_2", LM_2, 0x46000, MIXER_SC7180_MASK,
> &sc7180_lm_sblk, PINGPONG_2, LM_3, 0),
> LM_BLK("lm_3", LM_3, 0x47000, MIXER_SC7180_MASK,
> @@ -1792,6 +1792,8 @@ static void sc7280_cfg_init(struct dpu_mdss_cfg *dpu_cfg)
> .ctl = sc7280_ctl,
> .sspp_count = ARRAY_SIZE(sc7280_sspp),
> .sspp = sc7280_sspp,
> + .dspp_count = ARRAY_SIZE(sc7180_dspp),
> + .dspp = sc7180_dspp,
> .mixer_count = ARRAY_SIZE(sc7280_lm),
> .mixer = sc7280_lm,
> .pingpong_count = ARRAY_SIZE(sc7280_pp),
> --
> 2.7.4
>
--
With best wishes
Dmitry
Generic comment: [email protected] address bounces. Please remove it from
the cc list. If you need to send a patch for the internal reasons,
please use Bcc.
On Tue, 21 Jun 2022 at 12:06, Kalyan Thota <[email protected]> wrote:
>
> Crtc color management needs to be registered only for the crtc which has the
> capability to handle it. Since topology decides which encoder will get the
> dspp hw block, tie up the crtc and the encoder together (encoder->possible_crtcs)
>
> Change-Id: If5a0f33547b6f527ca4b8fbb78424b141dbbd711
No change-id's please. This is not the gerrit.
> Signed-off-by: Kalyan Thota <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 8 ++++++--
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 2 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 ++++++++++++++++----
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 5 +++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 22 ++++++++++++++++++----
> 5 files changed, 46 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 7763558..2913acb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1511,7 +1511,7 @@ static const struct drm_crtc_helper_funcs dpu_crtc_helper_funcs = {
>
> /* initialize crtc */
> struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
> - struct drm_plane *cursor)
> + struct drm_plane *cursor, unsigned int enc_mask)
> {
> struct drm_crtc *crtc = NULL;
> struct dpu_crtc *dpu_crtc = NULL;
> @@ -1544,7 +1544,11 @@ 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);
> + /* Register crtc color management if the encoder has dspp, use the
> + * crtc to mark it as possible_crtcs for that encoder.
> + */
> + if(BIT(crtc->index) & enc_mask)
So, we are checking CRTC's index against the encoder's mask? This is
counterintuitive.
> + 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 b8785c3..0a6458e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -269,7 +269,7 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc);
> * @Return: new crtc object or error
> */
> struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
> - struct drm_plane *cursor);
> + struct drm_plane *cursor, unsigned int enc_mask);
>
> /**
> * dpu_crtc_register_custom_event - api for enabling/disabling crtc event
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index f2cb497..893ce68 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -13,6 +13,8 @@
> #include <drm/drm_crtc.h>
> #include <drm/drm_file.h>
> #include <drm/drm_probe_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
>
> #include "msm_drv.h"
> #include "dpu_kms.h"
> @@ -511,13 +513,18 @@ void dpu_encoder_helper_split_config(
> }
> }
>
> -static struct msm_display_topology dpu_encoder_get_topology(
> - struct dpu_encoder_virt *dpu_enc,
> +struct msm_display_topology dpu_encoder_get_topology(
> + struct drm_encoder *drm_enc,
> struct dpu_kms *dpu_kms,
> struct drm_display_mode *mode)
> {
> struct msm_display_topology topology = {0};
> + struct dpu_encoder_virt *dpu_enc;
> + struct drm_bridge *bridge;
> int i, intf_count = 0;
> + bool primary_display = false;
> +
> + dpu_enc = to_dpu_encoder_virt(drm_enc);
>
> for (i = 0; i < MAX_PHYS_ENCODERS_PER_VIRTUAL; i++)
> if (dpu_enc->phys_encs[i])
> @@ -542,7 +549,12 @@ 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) {
> + drm_for_each_bridge_in_chain(drm_enc, bridge) {
> + if (bridge->type != DRM_MODE_CONNECTOR_DisplayPort)
> + primary_display = true;
> + }
I must admit, I never actually liked the original (intf_type == DSI)
check. However the new one is even worse. We are checking the whole
bridge chain in an attempt to rule out the DP ports for whatever
reason. What about the HDMI ports? Should they be also frowned upon?
The ugly part is that we are making the decision for the user, which
displays are "primary" for him. Can we let the user make this setting?
> +
> + if (primary_display) {
> if (dpu_kms->catalog->dspp &&
> (dpu_kms->catalog->dspp_count >= topology.num_lm))
> topology.num_dspp = topology.num_lm;
> @@ -601,7 +613,7 @@ static int dpu_encoder_virt_atomic_check(
> }
> }
>
> - topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
> + topology = dpu_encoder_get_topology(drm_enc, dpu_kms, adj_mode);
extra whitespace change. Please drop.
>
> /* Reserve dynamic resources now. */
> if (!ret) {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 1f39327..c4daf7c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -172,4 +172,9 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc);
>
> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc);
>
> +struct msm_display_topology dpu_encoder_get_topology(
> + struct drm_encoder *drm_enc,
> + struct dpu_kms *dpu_kms,
> + struct drm_display_mode *mode);
> +
> #endif /* __DPU_ENCODER_H__ */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 3a4da0d..486ff9d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -687,9 +687,12 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
> unsigned cursor_idx = 0;
> unsigned primary_idx = 0;
> bool pin_overlays;
> + unsigned int max_dspp_count = 0;
> + unsigned int enc_mask = 0;
>
> struct msm_drm_private *priv;
> struct dpu_mdss_cfg *catalog;
> + struct msm_display_topology topology = {0};
>
> int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret;
> int max_crtc_count;
> @@ -754,10 +757,19 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
> }
>
> max_crtc_count = min(max_crtc_count, primary_planes_idx);
> + max_dspp_count = catalog->dspp_count;
> +
> + drm_for_each_encoder(encoder, dev) {
> + topology = dpu_encoder_get_topology(encoder, dpu_kms, NULL);
This can crash dpu_encoder_get_topology() because it checks mode->hdisplay.
And the check anyway is futile here. We do not know if the encoder is
going to use 1 or 2 LMs (since we do not know the resolution), so we
do not know whether it will use 1 or 2 DSPP blocks.
> + if (topology.num_dspp > 0 && (topology.num_dspp <= max_dspp_count)) {
> + enc_mask |= BIT(encoder->index);
> + max_dspp_count -= topology.num_dspp;
> + }
> + }
>
> /* Create one CRTC per encoder */
> for (i = 0; i < max_crtc_count; i++) {
> - crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
> + crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i], enc_mask);
> if (IS_ERR(crtc)) {
> ret = PTR_ERR(crtc);
> return ret;
> @@ -765,9 +777,11 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
> priv->crtcs[priv->num_crtcs++] = crtc;
> }
>
> - /* All CRTCs are compatible with all encoders */
> - drm_for_each_encoder(encoder, dev)
> - encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
> + /* Attach CRTC's to compatiable encoders */
compatible
> + drm_for_each_encoder(encoder, dev) {
> + encoder->possible_crtcs = (enc_mask & BIT(encoder->index)) ?
> + BIT(encoder->index) : (((1 << priv->num_crtcs) - 1) & ~enc_mask);
> + }
>
> return 0;
> }
> --
> 2.7.4
>
--
With best wishes
Dmitry
Thanks for the comments, Dmitry. I haven't noticed mode->hdisplay being used. My idea was to run thru the topology and tie up the encoders with dspp to the CRTCs.
Since mode is available only in the commit, we cannot use the dpu_encoder_get_topology during initialization sequence.
The requirement here is that when we initialize the crtc, we need to enable drm_crtc_enable_color_mgmt only for the crtcs that support it. As I understand from Rob, chrome framework will check for the enablement in order to exercise the feature.
Do you have any ideas on how to handle this requirement ? Since we will reserve the dspp's only when a commit is issued, I guess it will be too late to enable color management by then.
@[email protected]
Is it okay, if we disable color management for all the crtcs during initialization and enable it when we have dspps available during modeset. Can we framework code query for the property before issuing a commit for the frame after modeset ?
Thanks,
Kalyan
> -----Original Message-----
> From: Dmitry Baryshkov <[email protected]>
> Sent: Tuesday, June 21, 2022 4:43 PM
> To: Kalyan Thota (QUIC) <[email protected]>
> Cc: [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Vinod Polimera (QUIC) <[email protected]>; Abhinav Kumar (QUIC)
> <[email protected]>
> Subject: Re: [v1 2/2] drm/msm/disp/dpu1: enable crtc color management based
> on encoder topology
>
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
>
> Generic comment: [email protected] address bounces. Please remove it from
> the cc list. If you need to send a patch for the internal reasons, please use Bcc.
>
> On Tue, 21 Jun 2022 at 12:06, Kalyan Thota <[email protected]> wrote:
> >
> > Crtc color management needs to be registered only for the crtc which
> > has the capability to handle it. Since topology decides which encoder
> > will get the dspp hw block, tie up the crtc and the encoder together
> > (encoder->possible_crtcs)
> >
> > Change-Id: If5a0f33547b6f527ca4b8fbb78424b141dbbd711
>
> No change-id's please. This is not the gerrit.
>
> > Signed-off-by: Kalyan Thota <[email protected]>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 8 ++++++--
> > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 2 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 ++++++++++++++++----
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 5 +++++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 22 ++++++++++++++++++----
> > 5 files changed, 46 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 7763558..2913acb 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -1511,7 +1511,7 @@ static const struct drm_crtc_helper_funcs
> > dpu_crtc_helper_funcs = {
> >
> > /* initialize crtc */
> > struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane
> *plane,
> > - struct drm_plane *cursor)
> > + struct drm_plane *cursor, unsigned int
> > + enc_mask)
> > {
> > struct drm_crtc *crtc = NULL;
> > struct dpu_crtc *dpu_crtc = NULL; @@ -1544,7 +1544,11 @@
> > 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);
> > + /* Register crtc color management if the encoder has dspp, use the
> > + * crtc to mark it as possible_crtcs for that encoder.
> > + */
> > + if(BIT(crtc->index) & enc_mask)
>
> So, we are checking CRTC's index against the encoder's mask? This is
> counterintuitive.
>
> > + 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 b8785c3..0a6458e 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > @@ -269,7 +269,7 @@ void dpu_crtc_complete_commit(struct drm_crtc
> *crtc);
> > * @Return: new crtc object or error
> > */
> > struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane
> *plane,
> > - struct drm_plane *cursor);
> > + struct drm_plane *cursor, unsigned int
> > + enc_mask);
> >
> > /**
> > * dpu_crtc_register_custom_event - api for enabling/disabling crtc
> > event diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index f2cb497..893ce68 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -13,6 +13,8 @@
> > #include <drm/drm_crtc.h>
> > #include <drm/drm_file.h>
> > #include <drm/drm_probe_helper.h>
> > +#include <drm/drm_bridge.h>
> > +#include <drm/drm_bridge_connector.h>
> >
> > #include "msm_drv.h"
> > #include "dpu_kms.h"
> > @@ -511,13 +513,18 @@ void dpu_encoder_helper_split_config(
> > }
> > }
> >
> > -static struct msm_display_topology dpu_encoder_get_topology(
> > - struct dpu_encoder_virt *dpu_enc,
> > +struct msm_display_topology dpu_encoder_get_topology(
> > + struct drm_encoder *drm_enc,
> > struct dpu_kms *dpu_kms,
> > struct drm_display_mode *mode) {
> > struct msm_display_topology topology = {0};
> > + struct dpu_encoder_virt *dpu_enc;
> > + struct drm_bridge *bridge;
> > int i, intf_count = 0;
> > + bool primary_display = false;
> > +
> > + dpu_enc = to_dpu_encoder_virt(drm_enc);
> >
> > for (i = 0; i < MAX_PHYS_ENCODERS_PER_VIRTUAL; i++)
> > if (dpu_enc->phys_encs[i]) @@ -542,7 +549,12 @@ 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) {
> > + drm_for_each_bridge_in_chain(drm_enc, bridge) {
> > + if (bridge->type != DRM_MODE_CONNECTOR_DisplayPort)
> > + primary_display = true;
> > + }
>
> I must admit, I never actually liked the original (intf_type == DSI) check. However
> the new one is even worse. We are checking the whole bridge chain in an
> attempt to rule out the DP ports for whatever reason. What about the HDMI
> ports? Should they be also frowned upon?
> The ugly part is that we are making the decision for the user, which displays are
> "primary" for him. Can we let the user make this setting?
>
> > +
> > + if (primary_display) {
> > if (dpu_kms->catalog->dspp &&
> > (dpu_kms->catalog->dspp_count >= topology.num_lm))
> > topology.num_dspp = topology.num_lm; @@ -601,7
> > +613,7 @@ static int dpu_encoder_virt_atomic_check(
> > }
> > }
> >
> > - topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
> > + topology = dpu_encoder_get_topology(drm_enc, dpu_kms,
> > + adj_mode);
>
> extra whitespace change. Please drop.
>
> >
> > /* Reserve dynamic resources now. */
> > if (!ret) {
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > index 1f39327..c4daf7c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > @@ -172,4 +172,9 @@ int dpu_encoder_get_vsync_count(struct
> drm_encoder
> > *drm_enc);
> >
> > bool dpu_encoder_is_widebus_enabled(const struct drm_encoder
> > *drm_enc);
> >
> > +struct msm_display_topology dpu_encoder_get_topology(
> > + struct drm_encoder *drm_enc,
> > + struct dpu_kms *dpu_kms,
> > + struct drm_display_mode *mode);
> > +
> > #endif /* __DPU_ENCODER_H__ */
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 3a4da0d..486ff9d 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -687,9 +687,12 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
> *dpu_kms)
> > unsigned cursor_idx = 0;
> > unsigned primary_idx = 0;
> > bool pin_overlays;
> > + unsigned int max_dspp_count = 0;
> > + unsigned int enc_mask = 0;
> >
> > struct msm_drm_private *priv;
> > struct dpu_mdss_cfg *catalog;
> > + struct msm_display_topology topology = {0};
> >
> > int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret;
> > int max_crtc_count;
> > @@ -754,10 +757,19 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
> *dpu_kms)
> > }
> >
> > max_crtc_count = min(max_crtc_count, primary_planes_idx);
> > + max_dspp_count = catalog->dspp_count;
> > +
> > + drm_for_each_encoder(encoder, dev) {
> > + topology = dpu_encoder_get_topology(encoder, dpu_kms,
> > + NULL);
>
> This can crash dpu_encoder_get_topology() because it checks mode->hdisplay.
> And the check anyway is futile here. We do not know if the encoder is going to
> use 1 or 2 LMs (since we do not know the resolution), so we do not know
> whether it will use 1 or 2 DSPP blocks.
>
> > + if (topology.num_dspp > 0 && (topology.num_dspp <=
> max_dspp_count)) {
> > + enc_mask |= BIT(encoder->index);
> > + max_dspp_count -= topology.num_dspp;
> > + }
> > + }
> >
> > /* Create one CRTC per encoder */
> > for (i = 0; i < max_crtc_count; i++) {
> > - crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
> > + crtc = dpu_crtc_init(dev, primary_planes[i],
> > + cursor_planes[i], enc_mask);
> > if (IS_ERR(crtc)) {
> > ret = PTR_ERR(crtc);
> > return ret;
> > @@ -765,9 +777,11 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
> *dpu_kms)
> > priv->crtcs[priv->num_crtcs++] = crtc;
> > }
> >
> > - /* All CRTCs are compatible with all encoders */
> > - drm_for_each_encoder(encoder, dev)
> > - encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
> > + /* Attach CRTC's to compatiable encoders */
>
> compatible
>
>
> > + drm_for_each_encoder(encoder, dev) {
> > + encoder->possible_crtcs = (enc_mask & BIT(encoder->index)) ?
> > + BIT(encoder->index) : (((1 << priv->num_crtcs) - 1) &
> ~enc_mask);
> > + }
> >
> > return 0;
> > }
> > --
> > 2.7.4
> >
>
>
> --
> With best wishes
> Dmitry
On Mon, Jun 27, 2022 at 4:56 AM Kalyan Thota <[email protected]> wrote:
>
> Thanks for the comments, Dmitry. I haven't noticed mode->hdisplay being used. My idea was to run thru the topology and tie up the encoders with dspp to the CRTCs.
> Since mode is available only in the commit, we cannot use the dpu_encoder_get_topology during initialization sequence.
>
> The requirement here is that when we initialize the crtc, we need to enable drm_crtc_enable_color_mgmt only for the crtcs that support it. As I understand from Rob, chrome framework will check for the enablement in order to exercise the feature.
>
> Do you have any ideas on how to handle this requirement ? Since we will reserve the dspp's only when a commit is issued, I guess it will be too late to enable color management by then.
>
> @[email protected]
> Is it okay, if we disable color management for all the crtcs during initialization and enable it when we have dspps available during modeset. Can we framework code query for the property before issuing a commit for the frame after modeset ?
>
So, I suppose it would work out, because the splashscreen/frecon is
doing the first modeset before chrome even starts. But that seems a
bit... delicate.
BR,
-R
> Thanks,
> Kalyan
>
> > -----Original Message-----
> > From: Dmitry Baryshkov <[email protected]>
> > Sent: Tuesday, June 21, 2022 4:43 PM
> > To: Kalyan Thota (QUIC) <[email protected]>
> > Cc: [email protected]; [email protected]; linux-arm-
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > Vinod Polimera (QUIC) <[email protected]>; Abhinav Kumar (QUIC)
> > <[email protected]>
> > Subject: Re: [v1 2/2] drm/msm/disp/dpu1: enable crtc color management based
> > on encoder topology
> >
> > WARNING: This email originated from outside of Qualcomm. Please be wary of
> > any links or attachments, and do not enable macros.
> >
> > Generic comment: [email protected] address bounces. Please remove it from
> > the cc list. If you need to send a patch for the internal reasons, please use Bcc.
> >
> > On Tue, 21 Jun 2022 at 12:06, Kalyan Thota <[email protected]> wrote:
> > >
> > > Crtc color management needs to be registered only for the crtc which
> > > has the capability to handle it. Since topology decides which encoder
> > > will get the dspp hw block, tie up the crtc and the encoder together
> > > (encoder->possible_crtcs)
> > >
> > > Change-Id: If5a0f33547b6f527ca4b8fbb78424b141dbbd711
> >
> > No change-id's please. This is not the gerrit.
> >
> > > Signed-off-by: Kalyan Thota <[email protected]>
> > > ---
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 8 ++++++--
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 2 +-
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 ++++++++++++++++----
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 5 +++++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 22 ++++++++++++++++++----
> > > 5 files changed, 46 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > index 7763558..2913acb 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > @@ -1511,7 +1511,7 @@ static const struct drm_crtc_helper_funcs
> > > dpu_crtc_helper_funcs = {
> > >
> > > /* initialize crtc */
> > > struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane
> > *plane,
> > > - struct drm_plane *cursor)
> > > + struct drm_plane *cursor, unsigned int
> > > + enc_mask)
> > > {
> > > struct drm_crtc *crtc = NULL;
> > > struct dpu_crtc *dpu_crtc = NULL; @@ -1544,7 +1544,11 @@
> > > 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);
> > > + /* Register crtc color management if the encoder has dspp, use the
> > > + * crtc to mark it as possible_crtcs for that encoder.
> > > + */
> > > + if(BIT(crtc->index) & enc_mask)
> >
> > So, we are checking CRTC's index against the encoder's mask? This is
> > counterintuitive.
> >
> > > + 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 b8785c3..0a6458e 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > @@ -269,7 +269,7 @@ void dpu_crtc_complete_commit(struct drm_crtc
> > *crtc);
> > > * @Return: new crtc object or error
> > > */
> > > struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane
> > *plane,
> > > - struct drm_plane *cursor);
> > > + struct drm_plane *cursor, unsigned int
> > > + enc_mask);
> > >
> > > /**
> > > * dpu_crtc_register_custom_event - api for enabling/disabling crtc
> > > event diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > index f2cb497..893ce68 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > @@ -13,6 +13,8 @@
> > > #include <drm/drm_crtc.h>
> > > #include <drm/drm_file.h>
> > > #include <drm/drm_probe_helper.h>
> > > +#include <drm/drm_bridge.h>
> > > +#include <drm/drm_bridge_connector.h>
> > >
> > > #include "msm_drv.h"
> > > #include "dpu_kms.h"
> > > @@ -511,13 +513,18 @@ void dpu_encoder_helper_split_config(
> > > }
> > > }
> > >
> > > -static struct msm_display_topology dpu_encoder_get_topology(
> > > - struct dpu_encoder_virt *dpu_enc,
> > > +struct msm_display_topology dpu_encoder_get_topology(
> > > + struct drm_encoder *drm_enc,
> > > struct dpu_kms *dpu_kms,
> > > struct drm_display_mode *mode) {
> > > struct msm_display_topology topology = {0};
> > > + struct dpu_encoder_virt *dpu_enc;
> > > + struct drm_bridge *bridge;
> > > int i, intf_count = 0;
> > > + bool primary_display = false;
> > > +
> > > + dpu_enc = to_dpu_encoder_virt(drm_enc);
> > >
> > > for (i = 0; i < MAX_PHYS_ENCODERS_PER_VIRTUAL; i++)
> > > if (dpu_enc->phys_encs[i]) @@ -542,7 +549,12 @@ 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) {
> > > + drm_for_each_bridge_in_chain(drm_enc, bridge) {
> > > + if (bridge->type != DRM_MODE_CONNECTOR_DisplayPort)
> > > + primary_display = true;
> > > + }
> >
> > I must admit, I never actually liked the original (intf_type == DSI) check. However
> > the new one is even worse. We are checking the whole bridge chain in an
> > attempt to rule out the DP ports for whatever reason. What about the HDMI
> > ports? Should they be also frowned upon?
> > The ugly part is that we are making the decision for the user, which displays are
> > "primary" for him. Can we let the user make this setting?
> >
> > > +
> > > + if (primary_display) {
> > > if (dpu_kms->catalog->dspp &&
> > > (dpu_kms->catalog->dspp_count >= topology.num_lm))
> > > topology.num_dspp = topology.num_lm; @@ -601,7
> > > +613,7 @@ static int dpu_encoder_virt_atomic_check(
> > > }
> > > }
> > >
> > > - topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
> > > + topology = dpu_encoder_get_topology(drm_enc, dpu_kms,
> > > + adj_mode);
> >
> > extra whitespace change. Please drop.
> >
> > >
> > > /* Reserve dynamic resources now. */
> > > if (!ret) {
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > index 1f39327..c4daf7c 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > @@ -172,4 +172,9 @@ int dpu_encoder_get_vsync_count(struct
> > drm_encoder
> > > *drm_enc);
> > >
> > > bool dpu_encoder_is_widebus_enabled(const struct drm_encoder
> > > *drm_enc);
> > >
> > > +struct msm_display_topology dpu_encoder_get_topology(
> > > + struct drm_encoder *drm_enc,
> > > + struct dpu_kms *dpu_kms,
> > > + struct drm_display_mode *mode);
> > > +
> > > #endif /* __DPU_ENCODER_H__ */
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > index 3a4da0d..486ff9d 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > @@ -687,9 +687,12 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
> > *dpu_kms)
> > > unsigned cursor_idx = 0;
> > > unsigned primary_idx = 0;
> > > bool pin_overlays;
> > > + unsigned int max_dspp_count = 0;
> > > + unsigned int enc_mask = 0;
> > >
> > > struct msm_drm_private *priv;
> > > struct dpu_mdss_cfg *catalog;
> > > + struct msm_display_topology topology = {0};
> > >
> > > int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret;
> > > int max_crtc_count;
> > > @@ -754,10 +757,19 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
> > *dpu_kms)
> > > }
> > >
> > > max_crtc_count = min(max_crtc_count, primary_planes_idx);
> > > + max_dspp_count = catalog->dspp_count;
> > > +
> > > + drm_for_each_encoder(encoder, dev) {
> > > + topology = dpu_encoder_get_topology(encoder, dpu_kms,
> > > + NULL);
> >
> > This can crash dpu_encoder_get_topology() because it checks mode->hdisplay.
> > And the check anyway is futile here. We do not know if the encoder is going to
> > use 1 or 2 LMs (since we do not know the resolution), so we do not know
> > whether it will use 1 or 2 DSPP blocks.
> >
> > > + if (topology.num_dspp > 0 && (topology.num_dspp <=
> > max_dspp_count)) {
> > > + enc_mask |= BIT(encoder->index);
> > > + max_dspp_count -= topology.num_dspp;
> > > + }
> > > + }
> > >
> > > /* Create one CRTC per encoder */
> > > for (i = 0; i < max_crtc_count; i++) {
> > > - crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
> > > + crtc = dpu_crtc_init(dev, primary_planes[i],
> > > + cursor_planes[i], enc_mask);
> > > if (IS_ERR(crtc)) {
> > > ret = PTR_ERR(crtc);
> > > return ret;
> > > @@ -765,9 +777,11 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
> > *dpu_kms)
> > > priv->crtcs[priv->num_crtcs++] = crtc;
> > > }
> > >
> > > - /* All CRTCs are compatible with all encoders */
> > > - drm_for_each_encoder(encoder, dev)
> > > - encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
> > > + /* Attach CRTC's to compatiable encoders */
> >
> > compatible
> >
> >
> > > + drm_for_each_encoder(encoder, dev) {
> > > + encoder->possible_crtcs = (enc_mask & BIT(encoder->index)) ?
> > > + BIT(encoder->index) : (((1 << priv->num_crtcs) - 1) &
> > ~enc_mask);
> > > + }
> > >
> > > return 0;
> > > }
> > > --
> > > 2.7.4
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry
On 27/06/2022 14:56, Kalyan Thota wrote:
> Thanks for the comments, Dmitry. I haven't noticed mode->hdisplay being used. My idea was to run thru the topology and tie up the encoders with dspp to the CRTCs.
> Since mode is available only in the commit, we cannot use the dpu_encoder_get_topology during initialization sequence.
>
> The requirement here is that when we initialize the crtc, we need to enable drm_crtc_enable_color_mgmt only for the crtcs that support it. As I understand from Rob, chrome framework will check for the enablement in order to exercise the feature.
>
> Do you have any ideas on how to handle this requirement ? Since we will reserve the dspp's only when a commit is issued, I guess it will be too late to enable color management by then.
I have been thinking about this for quite a while.
Basically I fear you have two options:
- Register the color management for all CRTCs. In dpu_rm_reserve() check
for the ctm, allocate LMs taking the available DSPPs into account. Fail
the atomic_check() if there are no available LMs with required
capabilities. Additional bonus point for moving LM/DSPP resource
allocation from dpu_encoder into dpu_crtc.
- Register CRTCs and it's colormanagement properties according to exact
available hardware. Let the userspace composer select the CRTC for the
connector basing on the availability of the CTM support.
I'd vote strongly against any attempt to put the policy ('e.g. enable
CTM only for the eDP and first DSI display') into the kernel, because we
can not predict the actual usecases the user needs. It well might be
that the user of the laptop will work with DP displays only and thus
require color management for DP.
>
> @[email protected]
> Is it okay, if we disable color management for all the crtcs during initialization and enable it when we have dspps available during modeset. Can we framework code query for the property before issuing a commit for the frame after modeset ?
>
--
With best wishes
Dmitry