Skip to contentSkip to conversation

drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate

RFC PATCH Superseded John Stultz 2 participants 4 messages Jul 18, 2017 – Jul 20, 2017

Subsystems (3)
DRM drivers DRM misc DRM drivers for HiSilicon kirin

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

John Stultz

author
#0 lore raw

RFC PATCH v3 1/1 drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate

Currently the hikey dsi logic cannot generate accurate byte clocks values for all pixel clock values. Thus if a mode clock is selected that cannot match the calculated byte clock, the device will boot with a blank screen.

This patch uses the new mode_valid callback (many thanks to Jose Abreu for upstreaming it!) to ensure we don't select modes we cannot generate.

Also, since the ade crtc code will adjust the mode in mode_set, this patch also adds a mode_fixup callback which we use to make sure we are validating the mode clock that will eventually be used.

Many thanks to Jose and Daniel for recent feedback. I think this version is looking much nicer. But I'd still welcome any feedback or suggestions!

Cc Daniel Vetter <[email protected]>
Cc Jani Nikula <[email protected]>
Cc Sean Paul <[email protected]>
Cc David Airlie <[email protected]>
Cc Rob Clark <[email protected]>
Cc Xinliang Liu <[email protected]>
Cc Xinliang Liu <[email protected]>
Cc Rongrong Zou <[email protected]>
Cc Xinwei Kong <[email protected]>
Cc Chen Feng <[email protected]>
Cc Jose Abreu <[email protected]>
Cc Archit Taneja <[email protected]>
Cc [email protected]
Signed-off-by John Stultz <[email protected]>

---
v2: Reworked to calculate if modeclock matches the phy's
byteclock, rather then using a whitelist of known modes.

v3: Reworked to check across all possible crtcs (even though for
us there is only one), and use mode_fixup instead of a custom function, as suggested by Jose and Daniel.

---
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c    | 62 +++++++++++++++++++++++++
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 14 ++++++
 2 files changed, 76 insertions(+)
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +62 -0
index f77dcfa..d7b5820 100644
@@ -603,6 +603,67 @@ static void dsi_encoder_enable(struct drm_encoder *encoder)
 	dsi->enable = true;
 }
 
+static enum drm_mode_status dsi_encoder_phy_mode_valid(struct drm_encoder *encoder,
+					const struct drm_display_mode *mode)
+{
+	struct dw_dsi *dsi = encoder_to_dsi(encoder);
+	struct mipi_phy_params phy;
+	u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+	u32 req_kHz, act_kHz, lane_byte_clk_kHz;
+
+	/* Calculate the lane byte clk using the adjusted mode clk */
+	memset(&phy, 0, sizeof(phy));
+	req_kHz = mode->clock * bpp / dsi->lanes;
+	act_kHz = dsi_calc_phy_rate(req_kHz, &phy);
+	lane_byte_clk_kHz = act_kHz / 8;
+
+	DRM_DEBUG_DRIVER("Checking mode %ix%i-%i@%i clock: %i...",
+			mode->hdisplay, mode->vdisplay, bpp,
+			drm_mode_vrefresh(mode), mode->clock);
+
+	/*
+	 * Make sure the adjused mode clock and the lane byte clk
+	 * have a common denominator base frequency
+	 */
+	if (mode->clock/dsi->lanes == lane_byte_clk_kHz/3) {
+		DRM_DEBUG_DRIVER("OK!\n");
+		return MODE_OK;
+	}
+
+	DRM_DEBUG_DRIVER("BAD!\n");
+	return MODE_BAD;
+}
+
+static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *encoder,
+					const struct drm_display_mode *mode)
+
+{
+	const struct drm_crtc_helper_funcs *crtc_funcs = NULL;
+	struct drm_crtc *crtc = NULL;
+	struct drm_display_mode adj_mode;
+	int ret;
+
+	memcpy(&adj_mode, mode, sizeof(adj_mode));
+
+	/*
+	 * The crtc might adjust the mode, so go through the
+	 * possible crtcs (technically just one) and call
+	 * mode_fixup to figure out the adjusted mode before we
+	 * validate it.
+	 */
+	drm_for_each_crtc(crtc, encoder->dev) {
+		crtc_funcs = crtc->helper_private;
+		if (crtc_funcs && crtc_funcs->mode_fixup)
+			ret = crtc_funcs->mode_fixup(crtc, mode,
+							&adj_mode);
+
+		ret = dsi_encoder_phy_mode_valid(encoder, &adj_mode);
+		if (ret != MODE_OK)
+			return ret;
+	}
+	return MODE_OK;
+}
+
 static void dsi_encoder_mode_set(struct drm_encoder *encoder,
 				 struct drm_display_mode *mode,
 				 struct drm_display_mode *adj_mode)

@@ -622,6 +683,7 @@ static int dsi_encoder_atomic_check(struct drm_encoder *encoder,
 
 static const struct drm_encoder_helper_funcs dw_encoder_helper_funcs = {
 	.atomic_check	= dsi_encoder_atomic_check,
+	.mode_valid	= dsi_encoder_mode_valid,
 	.mode_set	= dsi_encoder_mode_set,
 	.enable		= dsi_encoder_enable,
 	.disable	= dsi_encoder_disable
drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c +14 -0
index c96c228..dec7f4e 100644
@@ -178,6 +178,19 @@ static void ade_init(struct ade_hw_ctx *ctx)
 			FRM_END_START_MASK, REG_EFFECTIVE_IN_ADEEN_FRMEND);
 }
 
+static bool ade_crtc_mode_fixup(struct drm_crtc *crtc,
+				const struct drm_display_mode *mode,
+				struct drm_display_mode *adjusted_mode)
+{
+	struct ade_crtc *acrtc = to_ade_crtc(crtc);
+	struct ade_hw_ctx *ctx = acrtc->ctx;
+
+	adjusted_mode->clock =
+		clk_round_rate(ctx->ade_pix_clk, mode->clock * 1000) / 1000;
+	return true;
+}
+
+
 static void ade_set_pix_clk(struct ade_hw_ctx *ctx,
 			    struct drm_display_mode *mode,
 			    struct drm_display_mode *adj_mode)

@@ -555,6 +568,7 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc,
 static const struct drm_crtc_helper_funcs ade_crtc_helper_funcs = {
 	.enable		= ade_crtc_enable,
 	.disable	= ade_crtc_disable,
+	.mode_fixup	= ade_crtc_mode_fixup,
 	.mode_set_nofb	= ade_crtc_mode_set_nofb,
 	.atomic_begin	= ade_crtc_atomic_begin,
 	.atomic_flush	= ade_crtc_atomic_flush,
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#1 lore raw

Hi John,

Quoted 81 lines · John Stultz

Currently the hikey dsi logic cannot generate accurate byte clocks values for all pixel clock values. Thus if a mode clock is selected that cannot match the calculated byte clock, the device will boot with a blank screen.

This patch uses the new mode_valid callback (many thanks to Jose Abreu for upstreaming it!) to ensure we don't select modes we cannot generate.

Also, since the ade crtc code will adjust the mode in mode_set, this patch also adds a mode_fixup callback which we use to make sure we are validating the mode clock that will eventually be used.

Many thanks to Jose and Daniel for recent feedback. I think this version is looking much nicer. But I'd still welcome any feedback or suggestions!

Cc Daniel Vetter <[email protected]>
Cc Jani Nikula <[email protected]>
Cc Sean Paul <[email protected]>
Cc David Airlie <[email protected]>
Cc Rob Clark <[email protected]>
Cc Xinliang Liu <[email protected]>
Cc Xinliang Liu <[email protected]>
Cc Rongrong Zou <[email protected]>
Cc Xinwei Kong <[email protected]>
Cc Chen Feng <[email protected]>
Cc Jose Abreu <[email protected]>
Cc Archit Taneja <[email protected]>
Cc [email protected]
Signed-off-by John Stultz <[email protected]>

---
v2: Reworked to calculate if modeclock matches the phy's
byteclock, rather then using a whitelist of known modes.

v3: Reworked to check across all possible crtcs (even though for
us there is only one), and use mode_fixup instead of a custom function, as suggested by Jose and Daniel.

---
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c    | 62 +++++++++++++++++++++++++
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 14 ++++++
 2 files changed, 76 insertions(+)
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +29 -0
index f77dcfa..d7b5820 100644
@@ -603,6 +603,67 @@ static void dsi_encoder_enable(struct drm_encoder *encoder)
 	dsi->enable = true;
 }
 
+static enum drm_mode_status dsi_encoder_phy_mode_valid(struct drm_encoder *encoder,
+					const struct drm_display_mode *mode)
+{
+	struct dw_dsi *dsi = encoder_to_dsi(encoder);
+	struct mipi_phy_params phy;
+	u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+	u32 req_kHz, act_kHz, lane_byte_clk_kHz;
+
+	/* Calculate the lane byte clk using the adjusted mode clk */
+	memset(&phy, 0, sizeof(phy));
+	req_kHz = mode->clock * bpp / dsi->lanes;
+	act_kHz = dsi_calc_phy_rate(req_kHz, &phy);
+	lane_byte_clk_kHz = act_kHz / 8;
+
+	DRM_DEBUG_DRIVER("Checking mode %ix%i-%i@%i clock: %i...",
+			mode->hdisplay, mode->vdisplay, bpp,
+			drm_mode_vrefresh(mode), mode->clock);
+
+	/*
+	 * Make sure the adjused mode clock and the lane byte clk
+	 * have a common denominator base frequency
+	 */
+	if (mode->clock/dsi->lanes == lane_byte_clk_kHz/3) {
+		DRM_DEBUG_DRIVER("OK!\n");
+		return MODE_OK;
+	}
+
+	DRM_DEBUG_DRIVER("BAD!\n");
+	return MODE_BAD;

drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c static void dsi_encoder_enable(struct drm_encoder *encoder) · line 644

Quoted 10 lines
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +10 -0
+}
+
+static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *encoder,
+					const struct drm_display_mode *mode)
+
+{
+	const struct drm_crtc_helper_funcs *crtc_funcs = NULL;
+	struct drm_crtc *crtc = NULL;
+	struct drm_display_mode adj_mode;
+	int ret;

This int should be an enum drm_mode_status ...

drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c static void dsi_encoder_enable(struct drm_encoder *encoder) · line 646

Quoted 2 lines
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +2 -0
+
+	memcpy(&adj_mode, mode, sizeof(adj_mode));

Maybe you should move this to the loop so that you pass a "clean" adjusted mode (i.e. adj_mode == mode) for each crtc. You could also use drm_mode_duplicate()/drm_mode_destroy() ...

drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c static void dsi_encoder_enable(struct drm_encoder *encoder) · line 658

Quoted 12 lines
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +12 -0
+
+	/*
+	 * The crtc might adjust the mode, so go through the
+	 * possible crtcs (technically just one) and call
+	 * mode_fixup to figure out the adjusted mode before we
+	 * validate it.
+	 */
+	drm_for_each_crtc(crtc, encoder->dev) {
+		crtc_funcs = crtc->helper_private;
+		if (crtc_funcs && crtc_funcs->mode_fixup)
+			ret = crtc_funcs->mode_fixup(crtc, mode,
+							&adj_mode);

No return check?

Best regards,
Jose Miguel Abreu

Quoted 44 lines
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +9 -0
+
+		ret = dsi_encoder_phy_mode_valid(encoder, &adj_mode);
+		if (ret != MODE_OK)
+			return ret;
+	}
+	return MODE_OK;
+}
+
 static void dsi_encoder_mode_set(struct drm_encoder *encoder,
 				 struct drm_display_mode *mode,
 				 struct drm_display_mode *adj_mode)

@@ -622,6 +683,7 @@ static int dsi_encoder_atomic_check(struct drm_encoder *encoder,
 
 static const struct drm_encoder_helper_funcs dw_encoder_helper_funcs = {
 	.atomic_check	= dsi_encoder_atomic_check,
+	.mode_valid	= dsi_encoder_mode_valid,
 	.mode_set	= dsi_encoder_mode_set,
 	.enable		= dsi_encoder_enable,
 	.disable	= dsi_encoder_disable
drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c +13 -0
index c96c228..dec7f4e 100644
@@ -178,6 +178,19 @@ static void ade_init(struct ade_hw_ctx *ctx)
 			FRM_END_START_MASK, REG_EFFECTIVE_IN_ADEEN_FRMEND);
 }
 
