[v3,33/43] drm/panel: simple: Change mode for Sharp lq123p1jx31

Commit Message

Thierry Escande Jan. 30, 2018, 8:29 p.m. UTC

From: Sean Paul <[e-mail]>

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 <[e-mail]>
Cc: Stéphane Marchesin <[e-mail]>
Signed-off-by: Sean Paul <[e-mail]>
Signed-off-by: Thierry Escande <[e-mail]>
---
 drivers/gpu/drm/panel/panel-simple.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Lucas Stach Jan. 31, 2018, 12:54 p.m. UTC | #1

Am Dienstag, den 30.01.2018, 21:29 +0100 schrieb Thierry Escande:
> From: Sean Paul <[e-mail]>
> 
> 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

This should really switch to a display timing instead of exposing a
single mode. The display timing has min, typical, max tuples for all
the timings values, which would allow the attached driver to vary the
timings inside the allowed bounds if it makes sense.

Trying to hit a specific pixel clock to free up a PLL is exactly one of
the use cases envisioned for the display timings stuff.

Regards,
Lucas
 
> Cc: Chris Zhong <[e-mail]>
> Cc: Stéphane Marchesin <[e-mail]>
> Signed-off-by: Sean Paul <[e-mail]>
> Signed-off-by: Thierry Escande <[e-mail]>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> index 5591984a392b..a4a6ea3ca0e6 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1742,17 +1742,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 = {

Sean Paul Jan. 31, 2018, 3:16 p.m. UTC | #2

On Wed, Jan 31, 2018 at 7:54 AM, Lucas Stach <[e-mail]> wrote:
> Am Dienstag, den 30.01.2018, 21:29 +0100 schrieb Thierry Escande:
>> From: Sean Paul <[e-mail]>
>>
>> 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
>
> This should really switch to a display timing instead of exposing a
> single mode. The display timing has min, typical, max tuples for all
> the timings values, which would allow the attached driver to vary the
> timings inside the allowed bounds if it makes sense.
>
> Trying to hit a specific pixel clock to free up a PLL is exactly one of
> the use cases envisioned for the display timings stuff.
>

Agreed, I think we had this discussion the first time around. We
should drop this patch.

Thanks for catching this!

Sean

> Regards,
> Lucas
>
>> Cc: Chris Zhong <[e-mail]>
>> Cc: Stéphane Marchesin <[e-mail]>
>> Signed-off-by: Sean Paul <[e-mail]>
>> Signed-off-by: Thierry Escande <[e-mail]>
>> ---
>>  drivers/gpu/drm/panel/panel-simple.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-simple.c
>> b/drivers/gpu/drm/panel/panel-simple.c
>> index 5591984a392b..a4a6ea3ca0e6 100644
>> --- a/drivers/gpu/drm/panel/panel-simple.c
>> +++ b/drivers/gpu/drm/panel/panel-simple.c
>> @@ -1742,17 +1742,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 = {

Doug Anderson Jan. 31, 2018, 4:52 p.m. UTC | #3

Hi,


On Wed, Jan 31, 2018 at 7:16 AM, Sean Paul <[e-mail]> wrote:
> On Wed, Jan 31, 2018 at 7:54 AM, Lucas Stach <[e-mail]> wrote:
>> Am Dienstag, den 30.01.2018, 21:29 +0100 schrieb Thierry Escande:
>>> From: Sean Paul <[e-mail]>
>>>
>>> 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
>>
>> This should really switch to a display timing instead of exposing a
>> single mode. The display timing has min, typical, max tuples for all
>> the timings values, which would allow the attached driver to vary the
>> timings inside the allowed bounds if it makes sense.
>>
>> Trying to hit a specific pixel clock to free up a PLL is exactly one of
>> the use cases envisioned for the display timings stuff.
>>
>
> Agreed, I think we had this discussion the first time around. We
> should drop this patch.
>
> Thanks for catching this!

Are you sure we should drop this?  In order for things to work
properly (not generate noise on the digitizer or other EMI), this
needs to run at a very specific pixel clock with very specific
blanking times.  I know that earlier we had slightly different
blanking times and Samsung came back and said that there was noise on
the digitizer.  I could be wrong, but I don't think there's any way
currently to be able to specify exactly what timings should be used on
a particular board.

Don't get be wrong--I think a patch such as this one that claims a
single board's timings as the "right" ones for a generic panel is a
bit of a hack.  ...but at the same time there are no other users of
this panel (that I know of) in mainline and the timings presented here
are certainly sane timings for this panel.

In any case, previous discussion at: https://patchwork.kernel.org/patch/9614603/


...oh, and looking at the previous discussion reminds me that the
timings presented in this here patch are actually not the right ones
(they have the right PLL, but the wrong blankings to avoid the noise
issues).  See <//chromium-review.googlesource.com/381015>



-Doug

Enric Balletbo Serra Feb. 16, 2018, 12:34 p.m. UTC | #4

Hi,

2018-01-31 17:52 GMT+01:00 Doug Anderson <[e-mail]>:
> Hi,
>
>
> On Wed, Jan 31, 2018 at 7:16 AM, Sean Paul <[e-mail]> wrote:
>> On Wed, Jan 31, 2018 at 7:54 AM, Lucas Stach <[e-mail]> wrote:
>>> Am Dienstag, den 30.01.2018, 21:29 +0100 schrieb Thierry Escande:
>>>> From: Sean Paul <[e-mail]>
>>>>
>>>> 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
>>>
>>> This should really switch to a display timing instead of exposing a
>>> single mode. The display timing has min, typical, max tuples for all
>>> the timings values, which would allow the attached driver to vary the
>>> timings inside the allowed bounds if it makes sense.
>>>
>>> Trying to hit a specific pixel clock to free up a PLL is exactly one of
>>> the use cases envisioned for the display timings stuff.
>>>
>>
>> Agreed, I think we had this discussion the first time around. We
>> should drop this patch.
>>
>> Thanks for catching this!
>
> Are you sure we should drop this?  In order for things to work
> properly (not generate noise on the digitizer or other EMI), this
> needs to run at a very specific pixel clock with very specific
> blanking times.  I know that earlier we had slightly different
> blanking times and Samsung came back and said that there was noise on
> the digitizer.  I could be wrong, but I don't think there's any way
> currently to be able to specify exactly what timings should be used on
> a particular board.
>
> Don't get be wrong--I think a patch such as this one that claims a
> single board's timings as the "right" ones for a generic panel is a
> bit of a hack.  ...but at the same time there are no other users of
> this panel (that I know of) in mainline and the timings presented here
> are certainly sane timings for this panel.
>
> In any case, previous discussion at: https://patchwork.kernel.org/patch/9614603/
>
>
> ...oh, and looking at the previous discussion reminds me that the
> timings presented in this here patch are actually not the right ones
> (they have the right PLL, but the wrong blankings to avoid the noise
> issues).  See <//chromium-review.googlesource.com/381015>
>

As Thierry no longer has the hardware to test these patch series, I'll
take care of these and follow the upstreaming process. I think that
doesn't make sense send a v4 version of all 43 patches for this
change. Right now, only this patch received comments so I'll wait a
bit more for if we can get the other patches reviewed. If the others
are fine just and I don't need to send a new version just don't apply
this one and I will send a second version of that specific patch. Or
even better, is really trivial what needs to be changed, so maybe the
maintainer can do it? ;)

Regards,
 Enric

>
>
> -Doug
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Doug Anderson Feb. 16, 2018, 8:54 p.m. UTC | #5

Hi,

On Fri, Feb 16, 2018 at 4:34 AM, Enric Balletbo Serra
<[e-mail]> wrote:
> Hi,
>
> 2018-01-31 17:52 GMT+01:00 Doug Anderson <[e-mail]>:
>> Hi,
>>
>>
>> On Wed, Jan 31, 2018 at 7:16 AM, Sean Paul <[e-mail]> wrote:
>>> On Wed, Jan 31, 2018 at 7:54 AM, Lucas Stach <[e-mail]> wrote:
>>>> Am Dienstag, den 30.01.2018, 21:29 +0100 schrieb Thierry Escande:
>>>>> From: Sean Paul <[e-mail]>
>>>>>
>>>>> 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
>>>>
>>>> This should really switch to a display timing instead of exposing a
>>>> single mode. The display timing has min, typical, max tuples for all
>>>> the timings values, which would allow the attached driver to vary the
>>>> timings inside the allowed bounds if it makes sense.
>>>>
>>>> Trying to hit a specific pixel clock to free up a PLL is exactly one of
>>>> the use cases envisioned for the display timings stuff.
>>>>
>>>
>>> Agreed, I think we had this discussion the first time around. We
>>> should drop this patch.
>>>
>>> Thanks for catching this!
>>
>> Are you sure we should drop this?  In order for things to work
>> properly (not generate noise on the digitizer or other EMI), this
>> needs to run at a very specific pixel clock with very specific
>> blanking times.  I know that earlier we had slightly different
>> blanking times and Samsung came back and said that there was noise on
>> the digitizer.  I could be wrong, but I don't think there's any way
>> currently to be able to specify exactly what timings should be used on
>> a particular board.
>>
>> Don't get be wrong--I think a patch such as this one that claims a
>> single board's timings as the "right" ones for a generic panel is a
>> bit of a hack.  ...but at the same time there are no other users of
>> this panel (that I know of) in mainline and the timings presented here
>> are certainly sane timings for this panel.
>>
>> In any case, previous discussion at: https://patchwork.kernel.org/patch/9614603/
>>
>>
>> ...oh, and looking at the previous discussion reminds me that the
>> timings presented in this here patch are actually not the right ones
>> (they have the right PLL, but the wrong blankings to avoid the noise
>> issues).  See <//chromium-review.googlesource.com/381015>
>>
>
> As Thierry no longer has the hardware to test these patch series, I'll
> take care of these and follow the upstreaming process. I think that
> doesn't make sense send a v4 version of all 43 patches for this
> change. Right now, only this patch received comments so I'll wait a
> bit more for if we can get the other patches reviewed. If the others
> are fine just and I don't need to send a new version just don't apply
> this one and I will send a second version of that specific patch. Or
> even better, is really trivial what needs to be changed, so maybe the
> maintainer can do it? ;)

