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

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

[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);
+
 #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
-- 
2.19.0.rc1

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

[PATCH 2/2] drm/vmwgfx: Use ttm_dma_page_alloc_enabled

[Thomas Hellstrom]

Instead of guessing whether TTM has the dma page allocator enabled,
ask TTM.

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 77f422cd18ab..125a2b423847 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -35,6 +35,7 @@
 #include <drm/ttm/ttm_placement.h>
 #include <drm/ttm/ttm_bo_driver.h>
 #include <drm/ttm/ttm_module.h>
+#include <drm/ttm/ttm_page_alloc.h>
 
 #define VMWGFX_DRIVER_DESC "Linux drm driver for VMware graphics devices"
 #define VMWGFX_CHIP_SVGAII 0
@@ -594,8 +595,7 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 	if (dev_priv->map_mode == vmw_dma_map_populate && vmw_restrict_iommu)
 		dev_priv->map_mode = vmw_dma_map_bind;
 
-	/* No TTM coherent page pool? FIXME: Ask TTM instead! */
-        if (!(IS_ENABLED(CONFIG_SWIOTLB) || IS_ENABLED(CONFIG_INTEL_IOMMU)) &&
+        if (!ttm_dma_page_alloc_enabled() &&
 	    (dev_priv->map_mode == vmw_dma_alloc_coherent))
 		return -EINVAL;
 
-- 
2.19.0.rc1

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

Re: [PATCH 2/2] drm/vmwgfx: Use ttm_dma_page_alloc_enabled

[Sam Ravnborg]

Hi Thomas.

One little nit, and an improvment proposal (for another patch/day).

On Fri, Jan 25, 2019 at 02:05:48PM +0100, Thomas Hellstrom wrote:
> Instead of guessing whether TTM has the dma page allocator enabled,
> ask TTM.
> 
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 77f422cd18ab..125a2b423847 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -35,6 +35,7 @@
>  #include <drm/ttm/ttm_placement.h>
>  #include <drm/ttm/ttm_bo_driver.h>
>  #include <drm/ttm/ttm_module.h>
> +#include <drm/ttm/ttm_page_alloc.h>
>  
>  #define VMWGFX_DRIVER_DESC "Linux drm driver for VMware graphics devices"
>  #define VMWGFX_CHIP_SVGAII 0
> @@ -594,8 +595,7 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
>  	if (dev_priv->map_mode == vmw_dma_map_populate && vmw_restrict_iommu)
>  		dev_priv->map_mode = vmw_dma_map_bind;
>  
> -	/* No TTM coherent page pool? FIXME: Ask TTM instead! */
> -        if (!(IS_ENABLED(CONFIG_SWIOTLB) || IS_ENABLED(CONFIG_INTEL_IOMMU)) &&
> +        if (!ttm_dma_page_alloc_enabled() &&
The line uses spaces for indent, tabs?

>  	    (dev_priv->map_mode == vmw_dma_alloc_coherent))

The code could benefit from a:
static bool is_map_coherent(const struct vmw_private *dev_priv)
{
	return dev_priv->map_mode == vmw_dma_alloc_coherent;
}

Then the above would become:

	if (!ttm_dma_page_alloc_enabled() && is_map_coherent(dev_priv))

And the other places that test for vmw_dma_alloc_coherent
would be a bit simpler too.
Same goes for the other map types obviously.

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

Re: [PATCH 2/2] drm/vmwgfx: Use ttm_dma_page_alloc_enabled

[Thomas Hellstrom]

Hi, Sam,

On 01/25/2019 07:22 PM, Sam Ravnborg wrote:
> Hi Thomas.
>
> One little nit, and an improvment proposal (for another patch/day).
>
> On Fri, Jan 25, 2019 at 02:05:48PM +0100, Thomas Hellstrom wrote:
>> Instead of guessing whether TTM has the dma page allocator enabled,
>> ask TTM.
>>
>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>> ---
>>   drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
>> index 77f422cd18ab..125a2b423847 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
>> @@ -35,6 +35,7 @@
>>   #include <drm/ttm/ttm_placement.h>
>>   #include <drm/ttm/ttm_bo_driver.h>
>>   #include <drm/ttm/ttm_module.h>
>> +#include <drm/ttm/ttm_page_alloc.h>
>>   
>>   #define VMWGFX_DRIVER_DESC "Linux drm driver for VMware graphics devices"
>>   #define VMWGFX_CHIP_SVGAII 0
>> @@ -594,8 +595,7 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
>>   	if (dev_priv->map_mode == vmw_dma_map_populate && vmw_restrict_iommu)
>>   		dev_priv->map_mode = vmw_dma_map_bind;
>>   
>> -	/* No TTM coherent page pool? FIXME: Ask TTM instead! */
>> -        if (!(IS_ENABLED(CONFIG_SWIOTLB) || IS_ENABLED(CONFIG_INTEL_IOMMU)) &&
>> +        if (!ttm_dma_page_alloc_enabled() &&
> The line uses spaces for indent, tabs?

Ugh, I thought I ran this through checkpatch. Anyway that will get 
corrected. Thanks.


>
>>   	    (dev_priv->map_mode == vmw_dma_alloc_coherent))
> The code could benefit from a:
> static bool is_map_coherent(const struct vmw_private *dev_priv)
> {
> 	return dev_priv->map_mode == vmw_dma_alloc_coherent;
> }
>
> Then the above would become:
>
> 	if (!ttm_dma_page_alloc_enabled() && is_map_coherent(dev_priv))
>
> And the other places that test for vmw_dma_alloc_coherent
> would be a bit simpler too.
> Same goes for the other map types obviously.
Sure. I'll add that to the backlog for consideration, although if we get 
to it we'll use other names because all map modes are presumed to be 
coherent...

Thanks,
/Thomas

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

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

[Koenig, Christian]

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

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

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

[Thomas Hellstrom]

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

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

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

[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?

/Thomas

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

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

[Koenig, Christian]

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
>

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