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.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
