From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754572AbdGSKQ3 (ORCPT ); Wed, 19 Jul 2017 06:16:29 -0400 Received: from smtprelay4.synopsys.com ([198.182.47.9]:33098 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753526AbdGSKQ0 (ORCPT ); Wed, 19 Jul 2017 06:16:26 -0400 Subject: Re: [RFC][PATCH v3] drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate To: John Stultz , lkml References: <1500400781-18162-1-git-send-email-john.stultz@linaro.org> CC: Daniel Vetter , Jani Nikula , Sean Paul , "David Airlie" , Rob Clark , Xinliang Liu , Xinliang Liu , Rongrong Zou , Xinwei Kong , Chen Feng , "Jose Abreu" , Archit Taneja , From: Jose Abreu Message-ID: <3e13cc38-865d-993c-0571-2c0a4acb443a@synopsys.com> Date: Wed, 19 Jul 2017 11:16:19 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1500400781-18162-1-git-send-email-john.stultz@linaro.org> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.107.19.56] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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 > --- > 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,