Re: [PATCH 3/4] drm: add ARM flush implementations

["Ville Syrjälä" <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On Thu, Jan 18, 2018 at 08:42:55AM +0100, Daniel Vetter wrote:
> On Wed, Jan 17, 2018 at 11:46 PM, Gurchetan Singh
> <gurchetansingh-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> >> dma api just isn't quite sufficient for implementing fast gpu drivers.
> >
> >
> > Can you elaborate?  IIRC the DMA API has strong synchronization guarantees
> > and that can be problematic for GPU drivers.  However, using the DMA API for
> > flushing doesn't necessarily mean the driver has to use the rest of the DMA
> > API.
> >
> >> but then it'd need to directly flush cpu caches and bypass the dma api.
> >
> >
> > On ARM, cache flushing seems to vary from chipset to chipset.  For example,
> > on ARM32 a typical call-stack for dma_sync_single_for_device looks like:
> >
> > arm_dma_sync_single_for_device
> > __dma_page_cpu_to_dev
> > outer_clean_range
> > outer_cache.clean_range
> >
> > There are multiple clean_range implementations out there (i.e,
> > aurora_clean_range, l2c210_clean_range, feroceon_l2_clean_range), so that's
> > why the DMA API was used in this case.  On ARM64, things are a little
> > simpler, but the DMA API seems to go directly to assembly (__dma_map_area)
> > after a little indirection.  Why do you think that's inefficient?
> 
> I never said it's inefficient. My only gripe is with adding the
> pointless struct device * argument to flushing functions which really
> don't care (nor should care) about the device. Because what we really
> want to call is outer_clean_range here, and that doesn't need a struct
> device *. Imo that function (or something similar) needs to be
> exported and then used by drm_flush_* functions.
> 
> Also note that arm has both flush and invalidate functions, but on x86
> those are the same implementation (and we don't have a separate
> drm_invalidate_* set of functions). That doesn't look like a too good
> idea.

IMO if someone adds some new functions they should talk about
"writeback" and "invalidate". I think "flush" is a very vague
term that could mean different things to different people.

x86 has clflush which is writeback+invalidate, and more recently
x86 gained the clwb instruction for doing just writeback without
the invalidate. On ARM IIRC you can choose whether to do
writeback or invalidate or both.

> 
> Of course that doesn't solve the problem of who's supposed to call it
> and when in the dma-buf sharing situation.
> -Daniel
> 
> > On Wed, Jan 17, 2018 at 12:31 AM, Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org> wrote:
> >>
> >> On Tue, Jan 16, 2018 at 04:35:58PM -0800, Gurchetan Singh wrote:
> >> > The DMA API can be used to flush scatter gather tables and physical
> >> > pages on ARM devices.
> >> >
> >> > Signed-off-by: Gurchetan Singh <gurchetansingh-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >> > ---
> >> >  drivers/gpu/drm/drm_cache.c                 | 17 +++++++++++++++++
> >> >  drivers/gpu/drm/rockchip/rockchip_drm_gem.c |  7 ++-----
> >> >  drivers/gpu/drm/tegra/gem.c                 |  6 +-----
> >> >  3 files changed, 20 insertions(+), 10 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
> >> > index 3d2bb9d71a60..98d6ebb40e96 100644
> >> > --- a/drivers/gpu/drm/drm_cache.c
> >> > +++ b/drivers/gpu/drm/drm_cache.c
> >> > @@ -105,6 +105,18 @@ drm_flush_pages(struct device *dev, struct page
> >> > *pages[],
> >> >                                  (unsigned long)page_virtual +
> >> > PAGE_SIZE);
> >> >               kunmap_atomic(page_virtual);
> >> >       }
> >> > +#elif defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> >> > +     unsigned long i;
> >> > +     dma_addr_t dma_handle;
> >> > +
> >> > +     if (!dev)
> >> > +             return;
> >> > +
> >> > +     for (i = 0; i < num_pages; i++) {
> >> > +             dma_handle = phys_to_dma(drm->dev,
> >> > page_to_phys(pages[i]));
> >> > +             dma_sync_single_for_device(dev, dma_handle, PAGE_SIZE,
> >> > +                                        DMA_TO_DEVICE);
> >>
> >> Erm no. These functions here are super-low-level functions used by drivers
> >> which know exactly what they're doing. Which is reimplementing half of the
> >> dma api behind the dma api's back because the dma api just isn't quite
> >> sufficient for implementing fast gpu drivers.
> >>
> >> If all you end up doing is calling the dma api again, then pls just call
> >> it directly.
> >>
> >> And just to make it clear: I'd be perfectly fine with adding arm support
> >> here, but then it'd need to directly flush cpu caches and bypass the dma
> >> api. Otherwise this is pointless.
> >> -Daniel
> >>
> >> > +     }
> >> >  #else
> >> >       pr_err("Architecture has no drm_cache.c support\n");
> >> >       WARN_ON_ONCE(1);
> >> > @@ -136,6 +148,11 @@ drm_flush_sg(struct device *dev, struct sg_table
> >> > *st)
> >> >
> >> >       if (wbinvd_on_all_cpus())
> >> >               pr_err("Timed out waiting for cache flush\n");
> >> > +#elif defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> >> > +     if (!dev)
> >> > +             return;
> >> > +
> >> > +     dma_sync_sg_for_device(dev, st->sgl, st->nents, DMA_TO_DEVICE);
> >> >  #else
> >> >       pr_err("Architecture has no drm_cache.c support\n");
> >> >       WARN_ON_ONCE(1);
> >> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> >> > b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> >> > index 8ac7eb25e46d..0157f90b5d10 100644
> >> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> >> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> >> > @@ -14,6 +14,7 @@
> >> >
> >> >  #include <drm/drm.h>
> >> >  #include <drm/drmP.h>
> >> > +#include <drm/drm_cache.h>
> >> >  #include <drm/drm_gem.h>
> >> >  #include <drm/drm_vma_manager.h>
> >> >  #include <linux/iommu.h>
> >> > @@ -99,15 +100,11 @@ static int rockchip_gem_get_pages(struct
> >> > rockchip_gem_object *rk_obj)
> >> >       /*
> >> >        * Fake up the SG table so that dma_sync_sg_for_device() can be
> >> > used
> >> >        * to flush the pages associated with it.
> >> > -      *
> >> > -      * TODO: Replace this by drm_flush_sg() once it can be implemented
> >> > -      * without relying on symbols that are not exported.
> >> >        */
> >> >       for_each_sg(rk_obj->sgt->sgl, s, rk_obj->sgt->nents, i)
> >> >               sg_dma_address(s) = sg_phys(s);
> >> >
> >> > -     dma_sync_sg_for_device(drm->dev, rk_obj->sgt->sgl,
> >> > rk_obj->sgt->nents,
> >> > -                            DMA_TO_DEVICE);
> >> > +     drm_flush_sg(drm->dev, rk_obj->sgt);
> >> >
> >> >       return 0;
> >> >
> >> > diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
> >> > index ab1e53d434e8..9945fd2f6bd6 100644
> >> > --- a/drivers/gpu/drm/tegra/gem.c
> >> > +++ b/drivers/gpu/drm/tegra/gem.c
> >> > @@ -230,15 +230,11 @@ static int tegra_bo_get_pages(struct drm_device
> >> > *drm, struct tegra_bo *bo)
> >> >       /*
> >> >        * Fake up the SG table so that dma_sync_sg_for_device() can be
> >> > used
> >> >        * to flush the pages associated with it.
> >> > -      *
> >> > -      * TODO: Replace this by drm_clflash_sg() once it can be
> >> > implemented
> >> > -      * without relying on symbols that are not exported.
> >> >        */
> >> >       for_each_sg(bo->sgt->sgl, s, bo->sgt->nents, i)
> >> >               sg_dma_address(s) = sg_phys(s);
> >> >
> >> > -     dma_sync_sg_for_device(drm->dev, bo->sgt->sgl, bo->sgt->nents,
> >> > -                            DMA_TO_DEVICE);
> >> > +     drm_flush_sg(drm->dev, bo->sgt);
> >> >
> >> >       return 0;
> >> >
> >> > --
> >> > 2.13.5
> >> >
> >> > _______________________________________________
> >> > dri-devel mailing list
> >> > dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> >> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> >
> >
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC