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

Commit Message

John Stultz Aug. 22, 2017, 6:42 p.m. UTC

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.

Cc: Daniel Vetter <[e-mail]>
Cc: Jani Nikula <[e-mail]>
Cc: Sean Paul <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: Rob Clark <[e-mail]>
Cc: Xinliang Liu <[e-mail]>
Cc: Xinliang Liu <[e-mail]>
Cc: Rongrong Zou <[e-mail]>
Cc: Xinwei Kong <[e-mail]>
Cc: Chen Feng <[e-mail]>
Cc: Jose Abreu <[e-mail]>
Cc: Archit Taneja <[e-mail]>
Cc: [e-mail]
Reviewed-by: Sean Paul <[e-mail]>
Signed-off-by: John Stultz <[e-mail]>
---
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.

v4: Fixes and improved error handling as suggested by Jose.

v5: Small typo fix noted by Sean
---
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c    | 67 +++++++++++++++++++++++++
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 14 ++++++
 2 files changed, 81 insertions(+)

Comments

Xinliang Liu Aug. 28, 2017, 8:36 a.m. UTC | #1

On 23 August 2017 at 02:42, John Stultz <[e-mail]> wrote:

> 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.
>
> Cc: Daniel Vetter <[e-mail]>
> Cc: Jani Nikula <[e-mail]>
> Cc: Sean Paul <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Rob Clark <[e-mail]>
> Cc: Xinliang Liu <[e-mail]>
> Cc: Xinliang Liu <[e-mail]>
> Cc: Rongrong Zou <[e-mail]>
> Cc: Xinwei Kong <[e-mail]>
> Cc: Chen Feng <[e-mail]>
> Cc: Jose Abreu <[e-mail]>
> Cc: Archit Taneja <[e-mail]>
> Cc: [e-mail]
> Reviewed-by: Sean Paul <[e-mail]>
>

​Thanks John,
This patch looks good to me.
Reviewed-by: Xinliang Liu <[e-mail]>​

Thanks,
Xinliang



> Signed-off-by: John Stultz <[e-mail]>
> ---
> 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.
>
> v4: Fixes and improved error handling as suggested by Jose.
>
> v5: Small typo fix noted by Sean
> ---
>  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c    | 67
> +++++++++++++++++++++++++
>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 14 ++++++
>  2 files changed, 81 insertions(+)
>
> diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> index f77dcfa..b4c7af3 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> @@ -603,6 +603,72 @@ 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 adjusted 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;
> +       enum drm_mode_status ret;
> +
> +       /*
> +        * 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) {
> +               /*
> +                * reset adj_mode to the mode value each time,
> +                * so we don't adjust the mode twice
> +                */
> +               drm_mode_copy(&adj_mode, mode);
> +
> +               crtc_funcs = crtc->helper_private;
> +               if (crtc_funcs && crtc_funcs->mode_fixup)
> +                       if (!crtc_funcs->mode_fixup(crtc, mode, &adj_mode))
> +                               return MODE_BAD;
> +
> +               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 +688,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
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> index c96c228..dec7f4e 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> @@ -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,
> --
> 2.7.4
>
>

Patch

diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
index f77dcfa..b4c7af3 100644
--- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
+++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
@@ -603,6 +603,72 @@  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 adjusted 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;
+	enum drm_mode_status ret;
+
+	/*
+	 * 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) {
+		/*
+		 * reset adj_mode to the mode value each time,
+		 * so we don't adjust the mode twice
+		 */
+		drm_mode_copy(&adj_mode, mode);
+
+		crtc_funcs = crtc->helper_private;
+		if (crtc_funcs && crtc_funcs->mode_fixup)
+			if (!crtc_funcs->mode_fixup(crtc, mode, &adj_mode))
+				return MODE_BAD;
+
+		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 +688,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
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
index c96c228..dec7f4e 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
@@ -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,