Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755944AbdGKPFK (ORCPT ); Tue, 11 Jul 2017 11:05:10 -0400 Received: from mail-qk0-f180.google.com ([209.85.220.180]:33452 "EHLO mail-qk0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755872AbdGKPFI (ORCPT ); Tue, 11 Jul 2017 11:05:08 -0400 MIME-Version: 1.0 In-Reply-To: <20170711075625.peykmamxkt7slqdz@phenom.ffwll.local> References: <1499719682-31750-1-git-send-email-john.stultz@linaro.org> <20170710211833.bprcrq5tfpzxtpeg@art_vandelay> <20170711075625.peykmamxkt7slqdz@phenom.ffwll.local> From: John Stultz Date: Tue, 11 Jul 2017 08:05:06 -0700 Message-ID: Subject: Re: [RFC][PATCH] drm: kirin: Restrict modes to known good mode clocks To: John Stultz , Sean Paul , Jose Abreu , Chen Feng , lkml , Xinliang Liu , "dri-devel@lists.freedesktop.org" , Rongrong Zou , Daniel Vetter , Xinwei Kong 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: 6219 Lines: 109 On Tue, Jul 11, 2017 at 12:56 AM, Daniel Vetter wrote: > On Mon, Jul 10, 2017 at 03:47:54PM -0700, John Stultz wrote: >> 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! > > Yeah the proper approach is to compute your pll/clock settings and bail > out if those don't work. That's generally a magic spreadsheet supplied by > the hw validation engineers, and I honestly don't want to know how they > create it. Explicit modelist in the kernel sounds like a very bad hack. So without such a magic spreadsheet, how would you suggest I move this forward? Not having the whitelist hack and picking modes the device can't generate is a fairly major usability issue. thanks -john