| Message ID | 20200505160329.2976059-1-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/dsi: use stack buffer in mipi_dsi_dcs_write() | expand |
On Tue, 5 May 2020 at 17:05, Emil Velikov <emil.l.velikov@gmail.com> wrote: > > 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; > -- 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: > 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) { I think it would be clearer to do: if (size > ARRAY_SIZE(stack_tx)) but either way: Reviewed-by: Thierry Reding <treding@nvidia.com>
On Thu, May 28, 2020 at 05:47:41PM +0100, Emil Velikov wrote: > On Tue, 5 May 2020 at 17:05, Emil Velikov <emil.l.velikov@gmail.com> wrote: > > > > 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; > > -- > > Thierry, others - humble ping. > Can you check through the series? I don't see patch 2 of this series anywhere? Did it fall victim to some filter? Thierry
On Tue, May 05, 2020 at 05:03:27PM +0100, Emil Velikov wrote: > 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> Applied to drm-misc-next. Sorry for taking so long. Sam > --- > 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; > -- > 2.25.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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;
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(-)