Re: [RFC][PATCH v3] drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate

[Jose Abreu]

Hi John,


On 18-07-2017 18:59, John Stultz 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.
>
> 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 <daniel.vetter@xxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> Cc: David Airlie <airlied@xxxxxxxx>
> Cc: Rob Clark <robdclark@xxxxxxxxx>
> Cc: Xinliang Liu <xinliang.liu@xxxxxxxxxx>
> Cc: Xinliang Liu <z.liuxinliang@xxxxxxxxxxxxx>
> Cc: Rongrong Zou <zourongrong@xxxxxxxxx>
> Cc: Xinwei Kong <kong.kongxinwei@xxxxxxxxxxxxx>
> Cc: Chen Feng <puck.chen@xxxxxxxxxxxxx>
> Cc: Jose Abreu <Jose.Abreu@xxxxxxxxxxxx>
> Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
> 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(+)
>
> diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> index f77dcfa..d7b5820 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> @@ -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;

This int should be an enum drm_mode_status ...

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

> +
> +	/*
> +	 * 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

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