| Message ID | 20170310043305.17216-2-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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, > .vdisplay = 1600, > .vsync_start = 1600 + 3, > .vsync_end = 1600 + 3 + 10, > - .vtotal = 1600 + 3 + 10 + 33, > + .vtotal = 1600 + 3 + 10 + 84, > .vrefresh = 60, > .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC, > + .type = DRM_MODE_TYPE_PREFERRED | DRM_MODE_TYPE_DRIVER, The panel-simple driver will set these two flags itself, no need to explicitly specify them. Thierry
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. 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
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 >
Hi, On Mon, Mar 20, 2017 at 1:01 PM, Stéphane Marchesin <marcheu@chromium.org> wrote: >> 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? Yeah, that's what I did originally in my RFC patch at <http://crosreview.com/354165> but I personally didn't have a good way for the driver to figure out how to pick the right clock. When the patch actually landed I guess nobody else had a good way either and thus only one clock mode was there. -Doug
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, .vdisplay = 1600, .vsync_start = 1600 + 3, .vsync_end = 1600 + 3 + 10, - .vtotal = 1600 + 3 + 10 + 33, + .vtotal = 1600 + 3 + 10 + 84, .vrefresh = 60, .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC, + .type = DRM_MODE_TYPE_PREFERRED | DRM_MODE_TYPE_DRIVER, }; static const struct panel_desc sharp_lq123p1jx31 = {
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(-)