Hi Dan,
On Thu, Jun 4, 2020 at 7:06 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> On Wed, Jun 03, 2020 at 03:20:41PM -0400, Jim Quinlan wrote:
> > @@ -786,7 +787,7 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
> > const struct sun4i_backend_quirks *quirks;
> > struct resource *res;
> > void __iomem *regs;
> > - int i, ret;
> > + int i, ret = 0;
>
> No need for this.
Will fix.
>
> >
> > backend = devm_kzalloc(dev, sizeof(*backend), GFP_KERNEL);
> > if (!backend)
> > @@ -812,7 +813,9 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
> > * on our device since the RAM mapping is at 0 for the DMA bus,
> > * unlike the CPU.
> > */
> > - drm->dev->dma_pfn_offset = PHYS_PFN_OFFSET;
> > + ret = attach_uniform_dma_pfn_offset(dev, PHYS_PFN_OFFSET);
> > + if (ret)
> > + return ret;
> > }
> >
> > backend->engine.node = dev->of_node;
> > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> > index 04fbd4bf0ff9..e9cc1c2d47cd 100644
> > --- a/drivers/iommu/io-pgtable-arm.c
> > +++ b/drivers/iommu/io-pgtable-arm.c
> > @@ -754,7 +754,7 @@ arm_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg)
> > if (cfg->oas > ARM_LPAE_MAX_ADDR_BITS)
> > return NULL;
> >
> > - if (!selftest_running && cfg->iommu_dev->dma_pfn_offset) {
> > + if (!selftest_running && cfg->iommu_dev->dma_pfn_offset_map) {
> > dev_err(cfg->iommu_dev, "Cannot accommodate DMA offset for IOMMU page tables\n");
> > return NULL;
> > }
> > diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> > index eff34ded6305..7212da5e1076 100644
> > --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> > +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> > @@ -7,6 +7,7 @@
> > */
> >
> > #include <linux/clk.h>
> > +#include <linux/dma-mapping.h>
> > #include <linux/interrupt.h>
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > @@ -183,7 +184,9 @@ static int sun4i_csi_probe(struct platform_device *pdev)
> > return ret;
> > } else {
> > #ifdef PHYS_PFN_OFFSET
> > - csi->dev->dma_pfn_offset = PHYS_PFN_OFFSET;
> > + ret = attach_uniform_dma_pfn_offset(dev, PHYS_PFN_OFFSET);
> > + if (ret)
> > + return ret;
> > #endif
> > }
> >
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > index 055eb0b8e396..2d66d415b6c3 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > @@ -898,7 +898,10 @@ static int sun6i_csi_probe(struct platform_device *pdev)
> >
> > sdev->dev = &pdev->dev;
> > /* The DMA bus has the memory mapped at 0 */
> > - sdev->dev->dma_pfn_offset = PHYS_OFFSET >> PAGE_SHIFT;
> > + ret = attach_uniform_dma_pfn_offset(sdev->dev,
> > + PHYS_OFFSET >> PAGE_SHIFT);
> > + if (ret)
> > + return ret;
> >
> > ret = sun6i_csi_resource_request(sdev, pdev);
> > if (ret)
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > index 96d8cfb14a60..c89333b0a5fb 100644
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -918,6 +918,70 @@ void __iomem *of_io_request_and_map(struct device_node *np, int index,
> > }
> > EXPORT_SYMBOL(of_io_request_and_map);
> >
> > +static int attach_dma_pfn_offset_map(struct device *dev,
> > + struct device_node *node, int num_ranges)
> > +{
> > + struct of_range_parser parser;
> > + struct of_range range;
> > + struct dma_pfn_offset_region *r;
> > +
> > + r = devm_kcalloc(dev, num_ranges + 1,
> > + sizeof(struct dma_pfn_offset_region), GFP_KERNEL);
> > + if (!r)
> > + return -ENOMEM;
> > + dev->dma_pfn_offset_map = r;
> > + of_dma_range_parser_init(&parser, node);
> > +
> > + /*
> > + * Record all info for DMA ranges array. We could
> > + * just use the of_range struct, but if we did that it
> > + * would require more calculations for phys_to_dma and
> > + * dma_to_phys conversions.
> > + */
> > + for_each_of_range(&parser, &range) {
> > + r->cpu_start = range.cpu_addr;
> > + r->cpu_end = r->cpu_start + range.size - 1;
> > + r->dma_start = range.bus_addr;
> > + r->dma_end = r->dma_start + range.size - 1;
> > + r->pfn_offset = PFN_DOWN(range.cpu_addr)
> > + - PFN_DOWN(range.bus_addr);
> > + r++;
> > + }
> > + return 0;
> > +}
> > +
> > +
> > +
> > +/**
> > + * attach_dma_pfn_offset - Assign scalar offset for all addresses.
> > + * @dev: device pointer; only needed for a corner case.
> > + * @dma_pfn_offset: offset to apply when converting from phys addr
> ^^^^^^^^^^^^^^^
> This parameter name does not match.
Will fix.
>
> > + * to dma addr and vice versa.
> > + *
> > + * It returns -ENOMEM if out of memory, otherwise 0.
>
> It can also return -ENODEV. Why are we passing NULL dev pointers to
> all these functions anyway?
No one should be passing dev==NULL to these functions -- that is why I
have the error out for this case. Actually, there is a case of
dev==NULL being passed -- drivers/of/unittest.c makes a call with a
NULL device because it has no dev in its context. I wasn't sure how
to fix this.
>
> > + */
> > +int attach_uniform_dma_pfn_offset(struct device *dev, unsigned long pfn_offset)
> > +{
> > + struct dma_pfn_offset_region *r;
> > +
> > + if (!dev)
> > + return -ENODEV;
> > +
> > + if (!pfn_offset)
> > + return 0;
> > +
> > + r = devm_kcalloc(dev, 1, sizeof(struct dma_pfn_offset_region),
> > + GFP_KERNEL);
>
> Use: r = devm_kzalloc(dev, sizeof(*r), GFP_KERNEL);
Will fix.
>
>
> > + if (!r)
> > + return -ENOMEM;
> > +
> > + r->uniform_offset = true;
> > + r->pfn_offset = pfn_offset;
> > +
> > + return 0;
> > +}
>
> This function doesn't seem to do anything useful. Is part of it
> missing?
No, the uniform pfn offset is a special case. With it, there is only
one entry in the "map" and 'uniform_offset' is set to true and the
pfn_offset is also set. When the offset is to be computed, there are
no region bounds calculations needed -- the offset of the first entry
is returned immediately. This special case was intentional to (a)
preserve backwards compatibility to those using dev->dma_pfn_offset
and (b) it is faster than treating it as the general case -- I want to
minimize the execution cost of this case. If this is deemed
unnecessary, I can remove the boolean and threat all cases in the same
way.
The other case is where multiple pfn offsets are needed for multiple
regions. In this case the code checks the bounds for each successive
region, returning an offset if there is a match.
>
> > +EXPORT_SYMBOL_GPL(attach_uniform_dma_pfn_offset);
> > +
>
> regards,
> dan carpenter
>
Thanks!
Jim Quinlan
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