+static bool ade_crtc_mode_fixup(struct drm_crtc *crtc,
+				const struct drm_display_mode *mode,
+				struct drm_display_mode *adjusted_mode)
+{
+	struct ade_crtc *acrtc = to_ade_crtc(crtc);
+	struct ade_hw_ctx *ctx = acrtc->ctx;
+
+	adjusted_mode->clock =
+		clk_round_rate(ctx->ade_pix_clk, mode->clock * 1000) / 1000;
+	return true;
+}
+
+
 static void ade_set_pix_clk(struct ade_hw_ctx *ctx,
 			    struct drm_display_mode *mode,
 			    struct drm_display_mode *adj_mode)

drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c static void ade_crtc_atomic_flush(struct drm_crtc *crtc, · line 574

Quoted 7 lines
drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c +1 -0
@@ -555,6 +568,7 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc,
 static const struct drm_crtc_helper_funcs ade_crtc_helper_funcs = {
 	.enable		= ade_crtc_enable,
 	.disable	= ade_crtc_disable,
+	.mode_fixup	= ade_crtc_mode_fixup,
 	.mode_set_nofb	= ade_crtc_mode_set_nofb,
 	.atomic_begin	= ade_crtc_atomic_begin,
 	.atomic_flush	= ade_crtc_atomic_flush,

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#2 lore raw
Quoted 98 lines · Jose Abreu

Hi John,

Quoted 81 lines · John Stultz

Currently the hikey dsi logic cannot generate accurate byte clocks values for all pixel clock values. Thus if a mode clock is selected that cannot match the calculated byte clock, the device will boot with a blank screen.

This patch uses the new mode_valid callback (many thanks to Jose Abreu for upstreaming it!) to ensure we don't select modes we cannot generate.

Also, since the ade crtc code will adjust the mode in mode_set, this patch also adds a mode_fixup callback which we use to make sure we are validating the mode clock that will eventually be used.

Many thanks to Jose and Daniel for recent feedback. I think this version is looking much nicer. But I'd still welcome any feedback or suggestions!

Cc Daniel Vetter <[email protected]>
Cc Jani Nikula <[email protected]>
Cc Sean Paul <[email protected]>
Cc David Airlie <[email protected]>
Cc Rob Clark <[email protected]>
Cc Xinliang Liu <[email protected]>
Cc Xinliang Liu <[email protected]>
Cc Rongrong Zou <[email protected]>
Cc Xinwei Kong <[email protected]>
Cc Chen Feng <[email protected]>
Cc Jose Abreu <[email protected]>
Cc Archit Taneja <[email protected]>
Cc [email protected]
Signed-off-by John Stultz <[email protected]>

---
v2: Reworked to calculate if modeclock matches the phy's
byteclock, rather then using a whitelist of known modes.

v3: Reworked to check across all possible crtcs (even though for
us there is only one), and use mode_fixup instead of a custom function, as suggested by Jose and Daniel.

---
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c    | 62 +++++++++++++++++++++++++
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 14 ++++++
 2 files changed, 76 insertions(+)
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +29 -0
index f77dcfa..d7b5820 100644
@@ -603,6 +603,67 @@ static void dsi_encoder_enable(struct drm_encoder *encoder)
      dsi->enable = true;
 }
 
+static enum drm_mode_status dsi_encoder_phy_mode_valid(struct drm_encoder *encoder,
+                                     const struct drm_display_mode *mode)
+{
+     struct dw_dsi *dsi = encoder_to_dsi(encoder);
+     struct mipi_phy_params phy;
+     u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+     u32 req_kHz, act_kHz, lane_byte_clk_kHz;
+
+     /* Calculate the lane byte clk using the adjusted mode clk */
+     memset(&phy, 0, sizeof(phy));
+     req_kHz = mode->clock * bpp / dsi->lanes;
+     act_kHz = dsi_calc_phy_rate(req_kHz, &phy);
+     lane_byte_clk_kHz = act_kHz / 8;
+
+     DRM_DEBUG_DRIVER("Checking mode %ix%i-%i@%i clock: %i...",
+                     mode->hdisplay, mode->vdisplay, bpp,
+                     drm_mode_vrefresh(mode), mode->clock);
+
+     /*
+      * Make sure the adjused mode clock and the lane byte clk
+      * have a common denominator base frequency
+      */
+     if (mode->clock/dsi->lanes == lane_byte_clk_kHz/3) {
+             DRM_DEBUG_DRIVER("OK!\n");
+             return MODE_OK;
+     }
+
+     DRM_DEBUG_DRIVER("BAD!\n");
+     return MODE_BAD;

drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c static void dsi_encoder_enable(struct drm_encoder *encoder) · line 644

Quoted 10 lines
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +10 -0
+}
+
+static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *encoder,
+                                     const struct drm_display_mode *mode)
+
+{
+     const struct drm_crtc_helper_funcs *crtc_funcs = NULL;
+     struct drm_crtc *crtc = NULL;
+     struct drm_display_mode adj_mode;
+     int ret;

This int should be an enum drm_mode_status ...

Quoted 6 lines

drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c static void dsi_encoder_enable(struct drm_encoder *encoder) · line 646

Quoted 2 lines
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +2 -0
+
+     memcpy(&adj_mode, mode, sizeof(adj_mode));

Maybe you should move this to the loop so that you pass a "clean" adjusted mode (i.e. adj_mode == mode) for each crtc. You could also use drm_mode_duplicate()/drm_mode_destroy() ...

Ah! Good catch! Thanks for that!

What is the benefit of drm_mode_duplicate over just using memcpy? I see there's some base.id and head pointers that are kept unique, but we're not touching those for this case. The extra allocating/freeing seems a bit needless.

Quoted 13 lines

drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c static void dsi_encoder_enable(struct drm_encoder *encoder) · line 658

Quoted 12 lines
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +12 -0
+
+     /*
+      * The crtc might adjust the mode, so go through the
+      * possible crtcs (technically just one) and call
+      * mode_fixup to figure out the adjusted mode before we
+      * validate it.
+      */
+     drm_for_each_crtc(crtc, encoder->dev) {
+             crtc_funcs = crtc->helper_private;
+             if (crtc_funcs && crtc_funcs->mode_fixup)
+                     ret = crtc_funcs->mode_fixup(crtc, mode,
+                                                     &adj_mode);
Quoted 1 line

No return check?

Hrm. Actually, not sure what to do if mode_fixup failed there. I guess print a warning and validate the unadjusted mode? Other suggestions?

thanks
-john
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#3 lore raw
Quoted 103 lines · John Stultz
Quoted 97 lines · Jose Abreu

Hi John,

Quoted 81 lines · John Stultz

Currently the hikey dsi logic cannot generate accurate byte clocks values for all pixel clock values. Thus if a mode clock is selected that cannot match the calculated byte clock, the device will boot with a blank screen.

This patch uses the new mode_valid callback (many thanks to Jose Abreu for upstreaming it!) to ensure we don't select modes we cannot generate.

Also, since the ade crtc code will adjust the mode in mode_set, this patch also adds a mode_fixup callback which we use to make sure we are validating the mode clock that will eventually be used.

Many thanks to Jose and Daniel for recent feedback. I think this version is looking much nicer. But I'd still welcome any feedback or suggestions!

Cc Daniel Vetter <[email protected]>
Cc Jani Nikula <[email protected]>
Cc Sean Paul <[email protected]>
Cc David Airlie <[email protected]>
Cc Rob Clark <[email protected]>
Cc Xinliang Liu <[email protected]>
Cc Xinliang Liu <[email protected]>
Cc Rongrong Zou <[email protected]>
Cc Xinwei Kong <[email protected]>
Cc Chen Feng <[email protected]>
Cc Jose Abreu <[email protected]>
Cc Archit Taneja <[email protected]>
Cc [email protected]
Signed-off-by John Stultz <[email protected]>

---
v2: Reworked to calculate if modeclock matches the phy's
byteclock, rather then using a whitelist of known modes.

v3: Reworked to check across all possible crtcs (even though for
us there is only one), and use mode_fixup instead of a custom function, as suggested by Jose and Daniel.

---
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c    | 62 +++++++++++++++++++++++++
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 14 ++++++
 2 files changed, 76 insertions(+)
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +29 -0
index f77dcfa..d7b5820 100644
@@ -603,6 +603,67 @@ static void dsi_encoder_enable(struct drm_encoder *encoder)
      dsi->enable = true;
 }
 
+static enum drm_mode_status dsi_encoder_phy_mode_valid(struct drm_encoder *encoder,
+                                     const struct drm_display_mode *mode)
+{
+     struct dw_dsi *dsi = encoder_to_dsi(encoder);
+     struct mipi_phy_params phy;
+     u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+     u32 req_kHz, act_kHz, lane_byte_clk_kHz;
+
+     /* Calculate the lane byte clk using the adjusted mode clk */
+     memset(&phy, 0, sizeof(phy));
+     req_kHz = mode->clock * bpp / dsi->lanes;
+     act_kHz = dsi_calc_phy_rate(req_kHz, &phy);
+     lane_byte_clk_kHz = act_kHz / 8;
+
+     DRM_DEBUG_DRIVER("Checking mode %ix%i-%i@%i clock: %i...",
+                     mode->hdisplay, mode->vdisplay, bpp,
+                     drm_mode_vrefresh(mode), mode->clock);
+
+     /*
+      * Make sure the adjused mode clock and the lane byte clk
+      * have a common denominator base frequency
+      */
+     if (mode->clock/dsi->lanes == lane_byte_clk_kHz/3) {
+             DRM_DEBUG_DRIVER("OK!\n");
+             return MODE_OK;
+     }
+
+     DRM_DEBUG_DRIVER("BAD!\n");
+     return MODE_BAD;

drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c static void dsi_encoder_enable(struct drm_encoder *encoder) · line 644

Quoted 10 lines
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +10 -0
+}
+
+static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *encoder,
+                                     const struct drm_display_mode *mode)
+
+{
+     const struct drm_crtc_helper_funcs *crtc_funcs = NULL;
+     struct drm_crtc *crtc = NULL;
+     struct drm_display_mode adj_mode;
+     int ret;

This int should be an enum drm_mode_status ...

Quoted 5 lines

drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c static void dsi_encoder_enable(struct drm_encoder *encoder) · line 646

Quoted 2 lines
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +2 -0
+
+     memcpy(&adj_mode, mode, sizeof(adj_mode));

Maybe you should move this to the loop so that you pass a "clean" adjusted mode (i.e. adj_mode == mode) for each crtc. You could also use drm_mode_duplicate()/drm_mode_destroy() ...

Quoted 6 lines

Ah! Good catch! Thanks for that!

What is the benefit of drm_mode_duplicate over just using memcpy? I see there's some base.id and head pointers that are kept unique, but we're not touching those for this case. The extra allocating/freeing seems a bit needless.

Ah, I meant drm_mode_copy(), sorry.

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +12 -0
+
+     /*
+      * The crtc might adjust the mode, so go through the
+      * possible crtcs (technically just one) and call
+      * mode_fixup to figure out the adjusted mode before we
+      * validate it.
+      */
+     drm_for_each_crtc(crtc, encoder->dev) {
+             crtc_funcs = crtc->helper_private;
+             if (crtc_funcs && crtc_funcs->mode_fixup)
+                     ret = crtc_funcs->mode_fixup(crtc, mode,
+                                                     &adj_mode);
Quoted 3 lines
Quoted 1 line

No return check?

Hrm. Actually, not sure what to do if mode_fixup failed there. I guess print a warning and validate the unadjusted mode? Other suggestions?

Well, from the docs we get for the return value: "True if an acceptable configuration is possible, false if the modeset operation should be rejected." So, maybe return immediately MODE_BAD ?

Best regards,
Jose Miguel Abreu

Quoted 3 lines

thanks
-john

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel