diff mbox series

[04/49] drm/ttm: provide a driver-led init path for generic mm manager.

Message ID 20200731040520.3701599-5-airlied@gmail.com (mailing list archive)
State New, archived
Headers show
Series ttm mem manager refactoring. | expand

Commit Message

Dave Airlie July 31, 2020, 4:04 a.m. UTC 
From: Dave Airlie <airlied@redhat.com>

This lets the generic mm manager be initialised by the driver.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/ttm/ttm_bo_manager.c | 23 ++++++++++++++++++++---
 include/drm/ttm/ttm_bo_driver.h      |  3 +++
 2 files changed, 23 insertions(+), 3 deletions(-)

Comments

Thomas Zimmermann July 31, 2020, 6:57 a.m. UTC | #1 
Hi

Am 31.07.20 um 06:04 schrieb Dave Airlie:
> From: Dave Airlie <airlied@redhat.com>
> 
> This lets the generic mm manager be initialised by the driver.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo_manager.c | 23 ++++++++++++++++++++---
>  include/drm/ttm/ttm_bo_driver.h      |  3 +++
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_manager.c b/drivers/gpu/drm/ttm/ttm_bo_manager.c
> index facd3049c3aa..64234e5caee3 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_manager.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_manager.c
> @@ -104,8 +104,8 @@ static void ttm_bo_man_put_node(struct ttm_mem_type_manager *man,
>  	}
>  }
>  
> -static int ttm_bo_man_init(struct ttm_mem_type_manager *man,
> -			   unsigned long p_size)
> +static int ttm_bo_man_init_private(struct ttm_mem_type_manager *man,
> +		    unsigned long p_size)
>  {
>  	struct ttm_range_manager *rman;
>  
> @@ -119,6 +119,23 @@ static int ttm_bo_man_init(struct ttm_mem_type_manager *man,
>  	return 0;
>  }
>  
> +int ttm_bo_man_init(struct ttm_bo_device *bdev,
> +		    struct ttm_mem_type_manager *man,
> +		    unsigned long p_size)
> +{
> +	int ret;
> +
> +	man->func = &ttm_bo_manager_func;

Overriding man->func is the only reason for drivers to call
ttm_bo_man_init_mm_base and ttm_bo_use_mm directly (e.g., as in nouveau)?

If so, Wouldn't it be better to do

  if (!man->func)
      man->func = &ttm_bo_manager_func;

in ttm_bo_man_init and forget about the other fucntions?

Best regards
Thomas

> +
> +	ttm_bo_init_mm_base(bdev, man, p_size);
> +	ret = ttm_bo_man_init_private(man, p_size);
> +	if (ret)
> +		return ret;
> +	ttm_bo_use_mm(man);
> +	return 0;
> +}
> +EXPORT_SYMBOL(ttm_bo_man_init);
> +
>  static int ttm_bo_man_takedown(struct ttm_mem_type_manager *man)
>  {
>  	struct ttm_range_manager *rman = (struct ttm_range_manager *) man->priv;
> @@ -147,7 +164,7 @@ static void ttm_bo_man_debug(struct ttm_mem_type_manager *man,
>  }
>  
>  const struct ttm_mem_type_manager_func ttm_bo_manager_func = {
> -	.init = ttm_bo_man_init,
> +	.init = ttm_bo_man_init_private,
>  	.takedown = ttm_bo_man_takedown,
>  	.get_node = ttm_bo_man_get_node,
>  	.put_node = ttm_bo_man_put_node,
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 68e75c3b8c7a..5c4ccefd5393 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -799,6 +799,9 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo);
>   */
>  pgprot_t ttm_io_prot(uint32_t caching_flags, pgprot_t tmp);
>  
> +int ttm_bo_man_init(struct ttm_bo_device *bdev,
> +		    struct ttm_mem_type_manager *man,
> +		    unsigned long p_size);
>  extern const struct ttm_mem_type_manager_func ttm_bo_manager_func;
>  
>  #endif
>
Thomas Zimmermann July 31, 2020, 7:03 a.m. UTC | #2 
Am 31.07.20 um 08:57 schrieb Thomas Zimmermann:
> Hi
> 
> Am 31.07.20 um 06:04 schrieb Dave Airlie:
>> From: Dave Airlie <airlied@redhat.com>
>>
>> This lets the generic mm manager be initialised by the driver.
>>
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>> ---
>>  drivers/gpu/drm/ttm/ttm_bo_manager.c | 23 ++++++++++++++++++++---
>>  include/drm/ttm/ttm_bo_driver.h      |  3 +++
>>  2 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_manager.c b/drivers/gpu/drm/ttm/ttm_bo_manager.c
>> index facd3049c3aa..64234e5caee3 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo_manager.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo_manager.c
>> @@ -104,8 +104,8 @@ static void ttm_bo_man_put_node(struct ttm_mem_type_manager *man,
>>  	}
>>  }
>>  
>> -static int ttm_bo_man_init(struct ttm_mem_type_manager *man,
>> -			   unsigned long p_size)
>> +static int ttm_bo_man_init_private(struct ttm_mem_type_manager *man,
>> +		    unsigned long p_size)
>>  {
>>  	struct ttm_range_manager *rman;
>>  
>> @@ -119,6 +119,23 @@ static int ttm_bo_man_init(struct ttm_mem_type_manager *man,
>>  	return 0;
>>  }
>>  
>> +int ttm_bo_man_init(struct ttm_bo_device *bdev,
>> +		    struct ttm_mem_type_manager *man,
>> +		    unsigned long p_size)
>> +{
>> +	int ret;
>> +
>> +	man->func = &ttm_bo_manager_func;
> 
> Overriding man->func is the only reason for drivers to call
> ttm_bo_man_init_mm_base and ttm_bo_use_mm directly (e.g., as in nouveau)?
> 
> If so, Wouldn't it be better to do
> 
>   if (!man->func)
>       man->func = &ttm_bo_manager_func;
> 
> in ttm_bo_man_init and forget about the other fucntions?
> 
> Best regards
> Thomas
> 
>> +
>> +	ttm_bo_init_mm_base(bdev, man, p_size);
>> +	ret = ttm_bo_man_init_private(man, p_size);

Oh, I just realized that this line's also missing in nouveau.

>> +	if (ret)
>> +		return ret;
>> +	ttm_bo_use_mm(man);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(ttm_bo_man_init);
>> +
>>  static int ttm_bo_man_takedown(struct ttm_mem_type_manager *man)
>>  {
>>  	struct ttm_range_manager *rman = (struct ttm_range_manager *) man->priv;
>> @@ -147,7 +164,7 @@ static void ttm_bo_man_debug(struct ttm_mem_type_manager *man,
>>  }
>>  
>>  const struct ttm_mem_type_manager_func ttm_bo_manager_func = {
>> -	.init = ttm_bo_man_init,
>> +	.init = ttm_bo_man_init_private,
>>  	.takedown = ttm_bo_man_takedown,
>>  	.get_node = ttm_bo_man_get_node,
>>  	.put_node = ttm_bo_man_put_node,
>> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
>> index 68e75c3b8c7a..5c4ccefd5393 100644
>> --- a/include/drm/ttm/ttm_bo_driver.h
>> +++ b/include/drm/ttm/ttm_bo_driver.h
>> @@ -799,6 +799,9 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo);
>>   */
>>  pgprot_t ttm_io_prot(uint32_t caching_flags, pgprot_t tmp);
>>  
>> +int ttm_bo_man_init(struct ttm_bo_device *bdev,
>> +		    struct ttm_mem_type_manager *man,
>> +		    unsigned long p_size);
>>  extern const struct ttm_mem_type_manager_func ttm_bo_manager_func;
>>  
>>  #endif
>>
>
Dave Airlie July 31, 2020, 7:20 a.m. UTC | #3 
On Fri, 31 Jul 2020 at 16:57, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 31.07.20 um 06:04 schrieb Dave Airlie:
> > From: Dave Airlie <airlied@redhat.com>
> >
> > This lets the generic mm manager be initialised by the driver.
> >
> > Signed-off-by: Dave Airlie <airlied@redhat.com>
> > ---
> >  drivers/gpu/drm/ttm/ttm_bo_manager.c | 23 ++++++++++++++++++++---
> >  include/drm/ttm/ttm_bo_driver.h      |  3 +++
> >  2 files changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo_manager.c b/drivers/gpu/drm/ttm/ttm_bo_manager.c
> > index facd3049c3aa..64234e5caee3 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo_manager.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo_manager.c
> > @@ -104,8 +104,8 @@ static void ttm_bo_man_put_node(struct ttm_mem_type_manager *man,
> >       }
> >  }
> >
> > -static int ttm_bo_man_init(struct ttm_mem_type_manager *man,
> > -                        unsigned long p_size)
> > +static int ttm_bo_man_init_private(struct ttm_mem_type_manager *man,
> > +                 unsigned long p_size)
> >  {
> >       struct ttm_range_manager *rman;
> >
> > @@ -119,6 +119,23 @@ static int ttm_bo_man_init(struct ttm_mem_type_manager *man,
> >       return 0;
> >  }
> >
> > +int ttm_bo_man_init(struct ttm_bo_device *bdev,
> > +                 struct ttm_mem_type_manager *man,
> > +                 unsigned long p_size)
> > +{
> > +     int ret;
> > +
> > +     man->func = &ttm_bo_manager_func;
>
> Overriding man->func is the only reason for drivers to call
> ttm_bo_man_init_mm_base and ttm_bo_use_mm directly (e.g., as in nouveau)?
>
> If so, Wouldn't it be better to do
>
>   if (!man->func)
>       man->func = &ttm_bo_manager_func;
>
> in ttm_bo_man_init and forget about the other fucntions?

No ttm_bo_man_init, is just for the range manager, if you want to use
it, it's not generic code for driver manager that want to use a
different manager class (like nouveau).

Dave.
Christian König July 31, 2020, 1 p.m. UTC | #4 
Am 31.07.20 um 06:04 schrieb Dave Airlie:
> From: Dave Airlie <airlied@redhat.com>
>
> This lets the generic mm manager be initialised by the driver.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/ttm/ttm_bo_manager.c | 23 ++++++++++++++++++++---
>   include/drm/ttm/ttm_bo_driver.h      |  3 +++
>   2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_manager.c b/drivers/gpu/drm/ttm/ttm_bo_manager.c
> index facd3049c3aa..64234e5caee3 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_manager.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_manager.c
> @@ -104,8 +104,8 @@ static void ttm_bo_man_put_node(struct ttm_mem_type_manager *man,
>   	}
>   }
>   
> -static int ttm_bo_man_init(struct ttm_mem_type_manager *man,
> -			   unsigned long p_size)
> +static int ttm_bo_man_init_private(struct ttm_mem_type_manager *man,
> +		    unsigned long p_size)
>   {
>   	struct ttm_range_manager *rman;
>   
> @@ -119,6 +119,23 @@ static int ttm_bo_man_init(struct ttm_mem_type_manager *man,
>   	return 0;
>   }
>   
> +int ttm_bo_man_init(struct ttm_bo_device *bdev,
> +		    struct ttm_mem_type_manager *man,
> +		    unsigned long p_size)
> +{
> +	int ret;
> +
> +	man->func = &ttm_bo_manager_func;
> +
> +	ttm_bo_init_mm_base(bdev, man, p_size);
> +	ret = ttm_bo_man_init_private(man, p_size);
> +	if (ret)
> +		return ret;
> +	ttm_bo_use_mm(man);
> +	return 0;
> +}
> +EXPORT_SYMBOL(ttm_bo_man_init);
> +
>   static int ttm_bo_man_takedown(struct ttm_mem_type_manager *man)
>   {
>   	struct ttm_range_manager *rman = (struct ttm_range_manager *) man->priv;
> @@ -147,7 +164,7 @@ static void ttm_bo_man_debug(struct ttm_mem_type_manager *man,
>   }
>   
>   const struct ttm_mem_type_manager_func ttm_bo_manager_func = {
> -	.init = ttm_bo_man_init,
> +	.init = ttm_bo_man_init_private,
>   	.takedown = ttm_bo_man_takedown,
>   	.get_node = ttm_bo_man_get_node,
>   	.put_node = ttm_bo_man_put_node,
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 68e75c3b8c7a..5c4ccefd5393 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -799,6 +799,9 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo);
>    */
>   pgprot_t ttm_io_prot(uint32_t caching_flags, pgprot_t tmp);
>   
> +int ttm_bo_man_init(struct ttm_bo_device *bdev,
> +		    struct ttm_mem_type_manager *man,
> +		    unsigned long p_size);
>   extern const struct ttm_mem_type_manager_func ttm_bo_manager_func;
>   
>   #endif
diff mbox series

Patch

diff --git a/drivers/gpu/drm/ttm/ttm_bo_manager.c b/drivers/gpu/drm/ttm/ttm_bo_manager.c
index facd3049c3aa..64234e5caee3 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_manager.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_manager.c
@@ -104,8 +104,8 @@  static void ttm_bo_man_put_node(struct ttm_mem_type_manager *man,
 	}
 }
 
-static int ttm_bo_man_init(struct ttm_mem_type_manager *man,
-			   unsigned long p_size)
+static int ttm_bo_man_init_private(struct ttm_mem_type_manager *man,
+		    unsigned long p_size)
 {
 	struct ttm_range_manager *rman;
 
@@ -119,6 +119,23 @@  static int ttm_bo_man_init(struct ttm_mem_type_manager *man,
 	return 0;
 }
 
+int ttm_bo_man_init(struct ttm_bo_device *bdev,
+		    struct ttm_mem_type_manager *man,
+		    unsigned long p_size)
+{
+	int ret;
+
+	man->func = &ttm_bo_manager_func;
+
+	ttm_bo_init_mm_base(bdev, man, p_size);
+	ret = ttm_bo_man_init_private(man, p_size);
+	if (ret)
+		return ret;
+	ttm_bo_use_mm(man);
+	return 0;
+}
+EXPORT_SYMBOL(ttm_bo_man_init);
+
 static int ttm_bo_man_takedown(struct ttm_mem_type_manager *man)
 {
 	struct ttm_range_manager *rman = (struct ttm_range_manager *) man->priv;
@@ -147,7 +164,7 @@  static void ttm_bo_man_debug(struct ttm_mem_type_manager *man,
 }
 
 const struct ttm_mem_type_manager_func ttm_bo_manager_func = {
-	.init = ttm_bo_man_init,
+	.init = ttm_bo_man_init_private,
 	.takedown = ttm_bo_man_takedown,
 	.get_node = ttm_bo_man_get_node,
 	.put_node = ttm_bo_man_put_node,
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 68e75c3b8c7a..5c4ccefd5393 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -799,6 +799,9 @@  int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo);
  */
 pgprot_t ttm_io_prot(uint32_t caching_flags, pgprot_t tmp);
 
+int ttm_bo_man_init(struct ttm_bo_device *bdev,
+		    struct ttm_mem_type_manager *man,
+		    unsigned long p_size);
 extern const struct ttm_mem_type_manager_func ttm_bo_manager_func;
 
 #endif