[01/41] drm/panel: simple: Change mode for Sharp lq123p1jx31
diff mbox

Message ID 20170310043305.17216-2-seanpaul@chromium.org
State New
Headers show

Commit Message

Sean Paul March 10, 2017, 4:32 a.m. UTC
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(-)

Comments

Thierry Reding March 20, 2017, 1:59 p.m. UTC | #1
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
Doug Anderson March 20, 2017, 4:37 p.m. UTC | #2
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
Doug Anderson March 20, 2017, 8:05 p.m. UTC | #3
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

Patch
diff mbox

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 = {