Subject: Re: [PATCH 04/12] drm: Nuke mode->vrefresh Message-ID: 20200226115708.GH13686@intel.com References: 20200219203544.31013-1-ville.syrjala@linux.intel.com CGME20200219203620eucas1p24b4990a91e758dbcf3e9b943669b0c8f@eucas1p2.samsung.com 20200219203544.31013-5-ville.syrjala@linux.intel.com 0f278771-79ce-fe23-e72c-3935dbe82d24@samsung.com 20200225112114.GA13686@intel.com 3ca785f2-9032-aaf9-0965-8657d31116ba@samsung.com 20200225154506.GF13686@intel.com 20200225192720.GG13686@intel.com CACRpkdZk9QEy+Kzkmy4BXiHB+aq9hprf=dmA_-R23yqH3NCt1g@mail.gmail.com MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: CACRpkdZk9QEy+Kzkmy4BXiHB+aq9hprf=dmA_-R23yqH3NCt1g@mail.gmail.com X-Patchwork-Hint: comment User-Agent: Mutt/1.10.1 (2018-07-13)
On Tue, Feb 25, 2020 at 10:52:25PM +0100, Linus Walleij wrote:
On Tue, Feb 25, 2020 at 8:27 PM Ville Syrjälä ville.syrjala@linux.intel.com wrote:
OK, so I went ahead a wrote a bit of cocci [1] to find the bad apples.
That's impressive :D
Unfortunately it found a lot of strange stuff:
I will answer for the weirdness I caused.
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.
On Wed, Feb 26, 2020 at 12:57 PM Ville Syrjälä ville.syrjala@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?
Yours, Linus Walleij
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@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?
On Wed, Feb 26, 2020 at 3:34 PM Ville Syrjälä ville.syrjala@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@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-n9TLvr7uipF7L...
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@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@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-n9TLvr7uipF7L...
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.
dri-devel@lists.freedesktop.org
-
Linus Walleij -
Ville Syrjälä