| Message ID | 20161102123959.6098-1-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Peter, Thank you for the patch. On Wednesday 02 Nov 2016 14:39:59 Peter Ujfalusi wrote: > With the new dma_request_chan() the client driver does not need to look for > the DMA resource and it does not need to pass filter_fn anymore. > By switching to the new API the driver can now support deferred probing > against DMA. I believe this breaks the OMAP3 ISP driver. dma_request_slave_channel_compat() is a superset of dma_request_chan() that will, when called with omap_dma_filter_fn, return as a fallback any free channel handled by the OMAP DMA engine driver. This feature is actively used by this driver and must be preserved. > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > CC: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > --- > Hi, > > the original patch was sent 29.04.2016: > https://patchwork.kernel.org/patch/8981811/ > > I have rebased it on top of linux-next. > > Regards, > Peter > > drivers/media/platform/omap3isp/isphist.c | 27 +++++++++------------------ > 1 file changed, 9 insertions(+), 18 deletions(-) > > diff --git a/drivers/media/platform/omap3isp/isphist.c > b/drivers/media/platform/omap3isp/isphist.c index > 7138b043a4aa..e163e3d92517 100644 > --- a/drivers/media/platform/omap3isp/isphist.c > +++ b/drivers/media/platform/omap3isp/isphist.c > @@ -18,7 +18,6 @@ > #include <linux/delay.h> > #include <linux/device.h> > #include <linux/dmaengine.h> > -#include <linux/omap-dmaengine.h> > #include <linux/slab.h> > #include <linux/uaccess.h> > > @@ -486,27 +485,19 @@ int omap3isp_hist_init(struct isp_device *isp) > hist->isp = isp; > > if (HIST_CONFIG_DMA) { > - struct platform_device *pdev = to_platform_device(isp->dev); > - struct resource *res; > - unsigned int sig = 0; > - dma_cap_mask_t mask; > - > - dma_cap_zero(mask); > - dma_cap_set(DMA_SLAVE, mask); > - > - res = platform_get_resource_byname(pdev, IORESOURCE_DMA, > - "hist"); > - if (res) > - sig = res->start; > - > - hist->dma_ch = dma_request_slave_channel_compat(mask, > - omap_dma_filter_fn, &sig, isp->dev, "hist"); > - if (!hist->dma_ch) > + hist->dma_ch = dma_request_chan(isp->dev, "hist"); > + if (IS_ERR(hist->dma_ch)) { > + ret = PTR_ERR(hist->dma_ch); > + if (ret == -EPROBE_DEFER) > + return ret; > + > + hist->dma_ch = NULL; > dev_warn(isp->dev, > "hist: DMA channel request failed, using PIO\n"); > - else > + } else { > dev_dbg(isp->dev, "hist: using DMA channel %s\n", > dma_chan_name(hist->dma_ch)); > + } > } > > hist->ops = &hist_ops;
On 11/02/2016 11:19 PM, Laurent Pinchart wrote: > Hi Peter, > > Thank you for the patch. > > On Wednesday 02 Nov 2016 14:39:59 Peter Ujfalusi wrote: >> With the new dma_request_chan() the client driver does not need to look for >> the DMA resource and it does not need to pass filter_fn anymore. >> By switching to the new API the driver can now support deferred probing >> against DMA. > > I believe this breaks the OMAP3 ISP driver. dma_request_slave_channel_compat() > is a superset of dma_request_chan() that will, when called with > omap_dma_filter_fn, return as a fallback any free channel handled by the OMAP > DMA engine driver. This feature is actively used by this driver and must be > preserved. The fallback to use the filter_fn is used only when booted in legacy mode or when requesting a channel for non slave DMA operation. Based on the code in the driver it is handling slave transfers, so it must have DMA request line coming from somewhere. If that is missing the driver should not be able to work as it will not start the transfer. dma_request_chan() is to be used when you want to have slave channel with DMA request. If legacy mode needs to be supported then adding the hist DMA request number to the omap3xxx_sdma_map in arch/arm/mach-omap2/dma.c should be done. The reason the omap3isp is not in the table is that I could not find any place where the DMA resource was set (nor the DMA is specified in DT). I'm unsure how this driver could work w/o DMA request line over a random (and SW triggered) DMA channel with slave operation. For the slave DMA you need to have DMA request line.
On 11/03/2016 11:23 AM, Peter Ujfalusi wrote: > > On 11/02/2016 11:19 PM, Laurent Pinchart wrote: >> > Hi Peter, >> > >> > Thank you for the patch. >> > >> > On Wednesday 02 Nov 2016 14:39:59 Peter Ujfalusi wrote: >>> >> With the new dma_request_chan() the client driver does not need to look for >>> >> the DMA resource and it does not need to pass filter_fn anymore. >>> >> By switching to the new API the driver can now support deferred probing >>> >> against DMA. >> > >> > I believe this breaks the OMAP3 ISP driver. dma_request_slave_channel_compat() >> > is a superset of dma_request_chan() that will, when called with >> > omap_dma_filter_fn, return as a fallback any free channel handled by the OMAP >> > DMA engine driver. This feature is actively used by this driver and must be >> > preserved. > The fallback to use the filter_fn is used only when booted in legacy > mode or when requesting a channel for non slave DMA operation. > Based on the code in the driver it is handling slave transfers, so it > must have DMA request line coming from somewhere. If that is missing the > driver should not be able to work as it will not start the transfer. > > dma_request_chan() is to be used when you want to have slave channel > with DMA request. > > If legacy mode needs to be supported then adding the hist DMA request > number to the omap3xxx_sdma_map in arch/arm/mach-omap2/dma.c should be done. > The reason the omap3isp is not in the table is that I could not find any > place where the DMA resource was set (nor the DMA is specified in DT). > > I'm unsure how this driver could work w/o DMA request line over a random > (and SW triggered) DMA channel with slave operation. For the slave DMA > you need to have DMA request line. I see now. The omap3isp never supposed to get valid DMA request line via platform_get_resource_byname(), it should never find the "hist" DMA in the DT data. If it does, the driver would stop working as there is no DMA request associated with the histogram data reading. It is a bit misleading that it used dma_request_slave_channel_compat() for getting the channel. I think what would be correct is: dma_cap_mask_t mask; dma_cap_zero(mask); dma_cap_set(DMA_SLAVE, mask); hist->dma_ch = dma_request_chan_by_mask(&mask); We will get any DMA channel capable of slave configuration, but we will configure no DMA request number for the channel. and document this in the driver...
Hi Peter, On Thursday 03 Nov 2016 11:23:50 Peter Ujfalusi wrote: > On 11/02/2016 11:19 PM, Laurent Pinchart wrote: > > On Wednesday 02 Nov 2016 14:39:59 Peter Ujfalusi wrote: > >> With the new dma_request_chan() the client driver does not need to look > >> for the DMA resource and it does not need to pass filter_fn anymore. > >> By switching to the new API the driver can now support deferred probing > >> against DMA. > > > > I believe this breaks the OMAP3 ISP driver. > > dma_request_slave_channel_compat() is a superset of dma_request_chan() > > that will, when called with omap_dma_filter_fn, return as a fallback any > > free channel handled by the OMAP DMA engine driver. This feature is > > actively used by this driver and must be preserved. > > The fallback to use the filter_fn is used only when booted in legacy > mode or when requesting a channel for non slave DMA operation. > Based on the code in the driver it is handling slave transfers, so it > must have DMA request line coming from somewhere. If that is missing the > driver should not be able to work as it will not start the transfer. If I remember correctly, the OMAP3 ISP has no dedicated DMA channel is it doesn't issue any hardware trigger. It can use any DMA channel, and performs a memcpy-like operation from a FIFO exposed through a single register to a system memory location. > dma_request_chan() is to be used when you want to have slave channel > with DMA request. > > If legacy mode needs to be supported then adding the hist DMA request > number to the omap3xxx_sdma_map in arch/arm/mach-omap2/dma.c should be done. > The reason the omap3isp is not in the table is that I could not find any > place where the DMA resource was set (nor the DMA is specified in DT). That's because there's no DMA request number for the OMAP3 ISP :-) > I'm unsure how this driver could work w/o DMA request line over a random > (and SW triggered) DMA channel with slave operation. For the slave DMA > you need to have DMA request line. As far as I understand, the DMA channel is software triggered, and the DMA engine will just perform the programmed number of transfers, without waiting for a hardware trigger.
Hi Peter, On Thursday 03 Nov 2016 15:14:19 Peter Ujfalusi wrote: > On 11/03/2016 11:23 AM, Peter Ujfalusi wrote: > > On 11/02/2016 11:19 PM, Laurent Pinchart wrote: > >>> On Wednesday 02 Nov 2016 14:39:59 Peter Ujfalusi wrote: > >>>>> With the new dma_request_chan() the client driver does not need to > >>>>> look for the DMA resource and it does not need to pass filter_fn > >>>>> anymore. By switching to the new API the driver can now support > >>>>> deferred probing against DMA. > >>> > >>> I believe this breaks the OMAP3 ISP driver. > >>> dma_request_slave_channel_compat() is a superset of dma_request_chan() > >>> that will, when called with > >>> omap_dma_filter_fn, return as a fallback any free channel handled by > >>> the OMAP DMA engine driver. This feature is actively used by this > >>> driver and must be preserved. > > > > The fallback to use the filter_fn is used only when booted in legacy > > mode or when requesting a channel for non slave DMA operation. > > Based on the code in the driver it is handling slave transfers, so it > > must have DMA request line coming from somewhere. If that is missing the > > driver should not be able to work as it will not start the transfer. > > > > dma_request_chan() is to be used when you want to have slave channel > > with DMA request. > > > > If legacy mode needs to be supported then adding the hist DMA request > > number to the omap3xxx_sdma_map in arch/arm/mach-omap2/dma.c should be > > done. The reason the omap3isp is not in the table is that I could not > > find any place where the DMA resource was set (nor the DMA is specified > > in DT). > > > > I'm unsure how this driver could work w/o DMA request line over a random > > (and SW triggered) DMA channel with slave operation. For the slave DMA > > you need to have DMA request line. > > I see now. > The omap3isp never supposed to get valid DMA request line via > platform_get_resource_byname(), it should never find the "hist" DMA in > the DT data. If it does, the driver would stop working as there is no > DMA request associated with the histogram data reading. That's correct. > It is a bit misleading that it used dma_request_slave_channel_compat() > for getting the channel. > > I think what would be correct is: > dma_cap_mask_t mask; > > dma_cap_zero(mask); > dma_cap_set(DMA_SLAVE, mask); > hist->dma_ch = dma_request_chan_by_mask(&mask); > > We will get any DMA channel capable of slave configuration, but we will > configure no DMA request number for the channel. I believe that should work. It could in theory result in a different behaviour as it could return a DMA channel not handled by the OMAP SDMA engine, but I don't think that would be an issue. > and document this in the driver...
Hi Laurent, On 11/03/2016 05:12 PM, Laurent Pinchart wrote: >> It is a bit misleading that it used dma_request_slave_channel_compat() >> for getting the channel. >> >> I think what would be correct is: >> dma_cap_mask_t mask; >> >> dma_cap_zero(mask); >> dma_cap_set(DMA_SLAVE, mask); >> hist->dma_ch = dma_request_chan_by_mask(&mask); >> >> We will get any DMA channel capable of slave configuration, but we will >> configure no DMA request number for the channel. > > I believe that should work. It could in theory result in a different behaviour > as it could return a DMA channel not handled by the OMAP SDMA engine, but I > don't think that would be an issue. Yes, that could be the case if we would have more than one DMAs in SoCs where the omap3isp is used, but we only have sDMA. The reason why I would like to move the driver to use the generic API is that my plan is to remove the legacy sDMA support in the future so the filter_fn is not going to be available outside of the DMAengine driver. I do believe that this is safe to do in this way and if the IP shows up somewhere else where we have more than one DMAs - which is unlikely - I'm sure it can be fixed up, but w/o device it is hard to guess what needs to be done. FWIW: if omap3isp shows up where we have sDMA and eDMA we can set the mask as DMA_SLAVE | DMA_MEMCPY as eDMA does not set both for a channel - it is either slave or memcpy. >> and document this in the driver... >
diff --git a/drivers/media/platform/omap3isp/isphist.c b/drivers/media/platform/omap3isp/isphist.c index 7138b043a4aa..e163e3d92517 100644 --- a/drivers/media/platform/omap3isp/isphist.c +++ b/drivers/media/platform/omap3isp/isphist.c @@ -18,7 +18,6 @@ #include <linux/delay.h> #include <linux/device.h> #include <linux/dmaengine.h> -#include <linux/omap-dmaengine.h> #include <linux/slab.h> #include <linux/uaccess.h> @@ -486,27 +485,19 @@ int omap3isp_hist_init(struct isp_device *isp) hist->isp = isp; if (HIST_CONFIG_DMA) { - struct platform_device *pdev = to_platform_device(isp->dev); - struct resource *res; - unsigned int sig = 0; - dma_cap_mask_t mask; - - dma_cap_zero(mask); - dma_cap_set(DMA_SLAVE, mask); - - res = platform_get_resource_byname(pdev, IORESOURCE_DMA, - "hist"); - if (res) - sig = res->start; - - hist->dma_ch = dma_request_slave_channel_compat(mask, - omap_dma_filter_fn, &sig, isp->dev, "hist"); - if (!hist->dma_ch) + hist->dma_ch = dma_request_chan(isp->dev, "hist"); + if (IS_ERR(hist->dma_ch)) { + ret = PTR_ERR(hist->dma_ch); + if (ret == -EPROBE_DEFER) + return ret; + + hist->dma_ch = NULL; dev_warn(isp->dev, "hist: DMA channel request failed, using PIO\n"); - else + } else { dev_dbg(isp->dev, "hist: using DMA channel %s\n", dma_chan_name(hist->dma_ch)); + } } hist->ops = &hist_ops;
With the new dma_request_chan() the client driver does not need to look for the DMA resource and it does not need to pass filter_fn anymore. By switching to the new API the driver can now support deferred probing against DMA. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com> CC: Mauro Carvalho Chehab <mchehab@osg.samsung.com> --- Hi, the original patch was sent 29.04.2016: https://patchwork.kernel.org/patch/8981811/ I have rebased it on top of linux-next. Regards, Peter drivers/media/platform/omap3isp/isphist.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-)