Currently the function heap allocates when we have any payload. Where in many case the payload is 1 byte - ouch.
From casual observation, vast majority of the payloads are smaller than
8 bytes - so use a stack array tx[8] to avoid the senseless kmalloc and kfree dance.
Cc: Jani Nikula jani.nikula@intel.com Cc: Thierry Reding treding@nvidia.com Signed-off-by: Emil Velikov emil.velikov@collabora.com --- drivers/gpu/drm/drm_mipi_dsi.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index 55531895dde6..b96d5b4629d7 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -748,26 +748,26 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd, { ssize_t err; size_t size; + u8 stack_tx[8]; u8 *tx;
- if (len > 0) { - size = 1 + len; - + size = 1 + len; + if (len > ARRAY_SIZE(stack_tx) - 1) { tx = kmalloc(size, GFP_KERNEL); if (!tx) return -ENOMEM; - - /* concatenate the DCS command byte and the payload */ - tx[0] = cmd; - memcpy(&tx[1], data, len); } else { - tx = &cmd; - size = 1; + tx = stack_tx; }
+ /* concatenate the DCS command byte and the payload */ + tx[0] = cmd; + if (data) + memcpy(&tx[1], data, len); + err = mipi_dsi_dcs_write_buffer(dsi, tx, size);
- if (len > 0) + if (tx != stack_tx) kfree(tx);
return err;
From: Emil Velikov emil.velikov@collabora.com
A few of the new panels create a local macro wrapping around mipi_dsi_dcs_write. At the same time, they don't really care about the command/payload split.
mipi_dsi_dcs_write does a kmalloc/memcpy/kfree for payload > 7 bytes. kmalloc - avoid that all together by using the _buffer function.
Aside: panel-xinpeng-xpp055c272.c calls its wrapper "generic" although it should be "dcs". But that for another day/patch.
Cc: Heiko Stuebner heiko@sntech.de Cc: Heiko Stuebner heiko.stuebner@theobroma-systems.com Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Signed-off-by: Emil Velikov emil.velikov@collabora.com --- drivers/gpu/drm/panel/panel-elida-kd35t133.c | 4 ++-- drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c | 4 ++-- drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c b/drivers/gpu/drm/panel/panel-elida-kd35t133.c index 711ded453c44..00e3d67af812 100644 --- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c +++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c @@ -52,9 +52,9 @@ static inline struct kd35t133 *panel_to_kd35t133(struct drm_panel *panel) }
#define dsi_dcs_write_seq(dsi, cmd, seq...) do { \ - static const u8 d[] = { seq }; \ + static const u8 b[] = { cmd, seq }; \ int ret; \ - ret = mipi_dsi_dcs_write(dsi, cmd, d, ARRAY_SIZE(d)); \ + ret = mipi_dsi_dcs_write_buffer(dsi, b, ARRAY_SIZE(b)); \ if (ret < 0) \ return ret; \ } while (0) diff --git a/drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c b/drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c index 5a7a31c8513e..eaa9da3ebbea 100644 --- a/drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c +++ b/drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c @@ -246,9 +246,9 @@ struct ltk050h3146w *panel_to_ltk050h3146w(struct drm_panel *panel) }
#define dsi_dcs_write_seq(dsi, cmd, seq...) do { \ - static const u8 d[] = { seq }; \ + static const u8 b[] = { cmd, seq }; \ int ret; \ - ret = mipi_dsi_dcs_write(dsi, cmd, d, ARRAY_SIZE(d)); \ + ret = mipi_dsi_dcs_write_buffer(dsi, b, ARRAY_SIZE(b)); \ if (ret < 0) \ return ret; \ } while (0) diff --git a/drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c b/drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c index 1645aceab597..9e07d7e86807 100644 --- a/drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c +++ b/drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c @@ -62,9 +62,9 @@ static inline struct xpp055c272 *panel_to_xpp055c272(struct drm_panel *panel) }
#define dsi_generic_write_seq(dsi, cmd, seq...) do { \ - static const u8 d[] = { seq }; \ + static const u8 b[] = { cmd, seq }; \ int ret; \ - ret = mipi_dsi_dcs_write(dsi, cmd, d, ARRAY_SIZE(d)); \ + ret = mipi_dsi_dcs_write_buffer(dsi, b, ARRAY_SIZE(b)); \ if (ret < 0) \ return ret; \ } while (0)
On Tue, 5 May 2020 at 17:05, Emil Velikov emil.l.velikov@gmail.com wrote:
Seems like I've left an extra word here - will fix in v2, alongside any review feedback. s/kmalloc - avoid/Avoid/
Humble poke?
-Emil
From: Emil Velikov emil.velikov@collabora.com
The helper uses the MIPI_DCS_SET_TEAR_SCANLINE, although it's currently using the generic write. This does not look right.
Perhaps some platforms don't distinguish between the two writers?
Cc: Robert Chiras robert.chiras@nxp.com Cc: Vinay Simha BN simhavcs@gmail.com Cc: Jani Nikula jani.nikula@intel.com Cc: Thierry Reding treding@nvidia.com Fixes: e83950816367 ("drm/dsi: Implement set tear scanline") Signed-off-by: Emil Velikov emil.velikov@collabora.com --- Robert, can you please test this against the only user - the Raydium RM67191 panel driver that you introduced.
Thanks
Vinay, can you confirm if this is a genuine typo or there's something really subtle happening. --- drivers/gpu/drm/drm_mipi_dsi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index b96d5b4629d7..07102d8da58f 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -1082,11 +1082,11 @@ EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format); */ int mipi_dsi_dcs_set_tear_scanline(struct mipi_dsi_device *dsi, u16 scanline) { - u8 payload[3] = { MIPI_DCS_SET_TEAR_SCANLINE, scanline >> 8, - scanline & 0xff }; + u8 payload[2] = { scanline >> 8, scanline & 0xff }; ssize_t err;
- err = mipi_dsi_generic_write(dsi, payload, sizeof(payload)); + err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_SCANLINE, payload, + sizeof(payload)); if (err < 0) return err;
Emil,
Reply inline
On Tue, 5 May 2020 at 9:35 PM, Emil Velikov emil.l.velikov@gmail.com wrote:
this has been tested on nexus 7 with jdi panel. I did not understand what is the typo here? We need to use DC’s write instead of generic write?
regards, vinaysimha
On Thu, 7 May 2020 at 13:29, Vinay Simha B N simhavcs@gmail.com wrote:
The jdi panel (JDI LT070ME05000 I believe) does not use the function, hmm.
Looking through the ML archive - the call in the first 4 revisions of the patch. Then with v5 it has magically disappeared alongside mipi_dsi_dcs_set_tear_on().
No comment explaining why though - does the driver work w/o both of those?
I did not understand what is the typo here? We need to use DC’s write instead of generic write?
I believe the clue is in the command name - MIPI_DSI_DCS. I was going to double-check with the spec although it's members only :-\ Based on the usage in DRM, all DCS commands are issued via mipi_dsi_dcs_{read,write}
Thanks Emil
Hi Vinay,
On Thu, 7 May 2020 at 17:18, Emil Velikov emil.l.velikov@gmail.com wrote:
Any ideas, does the driver work in today's state?
Do you agree with the rationale? Alternatively, do you have a reference to the Android tree where the generic write is used.
Thanks Emil
hi emil,
On Wed, May 13, 2020 at 3:17 PM Emil Velikov emil.l.velikov@gmail.com wrote:
Initially I had used cmd mode, later modified to video mode panel, since to control the panel in cmd mode is not available for mdp4. so mipi_dsi_dcs_set_tear_on was not used.
default android nexus 7 kernel https://github.com/vinaysimhabn/msm/commits/nexus7-msm-flo-3.4-lollipop-rele...
Thanks Emil
-- regards, vinaysimha
On Tue, May 05, 2020 at 05:03:29PM +0100, Emil Velikov wrote:
I don't think platforms usually care about this level of detail. The Tegra driver for example doesn't really look at the packet type and just packets the data in the standard DSI format and then sends it off on the bus. It does inspect some fields of the packet, but none that are related to the packet type, I think.
So it's possible that the panel will accept the packet irrespective of type and handle it correctly. I can imagine that the decoding logic in these panels might be rather primitive, so perhaps it's not very strict as to what exactly the type is as long as it can do something with the data.
In any case, it does make sense to send DCS commands using the DCS type, so I'd say let's merge this and see if somebody complains:
Reviewed-by: Thierry Reding treding@nvidia.com
On Tue, 5 May 2020 at 17:05, Emil Velikov emil.l.velikov@gmail.com wrote:
Thierry, others - humble ping. Can you check through the series?
Thanks Emil
On Tue, May 05, 2020 at 05:03:27PM +0100, Emil Velikov wrote:
I think it would be clearer to do:
if (size > ARRAY_SIZE(stack_tx))
but either way:
Reviewed-by: Thierry Reding treding@nvidia.com
dri-devel@lists.freedesktop.org
-
Emil Velikov -
Sam Ravnborg -
Thierry Reding -
Vinay Simha B N