Just as a heads up, Sean Paul has a series of patches to replace this
patch.  The following are IDs from patchwork.kernel.org:

10207583 New          [v3,1/6] dt-bindings: Clarify timing subnode use
as panel-timing
10207585 New          [v3,2/6] dt-bindings: Add headings to
simple-panel bindings
10207591 New          [v3,3/6] dt-bindings: Add panel-timing subnode
to simple-panel
10207593 New          [v3,4/6] drm/panel: simple: Add ability to
override typical timing
10207595 New          [v3,5/6] drm/panel: simple: Use display_timing
for lq123p1jx31
10207603 New          [v3,6/6] arm64: dts: rockchip: Specify override
mode for kevin panel

-Doug

Enric Balletbo Serra Feb. 19, 2018, 9:42 a.m. UTC | #6

Hi,

2018-02-16 21:54 GMT+01:00 Doug Anderson <[e-mail]>:
> Hi,
>
> On Fri, Feb 16, 2018 at 4:34 AM, Enric Balletbo Serra
> <[e-mail]> wrote:
>> Hi,
>>
>> 2018-01-31 17:52 GMT+01:00 Doug Anderson <[e-mail]>:
>>> Hi,
>>>
>>>
>>> On Wed, Jan 31, 2018 at 7:16 AM, Sean Paul <[e-mail]> wrote:
>>>> On Wed, Jan 31, 2018 at 7:54 AM, Lucas Stach <[e-mail]> wrote:
>>>>> Am Dienstag, den 30.01.2018, 21:29 +0100 schrieb Thierry Escande:
>>>>>> From: Sean Paul <[e-mail]>
>>>>>>
>>>>>> 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
>>>>>
>>>>> This should really switch to a display timing instead of exposing a
>>>>> single mode. The display timing has min, typical, max tuples for all
>>>>> the timings values, which would allow the attached driver to vary the
>>>>> timings inside the allowed bounds if it makes sense.
>>>>>
>>>>> Trying to hit a specific pixel clock to free up a PLL is exactly one of
>>>>> the use cases envisioned for the display timings stuff.
>>>>>
>>>>
>>>> Agreed, I think we had this discussion the first time around. We
>>>> should drop this patch.
>>>>
>>>> Thanks for catching this!
>>>
>>> Are you sure we should drop this?  In order for things to work
>>> properly (not generate noise on the digitizer or other EMI), this
>>> needs to run at a very specific pixel clock with very specific
>>> blanking times.  I know that earlier we had slightly different
>>> blanking times and Samsung came back and said that there was noise on
>>> the digitizer.  I could be wrong, but I don't think there's any way
>>> currently to be able to specify exactly what timings should be used on
>>> a particular board.
>>>
>>> Don't get be wrong--I think a patch such as this one that claims a
>>> single board's timings as the "right" ones for a generic panel is a
>>> bit of a hack.  ...but at the same time there are no other users of
>>> this panel (that I know of) in mainline and the timings presented here
>>> are certainly sane timings for this panel.
>>>
>>> In any case, previous discussion at: https://patchwork.kernel.org/patch/9614603/
>>>
>>>
>>> ...oh, and looking at the previous discussion reminds me that the
>>> timings presented in this here patch are actually not the right ones
>>> (they have the right PLL, but the wrong blankings to avoid the noise
>>> issues).  See <//chromium-review.googlesource.com/381015>
>>>
>>
>> As Thierry no longer has the hardware to test these patch series, I'll
>> take care of these and follow the upstreaming process. I think that
>> doesn't make sense send a v4 version of all 43 patches for this
>> change. Right now, only this patch received comments so I'll wait a
>> bit more for if we can get the other patches reviewed. If the others
>> are fine just and I don't need to send a new version just don't apply
>> this one and I will send a second version of that specific patch. Or
>> even better, is really trivial what needs to be changed, so maybe the
>> maintainer can do it? ;)
>
> Just as a heads up, Sean Paul has a series of patches to replace this
> patch.  The following are IDs from patchwork.kernel.org:
>
> 10207583 New          [v3,1/6] dt-bindings: Clarify timing subnode use
> as panel-timing
> 10207585 New          [v3,2/6] dt-bindings: Add headings to
> simple-panel bindings
> 10207591 New          [v3,3/6] dt-bindings: Add panel-timing subnode
> to simple-panel
> 10207593 New          [v3,4/6] drm/panel: simple: Add ability to
> override typical timing
> 10207595 New          [v3,5/6] drm/panel: simple: Use display_timing
> for lq123p1jx31
> 10207603 New          [v3,6/6] arm64: dts: rockchip: Specify override
> mode for kevin panel
>
> -Doug

Nice, I was not aware of these, I'll test. That means that this patch
can be removed from these series as the Sean solution is a lot better.
Just a note that this patch can be removed without any collateral
impact on the other patches, so just ignore it.

Regards,
 Enric

Patch

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 5591984a392b..a4a6ea3ca0e6 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1742,17 +1742,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 = {