On Wed, Feb 26, 2020 at 01:08:06PM +0100, Linus Walleij wrote:> On Wed, Feb 26, 2020 at 12:57 PM Ville Syrj?l? > <ville.syrjala at linux.intel.com> wrote: > > On Tue, Feb 25, 2020 at 10:52:25PM +0100, Linus Walleij wrote: > > > > I have long suspected that a whole bunch of the "simple" displays > > > are not simple but contains a display controller and memory. > > > That means that the speed over the link to the display and > > > actual refresh rate on the actual display is asymmetric because > > > well we are just updating a RAM, the resolution just limits how > > > much data we are sending, the clock limits the speed on the > > > bus over to the RAM on the other side. > > > > IMO even in command mode mode->clock should probably be the actual > > dotclock used by the display. If there's another clock for the bus > > speed/etc. it should be stored somewhere else. > > Good point. For the DSI panels we have the field hs_rate > for the HS clock in struct mipi_dsi_device which is based > on exactly this reasoning. And that is what I actually use for > setting the HS clock. > > The problem is however that we in many cases have so > substandard documentation of these panels that we have > absolutely no idea about the dotclock. Maybe we should > just set it to 0 in these cases?Don't you always have a TE interrupt or something like that available? Could just measure it from that if no better information is available? -- Ville Syrj?l? Intel
On Wed, Feb 26, 2020 at 3:34 PM Ville Syrj?l? <ville.syrjala at linux.intel.com> wrote:> On Wed, Feb 26, 2020 at 01:08:06PM +0100, Linus Walleij wrote: > > On Wed, Feb 26, 2020 at 12:57 PM Ville Syrj?l? > > <ville.syrjala at linux.intel.com> wrote: > > > On Tue, Feb 25, 2020 at 10:52:25PM +0100, Linus Walleij wrote: > > > > > > I have long suspected that a whole bunch of the "simple" displays > > > > are not simple but contains a display controller and memory. > > > > That means that the speed over the link to the display and > > > > actual refresh rate on the actual display is asymmetric because > > > > well we are just updating a RAM, the resolution just limits how > > > > much data we are sending, the clock limits the speed on the > > > > bus over to the RAM on the other side. > > > > > > IMO even in command mode mode->clock should probably be the actual > > > dotclock used by the display. If there's another clock for the bus > > > speed/etc. it should be stored somewhere else. > > > > Good point. For the DSI panels we have the field hs_rate > > for the HS clock in struct mipi_dsi_device which is based > > on exactly this reasoning. And that is what I actually use for > > setting the HS clock. > > > > The problem is however that we in many cases have so > > substandard documentation of these panels that we have > > absolutely no idea about the dotclock. Maybe we should > > just set it to 0 in these cases? > > Don't you always have a TE interrupt or something like that > available? Could just measure it from that if no better > information is available?Yes and I did exactly that, so that is why this comment is in the driver: static const struct drm_display_mode sony_acx424akp_cmd_mode = { (...) /* * Some desired refresh rate, experiments at the maximum "pixel" * clock speed (HS clock 420 MHz) yields around 117Hz. */ .vrefresh = 60, I got a review comment at the time saying 117 Hz was weird. We didn't reach a proper conclusion on this: https://lore.kernel.org/dri-devel/CACRpkdYW3YNPSNMY3A44GQn8DqK-n9TLvr7uipF7LM_DHZ5=Lg at mail.gmail.com/ Thierry wasn't sure if 60Hz was good or not, so I just had to go with something. We could calculate the resulting pixel clock for ~117 Hz with this resolution and put that in the clock field but ... don't know what is the best? Yours, Linus Walleij
On Wed, Feb 26, 2020 at 03:56:36PM +0100, Linus Walleij wrote:> On Wed, Feb 26, 2020 at 3:34 PM Ville Syrj?l? > <ville.syrjala at linux.intel.com> wrote: > > On Wed, Feb 26, 2020 at 01:08:06PM +0100, Linus Walleij wrote: > > > On Wed, Feb 26, 2020 at 12:57 PM Ville Syrj?l? > > > <ville.syrjala at linux.intel.com> wrote: > > > > On Tue, Feb 25, 2020 at 10:52:25PM +0100, Linus Walleij wrote: > > > > > > > > I have long suspected that a whole bunch of the "simple" displays > > > > > are not simple but contains a display controller and memory. > > > > > That means that the speed over the link to the display and > > > > > actual refresh rate on the actual display is asymmetric because > > > > > well we are just updating a RAM, the resolution just limits how > > > > > much data we are sending, the clock limits the speed on the > > > > > bus over to the RAM on the other side. > > > > > > > > IMO even in command mode mode->clock should probably be the actual > > > > dotclock used by the display. If there's another clock for the bus > > > > speed/etc. it should be stored somewhere else. > > > > > > Good point. For the DSI panels we have the field hs_rate > > > for the HS clock in struct mipi_dsi_device which is based > > > on exactly this reasoning. And that is what I actually use for > > > setting the HS clock. > > > > > > The problem is however that we in many cases have so > > > substandard documentation of these panels that we have > > > absolutely no idea about the dotclock. Maybe we should > > > just set it to 0 in these cases? > > > > Don't you always have a TE interrupt or something like that > > available? Could just measure it from that if no better > > information is available? > > Yes and I did exactly that, so that is why this comment is in > the driver: > > static const struct drm_display_mode sony_acx424akp_cmd_mode = { > (...) > /* > * Some desired refresh rate, experiments at the maximum "pixel" > * clock speed (HS clock 420 MHz) yields around 117Hz. > */ > .vrefresh = 60, > > I got a review comment at the time saying 117 Hz was weird. > We didn't reach a proper conclusion on this: > https://lore.kernel.org/dri-devel/CACRpkdYW3YNPSNMY3A44GQn8DqK-n9TLvr7uipF7LM_DHZ5=Lg at mail.gmail.com/ > > Thierry wasn't sure if 60Hz was good or not, so I just had to > go with something. > > We could calculate the resulting pixel clock for ~117 Hz with > this resolution and put that in the clock field but ... don't know > what is the best?I would vote for that approach. -- Ville Syrj?l? Intel