On Mon, Mar 20, 2017 at 9:37 AM, Doug Anderson <dianders@chromium.org>
wrote:

> Hi,
>
> On Mon, Mar 20, 2017 at 6:59 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Thu, Mar 09, 2017 at 11:32:16PM -0500, Sean Paul wrote:
> >> Change the mode for Sharp lq123p1jx31 panel to something more
> >> rockchip-friendly such that we can use the fixed PLLs to
> >> generate the pixel clock
> >>
> >> Cc: Chris Zhong <zyw@rock-chips.com>
> >> Cc: Stéphane Marchesin <marcheu@chromium.org>
> >> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> >> ---
> >>  drivers/gpu/drm/panel/panel-simple.c | 7 ++++---
> >>  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > That's really not how you should be doing this. If you want to support
> > this panel on more than one type of hardware, especially if they have
> > different restrictions on what clocks and timings they can generate,
> > the driver should specify the timings using a struct display_timing and
> > drivers should use that data to generate a mode that matches their
> > requirements but is still within the valid ranges specified in the .min
> > and .max values.
> >
> > That said, in this particular case nobody seems to be using the panel
> > in the upstream kernel.
>
> Sean and I had a private conversation about this too.  Roughly summarizing:
>
> 1. We have validated with the panel manufacturer that 266.667 MHz is
> valid and within spec.  The panel's datasheet itself says something
> like "if you want to try other values you can, but they might not
> work", so technically the only values "known" to work are those that
> were in the original patch and the values here in Sean's patch.
>

So why don't you add 2 modes, and let the drivers pick the clock they
prefer?

Stéphane


>
> 2. In the particular case of rk3399-kevin (which uses this panel), we
> actually went through several iterations before we found a mode that
> not only worked with the limited PLLs but also that didn't generate
> excessive noise on the digitizer (used for stylus input).  The
> digitizer is (as I understand) a separate component from the panel
> itself, so this restriction isn't really one on the panel but is a
> reality of how this panel was used in real hardware.  I have no idea
> how one expresses this board-centric view of the world.
>
> NOTE: Point #2 actually made me check, and Sean's patch is the wrong
> iteration of these changes.  Please see http://crosreview.com/381015
>
> 3. In an ideal world, even on rk3399-kevin we'd be able to choose the
> 252.75 MHz clock if the variable PLL on rk3399 happened to be
> available (if there was no external display whose pixel clock needed
> the variable PLL).  This would save a bit of power, and I believe the
> 252.75 MHz also avoids noise on the digitizer.  ...but trying to deal
> with all this was very complicated.
>
>
> That all being said: I'd personally be in favor for something like
> Sean's patch to land since, as you said, there are no other current
> users of the panel.  It's nice to start with something working and
> hopefully we can figure out a more advanced / dynamic system sometime
> in the future.
>
>
> > One minor nit below...
> >
> >>
> >> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> >> index 89eb0422821c..787b4d143220 100644
> >> --- a/drivers/gpu/drm/panel/panel-simple.c
> >> +++ b/drivers/gpu/drm/panel/panel-simple.c
> >> @@ -1598,17 +1598,18 @@ static const struct panel_desc
> sharp_lq101k1ly04 = {
> >>  };
> >>
> >>  static const struct drm_display_mode sharp_lq123p1jx31_mode = {
> >> -     .clock = 252750,
> >> +     .clock = 266667,
> >>       .hdisplay = 2400,
> >>       .hsync_start = 2400 + 48,
> >>       .hsync_end = 2400 + 48 + 32,
> >> -     .htotal = 2400 + 48 + 32 + 80,
> >> +     .htotal = 2400 + 48 + 32 + 139,
>
> Please fold in <https://chromium-review.googlesource.com/381015> to
> get noise-free timings.
>
>
> >>       .vdisplay = 1600,
> >>       .vsync_start = 1600 + 3,
> >>       .vsync_end = 1600 + 3 + 10,
> >> -     .vtotal = 1600 + 3 + 10 + 33,
> >> +     .vtotal = 1600 + 3 + 10 + 84,
>
> Here too.
>
> >>       .vrefresh = 60,
> >>       .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> >> +     .type = DRM_MODE_TYPE_PREFERRED | DRM_MODE_TYPE_DRIVER,
>
> IIRC this was an attempt to deal with the fact that the EDID had a
> different mode than we were specifying here, but I could be wrong.
>
>
>
> -Doug
>