Re: [PATCH 1/2] drm/ttm: Implement and export ttm_dma_page_alloc_enabled

[Thomas Hellstrom <thellstrom@vmware.com>]

On Mon, 2019-01-28 at 14:29 +0100, Thomas Hellström wrote:
> Hi,
> 
> On Mon, 2019-01-28 at 12:21 +0000, Koenig, Christian wrote:
> > Am 25.01.19 um 14:05 schrieb Thomas Hellstrom:
> > > The vmwgfx driver needs to know whether the dma page pool is
> > > enabled
> > > to determine whether to refuse loading if the dma mode logic
> > > requests coherent memory from the dma page pool.
> > > 
> > > Cc: "Koenig, Christian" <Christian.Koenig@amd.com>
> > > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> > > ---
> > >   drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 11 +++++++++++
> > >   include/drm/ttm/ttm_page_alloc.h         |  4 ++++
> > >   2 files changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> > > b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> > > index d594f7520b7b..adf8cc189ecc 100644
> > > --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> > > +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> > > @@ -1235,4 +1235,15 @@ int ttm_dma_page_alloc_debugfs(struct
> > > seq_file *m, void *data)
> > >   }
> > >   EXPORT_SYMBOL_GPL(ttm_dma_page_alloc_debugfs);
> > >   
> > > +/**
> > > + * ttm_dma_page_alloc_enabled - Is the dma page pool enabled?
> > > + *
> > > + * Returns true if the dma page pool is enabled. false
> > > otherwise.
> > > + */
> > > +bool ttm_dma_page_alloc_enabled(void)
> > > +{
> > > +	return !!_manager;
> > > +}
> > > +EXPORT_SYMBOL(ttm_dma_page_alloc_enabled);
> > > +
> > 
> > This is completely superfluous cause the manager is enabled as soon
> > as 
> > it is compiled in.
> > 
> > You could just use "#if defined(CONFIG_SWIOTLB) || 
> > defined(CONFIG_INTEL_IOMMU)" instead.
> > 
> > Christian.
> > 
> 
> Yes, that's what was done before in vmgwfx, but it's very fragile and
> based on assumptions about what various parts of TTM does and doesn't
> do. Chances are somebody would modify the enablement and forget to
> modify this function. 
> 
> But of course I can change it if you think that'd be better.
> 
> /Thomas
> 

What about

static inline bool ttm_dma_page_alloc_enabled(void)
{
	return (IS_ENABLED(CONFIG_INTEL_IOMMU) ||
IS_ENABLED(CONFIG_SWIOTLB)) && _manager;
}

to avoid that layering violation?

/Thomas

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