On 23 August 2017 at 02:42, John Stultz <john.stu...@linaro.org> 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 <daniel.vet...@intel.com>
> Cc: Jani Nikula <jani.nik...@linux.intel.com>
> Cc: Sean Paul <seanp...@chromium.org>
> Cc: David Airlie <airl...@linux.ie>
> Cc: Rob Clark <robdcl...@gmail.com>
> Cc: Xinliang Liu <xinliang....@linaro.org>
> Cc: Xinliang Liu <z.liuxinli...@hisilicon.com>
> Cc: Rongrong Zou <zourongr...@gmail.com>
> Cc: Xinwei Kong <kong.kongxin...@hisilicon.com>
> Cc: Chen Feng <puck.c...@hisilicon.com>
> Cc: Jose Abreu <jose.ab...@synopsys.com>
> Cc: Archit Taneja <arch...@codeaurora.org>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Sean Paul <seanp...@chromium.org>
>

​Thanks John,
This patch looks good to me.
Reviewed-by: Xinliang Liu <xinliang....@linaro.org>​

Thanks,
Xinliang



> Signed-off-by: John Stultz <john.stu...@linaro.org>
> ---
> 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
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to