Date:   Tue, 11 Jul 2017 09:56:25 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     John Stultz <john.stultz@...aro.org>
Cc:     Sean Paul <seanpaul@...omium.org>,
        Jose Abreu <Jose.Abreu@...opsys.com>,
        Chen Feng <puck.chen@...ilicon.com>,
        lkml <linux-kernel@...r.kernel.org>,
        Xinliang Liu <z.liuxinliang@...ilicon.com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        Rongrong Zou <zourongrong@...il.com>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Xinwei Kong <kong.kongxinwei@...ilicon.com>
Subject: Re: [RFC][PATCH] drm: kirin: Restrict modes to known good mode clocks

On Mon, Jul 10, 2017 at 03:47:54PM -0700, John Stultz wrote:
> On Mon, Jul 10, 2017 at 2:18 PM, Sean Paul <seanpaul@...omium.org> 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 <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>
> >> ---
> >>  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

Powered by blists - more mailing lists