diff mbox series

[24/33] drm/panel-simple: Fix dotclock for Ortustech COM37H3M

Message ID 20200302203452.17977-25-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/panel: Fix dotclocks | expand

Commit Message

Ville Syrjälä March 2, 2020, 8:34 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The currently listed dotclock disagrees with the currently
listed vrefresh rate. Change the dotclock to match the vrefresh.

Someone tell me which (if either) of the dotclock or vreresh is
correct?

Cc: H. Nikolaus Schaller <hns@goldelico.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/panel/panel-simple.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

H. Nikolaus Schaller March 2, 2020, 9:24 p.m. UTC | #1 
Hi Ville,

> Am 02.03.2020 um 21:34 schrieb Ville Syrjala <ville.syrjala@linux.intel.com>:
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The currently listed dotclock disagrees with the currently
> listed vrefresh rate. Change the dotclock to match the vrefresh.
> 
> Someone tell me which (if either) of the dotclock or vreresh is
> correct?

Data sheet of COM37H3M99DTC says:

			MIN	TYP	MAX
CLK frequency 	fCLK	18	19.8	27	MHz
VSYNC Frequency	fVSYNC	54	60	66	Hz
VSYNC cycle time tv	646	650	700	H
HSYNC frequency	fHSYNC	--	39.0	50.0	Khz
HSYNC cycle time th	504	508	630	CLK

But data sheet of COM37H3M05DTC says

			MIN	TYP	MAX
CLK frequency 	fCLK	--	22.4	26.3	MHz (in VGA mode - there is also an QVGA mode)
VSYNC Frequency	fVSYNC	54	60	66	Hz
VSYNC cycle time tv	--	650	--	H
HSYNC frequency	fHSYNC	--	39.3	--	Khz
HSYNC cycle time th	--	570	--	CLK

So there are two different subvariants of the same panel.

If I remember correctly, the 05 is older (April 2010)
and the 99DTC newer (Dec 2011).

So 22 MHz isn't outside of either spec but may be higher
than needed for the 99DTC. I.e. the panel is probably
running at higher frame rate than 60 fps.

Hm. I think we should define some compromise. I.e.

.clock = 22230
.vrefresh = 60
.vtotal = 650
.htotal = 570

Probably we originally tried to do this with the parameter
set but got something wrong.

If you agree with this approach, I can try to figure out
the other parameters so that they should fit both panel
variants. I can only test with COM37H3M99DTC since I
do no longer have a device with COM37H3M05DTC.

In general it seems that the structure drm_display_mode
is overdetermined.

Either .clock could be calculated from .vrefresh (and
the other .vtotal and .htotal) or vice versa like I
did for the proposal above.

I haven't looked into the driver code, but would it be
possible to specify .clock = 0 (or leave it out) to
calculate it bottom up? This would avoid such inconsistencies.

On the other hand it is not easily visible any more
from the code if the clock is in range of the data
sheet limits.

BR and thanks,
Nikolaus

