Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755359AbdGKH4c (ORCPT ); Tue, 11 Jul 2017 03:56:32 -0400 Received: from mail-lf0-f68.google.com ([209.85.215.68]:34635 "EHLO mail-lf0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754126AbdGKH4a (ORCPT ); Tue, 11 Jul 2017 03:56:30 -0400 Date: Tue, 11 Jul 2017 09:56:25 +0200 From: Daniel Vetter To: John Stultz Cc: Sean Paul , Jose Abreu , Chen Feng , lkml , Xinliang Liu , "dri-devel@lists.freedesktop.org" , Rongrong Zou , Daniel Vetter , Xinwei Kong Subject: Re: [RFC][PATCH] drm: kirin: Restrict modes to known good mode clocks Message-ID: <20170711075625.peykmamxkt7slqdz@phenom.ffwll.local> Mail-Followup-To: John Stultz , Sean Paul , Jose Abreu , Chen Feng , lkml , Xinliang Liu , "dri-devel@lists.freedesktop.org" , Rongrong Zou , Daniel Vetter , Xinwei Kong References: <1499719682-31750-1-git-send-email-john.stultz@linaro.org> <20170710211833.bprcrq5tfpzxtpeg@art_vandelay> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux phenom 4.11.0-1-amd64 User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5926 Lines: 106 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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch