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

[John Stultz]

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!
-john