| Message ID | 20200302203452.17977-29-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: Fix dotclocks | expand |
On 2020-03-02 21:34, Ville Syrjala wrote: > 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? TL/DR; I do not care if you change the refresh rate or the dotclock. The whole entry for that panel in simple-panel is dubious. The panel is really an LVDS panel (capable of both VESA/Jeida RGB888, selectable with the SELLVDS pin). With Jeida you can, as usual, omit the 4th data channel and use the panel with RGB666. In either case, you need an LVDS signal and nothing else... The panel can also rotate the picture 180 degrees using the RL/UD pin. These options are of course not expressed in the simple panel driver (and we have always used fixed signals for those pins in our designs, IIRC). As far as I'm concerned, the panel can be removed from simple-panel. Our device trees are nowadays correctly expressing the hardware with an LVDS encoder between the RGB output and the panel and points to the panel-lvds driver for the panel. The reason that it is as it is, is that we obviously didn't understand what we were doing when we added the entry, and this garbage was what we came up with that produced a picture. If you want to keep the panel in simple-panel despite all this, the timing constraints are as follows: Pixel clock 50-80 MHz, 65 MHz typical Horizontal period 1094-1720 clocks, 1344 typical 16.0-23.4 us 20.7 us Horizontal enable 1024 clocks, always Vertical period 776-990 lines, 806 typical 13.3-18.0 ms 16.7 ms Vertical enable 768 lines, always Using a "long" (the datasheet is not very specific on this issue) vertical period may introduce deterioration of display quality, flicker etc. I don't think the division between front-porch/back-porch matters much. That said, I have no idea whatsoever if others have started using this panel entry. My guess is that it has zero users, but who can tell? Cheers, Peter > Cc: Gustaf Lindström <gl@axentia.se> > Cc: Peter Rosin <peda@axentia.se> > Cc: Thierry Reding <treding@nvidia.com> > 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 7526af2d6d95..cb587de8c49e 100644 > --- a/drivers/gpu/drm/panel/panel-simple.c > +++ b/drivers/gpu/drm/panel/panel-simple.c > @@ -3026,7 +3026,7 @@ static const struct panel_desc sharp_lq123p1jx31 = { > }; > > static const struct drm_display_mode sharp_lq150x1lg11_mode = { > - .clock = 71100, > + .clock = 65722, > .hdisplay = 1024, > .hsync_start = 1024 + 168, > .hsync_end = 1024 + 168 + 64, >
On Mon, Mar 02, 2020 at 10:53:56PM +0000, Peter Rosin wrote: > On 2020-03-02 21:34, Ville Syrjala wrote: > > 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? > > TL/DR; I do not care if you change the refresh rate or the dotclock. > > The whole entry for that panel in simple-panel is dubious. The panel > is really an LVDS panel (capable of both VESA/Jeida RGB888, selectable > with the SELLVDS pin). With Jeida you can, as usual, omit the 4th > data channel and use the panel with RGB666. In either case, you need > an LVDS signal and nothing else... > > The panel can also rotate the picture 180 degrees using the RL/UD pin. > > These options are of course not expressed in the simple panel driver > (and we have always used fixed signals for those pins in our designs, > IIRC). As far as I'm concerned, the panel can be removed from > simple-panel. Our device trees are nowadays correctly expressing the > hardware with an LVDS encoder between the RGB output and the panel > and points to the panel-lvds driver for the panel. How do you make sure that you always bind against the correct driver? If it matches simple-panel and panel-lvds, it's not deterministically going to pick the right one. Well, it may actually be deterministic on Linux, but perhaps only by accident. > The reason that it is as it is, is that we obviously didn't understand > what we were doing when we added the entry, and this garbage was what > we came up with that produced a picture. > > If you want to keep the panel in simple-panel despite all this, the > timing constraints are as follows: > > Pixel clock 50-80 MHz, 65 MHz typical > Horizontal period 1094-1720 clocks, 1344 typical > 16.0-23.4 us 20.7 us > Horizontal enable 1024 clocks, always > Vertical period 776-990 lines, 806 typical > 13.3-18.0 ms 16.7 ms > Vertical enable 768 lines, always > > Using a "long" (the datasheet is not very specific on this issue) vertical > period may introduce deterioration of display quality, flicker etc. > > I don't think the division between front-porch/back-porch matters much. > > That said, I have no idea whatsoever if others have started using this > panel entry. My guess is that it has zero users, but who can tell? A quick grep shows that arch/arm/boot/dts/at91-nattis-2-natte-2.dts is the only device tree that uses this panel in the upstream kernel. Thierry
On 2020-03-03 15:20, Thierry Reding wrote: > On Mon, Mar 02, 2020 at 10:53:56PM +0000, Peter Rosin wrote: >> On 2020-03-02 21:34, Ville Syrjala wrote: >>> 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? >> >> TL/DR; I do not care if you change the refresh rate or the dotclock. >> >> The whole entry for that panel in simple-panel is dubious. The panel >> is really an LVDS panel (capable of both VESA/Jeida RGB888, selectable >> with the SELLVDS pin). With Jeida you can, as usual, omit the 4th >> data channel and use the panel with RGB666. In either case, you need >> an LVDS signal and nothing else... >> >> The panel can also rotate the picture 180 degrees using the RL/UD pin. >> >> These options are of course not expressed in the simple panel driver >> (and we have always used fixed signals for those pins in our designs, >> IIRC). As far as I'm concerned, the panel can be removed from >> simple-panel. Our device trees are nowadays correctly expressing the >> hardware with an LVDS encoder between the RGB output and the panel >> and points to the panel-lvds driver for the panel. > > How do you make sure that you always bind against the correct driver? If > it matches simple-panel and panel-lvds, it's not deterministically going > to pick the right one. Well, it may actually be deterministic on Linux, > but perhaps only by accident. You are probably right that it's fragile, but no problems so far. That said, I did wonder why the panel-lvds driver "wins" over simple-panel for compatible = "sharp,lq150x1lg11", "panel-lvds"; I figured it was by design and didn't spend too much time thinking about it. Maybe I should have? >> The reason that it is as it is, is that we obviously didn't understand >> what we were doing when we added the entry, and this garbage was what >> we came up with that produced a picture. >> >> If you want to keep the panel in simple-panel despite all this, the >> timing constraints are as follows: >> >> Pixel clock 50-80 MHz, 65 MHz typical >> Horizontal period 1094-1720 clocks, 1344 typical >> 16.0-23.4 us 20.7 us >> Horizontal enable 1024 clocks, always >> Vertical period 776-990 lines, 806 typical >> 13.3-18.0 ms 16.7 ms >> Vertical enable 768 lines, always >> >> Using a "long" (the datasheet is not very specific on this issue) vertical >> period may introduce deterioration of display quality, flicker etc. >> >> I don't think the division between front-porch/back-porch matters much. >> >> That said, I have no idea whatsoever if others have started using this >> panel entry. My guess is that it has zero users, but who can tell? > > A quick grep shows that arch/arm/boot/dts/at91-nattis-2-natte-2.dts is > the only device tree that uses this panel in the upstream kernel. This is our design, and what made us originally add the entry to simple panel, but as I said, we no longer need simple-panel support for it... Cheers, Peter
On Tue, Mar 03, 2020 at 02:57:45PM +0000, Peter Rosin wrote: > > On 2020-03-03 15:20, Thierry Reding wrote: > > On Mon, Mar 02, 2020 at 10:53:56PM +0000, Peter Rosin wrote: > >> On 2020-03-02 21:34, Ville Syrjala wrote: > >>> 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? > >> > >> TL/DR; I do not care if you change the refresh rate or the dotclock. > >> > >> The whole entry for that panel in simple-panel is dubious. The panel > >> is really an LVDS panel (capable of both VESA/Jeida RGB888, selectable > >> with the SELLVDS pin). With Jeida you can, as usual, omit the 4th > >> data channel and use the panel with RGB666. In either case, you need > >> an LVDS signal and nothing else... > >> > >> The panel can also rotate the picture 180 degrees using the RL/UD pin. > >> > >> These options are of course not expressed in the simple panel driver > >> (and we have always used fixed signals for those pins in our designs, > >> IIRC). As far as I'm concerned, the panel can be removed from > >> simple-panel. Our device trees are nowadays correctly expressing the > >> hardware with an LVDS encoder between the RGB output and the panel > >> and points to the panel-lvds driver for the panel. > > > > How do you make sure that you always bind against the correct driver? If > > it matches simple-panel and panel-lvds, it's not deterministically going > > to pick the right one. Well, it may actually be deterministic on Linux, > > but perhaps only by accident. > > You are probably right that it's fragile, but no problems so far. That > said, I did wonder why the panel-lvds driver "wins" over simple-panel for > > compatible = "sharp,lq150x1lg11", "panel-lvds"; > > I figured it was by design and didn't spend too much time thinking about > it. Maybe I should have? It's most likely because panel-lvds.o is linked into the kernel before panel-simple.o and the first match wins. You may want to move the entry to panel-lvds to make this a little more robust. Thierry
On 2020-03-03 16:05, Thierry Reding wrote: > On Tue, Mar 03, 2020 at 02:57:45PM +0000, Peter Rosin wrote: >> >> On 2020-03-03 15:20, Thierry Reding wrote: >>> On Mon, Mar 02, 2020 at 10:53:56PM +0000, Peter Rosin wrote: >>>> On 2020-03-02 21:34, Ville Syrjala wrote: >>>>> 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? >>>> >>>> TL/DR; I do not care if you change the refresh rate or the dotclock. >>>> >>>> The whole entry for that panel in simple-panel is dubious. The panel >>>> is really an LVDS panel (capable of both VESA/Jeida RGB888, selectable >>>> with the SELLVDS pin). With Jeida you can, as usual, omit the 4th >>>> data channel and use the panel with RGB666. In either case, you need >>>> an LVDS signal and nothing else... >>>> >>>> The panel can also rotate the picture 180 degrees using the RL/UD pin. >>>> >>>> These options are of course not expressed in the simple panel driver >>>> (and we have always used fixed signals for those pins in our designs, >>>> IIRC). As far as I'm concerned, the panel can be removed from >>>> simple-panel. Our device trees are nowadays correctly expressing the >>>> hardware with an LVDS encoder between the RGB output and the panel >>>> and points to the panel-lvds driver for the panel. >>> >>> How do you make sure that you always bind against the correct driver? If >>> it matches simple-panel and panel-lvds, it's not deterministically going >>> to pick the right one. Well, it may actually be deterministic on Linux, >>> but perhaps only by accident. >> >> You are probably right that it's fragile, but no problems so far. That >> said, I did wonder why the panel-lvds driver "wins" over simple-panel for >> >> compatible = "sharp,lq150x1lg11", "panel-lvds"; >> >> I figured it was by design and didn't spend too much time thinking about >> it. Maybe I should have? > > It's most likely because panel-lvds.o is linked into the kernel before > panel-simple.o and the first match wins. You may want to move the entry > to panel-lvds to make this a little more robust. Ok, or because I dropped the simple-panel driver when we no longer depended on it. Either way, what do you mean "move to [..] panel-lvds"? It has no list of panels, you have to end with "panel-lvds" in the compatible for the driver to bind. Cheers, Peter
Hi Peter. > >> > >> That said, I have no idea whatsoever if others have started using this > >> panel entry. My guess is that it has zero users, but who can tell? > > > > A quick grep shows that arch/arm/boot/dts/at91-nattis-2-natte-2.dts is > > the only device tree that uses this panel in the upstream kernel. > > This is our design, and what made us originally add the entry to simple > panel, but as I said, we no longer need simple-panel support for it... With the only upstream user using panel-lvds we should delete it from panel-simple. With all the features it does not belong in panel-simple anyway. Peter - care to send a patch for that? Sam
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 7526af2d6d95..cb587de8c49e 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -3026,7 +3026,7 @@ static const struct panel_desc sharp_lq123p1jx31 = { }; static const struct drm_display_mode sharp_lq150x1lg11_mode = { - .clock = 71100, + .clock = 65722, .hdisplay = 1024, .hsync_start = 1024 + 168, .hsync_end = 1024 + 168 + 64,