> 
> Cc: H. Nikolaus Schaller <hns@goldelico.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/panel/panel-simple.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index ca72b73408e9..f9b4f84bffb3 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -2617,7 +2617,7 @@ static const struct panel_desc ontat_yx700wv03 = {
> };
> 
> static const struct drm_display_mode ortustech_com37h3m_mode  = {
> -	.clock = 22153,
> +	.clock = 19842,
> 	.hdisplay = 480,
> 	.hsync_start = 480 + 8,
> 	.hsync_end = 480 + 8 + 10,
> -- 
> 2.24.1
>
Ville Syrjälä March 3, 2020, 3:03 p.m. UTC | #2 
On Mon, Mar 02, 2020 at 10:24:14PM +0100, H. Nikolaus Schaller wrote:
> Hi Ville,
> 
> > Am 02.03.2020 um 21:34 schrieb Ville Syrjala <ville.syrjala@linux.intel.com>:
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The currently listed dotclock disagrees with the currently
> > listed vrefresh rate. Change the dotclock to match the vrefresh.
> > 
> > Someone tell me which (if either) of the dotclock or vreresh is
> > correct?
> 
> Data sheet of COM37H3M99DTC says:
> 
> 			MIN	TYP	MAX
> CLK frequency 	fCLK	18	19.8	27	MHz
> VSYNC Frequency	fVSYNC	54	60	66	Hz
> VSYNC cycle time tv	646	650	700	H
> HSYNC frequency	fHSYNC	--	39.0	50.0	Khz
> HSYNC cycle time th	504	508	630	CLK
> 
> But data sheet of COM37H3M05DTC says
> 
> 			MIN	TYP	MAX
> CLK frequency 	fCLK	--	22.4	26.3	MHz (in VGA mode - there is also an QVGA mode)
> VSYNC Frequency	fVSYNC	54	60	66	Hz
> VSYNC cycle time tv	--	650	--	H
> HSYNC frequency	fHSYNC	--	39.3	--	Khz
> HSYNC cycle time th	--	570	--	CLK
> 
> So there are two different subvariants of the same panel.
> 
> If I remember correctly, the 05 is older (April 2010)
> and the 99DTC newer (Dec 2011).
> 
> So 22 MHz isn't outside of either spec but may be higher
> than needed for the 99DTC. I.e. the panel is probably
> running at higher frame rate than 60 fps.
> 
> Hm. I think we should define some compromise. I.e.
> 
> .clock = 22230
> .vrefresh = 60
> .vtotal = 650
> .htotal = 570
> 
> Probably we originally tried to do this with the parameter
> set but got something wrong.
> 
> If you agree with this approach, I can try to figure out
> the other parameters so that they should fit both panel
> variants. I can only test with COM37H3M99DTC since I
> do no longer have a device with COM37H3M05DTC.
> 
> In general it seems that the structure drm_display_mode
> is overdetermined.
> 
> Either .clock could be calculated from .vrefresh (and
> the other .vtotal and .htotal) or vice versa like I
> did for the proposal above.
> 
> I haven't looked into the driver code, but would it be
> possible to specify .clock = 0 (or leave it out) to
> calculate it bottom up? This would avoid such inconsistencies.

I'm going to remove .vrefresh entirely from the struct.
It'll just be calculated from the other timings as needed.

> 
> On the other hand it is not easily visible any more
> from the code if the clock is in range of the data
> sheet limits.
> 
> BR and thanks,
> Nikolaus
> 
> > 
> > Cc: H. Nikolaus Schaller <hns@goldelico.com>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > drivers/gpu/drm/panel/panel-simple.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > index ca72b73408e9..f9b4f84bffb3 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -2617,7 +2617,7 @@ static const struct panel_desc ontat_yx700wv03 = {
> > };
> > 
> > static const struct drm_display_mode ortustech_com37h3m_mode  = {
> > -	.clock = 22153,
> > +	.clock = 19842,
> > 	.hdisplay = 480,
> > 	.hsync_start = 480 + 8,
> > 	.hsync_end = 480 + 8 + 10,
> > -- 
> > 2.24.1
> >
H. Nikolaus Schaller March 3, 2020, 3:49 p.m. UTC | #3 
Hi,

> Am 03.03.2020 um 16:03 schrieb Ville Syrjälä <ville.syrjala@linux.intel.com>:
> 
>> I haven't looked into the driver code, but would it be
>> possible to specify .clock = 0 (or leave it out) to
>> calculate it bottom up? This would avoid such inconsistencies.
> 
> I'm going to remove .vrefresh entirely from the struct.
> It'll just be calculated from the other timings as needed.

Ok!

Anyways we should fix the panel timings so that it is compatible to .vrefresh = 60.

I'll give it a try and let you know.

BR and thanks,
Nikolaus
H. Nikolaus Schaller March 5, 2020, 7:41 p.m. UTC | #4 
> Am 03.03.2020 um 16:49 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi,
> 
>> Am 03.03.2020 um 16:03 schrieb Ville Syrjälä <ville.syrjala@linux.intel.com>:
>> 
>>> I haven't looked into the driver code, but would it be
>>> possible to specify .clock = 0 (or leave it out) to
>>> calculate it bottom up? This would avoid such inconsistencies.
>> 
>> I'm going to remove .vrefresh entirely from the struct.
>> It'll just be calculated from the other timings as needed.
> 
> Ok!
> 
> Anyways we should fix the panel timings so that it is compatible to .vrefresh = 60.
> 
> I'll give it a try and let you know.

Ok, here is a new parameter set within data sheet limits for both
panel variants:

static const struct drm_display_mode ortustech_com37h3m_mode  = {
	.clock = 22153,
	.hdisplay = 480,
	.hsync_start = 480 + 40,
	.hsync_end = 480 + 40 + 10,
	.htotal = 480 + 40 + 10 + 40,
	.vdisplay = 640,
	.vsync_start = 640 + 4,
	.vsync_end = 640 + 4 + 2,
	.vtotal = 640 + 4 + 2 + 4,
	.vrefresh = 60,
	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
};

I have tested on our omap3 based board and didn't find an issue
so you can insert into your patch.

BR and thanks,
Nikolaus
Ville Syrjälä March 9, 2020, 1 p.m. UTC | #5 
On Thu, Mar 05, 2020 at 08:41:43PM +0100, H. Nikolaus Schaller wrote:
> 
> > Am 03.03.2020 um 16:49 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> > 
> > Hi,
> > 
> >> Am 03.03.2020 um 16:03 schrieb Ville Syrjälä <ville.syrjala@linux.intel.com>:
> >> 
> >>> I haven't looked into the driver code, but would it be
> >>> possible to specify .clock = 0 (or leave it out) to
> >>> calculate it bottom up? This would avoid such inconsistencies.
> >> 
> >> I'm going to remove .vrefresh entirely from the struct.
> >> It'll just be calculated from the other timings as needed.
> > 
> > Ok!
> > 
> > Anyways we should fix the panel timings so that it is compatible to .vrefresh = 60.
> > 
> > I'll give it a try and let you know.
> 
> Ok, here is a new parameter set within data sheet limits for both
> panel variants:
> 
> static const struct drm_display_mode ortustech_com37h3m_mode  = {
> 	.clock = 22153,
> 	.hdisplay = 480,
> 	.hsync_start = 480 + 40,
> 	.hsync_end = 480 + 40 + 10,
> 	.htotal = 480 + 40 + 10 + 40,
> 	.vdisplay = 640,
> 	.vsync_start = 640 + 4,
> 	.vsync_end = 640 + 4 + 2,
> 	.vtotal = 640 + 4 + 2 + 4,
> 	.vrefresh = 60,
> 	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> };
> 
> I have tested on our omap3 based board and didn't find an issue
> so you can insert into your patch.

Migth be better if you send that so we get proper attribution and
you can explain the change properly in the commit message.
H. Nikolaus Schaller March 9, 2020, 1:03 p.m. UTC | #6 
> Am 09.03.2020 um 14:00 schrieb Ville Syrjälä <ville.syrjala@linux.intel.com>:
> 
> On Thu, Mar 05, 2020 at 08:41:43PM +0100, H. Nikolaus Schaller wrote:
>> 
>>> Am 03.03.2020 um 16:49 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>>> 
>>> Hi,
>>> 
>>>> Am 03.03.2020 um 16:03 schrieb Ville Syrjälä <ville.syrjala@linux.intel.com>:
>>>> 
>>>>> I haven't looked into the driver code, but would it be
>>>>> possible to specify .clock = 0 (or leave it out) to
>>>>> calculate it bottom up? This would avoid such inconsistencies.
>>>> 
>>>> I'm going to remove .vrefresh entirely from the struct.
>>>> It'll just be calculated from the other timings as needed.
>>> 
>>> Ok!
>>> 
>>> Anyways we should fix the panel timings so that it is compatible to .vrefresh = 60.
>>> 
>>> I'll give it a try and let you know.
>> 
>> Ok, here is a new parameter set within data sheet limits for both
>> panel variants:
>> 
>> static const struct drm_display_mode ortustech_com37h3m_mode  = {
>> 	.clock = 22153,
>> 	.hdisplay = 480,
>> 	.hsync_start = 480 + 40,
>> 	.hsync_end = 480 + 40 + 10,
>> 	.htotal = 480 + 40 + 10 + 40,
>> 	.vdisplay = 640,
>> 	.vsync_start = 640 + 4,
>> 	.vsync_end = 640 + 4 + 2,
>> 	.vtotal = 640 + 4 + 2 + 4,
>> 	.vrefresh = 60,
>> 	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
>> };
>> 
>> I have tested on our omap3 based board and didn't find an issue
>> so you can insert into your patch.
> 
> Migth be better if you send that so we get proper attribution and
> you can explain the change properly in the commit message.

Ok, will do asap.

BR and thanks,
Nikolaus
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index ca72b73408e9..f9b4f84bffb3 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -2617,7 +2617,7 @@  static const struct panel_desc ontat_yx700wv03 = {
 };
 
 static const struct drm_display_mode ortustech_com37h3m_mode  = {
-	.clock = 22153,
+	.clock = 19842,
 	.hdisplay = 480,
 	.hsync_start = 480 + 8,
 	.hsync_end = 480 + 8 + 10,