| Message ID | 20211115085403.360194-9-arnd@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dmaengine: kill off dma_slave_config->slave_id | expand |
Hi Arnd, Thank you for the patch. On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The display driver wants to pass a custom flag to the DMA engine driver, > which it started doing by using the slave_id field that was traditionally > used for a different purpose. > > As there is no longer a correct use for the slave_id field, it should > really be removed, and the remaining users changed over to something > different. > > The new mechanism for passing nonstandard settings is using the > .peripheral_config field, so use that to pass a newly defined structure > here, making it clear that this will not work in portable drivers. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/dma/xilinx/xilinx_dpdma.c | 12 ++++++++---- > drivers/gpu/drm/xlnx/zynqmp_disp.c | 9 +++++++-- > include/linux/dma/xilinx_dpdma.h | 11 +++++++++++ > 3 files changed, 26 insertions(+), 6 deletions(-) > create mode 100644 include/linux/dma/xilinx_dpdma.h > > diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c > index ce5c66e6897d..e2c1ef7a659c 100644 > --- a/drivers/dma/xilinx/xilinx_dpdma.c > +++ b/drivers/dma/xilinx/xilinx_dpdma.c > @@ -12,6 +12,7 @@ > #include <linux/clk.h> > #include <linux/debugfs.h> > #include <linux/delay.h> > +#include <linux/dma/xilinx_dpdma.h> > #include <linux/dmaengine.h> > #include <linux/dmapool.h> > #include <linux/interrupt.h> > @@ -1273,6 +1274,7 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, > struct dma_slave_config *config) > { > struct xilinx_dpdma_chan *chan = to_xilinx_chan(dchan); > + struct xilinx_dpdma_peripheral_config *pconfig; > unsigned long flags; > > /* > @@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, > spin_lock_irqsave(&chan->lock, flags); > > /* > - * Abuse the slave_id to indicate that the channel is part of a video > - * group. > + * Abuse the peripheral_config to indicate that the channel is part Is it still an abuse, or is this now the right way to pass custom data to the DMA engine driver ? > + * of a video group. > */ > - if (chan->id <= ZYNQMP_DPDMA_VIDEO2) > - chan->video_group = config->slave_id != 0; > + pconfig = config->peripheral_config; This could be moved to the variable declaration above, up to you. > + if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && > + config->peripheral_size == sizeof(*pconfig)) Silently ignoring a size mismatch isn't nice. Could we validate the size at the beginning of the function and return an error ? With these issues addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + chan->video_group = pconfig->video_group; > > spin_unlock_irqrestore(&chan->lock, flags); > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c > index ff2b308d8651..11c409cbc88e 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c > @@ -24,6 +24,7 @@ > > #include <linux/clk.h> > #include <linux/delay.h> > +#include <linux/dma/xilinx_dpdma.h> > #include <linux/dma-mapping.h> > #include <linux/dmaengine.h> > #include <linux/module.h> > @@ -1058,14 +1059,18 @@ static void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer, > zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt); > > /* > - * Set slave_id for each DMA channel to indicate they're part of a > + * Set pconfig for each DMA channel to indicate they're part of a > * video group. > */ > for (i = 0; i < info->num_planes; i++) { > struct zynqmp_disp_layer_dma *dma = &layer->dmas[i]; > + struct xilinx_dpdma_peripheral_config pconfig = { > + .video_group = true, > + }; > struct dma_slave_config config = { > .direction = DMA_MEM_TO_DEV, > - .slave_id = 1, > + .peripheral_config = &pconfig, > + .peripheral_size = sizeof(pconfig), > }; > > dmaengine_slave_config(dma->chan, &config); > diff --git a/include/linux/dma/xilinx_dpdma.h b/include/linux/dma/xilinx_dpdma.h > new file mode 100644 > index 000000000000..83a1377f03f8 > --- /dev/null > +++ b/include/linux/dma/xilinx_dpdma.h > @@ -0,0 +1,11 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#ifndef __LINUX_DMA_XILINX_DPDMA_H > +#define __LINUX_DMA_XILINX_DPDMA_H > + > +#include <linux/types.h> > + > +struct xilinx_dpdma_peripheral_config { > + bool video_group; > +}; > + > +#endif /* __LINUX_DMA_XILINX_DPDMA_H */
On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote: > > @@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, > > spin_lock_irqsave(&chan->lock, flags); > > > > /* > > - * Abuse the slave_id to indicate that the channel is part of a video > > - * group. > > + * Abuse the peripheral_config to indicate that the channel is part > > Is it still an abuse, or is this now the right way to pass custom data > to the DMA engine driver ? It doesn't make the driver any more portable, but it's now being more explicit about it. As far as I can tell, this is the best way to pass data that cannot be expressed through the regular interfaces in DT and the dmaengine API. Ideally there would be a generic way to pass this flag, but I couldn't figure out what this is actually doing, or whether there is a better way. Maybe Vinod has an idea. I'll change s/Abuse/Use/ for the moment until I get a definite answer. > > + * of a video group. > > */ > > - if (chan->id <= ZYNQMP_DPDMA_VIDEO2) > > - chan->video_group = config->slave_id != 0; > > + pconfig = config->peripheral_config; > > This could be moved to the variable declaration above, up to you. I considered that but since it doesn't fit in a normal 80-column line, it seemed best to do it here. > > + if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && > > + config->peripheral_size == sizeof(*pconfig)) > > Silently ignoring a size mismatch isn't nice. Could we validate the size > at the beginning of the function and return an error ? Yes, good idea. Since this would mean a bug in another driver, I'll add a WARN_ON() as well to make it clear which driver caused it. This is what I have now, let me know if you have any further suggestions: /* * Use the peripheral_config to indicate that the channel is part * of a video group. This requires matching use of the custom * structure in each driver. */ pconfig = config->peripheral_config; if (WARN_ON(config->peripheral_size != 0 && config->peripheral_size != sizeof(*pconfig))) return -EINVAL; spin_lock_irqsave(&chan->lock, flags); if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && config->peripheral_size == sizeof(*pconfig)) chan->video_group = pconfig->video_group; spin_unlock_irqrestore(&chan->lock, flags); return 0; > With these issues addressed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks, Arnd
Hi Arnd, On Mon, Nov 15, 2021 at 11:21:30AM +0100, Arnd Bergmann wrote: > On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart wrote: > > On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote: > > > @@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, > > > spin_lock_irqsave(&chan->lock, flags); > > > > > > /* > > > - * Abuse the slave_id to indicate that the channel is part of a video > > > - * group. > > > + * Abuse the peripheral_config to indicate that the channel is part > > > > Is it still an abuse, or is this now the right way to pass custom data > > to the DMA engine driver ? > > It doesn't make the driver any more portable, but it's now being > more explicit about it. As far as I can tell, this is the best way > to pass data that cannot be expressed through the regular interfaces > in DT and the dmaengine API. > > Ideally there would be a generic way to pass this flag, but I couldn't > figure out what this is actually doing, or whether there is a better > way. Maybe Vinod has an idea. I don't think we need a generic API in this case. The DMA engine is specific to the display device, I don't foresee a need to mix-n-match. > I'll change s/Abuse/Use/ for the moment until I get a definite answer. > > > > + * of a video group. > > > */ > > > - if (chan->id <= ZYNQMP_DPDMA_VIDEO2) > > > - chan->video_group = config->slave_id != 0; > > > + pconfig = config->peripheral_config; > > > > This could be moved to the variable declaration above, up to you. > > I considered that but since it doesn't fit in a normal 80-column > line, it seemed best to do it here. > > > > + if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && > > > + config->peripheral_size == sizeof(*pconfig)) > > > > Silently ignoring a size mismatch isn't nice. Could we validate the size > > at the beginning of the function and return an error ? > > Yes, good idea. Since this would mean a bug in another driver, > I'll add a WARN_ON() as well to make it clear which driver caused it. > This is what I have now, let me know if you have any further suggestions: > > /* > * Use the peripheral_config to indicate that the channel is part > * of a video group. This requires matching use of the custom > * structure in each driver. > */ > pconfig = config->peripheral_config; > if (WARN_ON(config->peripheral_size != 0 && > config->peripheral_size != sizeof(*pconfig))) > return -EINVAL; How about if (WARN_ON(config->peripheral_config && config->peripheral_size != sizeof(*pconfig))) > > spin_lock_irqsave(&chan->lock, flags); > if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && > config->peripheral_size == sizeof(*pconfig)) And here you can test pconfig != NULL. > chan->video_group = pconfig->video_group; > spin_unlock_irqrestore(&chan->lock, flags); > > return 0; > > > With these issues addressed, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
On Mon, Nov 15, 2021 at 12:49 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Mon, Nov 15, 2021 at 11:21:30AM +0100, Arnd Bergmann wrote: > > On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart wrote: > > > On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote: > > > > @@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, > > > > spin_lock_irqsave(&chan->lock, flags); > > > > > > > > /* > > > > - * Abuse the slave_id to indicate that the channel is part of a video > > > > - * group. > > > > + * Abuse the peripheral_config to indicate that the channel is part > > > > > > Is it still an abuse, or is this now the right way to pass custom data > > > to the DMA engine driver ? > > > > It doesn't make the driver any more portable, but it's now being > > more explicit about it. As far as I can tell, this is the best way > > to pass data that cannot be expressed through the regular interfaces > > in DT and the dmaengine API. > > > > Ideally there would be a generic way to pass this flag, but I couldn't > > figure out what this is actually doing, or whether there is a better > > way. Maybe Vinod has an idea. > > I don't think we need a generic API in this case. The DMA engine is > specific to the display device, I don't foresee a need to mix-n-match. Right. I wonder if there is even a point in using the dmaengine API in that case, I think for other single-purpose drivers we tend to just integrate the functionality in the client driver. No point changing this now of course, but it does feel odd. From my earlier reading of the driver, my impression was that this is just a memory-to-memory device, so it could be used that way as well, but does need a flag when working on the video memory. I couldn't quite make sense of that though. > > /* > > * Use the peripheral_config to indicate that the channel is part > > * of a video group. This requires matching use of the custom > > * structure in each driver. > > */ > > pconfig = config->peripheral_config; > > if (WARN_ON(config->peripheral_size != 0 && > > config->peripheral_size != sizeof(*pconfig))) > > return -EINVAL; > > How about > > if (WARN_ON(config->peripheral_config && > config->peripheral_size != sizeof(*pconfig))) > > > > > spin_lock_irqsave(&chan->lock, flags); > > if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && > > config->peripheral_size == sizeof(*pconfig)) > > And here you can test pconfig != NULL. Good idea. Changed now, using 'if (pconfig)' without the '!= NULL' in both expressions. Arnd
Hi Arnd, On Mon, Nov 15, 2021 at 01:38:07PM +0100, Arnd Bergmann wrote: > On Mon, Nov 15, 2021 at 12:49 PM Laurent Pinchart wrote: > > On Mon, Nov 15, 2021 at 11:21:30AM +0100, Arnd Bergmann wrote: > > > On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart wrote: > > > > On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote: > > > > > @@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, > > > > > spin_lock_irqsave(&chan->lock, flags); > > > > > > > > > > /* > > > > > - * Abuse the slave_id to indicate that the channel is part of a video > > > > > - * group. > > > > > + * Abuse the peripheral_config to indicate that the channel is part > > > > > > > > Is it still an abuse, or is this now the right way to pass custom data > > > > to the DMA engine driver ? > > > > > > It doesn't make the driver any more portable, but it's now being > > > more explicit about it. As far as I can tell, this is the best way > > > to pass data that cannot be expressed through the regular interfaces > > > in DT and the dmaengine API. > > > > > > Ideally there would be a generic way to pass this flag, but I couldn't > > > figure out what this is actually doing, or whether there is a better > > > way. Maybe Vinod has an idea. > > > > I don't think we need a generic API in this case. The DMA engine is > > specific to the display device, I don't foresee a need to mix-n-match. > > Right. I wonder if there is even a point in using the dmaengine API > in that case, I think for other single-purpose drivers we tend to just > integrate the functionality in the client driver. No point changing this > now of course, but it does feel odd. I agree, and that's what I would have done as well, if it wasn't for the fact that the DMA engine also supports a second client for audio. This isn't supported in upstream yet. We could still have created an ad-hoc solution, possibly based on the components framework, but the DMA engine subsystem wasn't a bad fit. > From my earlier reading of the driver, my impression was that this > is just a memory-to-memory device, so it could be used that way > as well, but does need a flag when working on the video memory. > I couldn't quite make sense of that though. It's only memory-to-device (video and audio). See figures 33-1 and 33-16 in https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf > > > /* > > > * Use the peripheral_config to indicate that the channel is part > > > * of a video group. This requires matching use of the custom > > > * structure in each driver. > > > */ > > > pconfig = config->peripheral_config; > > > if (WARN_ON(config->peripheral_size != 0 && > > > config->peripheral_size != sizeof(*pconfig))) > > > return -EINVAL; > > > > How about > > > > if (WARN_ON(config->peripheral_config && > > config->peripheral_size != sizeof(*pconfig))) > > > > > > > > spin_lock_irqsave(&chan->lock, flags); > > > if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && > > > config->peripheral_size == sizeof(*pconfig)) > > > > And here you can test pconfig != NULL. > > Good idea. Changed now, using 'if (pconfig)' without the '!= NULL' > in both expressions. Sounds good to me.
On Mon, Nov 15, 2021 at 2:05 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Mon, Nov 15, 2021 at 01:38:07PM +0100, Arnd Bergmann wrote: > > On Mon, Nov 15, 2021 at 12:49 PM Laurent Pinchart wrote: > > > > Right. I wonder if there is even a point in using the dmaengine API > > in that case, I think for other single-purpose drivers we tend to just > > integrate the functionality in the client driver. No point changing this > > now of course, but it does feel odd. > > I agree, and that's what I would have done as well, if it wasn't for the > fact that the DMA engine also supports a second client for audio. This > isn't supported in upstream yet. We could still have created an ad-hoc > solution, possibly based on the components framework, but the DMA engine > subsystem wasn't a bad fit. Ah, makes sense. In this case, I guess the data could have been part of the DMA specifier after all, in a second cell after the channel number. Arnd
On 15-11-21, 11:21, Arnd Bergmann wrote: > On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote: > > > @@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, > > > spin_lock_irqsave(&chan->lock, flags); > > > > > > /* > > > - * Abuse the slave_id to indicate that the channel is part of a video > > > - * group. > > > + * Abuse the peripheral_config to indicate that the channel is part > > > > Is it still an abuse, or is this now the right way to pass custom data > > to the DMA engine driver ? > > It doesn't make the driver any more portable, but it's now being > more explicit about it. As far as I can tell, this is the best way > to pass data that cannot be expressed through the regular interfaces > in DT and the dmaengine API. > > Ideally there would be a generic way to pass this flag, but I couldn't > figure out what this is actually doing, or whether there is a better > way. Maybe Vinod has an idea. > > I'll change s/Abuse/Use/ for the moment until I get a definite answer. I would feel this is still not use for the peripheral_config, but lets keep it to get rid of slave_id. Also, I would be better if this was moved to DT as the next cell, don't recall why that was not done/feasible.
diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c index ce5c66e6897d..e2c1ef7a659c 100644 --- a/drivers/dma/xilinx/xilinx_dpdma.c +++ b/drivers/dma/xilinx/xilinx_dpdma.c @@ -12,6 +12,7 @@ #include <linux/clk.h> #include <linux/debugfs.h> #include <linux/delay.h> +#include <linux/dma/xilinx_dpdma.h> #include <linux/dmaengine.h> #include <linux/dmapool.h> #include <linux/interrupt.h> @@ -1273,6 +1274,7 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, struct dma_slave_config *config) { struct xilinx_dpdma_chan *chan = to_xilinx_chan(dchan); + struct xilinx_dpdma_peripheral_config *pconfig; unsigned long flags; /* @@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, spin_lock_irqsave(&chan->lock, flags); /* - * Abuse the slave_id to indicate that the channel is part of a video - * group. + * Abuse the peripheral_config to indicate that the channel is part + * of a video group. */ - if (chan->id <= ZYNQMP_DPDMA_VIDEO2) - chan->video_group = config->slave_id != 0; + pconfig = config->peripheral_config; + if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && + config->peripheral_size == sizeof(*pconfig)) + chan->video_group = pconfig->video_group; spin_unlock_irqrestore(&chan->lock, flags); diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c index ff2b308d8651..11c409cbc88e 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c @@ -24,6 +24,7 @@ #include <linux/clk.h> #include <linux/delay.h> +#include <linux/dma/xilinx_dpdma.h> #include <linux/dma-mapping.h> #include <linux/dmaengine.h> #include <linux/module.h> @@ -1058,14 +1059,18 @@ static void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer, zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt); /* - * Set slave_id for each DMA channel to indicate they're part of a + * Set pconfig for each DMA channel to indicate they're part of a * video group. */ for (i = 0; i < info->num_planes; i++) { struct zynqmp_disp_layer_dma *dma = &layer->dmas[i]; + struct xilinx_dpdma_peripheral_config pconfig = { + .video_group = true, + }; struct dma_slave_config config = { .direction = DMA_MEM_TO_DEV, - .slave_id = 1, + .peripheral_config = &pconfig, + .peripheral_size = sizeof(pconfig), }; dmaengine_slave_config(dma->chan, &config); diff --git a/include/linux/dma/xilinx_dpdma.h b/include/linux/dma/xilinx_dpdma.h new file mode 100644 index 000000000000..83a1377f03f8 --- /dev/null +++ b/include/linux/dma/xilinx_dpdma.h @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: GPL-2.0 +#ifndef __LINUX_DMA_XILINX_DPDMA_H +#define __LINUX_DMA_XILINX_DPDMA_H + +#include <linux/types.h> + +struct xilinx_dpdma_peripheral_config { + bool video_group; +}; + +#endif /* __LINUX_DMA_XILINX_DPDMA_H */