Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752216AbdHRP2e (ORCPT ); Fri, 18 Aug 2017 11:28:34 -0400 Received: from mail-yw0-f179.google.com ([209.85.161.179]:34078 "EHLO mail-yw0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750846AbdHRP2c (ORCPT ); Fri, 18 Aug 2017 11:28:32 -0400 Date: Fri, 18 Aug 2017 11:28:30 -0400 From: Sean Paul To: John Stultz Cc: lkml , Daniel Vetter , Jani Nikula , Sean Paul , David Airlie , Rob Clark , Xinliang Liu , Xinliang Liu , Rongrong Zou , Xinwei Kong , Chen Feng , Jose Abreu , Archit Taneja , dri-devel@lists.freedesktop.org Subject: Re: [RESEND][PATCH v4] drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate Message-ID: <20170818152830.k5mvkgnunlzr2enk@art_vandelay> References: <1502996404-15143-1-git-send-email-john.stultz@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1502996404-15143-1-git-send-email-john.stultz@linaro.org> User-Agent: NeoMutt/20170306-66-6ddb52-dirty (1.8.0) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6385 Lines: 177 On Thu, Aug 17, 2017 at 12:00:04PM -0700, 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. > > Cc: Daniel Vetter > Cc: Jani Nikula > Cc: Sean Paul > Cc: David Airlie > Cc: Rob Clark > Cc: Xinliang Liu > Cc: Xinliang Liu > Cc: Rongrong Zou > Cc: Xinwei Kong > Cc: Chen Feng > Cc: Jose Abreu > Cc: Archit Taneja > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: John Stultz Hi John, Thanks for continuing to send new versions for this patch. It looks good to me (there's a small spelling mistake in a comment below that perhaps can be fixed when applied, no biggy). Reviewed-by: Sean Paul > --- > 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. > --- > 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..043a50d 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 adjused mode clock and the lane byte clk s/adjused/adjusted/ > + * 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 > -- Sean Paul, Software Engineer, Google / Chromium OS