| Message ID | 20190719155916.62465-6-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tinydrm: Remove tinydrm.ko | expand |
On Fri, Jul 19, 2019 at 05:59:10PM +0200, Noralf Trønnes wrote: > spi-bcm2835 can handle >64kB buffers now so there is no need to check > ->max_dma_len. The tinydrm_spi_max_transfer_size() max_len argument is > not used by any callers, so not needed. > > Then we have the spi_max module parameter. It was added because > staging/fbtft has support for it and there was a report that someone used > it to set a small buffer size to avoid popping on a USB soundcard on a > Raspberry Pi. In hindsight it shouldn't have been added, I should have > waited for it to become a problem first. I don't know it anyone is > actually using it, but since tinydrm_spi_transfer() is being moved to > mipi-dbi, I'm taking the opportunity to remove it. I'll add it back to > mipi-dbi if someone complains. > > With that out of the way, spi_max_transfer_size() can be used instead. > > The chosen 16kB buffer size for Type C Option 1 (9-bit) interface is > somewhat arbitrary, but a bigger buffer will have a miniscule impact on > transfer speed, so it's probably fine. This breaks the SPI PXA2xx case I'm using. The world is not a Pi:e. [ 388.445752] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 [ 388.634437] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 [ 388.822933] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 The crucial thing is to check the transfer size against maximum DMA length of the master. Please, fix.
On Tue, Oct 15, 2019 at 05:32:36PM +0300, Andy Shevchenko wrote: > On Fri, Jul 19, 2019 at 05:59:10PM +0200, Noralf Trønnes wrote: > > spi-bcm2835 can handle >64kB buffers now so there is no need to check > > ->max_dma_len. The tinydrm_spi_max_transfer_size() max_len argument is > > not used by any callers, so not needed. > > > > Then we have the spi_max module parameter. It was added because > > staging/fbtft has support for it and there was a report that someone used > > it to set a small buffer size to avoid popping on a USB soundcard on a > > Raspberry Pi. In hindsight it shouldn't have been added, I should have > > waited for it to become a problem first. I don't know it anyone is > > actually using it, but since tinydrm_spi_transfer() is being moved to > > mipi-dbi, I'm taking the opportunity to remove it. I'll add it back to > > mipi-dbi if someone complains. > > > > With that out of the way, spi_max_transfer_size() can be used instead. > > > > The chosen 16kB buffer size for Type C Option 1 (9-bit) interface is > > somewhat arbitrary, but a bigger buffer will have a miniscule impact on > > transfer speed, so it's probably fine. > > This breaks the SPI PXA2xx case I'm using. The world is not a Pi:e. > > [ 388.445752] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > [ 388.634437] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > [ 388.822933] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > The crucial thing is to check the transfer size against maximum DMA length > of the master. > > Please, fix. Partial revert fixes the issue, though I'm not sure it's the best approach. --- a/drivers/gpu/drm/drm_mipi_dbi.c +++ b/drivers/gpu/drm/drm_mipi_dbi.c @@ -1147,7 +1147,7 @@ EXPORT_SYMBOL(mipi_dbi_spi_init); int mipi_dbi_spi_transfer(struct spi_device *spi, u32 speed_hz, u8 bpw, const void *buf, size_t len) { - size_t max_chunk = spi_max_transfer_size(spi); + size_t max_chunk = min(spi_max_transfer_size(spi), spi->master->max_dma_len); struct spi_transfer tr = { .bits_per_word = bpw, .speed_hz = speed_hz,
Den 15.10.2019 16.32, skrev Andy Shevchenko: > On Fri, Jul 19, 2019 at 05:59:10PM +0200, Noralf Trønnes wrote: >> spi-bcm2835 can handle >64kB buffers now so there is no need to check >> ->max_dma_len. The tinydrm_spi_max_transfer_size() max_len argument is >> not used by any callers, so not needed. >> >> Then we have the spi_max module parameter. It was added because >> staging/fbtft has support for it and there was a report that someone used >> it to set a small buffer size to avoid popping on a USB soundcard on a >> Raspberry Pi. In hindsight it shouldn't have been added, I should have >> waited for it to become a problem first. I don't know it anyone is >> actually using it, but since tinydrm_spi_transfer() is being moved to >> mipi-dbi, I'm taking the opportunity to remove it. I'll add it back to >> mipi-dbi if someone complains. >> >> With that out of the way, spi_max_transfer_size() can be used instead. >> >> The chosen 16kB buffer size for Type C Option 1 (9-bit) interface is >> somewhat arbitrary, but a bigger buffer will have a miniscule impact on >> transfer speed, so it's probably fine. > > This breaks the SPI PXA2xx case I'm using. The world is not a Pi:e. > > [ 388.445752] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > [ 388.634437] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > [ 388.822933] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > The crucial thing is to check the transfer size against maximum DMA length > of the master. > Isn't this a spi controller driver problem? spi_max_transfer_size() tells the client what the maximum transfer length is. The controller driver can use ctlr->max_transfer_size if it has restrictions. AFAIUI max_dma_len is used when splitting up the buffer for the sg table in spi_map_buf(). Noralf. > Please, fix. >
On Tue, Oct 15, 2019 at 05:41:53PM +0200, Noralf Trønnes wrote: > Den 15.10.2019 16.32, skrev Andy Shevchenko: > > On Fri, Jul 19, 2019 at 05:59:10PM +0200, Noralf Trønnes wrote: > >> spi-bcm2835 can handle >64kB buffers now so there is no need to check > >> ->max_dma_len. The tinydrm_spi_max_transfer_size() max_len argument is > >> not used by any callers, so not needed. > >> > >> Then we have the spi_max module parameter. It was added because > >> staging/fbtft has support for it and there was a report that someone used > >> it to set a small buffer size to avoid popping on a USB soundcard on a > >> Raspberry Pi. In hindsight it shouldn't have been added, I should have > >> waited for it to become a problem first. I don't know it anyone is > >> actually using it, but since tinydrm_spi_transfer() is being moved to > >> mipi-dbi, I'm taking the opportunity to remove it. I'll add it back to > >> mipi-dbi if someone complains. > >> > >> With that out of the way, spi_max_transfer_size() can be used instead. > >> > >> The chosen 16kB buffer size for Type C Option 1 (9-bit) interface is > >> somewhat arbitrary, but a bigger buffer will have a miniscule impact on > >> transfer speed, so it's probably fine. > > > > This breaks the SPI PXA2xx case I'm using. The world is not a Pi:e. > > > > [ 388.445752] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > [ 388.634437] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > [ 388.822933] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > > > The crucial thing is to check the transfer size against maximum DMA length > > of the master. > > > > Isn't this a spi controller driver problem? > spi_max_transfer_size() tells the client what the maximum transfer > length is. The controller driver can use ctlr->max_transfer_size if it > has restrictions. > AFAIUI max_dma_len is used when splitting up the buffer for the sg table > in spi_map_buf(). Something like this, as a test patch. Cheers, Daniel diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c index bb6a14d1ab0f..f77201915033 100644 --- a/drivers/spi/spi-pxa2xx.c +++ b/drivers/spi/spi-pxa2xx.c @@ -1707,6 +1707,7 @@ static int pxa2xx_spi_probe(struct platform_device *pdev) } else { controller->can_dma = pxa2xx_spi_can_dma; controller->max_dma_len = MAX_DMA_LEN; + controller->max_transfer_size = MAX_DMA_LEN; } }
On Tue, Oct 15, 2019 at 05:41:53PM +0200, Noralf Trønnes wrote: > Den 15.10.2019 16.32, skrev Andy Shevchenko: > > On Fri, Jul 19, 2019 at 05:59:10PM +0200, Noralf Trønnes wrote: > >> spi-bcm2835 can handle >64kB buffers now so there is no need to check > >> ->max_dma_len. The tinydrm_spi_max_transfer_size() max_len argument is > >> not used by any callers, so not needed. > >> > >> Then we have the spi_max module parameter. It was added because > >> staging/fbtft has support for it and there was a report that someone used > >> it to set a small buffer size to avoid popping on a USB soundcard on a > >> Raspberry Pi. In hindsight it shouldn't have been added, I should have > >> waited for it to become a problem first. I don't know it anyone is > >> actually using it, but since tinydrm_spi_transfer() is being moved to > >> mipi-dbi, I'm taking the opportunity to remove it. I'll add it back to > >> mipi-dbi if someone complains. > >> > >> With that out of the way, spi_max_transfer_size() can be used instead. > >> > >> The chosen 16kB buffer size for Type C Option 1 (9-bit) interface is > >> somewhat arbitrary, but a bigger buffer will have a miniscule impact on > >> transfer speed, so it's probably fine. > > > > This breaks the SPI PXA2xx case I'm using. The world is not a Pi:e. > > > > [ 388.445752] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > [ 388.634437] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > [ 388.822933] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > > > The crucial thing is to check the transfer size against maximum DMA length > > of the master. > > > > Isn't this a spi controller driver problem? It doesn't matter. This patch made a regression. Before it worked, now it doesn't. > spi_max_transfer_size() tells the client what the maximum transfer > length is. The controller driver can use ctlr->max_transfer_size if it > has restrictions. It might be a problem of the SPI core. > AFAIUI max_dma_len is used when splitting up the buffer for the sg table > in spi_map_buf(). > > Please, fix. Should I send the revert?
On Tue, Oct 15, 2019 at 5:59 PM Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > On Tue, Oct 15, 2019 at 05:41:53PM +0200, Noralf Trønnes wrote: > > Den 15.10.2019 16.32, skrev Andy Shevchenko: > > > On Fri, Jul 19, 2019 at 05:59:10PM +0200, Noralf Trønnes wrote: > > >> spi-bcm2835 can handle >64kB buffers now so there is no need to check > > >> ->max_dma_len. The tinydrm_spi_max_transfer_size() max_len argument is > > >> not used by any callers, so not needed. > > >> > > >> Then we have the spi_max module parameter. It was added because > > >> staging/fbtft has support for it and there was a report that someone used > > >> it to set a small buffer size to avoid popping on a USB soundcard on a > > >> Raspberry Pi. In hindsight it shouldn't have been added, I should have > > >> waited for it to become a problem first. I don't know it anyone is > > >> actually using it, but since tinydrm_spi_transfer() is being moved to > > >> mipi-dbi, I'm taking the opportunity to remove it. I'll add it back to > > >> mipi-dbi if someone complains. > > >> > > >> With that out of the way, spi_max_transfer_size() can be used instead. > > >> > > >> The chosen 16kB buffer size for Type C Option 1 (9-bit) interface is > > >> somewhat arbitrary, but a bigger buffer will have a miniscule impact on > > >> transfer speed, so it's probably fine. > > > > > > This breaks the SPI PXA2xx case I'm using. The world is not a Pi:e. > > > > > > [ 388.445752] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > > [ 388.634437] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > > [ 388.822933] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > > > > > The crucial thing is to check the transfer size against maximum DMA length > > > of the master. > > > > > > > Isn't this a spi controller driver problem? > > It doesn't matter. This patch made a regression. Before it worked, > now it doesn't. Yes this is clear. > > spi_max_transfer_size() tells the client what the maximum transfer > > length is. The controller driver can use ctlr->max_transfer_size if it > > has restrictions. > > It might be a problem of the SPI core. > > > AFAIUI max_dma_len is used when splitting up the buffer for the sg table > > in spi_map_buf(). > > > > Please, fix. > > Should I send the revert? Can we please not roll in with the cavalry before everyone had a chance to wake up and look at this properly? It's less than 1h since your initial bug report. Also, you _do_ have a test patch in your inbox ... Seriously. -Daniel
Den 15.10.2019 17.59, skrev Andy Shevchenko: > On Tue, Oct 15, 2019 at 05:41:53PM +0200, Noralf Trønnes wrote: >> Den 15.10.2019 16.32, skrev Andy Shevchenko: >>> On Fri, Jul 19, 2019 at 05:59:10PM +0200, Noralf Trønnes wrote: >>>> spi-bcm2835 can handle >64kB buffers now so there is no need to check >>>> ->max_dma_len. The tinydrm_spi_max_transfer_size() max_len argument is >>>> not used by any callers, so not needed. >>>> >>>> Then we have the spi_max module parameter. It was added because >>>> staging/fbtft has support for it and there was a report that someone used >>>> it to set a small buffer size to avoid popping on a USB soundcard on a >>>> Raspberry Pi. In hindsight it shouldn't have been added, I should have >>>> waited for it to become a problem first. I don't know it anyone is >>>> actually using it, but since tinydrm_spi_transfer() is being moved to >>>> mipi-dbi, I'm taking the opportunity to remove it. I'll add it back to >>>> mipi-dbi if someone complains. >>>> >>>> With that out of the way, spi_max_transfer_size() can be used instead. >>>> >>>> The chosen 16kB buffer size for Type C Option 1 (9-bit) interface is >>>> somewhat arbitrary, but a bigger buffer will have a miniscule impact on >>>> transfer speed, so it's probably fine. >>> >>> This breaks the SPI PXA2xx case I'm using. The world is not a Pi:e. >>> >>> [ 388.445752] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 >>> [ 388.634437] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 >>> [ 388.822933] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 >>> >>> The crucial thing is to check the transfer size against maximum DMA length >>> of the master. >>> >> >> Isn't this a spi controller driver problem? > > It doesn't matter. This patch made a regression. Before it worked, > now it doesn't. > >> spi_max_transfer_size() tells the client what the maximum transfer >> length is. The controller driver can use ctlr->max_transfer_size if it >> has restrictions. > > It might be a problem of the SPI core. > >> AFAIUI max_dma_len is used when splitting up the buffer for the sg table >> in spi_map_buf(). > >>> Please, fix. > > Should I send the revert? > Please do. I still believe that this papers over a controller driver bug/shortcoming, but there could be more drivers that have the same problem so it's better to revert to the old behaviour. I guess the problem is that not many controller drivers are tested with buffers bigger than max_dma_len (64kB in this case, same as for the Pi). If I have the revert by tomorrow, then I can apply it before Maxime sends the -rc PR to Dave on Thursday. Noralf.
On Tue, Oct 15, 2019 at 06:05:16PM +0200, Daniel Vetter wrote: > On Tue, Oct 15, 2019 at 5:59 PM Andy Shevchenko > <andriy.shevchenko@intel.com> wrote: > > On Tue, Oct 15, 2019 at 05:41:53PM +0200, Noralf Trønnes wrote: > > > Den 15.10.2019 16.32, skrev Andy Shevchenko: > > > > On Fri, Jul 19, 2019 at 05:59:10PM +0200, Noralf Trønnes wrote: > > > >> spi-bcm2835 can handle >64kB buffers now so there is no need to check > > > >> ->max_dma_len. The tinydrm_spi_max_transfer_size() max_len argument is > > > >> not used by any callers, so not needed. > > > >> > > > >> Then we have the spi_max module parameter. It was added because > > > >> staging/fbtft has support for it and there was a report that someone used > > > >> it to set a small buffer size to avoid popping on a USB soundcard on a > > > >> Raspberry Pi. In hindsight it shouldn't have been added, I should have > > > >> waited for it to become a problem first. I don't know it anyone is > > > >> actually using it, but since tinydrm_spi_transfer() is being moved to > > > >> mipi-dbi, I'm taking the opportunity to remove it. I'll add it back to > > > >> mipi-dbi if someone complains. > > > >> > > > >> With that out of the way, spi_max_transfer_size() can be used instead. > > > >> > > > >> The chosen 16kB buffer size for Type C Option 1 (9-bit) interface is > > > >> somewhat arbitrary, but a bigger buffer will have a miniscule impact on > > > >> transfer speed, so it's probably fine. > > > > > > > > This breaks the SPI PXA2xx case I'm using. The world is not a Pi:e. > > > > > > > > [ 388.445752] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > > > [ 388.634437] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > > > [ 388.822933] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > > > > > > > The crucial thing is to check the transfer size against maximum DMA length > > > > of the master. > > > > > > > > > > Isn't this a spi controller driver problem? > > > > It doesn't matter. This patch made a regression. Before it worked, > > now it doesn't. > > Yes this is clear. > > > > spi_max_transfer_size() tells the client what the maximum transfer > > > length is. The controller driver can use ctlr->max_transfer_size if it > > > has restrictions. > > > > It might be a problem of the SPI core. > > > > > AFAIUI max_dma_len is used when splitting up the buffer for the sg table > > > in spi_map_buf(). > > > > > > Please, fix. > > > > Should I send the revert? > > Can we please not roll in with the cavalry before everyone had a > chance to wake up and look at this properly? It's less than 1h since > your initial bug report. It was a simple question to the 1st level maintainer of the subsystem, who already read the report and answered to it. > Also, you _do_ have a test patch in your inbox ... Yes, thanks for it, I'm going to test right now.
On Tue, Oct 15, 2019 at 05:57:20PM +0200, Daniel Vetter wrote: > On Tue, Oct 15, 2019 at 05:41:53PM +0200, Noralf Trønnes wrote: > > Den 15.10.2019 16.32, skrev Andy Shevchenko: > > > On Fri, Jul 19, 2019 at 05:59:10PM +0200, Noralf Trønnes wrote: > > >> spi-bcm2835 can handle >64kB buffers now so there is no need to check > > >> ->max_dma_len. The tinydrm_spi_max_transfer_size() max_len argument is > > >> not used by any callers, so not needed. > > >> > > >> Then we have the spi_max module parameter. It was added because > > >> staging/fbtft has support for it and there was a report that someone used > > >> it to set a small buffer size to avoid popping on a USB soundcard on a > > >> Raspberry Pi. In hindsight it shouldn't have been added, I should have > > >> waited for it to become a problem first. I don't know it anyone is > > >> actually using it, but since tinydrm_spi_transfer() is being moved to > > >> mipi-dbi, I'm taking the opportunity to remove it. I'll add it back to > > >> mipi-dbi if someone complains. > > >> > > >> With that out of the way, spi_max_transfer_size() can be used instead. > > >> > > >> The chosen 16kB buffer size for Type C Option 1 (9-bit) interface is > > >> somewhat arbitrary, but a bigger buffer will have a miniscule impact on > > >> transfer speed, so it's probably fine. > > > > > > This breaks the SPI PXA2xx case I'm using. The world is not a Pi:e. > > > > > > [ 388.445752] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > > [ 388.634437] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > > [ 388.822933] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > > > > > The crucial thing is to check the transfer size against maximum DMA length > > > of the master. > > > > > > > Isn't this a spi controller driver problem? > > spi_max_transfer_size() tells the client what the maximum transfer > > length is. The controller driver can use ctlr->max_transfer_size if it > > has restrictions. > > AFAIUI max_dma_len is used when splitting up the buffer for the sg table > > in spi_map_buf(). > > Something like this, as a test patch. max_transfer_size should be a function. In that case it works. However I'm not sure it's the best approach, thus, Cc to SPI PXA people. > diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c > index bb6a14d1ab0f..f77201915033 100644 > --- a/drivers/spi/spi-pxa2xx.c > +++ b/drivers/spi/spi-pxa2xx.c > @@ -1707,6 +1707,7 @@ static int pxa2xx_spi_probe(struct platform_device *pdev) > } else { > controller->can_dma = pxa2xx_spi_can_dma; > controller->max_dma_len = MAX_DMA_LEN; > + controller->max_transfer_size = MAX_DMA_LEN; > } > } >
On Wed, Oct 16, 2019 at 6:13 PM Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > On Tue, Oct 15, 2019 at 05:57:20PM +0200, Daniel Vetter wrote: > > On Tue, Oct 15, 2019 at 05:41:53PM +0200, Noralf Trønnes wrote: > > > Den 15.10.2019 16.32, skrev Andy Shevchenko: > > > > On Fri, Jul 19, 2019 at 05:59:10PM +0200, Noralf Trønnes wrote: > > > >> spi-bcm2835 can handle >64kB buffers now so there is no need to check > > > >> ->max_dma_len. The tinydrm_spi_max_transfer_size() max_len argument is > > > >> not used by any callers, so not needed. > > > >> > > > >> Then we have the spi_max module parameter. It was added because > > > >> staging/fbtft has support for it and there was a report that someone used > > > >> it to set a small buffer size to avoid popping on a USB soundcard on a > > > >> Raspberry Pi. In hindsight it shouldn't have been added, I should have > > > >> waited for it to become a problem first. I don't know it anyone is > > > >> actually using it, but since tinydrm_spi_transfer() is being moved to > > > >> mipi-dbi, I'm taking the opportunity to remove it. I'll add it back to > > > >> mipi-dbi if someone complains. > > > >> > > > >> With that out of the way, spi_max_transfer_size() can be used instead. > > > >> > > > >> The chosen 16kB buffer size for Type C Option 1 (9-bit) interface is > > > >> somewhat arbitrary, but a bigger buffer will have a miniscule impact on > > > >> transfer speed, so it's probably fine. > > > > > > > > This breaks the SPI PXA2xx case I'm using. The world is not a Pi:e. > > > > > > > > [ 388.445752] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > > > [ 388.634437] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > > > [ 388.822933] mi0283qt spi-PRP0001:01: DMA disabled for transfer length 153600 greater than 65536 > > > > > > > > The crucial thing is to check the transfer size against maximum DMA length > > > > of the master. > > > > > > > > > > Isn't this a spi controller driver problem? > > > spi_max_transfer_size() tells the client what the maximum transfer > > > length is. The controller driver can use ctlr->max_transfer_size if it > > > has restrictions. > > > AFAIUI max_dma_len is used when splitting up the buffer for the sg table > > > in spi_map_buf(). > > > > Something like this, as a test patch. > > max_transfer_size should be a function. In that case it works. Why do you want to make it a function? At least from my reading of the code, the dma vs pio decision seems to be done once. So no need to change this at runtime. Changing at runtime would also be a pretty big surprise I think for users of spi. > However I'm not sure it's the best approach, thus, Cc to SPI PXA people. Hm didn't spot the pxa people, added them. Mark, should I just go ahead and bake this into a proper patch for discussion? Or fundamentally wrong approach? -Daniel > > diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c > > index bb6a14d1ab0f..f77201915033 100644 > > --- a/drivers/spi/spi-pxa2xx.c > > +++ b/drivers/spi/spi-pxa2xx.c > > @@ -1707,6 +1707,7 @@ static int pxa2xx_spi_probe(struct platform_device *pdev) > > } else { > > controller->can_dma = pxa2xx_spi_can_dma; > > controller->max_dma_len = MAX_DMA_LEN; > > + controller->max_transfer_size = MAX_DMA_LEN; > > } > > } > > > > -- > With Best Regards, > Andy Shevchenko > >
On Wed, Oct 16, 2019 at 07:44:51PM +0200, Daniel Vetter wrote: > On Wed, Oct 16, 2019 at 6:13 PM Andy Shevchenko > > On Tue, Oct 15, 2019 at 05:57:20PM +0200, Daniel Vetter wrote: > > > Something like this, as a test patch. > > max_transfer_size should be a function. In that case it works. > Why do you want to make it a function? At least from my reading of the > code, the dma vs pio decision seems to be done once. So no need to > change this at runtime. Changing at runtime would also be a pretty big > surprise I think for users of spi. Yeah, I'd expect it to be a fixed property of the hardware that doesn't vary at runtime though I'm sure there must be some innovation out there which challenges that assumption. > > However I'm not sure it's the best approach, thus, Cc to SPI PXA people. > Hm didn't spot the pxa people, added them. Mark, should I just go > ahead and bake this into a proper patch for discussion? Or > fundamentally wrong approach? That seems sensible enough, it should certainly fix the immediate issue.
On Wed, Oct 16, 2019 at 07:44:51PM +0200, Daniel Vetter wrote: > On Wed, Oct 16, 2019 at 6:13 PM Andy Shevchenko > <andriy.shevchenko@intel.com> wrote: > > On Tue, Oct 15, 2019 at 05:57:20PM +0200, Daniel Vetter wrote: > > max_transfer_size should be a function. In that case it works. > > Why do you want to make it a function? Me?! commit 4acad4aae10d1fa79a075b38b5c73772c44f576c Author: Michal Suchanek <hramrach@gmail.com> Date: Wed Dec 2 10:38:21 2015 +0000 spi: expose master transfer size limitation.
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c index 272616a246cd..af5bec8861de 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c @@ -18,41 +18,8 @@ #include <drm/drm_rect.h> #include <drm/tinydrm/tinydrm-helpers.h> -static unsigned int spi_max; -module_param(spi_max, uint, 0400); -MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size"); - #if IS_ENABLED(CONFIG_SPI) -/** - * tinydrm_spi_max_transfer_size - Determine max SPI transfer size - * @spi: SPI device - * @max_len: Maximum buffer size needed (optional) - * - * This function returns the maximum size to use for SPI transfers. It checks - * the SPI master, the optional @max_len and the module parameter spi_max and - * returns the smallest. - * - * Returns: - * Maximum size for SPI transfers - */ -size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len) -{ - size_t ret; - - ret = min(spi_max_transfer_size(spi), spi->master->max_dma_len); - if (max_len) - ret = min(ret, max_len); - if (spi_max) - ret = min_t(size_t, ret, spi_max); - ret &= ~0x3; - if (ret < 4) - ret = 4; - - return ret; -} -EXPORT_SYMBOL(tinydrm_spi_max_transfer_size); - /** * tinydrm_spi_transfer - SPI transfer helper * @spi: SPI device @@ -75,21 +42,19 @@ int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz, struct spi_transfer *header, u8 bpw, const void *buf, size_t len) { + size_t max_chunk = spi_max_transfer_size(spi); struct spi_transfer tr = { .bits_per_word = bpw, .speed_hz = speed_hz, }; struct spi_message m; u16 *swap_buf = NULL; - size_t max_chunk; size_t chunk; int ret = 0; if (WARN_ON_ONCE(bpw != 8 && bpw != 16)) return -EINVAL; - max_chunk = tinydrm_spi_max_transfer_size(spi, 0); - if (bpw == 16 && !spi_is_bpw_supported(spi, 16)) { tr.bits_per_word = 8; if (tinydrm_machine_little_endian()) { diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index 99509d16b037..ae31a5c9aa1b 100644 --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c @@ -964,15 +964,9 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 *cmd, int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi, struct gpio_desc *dc) { - size_t tx_size = tinydrm_spi_max_transfer_size(spi, 0); struct device *dev = &spi->dev; int ret; - if (tx_size < 16) { - DRM_ERROR("SPI transmit buffer too small: %zu\n", tx_size); - return -EINVAL; - } - /* * Even though it's not the SPI device that does DMA (the master does), * the dma mask is necessary for the dma_alloc_wc() in @@ -1001,8 +995,8 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi, mipi->swap_bytes = true; } else { mipi->command = mipi_dbi_typec1_command; - mipi->tx_buf9_len = tx_size; - mipi->tx_buf9 = devm_kmalloc(dev, tx_size, GFP_KERNEL); + mipi->tx_buf9_len = SZ_16K; + mipi->tx_buf9 = devm_kmalloc(dev, mipi->tx_buf9_len, GFP_KERNEL); if (!mipi->tx_buf9) return -ENOMEM; } diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h index dca75de3a359..10b35375a009 100644 --- a/include/drm/tinydrm/tinydrm-helpers.h +++ b/include/drm/tinydrm/tinydrm-helpers.h @@ -41,7 +41,6 @@ int tinydrm_display_pipe_init(struct drm_device *drm, const struct drm_display_mode *mode, unsigned int rotation); -size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len); int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz, struct spi_transfer *header, u8 bpw, const void *buf, size_t len);