Re: [PATCH v2 05/11] drm/tinydrm: Remove tinydrm_spi_max_transfer_size()

Re: [PATCH v2 05/11] drm/tinydrm: Remove tinydrm_spi_max_transfer_size()

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 05/11] drm/tinydrm: Remove tinydrm_spi_max_transfer_size()

[Daniel Vetter]

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


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 05/11] drm/tinydrm: Remove tinydrm_spi_max_transfer_size()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1039 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 05/11] drm/tinydrm: Remove tinydrm_spi_max_transfer_size()

[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
> <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.

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel