[1/2] drm/ttm: Implement and export ttm_dma_page_alloc_enabled
diff mbox series

Message ID 20190125130548.3266-1-thellstrom@vmware.com
State New
Headers show
Series
  • [1/2] drm/ttm: Implement and export ttm_dma_page_alloc_enabled
Related show

Commit Message

Thomas Hellstrom Jan. 25, 2019, 1:05 p.m. UTC
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(+)

Comments

Christian König Jan. 28, 2019, 12:21 p.m. UTC | #1
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.

>   #endif
> diff --git a/include/drm/ttm/ttm_page_alloc.h b/include/drm/ttm/ttm_page_alloc.h
> index 4d9b019d253c..f810d389f5ad 100644
> --- a/include/drm/ttm/ttm_page_alloc.h
> +++ b/include/drm/ttm/ttm_page_alloc.h
> @@ -94,6 +94,8 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev,
>   			struct ttm_operation_ctx *ctx);
>   void ttm_dma_unpopulate(struct ttm_dma_tt *ttm_dma, struct device *dev);
>   
> +bool ttm_dma_page_alloc_enabled(void);
> +
>   #else
>   static inline int ttm_dma_page_alloc_init(struct ttm_mem_global *glob,
>   					  unsigned max_pages)
> @@ -117,6 +119,8 @@ static inline void ttm_dma_unpopulate(struct ttm_dma_tt *ttm_dma,
>   				      struct device *dev)
>   {
>   }
> +
> +static inline bool ttm_dma_page_alloc_enabled(void) { return false; }
>   #endif
>   
>   #endif
Thomas Hellstrom Jan. 28, 2019, 1:29 p.m. UTC | #2
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
Thomas Hellstrom Jan. 28, 2019, 2:21 p.m. UTC | #3
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
Christian König Jan. 28, 2019, 3:56 p.m. UTC | #4
Am 28.01.19 um 15:21 schrieb Thomas Hellstrom:
> 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?

If you drop the "&& _manager" check and move it into the header then 
that should work.

But we could as well really clean it up and add a hidden 
CONFIG_DRM_TTM_DMA_PAGE_ALLOC and remove all the references to 
CONFIG_INTEL_IOMMU and CONFIG_SWIOTLB from the code.

Christian.

>
> /Thomas
>

Patch
diff mbox series

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);
+
 #endif
diff --git a/include/drm/ttm/ttm_page_alloc.h b/include/drm/ttm/ttm_page_alloc.h
index 4d9b019d253c..f810d389f5ad 100644
--- a/include/drm/ttm/ttm_page_alloc.h
+++ b/include/drm/ttm/ttm_page_alloc.h
@@ -94,6 +94,8 @@  int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev,
 			struct ttm_operation_ctx *ctx);
 void ttm_dma_unpopulate(struct ttm_dma_tt *ttm_dma, struct device *dev);
 
+bool ttm_dma_page_alloc_enabled(void);
+
 #else
 static inline int ttm_dma_page_alloc_init(struct ttm_mem_global *glob,
 					  unsigned max_pages)
@@ -117,6 +119,8 @@  static inline void ttm_dma_unpopulate(struct ttm_dma_tt *ttm_dma,
 				      struct device *dev)
 {
 }
+
+static inline bool ttm_dma_page_alloc_enabled(void) { return false; }
 #endif
 
 #endif