| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170818152830.k5mvkgnunlzr2enk@art_vandelay>
Date: Fri, 18 Aug 2017 11:28:30 -0400
From: Sean Paul <seanpaul@...omium.org>
To: John Stultz <john.stultz@...aro.org>
Cc: lkml <linux-kernel@...r.kernel.org>,
Daniel Vetter <daniel.vetter@...el.com>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Sean Paul <seanpaul@...omium.org>,
David Airlie <airlied@...ux.ie>,
Rob Clark <robdclark@...il.com>,
Xinliang Liu <xinliang.liu@...aro.org>,
Xinliang Liu <z.liuxinliang@...ilicon.com>,
Rongrong Zou <zourongrong@...il.com>,
Xinwei Kong <kong.kongxinwei@...ilicon.com>,
Chen Feng <puck.chen@...ilicon.com>,
Jose Abreu <Jose.Abreu@...opsys.com>,
Archit Taneja <architt@...eaurora.org>,
dri-devel@...ts.freedesktop.org
Subject: Re: [RESEND][PATCH v4] drm: kirin: Add mode_valid logic to avoid
mode clocks we can't generate
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 <daniel.vetter@...el.com>
> Cc: Jani Nikula <jani.nikula@...ux.intel.com>
> Cc: Sean Paul <seanpaul@...omium.org>
> Cc: David Airlie <airlied@...ux.ie>
> Cc: Rob Clark <robdclark@...il.com>
> Cc: Xinliang Liu <xinliang.liu@...aro.org>
> Cc: Xinliang Liu <z.liuxinliang@...ilicon.com>
> Cc: Rongrong Zou <zourongrong@...il.com>
> Cc: Xinwei Kong <kong.kongxinwei@...ilicon.com>
> Cc: Chen Feng <puck.chen@...ilicon.com>
> Cc: Jose Abreu <Jose.Abreu@...opsys.com>
> Cc: Archit Taneja <architt@...eaurora.org>
> Cc: dri-devel@...ts.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@...aro.org>
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 <seanpaul@...omium.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.
> ---
> 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
Powered by blists - more mailing lists