[v2,05/11] drm/tinydrm: Remove tinydrm_spi_max_transfer_size()
diff mbox series

Message ID 20190719155916.62465-6-noralf@tronnes.org
State New
Headers show
Series
  • drm/tinydrm: Remove tinydrm.ko
Related show

Commit Message

Noralf Trønnes July 19, 2019, 3:59 p.m. UTC
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.

Acked-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 .../gpu/drm/tinydrm/core/tinydrm-helpers.c    | 37 +------------------
 drivers/gpu/drm/tinydrm/mipi-dbi.c            | 10 +----
 include/drm/tinydrm/tinydrm-helpers.h         |  1 -
 3 files changed, 3 insertions(+), 45 deletions(-)

Comments

Andy Shevchenko Oct. 15, 2019, 2:32 p.m. UTC | #1
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.
Andy Shevchenko Oct. 15, 2019, 3:02 p.m. UTC | #2
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,
Noralf Trønnes Oct. 15, 2019, 3:41 p.m. UTC | #3
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.
>
Daniel Vetter Oct. 15, 2019, 3:57 p.m. UTC | #4
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;
 		}
 	}
Andy Shevchenko Oct. 15, 2019, 3:59 p.m. UTC | #5
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?
Daniel Vetter Oct. 15, 2019, 4:05 p.m. UTC | #6
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
Noralf Trønnes Oct. 15, 2019, 4:55 p.m. UTC | #7
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.
Andy Shevchenko Oct. 16, 2019, 8:07 a.m. UTC | #8
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.
Andy Shevchenko Oct. 16, 2019, 4:13 p.m. UTC | #9
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;
>  		}
>  	}
>
Daniel Vetter Oct. 16, 2019, 5:44 p.m. UTC | #10
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
>
>
Mark Brown Oct. 16, 2019, 6 p.m. UTC | #11
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.
Andy Shevchenko Oct. 17, 2019, 6:58 a.m. UTC | #12
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.

Patch
diff mbox series

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);