From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Paul Subject: Re: [RFC][PATCH] drm: kirin: Restrict modes to known good mode clocks Date: Mon, 10 Jul 2017 17:18:33 -0400 Message-ID: <20170710211833.bprcrq5tfpzxtpeg@art_vandelay> References: <1499719682-31750-1-git-send-email-john.stultz@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1499719682-31750-1-git-send-email-john.stultz@linaro.org> Sender: linux-kernel-owner@vger.kernel.org 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 List-Id: dri-devel@lists.freedesktop.org On Mon, Jul 10, 2017 at 01:48:02PM -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 enforces known good mode > clocks for well known resolutions, which should allow the > display to work from given EDID options, and ensures for a given > resolution & refresh, the right mode clock is selected. > > 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 > --- > drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 37 ++++++++++++++++++++++++++++ > 1 file changed, 37 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..a84f4bb 100644 > --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c > +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c > @@ -603,6 +603,42 @@ static void dsi_encoder_enable(struct drm_encoder *encoder) > dsi->enable = true; > } > > +static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *crtc, > + const struct drm_display_mode *mode) > +{ > + /* > + * kirin cannot generate all modes, so use the whitelist below > + */ > + DRM_DEBUG("%s: Checking mode %ix%i@%i clock: %i...", __func__, > + mode->hdisplay, mode->vdisplay, drm_mode_vrefresh(mode), mode->clock); > + if ((mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 148500) || > + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 80192) || > + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 74250) || > + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 61855) || > + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 147116) || > + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 146250) || > + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 144589) || > + (mode->hdisplay == 1600 && mode->vdisplay == 1200 && mode->clock == 160961) || > + (mode->hdisplay == 1600 && mode->vdisplay == 900 && mode->clock == 118963) || > + (mode->hdisplay == 1440 && mode->vdisplay == 900 && mode->clock == 126991) || > + (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 128946) || > + (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 98619) || > + (mode->hdisplay == 1280 && mode->vdisplay == 960 && mode->clock == 102081) || > + (mode->hdisplay == 1280 && mode->vdisplay == 800 && mode->clock == 83496) || > + (mode->hdisplay == 1280 && mode->vdisplay == 720 && mode->clock == 74440) || > + (mode->hdisplay == 1280 && mode->vdisplay == 720 && mode->clock == 74250) || > + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 78800) || > + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 75000) || > + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 81833) || > + (mode->hdisplay == 800 && mode->vdisplay == 600 && mode->clock == 48907) || > + (mode->hdisplay == 800 && mode->vdisplay == 600 && mode->clock == 40000)) { Bikeshed incoming: Can you break this out into a lookup table? It's kind of long-winded as-is. It'd be even better if you could calculate whether the mode is valid, but I didn't spend enough time to figure out if this is possible. Sean > + DRM_DEBUG("OK\n"); > + return MODE_OK; > + } > + DRM_DEBUG("BAD\n"); > + return MODE_BAD; > +} > + > static void dsi_encoder_mode_set(struct drm_encoder *encoder, > struct drm_display_mode *mode, > struct drm_display_mode *adj_mode) > @@ -622,6 +658,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 > -- > 2.7.4 > -- Sean Paul, Software Engineer, Google / Chromium OS