From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935773AbdGTJQe (ORCPT ); Thu, 20 Jul 2017 05:16:34 -0400 Received: from us01smtprelay-2.synopsys.com ([198.182.60.111]:49131 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933179AbdGTJQ3 (ORCPT ); Thu, 20 Jul 2017 05:16:29 -0400 Subject: Re: [RFC][PATCH v3] drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate To: John Stultz , Jose Abreu References: <1500400781-18162-1-git-send-email-john.stultz@linaro.org> <3e13cc38-865d-993c-0571-2c0a4acb443a@synopsys.com> CC: lkml , Daniel Vetter , Jani Nikula , "Sean Paul" , David Airlie , Rob Clark , Xinliang Liu , Xinliang Liu , Rongrong Zou , "Xinwei Kong" , Chen Feng , Archit Taneja , "dri-devel@lists.freedesktop.org" From: Jose Abreu Message-ID: <1a82f632-0a58-9866-ede2-f7cbf4e115aa@synopsys.com> Date: Thu, 20 Jul 2017 10:16:20 +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: Content-Type: text/plain; charset="utf-8" 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 On 19-07-2017 20:21, John Stultz wrote: > On Wed, Jul 19, 2017 at 3:16 AM, Jose Abreu wrote: >> 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() ... > Ah! Good catch! Thanks for that! > > What is the benefit of drm_mode_duplicate over just using memcpy? I > see there's some base.id and head pointers that are kept unique, but > we're not touching those for this case. The extra allocating/freeing > seems a bit needless. Ah, I meant drm_mode_copy(), sorry. > > >>> + >>> + /* >>> + * 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? > Hrm. Actually, not sure what to do if mode_fixup failed there. I guess > print a warning and validate the unadjusted mode? Other suggestions? Well, from the docs we get for the return value: "True if an acceptable configuration is possible, false if the modeset operation should be rejected." So, maybe return immediately MODE_BAD ? Best regards, Jose Miguel Abreu > > thanks > -john