| Message ID | 20220313141008.1503638-1-trix@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/repaper: combine allocs in repaper_spi_transfer() | expand |
Den 13.03.2022 15.10, skrev trix@redhat.com: > From: Tom Rix <trix@redhat.com> > > repaper_spi_transfer() allocates a single byte > for the spi header and then another buffer for > the payload. Combine the allocs into a single > buffer with offsets. To simplify the offsets > put the header after the payload. > > Signed-off-by: Tom Rix <trix@redhat.com> > --- > drivers/gpu/drm/tiny/repaper.c | 40 ++++++++++------------------------ > 1 file changed, 12 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c > index 37b6bb90e46e1..22a6732f35a01 100644 > --- a/drivers/gpu/drm/tiny/repaper.c > +++ b/drivers/gpu/drm/tiny/repaper.c > @@ -100,50 +100,34 @@ static inline struct repaper_epd *drm_to_epd(struct drm_device *drm) > static int repaper_spi_transfer(struct spi_device *spi, u8 header, > const void *tx, void *rx, size_t len) > { > - void *txbuf = NULL, *rxbuf = NULL; > struct spi_transfer tr[2] = {}; > - u8 *headerbuf; > + u8 *buf; > int ret; > > - headerbuf = kmalloc(1, GFP_KERNEL); > - if (!headerbuf) > + buf = kmalloc(1 + len, GFP_KERNEL); > + if (!buf) > return -ENOMEM; > > - headerbuf[0] = header; > - tr[0].tx_buf = headerbuf; > + buf[len] = header; > + tr[0].tx_buf = &buf[len]; I don't think this will work since the buffer is used directly for DMA on some platforms[1] so the buffers need to be at the correct alignment for that to work. For this reason I think it's better to leave this as-is since we know the kmalloc buffers will always be useable for DMA and the code is also easy to read and understand instead of calculating offsets. [1] tinydrm/mipi-dbi: Use dma-safe buffers for all SPI transfers (a89bfc5d9a07) Noralf. > tr[0].len = 1; > > - /* Stack allocated tx? */ > - if (tx && len <= 32) { > - txbuf = kmemdup(tx, len, GFP_KERNEL); > - if (!txbuf) { > - ret = -ENOMEM; > - goto out_free; > - } > + if (tx) { > + memcpy(buf, tx, len); > + tr[1].tx_buf = buf; > } > > - if (rx) { > - rxbuf = kmalloc(len, GFP_KERNEL); > - if (!rxbuf) { > - ret = -ENOMEM; > - goto out_free; > - } > - } > + if (rx) > + tr[1].rx_buf = buf; > > - tr[1].tx_buf = txbuf ? txbuf : tx; > - tr[1].rx_buf = rxbuf; > tr[1].len = len; > > ndelay(80); > ret = spi_sync_transfer(spi, tr, 2); > if (rx && !ret) > - memcpy(rx, rxbuf, len); > - > -out_free: > - kfree(headerbuf); > - kfree(txbuf); > - kfree(rxbuf); > + memcpy(rx, buf, len); > > + kfree(buf); > return ret; > } >
On 3/13/22 8:41 AM, Noralf Trønnes wrote: > > Den 13.03.2022 15.10, skrev trix@redhat.com: >> From: Tom Rix <trix@redhat.com> >> >> repaper_spi_transfer() allocates a single byte >> for the spi header and then another buffer for >> the payload. Combine the allocs into a single >> buffer with offsets. To simplify the offsets >> put the header after the payload. >> >> Signed-off-by: Tom Rix <trix@redhat.com> >> --- >> drivers/gpu/drm/tiny/repaper.c | 40 ++++++++++------------------------ >> 1 file changed, 12 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c >> index 37b6bb90e46e1..22a6732f35a01 100644 >> --- a/drivers/gpu/drm/tiny/repaper.c >> +++ b/drivers/gpu/drm/tiny/repaper.c >> @@ -100,50 +100,34 @@ static inline struct repaper_epd *drm_to_epd(struct drm_device *drm) >> static int repaper_spi_transfer(struct spi_device *spi, u8 header, >> const void *tx, void *rx, size_t len) >> { >> - void *txbuf = NULL, *rxbuf = NULL; >> struct spi_transfer tr[2] = {}; >> - u8 *headerbuf; >> + u8 *buf; >> int ret; >> >> - headerbuf = kmalloc(1, GFP_KERNEL); >> - if (!headerbuf) >> + buf = kmalloc(1 + len, GFP_KERNEL); >> + if (!buf) >> return -ENOMEM; >> >> - headerbuf[0] = header; >> - tr[0].tx_buf = headerbuf; >> + buf[len] = header; >> + tr[0].tx_buf = &buf[len]; > I don't think this will work since the buffer is used directly for DMA > on some platforms[1] so the buffers need to be at the correct alignment > for that to work. For this reason I think it's better to leave this > as-is since we know the kmalloc buffers will always be useable for DMA > and the code is also easy to read and understand instead of calculating > offsets. > > [1] tinydrm/mipi-dbi: Use dma-safe buffers for all SPI transfers > (a89bfc5d9a07) > > Noralf. > >> tr[0].len = 1; >> >> - /* Stack allocated tx? */ >> - if (tx && len <= 32) { How about a change to remove this ? It seems like you are getting lucky. reduce the to single txrx_buf Tom >> - txbuf = kmemdup(tx, len, GFP_KERNEL); >> - if (!txbuf) { >> - ret = -ENOMEM; >> - goto out_free; >> - } >> + if (tx) { >> + memcpy(buf, tx, len); >> + tr[1].tx_buf = buf; >> } >> >> - if (rx) { >> - rxbuf = kmalloc(len, GFP_KERNEL); >> - if (!rxbuf) { >> - ret = -ENOMEM; >> - goto out_free; >> - } >> - } >> + if (rx) >> + tr[1].rx_buf = buf; >> >> - tr[1].tx_buf = txbuf ? txbuf : tx; >> - tr[1].rx_buf = rxbuf; >> tr[1].len = len; >> >> ndelay(80); >> ret = spi_sync_transfer(spi, tr, 2); >> if (rx && !ret) >> - memcpy(rx, rxbuf, len); >> - >> -out_free: >> - kfree(headerbuf); >> - kfree(txbuf); >> - kfree(rxbuf); >> + memcpy(rx, buf, len); >> >> + kfree(buf); >> return ret; >> } >>
Den 13.03.2022 17.07, skrev Tom Rix: > > On 3/13/22 8:41 AM, Noralf Trønnes wrote: >> >> Den 13.03.2022 15.10, skrev trix@redhat.com: >>> From: Tom Rix <trix@redhat.com> >>> >>> repaper_spi_transfer() allocates a single byte >>> for the spi header and then another buffer for >>> the payload. Combine the allocs into a single >>> buffer with offsets. To simplify the offsets >>> put the header after the payload. >>> >>> Signed-off-by: Tom Rix <trix@redhat.com> >>> --- >>> drivers/gpu/drm/tiny/repaper.c | 40 ++++++++++------------------------ >>> 1 file changed, 12 insertions(+), 28 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/tiny/repaper.c >>> b/drivers/gpu/drm/tiny/repaper.c >>> index 37b6bb90e46e1..22a6732f35a01 100644 >>> --- a/drivers/gpu/drm/tiny/repaper.c >>> +++ b/drivers/gpu/drm/tiny/repaper.c >>> @@ -100,50 +100,34 @@ static inline struct repaper_epd >>> *drm_to_epd(struct drm_device *drm) >>> static int repaper_spi_transfer(struct spi_device *spi, u8 header, >>> const void *tx, void *rx, size_t len) >>> { >>> - void *txbuf = NULL, *rxbuf = NULL; >>> struct spi_transfer tr[2] = {}; >>> - u8 *headerbuf; >>> + u8 *buf; >>> int ret; >>> - headerbuf = kmalloc(1, GFP_KERNEL); >>> - if (!headerbuf) >>> + buf = kmalloc(1 + len, GFP_KERNEL); >>> + if (!buf) >>> return -ENOMEM; >>> - headerbuf[0] = header; >>> - tr[0].tx_buf = headerbuf; >>> + buf[len] = header; >>> + tr[0].tx_buf = &buf[len]; >> I don't think this will work since the buffer is used directly for DMA >> on some platforms[1] so the buffers need to be at the correct alignment >> for that to work. For this reason I think it's better to leave this >> as-is since we know the kmalloc buffers will always be useable for DMA >> and the code is also easy to read and understand instead of calculating >> offsets. >> >> [1] tinydrm/mipi-dbi: Use dma-safe buffers for all SPI transfers >> (a89bfc5d9a07) >> >> Noralf. >> >>> tr[0].len = 1; >>> - /* Stack allocated tx? */ >>> - if (tx && len <= 32) { > > How about a change to remove this ? > > It seems like you are getting lucky. > Lucky how? If tx len is 32 bytes or less it's config data which lives on the stack so it's put in a kmalloc buffer, if it's more it's pixel data which already comes in a kmalloc buffer. Ideally we should have had a spi_sync_transfer_safe() that would check if the buffer comes from the stack and allocates buffers for us so the driver didn't have to do this. Alternatively the spi core could have done this, maybe spi_map_buf() could have done it since it already has special handling for vmalloc buffers. > reduce the to single txrx_buf > I see that repaper_spi_transfer() is written so tx and rx can be both set, but the driver never does tx and rx in the same transfer so it's possible to use one txrx buffer. I'm not entirely convinced that it will be more readable. Noralf. > Tom > >>> - txbuf = kmemdup(tx, len, GFP_KERNEL); >>> - if (!txbuf) { >>> - ret = -ENOMEM; >>> - goto out_free; >>> - } >>> + if (tx) { >>> + memcpy(buf, tx, len); >>> + tr[1].tx_buf = buf; >>> } >>> - if (rx) { >>> - rxbuf = kmalloc(len, GFP_KERNEL); >>> - if (!rxbuf) { >>> - ret = -ENOMEM; >>> - goto out_free; >>> - } >>> - } >>> + if (rx) >>> + tr[1].rx_buf = buf; >>> - tr[1].tx_buf = txbuf ? txbuf : tx; >>> - tr[1].rx_buf = rxbuf; >>> tr[1].len = len; >>> ndelay(80); >>> ret = spi_sync_transfer(spi, tr, 2); >>> if (rx && !ret) >>> - memcpy(rx, rxbuf, len); >>> - >>> -out_free: >>> - kfree(headerbuf); >>> - kfree(txbuf); >>> - kfree(rxbuf); >>> + memcpy(rx, buf, len); >>> + kfree(buf); >>> return ret; >>> } >>> >
diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c index 37b6bb90e46e1..22a6732f35a01 100644 --- a/drivers/gpu/drm/tiny/repaper.c +++ b/drivers/gpu/drm/tiny/repaper.c @@ -100,50 +100,34 @@ static inline struct repaper_epd *drm_to_epd(struct drm_device *drm) static int repaper_spi_transfer(struct spi_device *spi, u8 header, const void *tx, void *rx, size_t len) { - void *txbuf = NULL, *rxbuf = NULL; struct spi_transfer tr[2] = {}; - u8 *headerbuf; + u8 *buf; int ret; - headerbuf = kmalloc(1, GFP_KERNEL); - if (!headerbuf) + buf = kmalloc(1 + len, GFP_KERNEL); + if (!buf) return -ENOMEM; - headerbuf[0] = header; - tr[0].tx_buf = headerbuf; + buf[len] = header; + tr[0].tx_buf = &buf[len]; tr[0].len = 1; - /* Stack allocated tx? */ - if (tx && len <= 32) { - txbuf = kmemdup(tx, len, GFP_KERNEL); - if (!txbuf) { - ret = -ENOMEM; - goto out_free; - } + if (tx) { + memcpy(buf, tx, len); + tr[1].tx_buf = buf; } - if (rx) { - rxbuf = kmalloc(len, GFP_KERNEL); - if (!rxbuf) { - ret = -ENOMEM; - goto out_free; - } - } + if (rx) + tr[1].rx_buf = buf; - tr[1].tx_buf = txbuf ? txbuf : tx; - tr[1].rx_buf = rxbuf; tr[1].len = len; ndelay(80); ret = spi_sync_transfer(spi, tr, 2); if (rx && !ret) - memcpy(rx, rxbuf, len); - -out_free: - kfree(headerbuf); - kfree(txbuf); - kfree(rxbuf); + memcpy(rx, buf, len); + kfree(buf); return ret; }