Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755031AbdGJWsE (ORCPT ); Mon, 10 Jul 2017 18:48:04 -0400 Received: from mail-qt0-f180.google.com ([209.85.216.180]:35574 "EHLO mail-qt0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755016AbdGJWsB (ORCPT ); Mon, 10 Jul 2017 18:48:01 -0400 MIME-Version: 1.0 In-Reply-To: <20170710211833.bprcrq5tfpzxtpeg@art_vandelay> References: <1499719682-31750-1-git-send-email-john.stultz@linaro.org> <20170710211833.bprcrq5tfpzxtpeg@art_vandelay> From: John Stultz Date: Mon, 10 Jul 2017 15:47:54 -0700 Message-ID: Subject: Re: [RFC][PATCH] drm: kirin: Restrict modes to known good mode clocks To: Sean Paul Cc: lkml , Daniel Vetter , Jani Nikula , David Airlie , Rob Clark , Xinliang Liu , Xinliang Liu , Rongrong Zou , Xinwei Kong , Chen Feng , Jose Abreu , Archit Taneja , "dri-devel@lists.freedesktop.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5306 Lines: 96 On Mon, Jul 10, 2017 at 2:18 PM, Sean Paul wrote: > 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 That's fair. The list has slowly grown from 4 or so modes to what it is now, so it is a bit longer then it was originally. I'll spin the patches with that change. > 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. Theoretically that might be possible, checking if the requested freq matches the calculated freq, and I've tried that but so far I've not been able to get it to work, as in some cases the freq on the current whitelist don't exactly match but do work on the large majority of monitors tested (while other freq have a similar error but don't work on most monitors). I'd like to spend some more time to try to refine and tune this, but having the current whitelist works fairly well, so I'm not sure its worth sitting on (this is basically the last functional patch outstanding for HiKey to fully work upstream - except the mali gpu of course), while I try to tinker and tune it. Thanks so much for the feedback! -john