Re: [RFC][PATCH] drm: kirin: Restrict modes to known good mode clocks

[John Stultz]

On Tue, Jul 11, 2017 at 12:56 AM, Daniel Vetter <daniel@xxxxxxxx> 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 <seanpaul@xxxxxxxxxxxx> 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@xxxxxxxxx>
>> >> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> >> Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
>> >> Cc: David Airlie <airlied@xxxxxxxx>
>> >> Cc: Rob Clark <robdclark@xxxxxxxxx>
>> >> Cc: Xinliang Liu <xinliang.liu@xxxxxxxxxx>
>> >> Cc: Xinliang Liu <z.liuxinliang@xxxxxxxxxxxxx>
>> >> Cc: Rongrong Zou <zourongrong@xxxxxxxxx>
>> >> Cc: Xinwei Kong <kong.kongxinwei@xxxxxxxxxxxxx>
>> >> Cc: Chen Feng <puck.chen@xxxxxxxxxxxxx>
>> >> Cc: Jose Abreu <Jose.Abreu@xxxxxxxxxxxx>
>> >> Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
>> >> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> >> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
>> >> ---
>> >>  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