[PATCH 0/6] drm/ast: Managed MM

[PATCH 0/6] drm/ast: Managed MM

[Thomas Zimmermann]

This is the second patchset for converting ast to managed DRM interfaces.
This one addresses memory management. There will be another, final round
of patches for converting DRM device structures as well.

Patch #1 introduces managed initialization for VRAM MM. Other drivers
using the VRAM helpers should be converted to this at some point.

Patches #2 to #4 do some preparation that make ast look slightly nicer.

Patch #5 fixes a long-standing bug that I found as part of the rework.
Posting the GPU requires information about the installed DRAM. So the DRAM
detection has to run before the GPU-posting code. This got reversed by a
fix in v4.11. The patch restores the original correct order of these
operations. As the GPU is usually posted by the VGA BIOS, the problem might
not have shown up in practice.

With all the cleanups in place, patch #6 switches memory management to
mnaged interfaces.

Tested on AST2100 HW.

Thomas Zimmermann (6):
  drm/vram-helper: Managed vram helpers
  drm/ast: Rename ast_ttm.c to ast_mm.c
  drm/ast: Use managed VRAM-helper initialization
  drm/ast: Move VRAM size detection to ast_mm.c
  drm/ast: Initialize DRAM type before posting GPU
  drm/ast: Use managed MM initialization

 drivers/gpu/drm/ast/Makefile                |  2 +-
 drivers/gpu/drm/ast/ast_drv.h               |  2 -
 drivers/gpu/drm/ast/ast_main.c              | 45 ++-----------
 drivers/gpu/drm/ast/{ast_ttm.c => ast_mm.c} | 73 ++++++++++++++++-----
 drivers/gpu/drm/drm_gem_vram_helper.c       | 68 ++++++++++++++-----
 include/drm/drm_gem_vram_helper.h           |  4 ++
 6 files changed, 118 insertions(+), 76 deletions(-)
 rename drivers/gpu/drm/ast/{ast_ttm.c => ast_mm.c} (64%)

--
2.27.0

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

[PATCH 1/6] drm/vram-helper: Managed vram helpers

[Thomas Zimmermann]

Calling drmm_vram_helper_alloc_mm() sets up a managed instance of
VRAM MM. Releasing the DRM device also frees the memory manager.

The patch also updates the DRM documentation for VRAM helpers. The
tutorial now only describes the new managed interface. The older
interfaces are deprecated and should not be used in new code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 68 ++++++++++++++++++++-------
 include/drm/drm_gem_vram_helper.h     |  4 ++
 2 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index ad096600d89f..af116999b193 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -10,6 +10,7 @@
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_ttm_helper.h>
 #include <drm/drm_gem_vram_helper.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_mode.h>
 #include <drm/drm_plane.h>
 #include <drm/drm_prime.h>
@@ -41,7 +42,7 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
  * left in VRAM, inactive GEM objects can be moved to system memory.
  *
  * The easiest way to use the VRAM helper library is to call
- * drm_vram_helper_alloc_mm(). The function allocates and initializes an
+ * drmm_vram_helper_alloc_mm(). The function allocates and initializes an
  * instance of &struct drm_vram_mm in &struct drm_device.vram_mm . Use
  * &DRM_GEM_VRAM_DRIVER to initialize &struct drm_driver and
  * &DRM_VRAM_MM_FILE_OPERATIONS to initialize &struct file_operations;
@@ -69,7 +70,7 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
  *		// setup device, vram base and size
  *		// ...
  *
- *		ret = drm_vram_helper_alloc_mm(dev, vram_base, vram_size);
+ *		ret = drmm_vram_helper_alloc_mm(dev, vram_base, vram_size);
  *		if (ret)
  *			return ret;
  *		return 0;
@@ -81,20 +82,12 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
  * manages an area of video RAM with VRAM MM and provides GEM VRAM objects
  * to userspace.
  *
- * To clean up the VRAM memory management, call drm_vram_helper_release_mm()
- * in the driver's clean-up code.
+ * You don't have to clean up the instance of VRAM MM.
+ * drmm_vram_helper_alloc_mm() is a managed interface that installs a
+ * clean-up handler to run during the DRM device's release.
  *
- * .. code-block:: c
- *
- *	void fini_drm_driver()
- *	{
- *		struct drm_device *dev = ...;
- *
- *		drm_vram_helper_release_mm(dev);
- *	}
- *
- * For drawing or scanout operations, buffer object have to be pinned in video
- * RAM. Call drm_gem_vram_pin() with &DRM_GEM_VRAM_PL_FLAG_VRAM or
+ * For drawing or scanout operations, rsp. buffer objects have to be pinned
+ * in video RAM. Call drm_gem_vram_pin() with &DRM_GEM_VRAM_PL_FLAG_VRAM or
  * &DRM_GEM_VRAM_PL_FLAG_SYSTEM to pin a buffer object in video RAM or system
  * memory. Call drm_gem_vram_unpin() to release the pinned object afterwards.
  *
@@ -1177,12 +1170,16 @@ static void drm_vram_mm_cleanup(struct drm_vram_mm *vmm)
  */
 
 /**
- * drm_vram_helper_alloc_mm - Allocates a device's instance of \
-	&struct drm_vram_mm
+ * drm_vram_helper_alloc_mm - Allocates a device's instance of
+ *                            &struct drm_vram_mm
  * @dev:	the DRM device
  * @vram_base:	the base address of the video memory
  * @vram_size:	the size of the video memory in bytes
  *
+ * The driver is responsible to clean-up the VRAM manager during
+ * device cleanup by calling drm_vram_helper_release_mm(). Use
+ * drmm_vram_helper_alloc_mm() if possible.
+ *
  * Returns:
  * The new instance of &struct drm_vram_mm on success, or
  * an ERR_PTR()-encoded errno code otherwise.
@@ -1228,6 +1225,43 @@ void drm_vram_helper_release_mm(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_vram_helper_release_mm);
 
+static void drm_vram_mm_release(struct drm_device *dev, void *ptr)
+{
+	drm_vram_helper_release_mm(dev);
+}
+
+/**
+ * drmm_vram_helper_alloc_mm - Allocates a device's managed instance of
+ *                             &struct drm_vram_mm
+ * @dev:	the DRM device
+ * @vram_base:	the base address of the video memory
+ * @vram_size:	the size of the video memory in bytes
+ *
+ * The returned instance of &struct drm_vram_mm is auto-managed and
+ * cleaned up as part of device cleanup.
+ *
+ * Returns:
+ * The new instance of &struct drm_vram_mm on success, or
+ * an ERR_PTR()-encoded errno code otherwise.
+ */
+struct drm_vram_mm *drmm_vram_helper_alloc_mm(struct drm_device *dev,
+					      uint64_t vram_base,
+					      size_t vram_size)
+{
+	struct drm_vram_mm *vram_mm;
+	int ret;
+
+	vram_mm = drm_vram_helper_alloc_mm(dev, vram_base, vram_size);
+	if (IS_ERR(vram_mm))
+		return vram_mm;
+	ret = drmm_add_action_or_reset(dev, drm_vram_mm_release, NULL);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return vram_mm;
+}
+EXPORT_SYMBOL(drmm_vram_helper_alloc_mm);
+
 /*
  * Mode-config helpers
  */
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index b63bcd1b996d..a456a272968b 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -206,6 +206,10 @@ struct drm_vram_mm *drm_vram_helper_alloc_mm(
 	struct drm_device *dev, uint64_t vram_base, size_t vram_size);
 void drm_vram_helper_release_mm(struct drm_device *dev);
 
+struct drm_vram_mm *drmm_vram_helper_alloc_mm(struct drm_device *dev,
+					      uint64_t vram_base,
+					      size_t vram_size);
+
 /*
  * Mode-config helpers
  */
-- 
2.27.0

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

Re: [PATCH 1/6] drm/vram-helper: Managed vram helpers

[Sam Ravnborg]

Hi Thomas.

On Wed, Jul 08, 2020 at 09:49:07AM +0200, Thomas Zimmermann wrote:
> Calling drmm_vram_helper_alloc_mm() sets up a managed instance of
> VRAM MM. Releasing the DRM device also frees the memory manager.
> 
> The patch also updates the DRM documentation for VRAM helpers. The
> tutorial now only describes the new managed interface. The older
> interfaces are deprecated and should not be used in new code.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

A couple of comments...



> ---
>  drivers/gpu/drm/drm_gem_vram_helper.c | 68 ++++++++++++++++++++-------
>  include/drm/drm_gem_vram_helper.h     |  4 ++
>  2 files changed, 55 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index ad096600d89f..af116999b193 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -10,6 +10,7 @@
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_gem_ttm_helper.h>
>  #include <drm/drm_gem_vram_helper.h>
> +#include <drm/drm_managed.h>
>  #include <drm/drm_mode.h>
>  #include <drm/drm_plane.h>
>  #include <drm/drm_prime.h>
> @@ -41,7 +42,7 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
>   * left in VRAM, inactive GEM objects can be moved to system memory.
>   *
>   * The easiest way to use the VRAM helper library is to call
> - * drm_vram_helper_alloc_mm(). The function allocates and initializes an
> + * drmm_vram_helper_alloc_mm(). The function allocates and initializes an
>   * instance of &struct drm_vram_mm in &struct drm_device.vram_mm . Use
>   * &DRM_GEM_VRAM_DRIVER to initialize &struct drm_driver and
>   * &DRM_VRAM_MM_FILE_OPERATIONS to initialize &struct file_operations;
> @@ -69,7 +70,7 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
>   *		// setup device, vram base and size
>   *		// ...
>   *
> - *		ret = drm_vram_helper_alloc_mm(dev, vram_base, vram_size);
> + *		ret = drmm_vram_helper_alloc_mm(dev, vram_base, vram_size);
>   *		if (ret)
>   *			return ret;
This example is how this should be done - but we are not there yet..
See below.

>   *		return 0;
> @@ -81,20 +82,12 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
>   * manages an area of video RAM with VRAM MM and provides GEM VRAM objects
>   * to userspace.
>   *
> - * To clean up the VRAM memory management, call drm_vram_helper_release_mm()
> - * in the driver's clean-up code.
> + * You don't have to clean up the instance of VRAM MM.
> + * drmm_vram_helper_alloc_mm() is a managed interface that installs a
> + * clean-up handler to run during the DRM device's release.
>   *
> - * .. code-block:: c
> - *
> - *	void fini_drm_driver()
> - *	{
> - *		struct drm_device *dev = ...;
> - *
> - *		drm_vram_helper_release_mm(dev);
> - *	}
> - *
> - * For drawing or scanout operations, buffer object have to be pinned in video
> - * RAM. Call drm_gem_vram_pin() with &DRM_GEM_VRAM_PL_FLAG_VRAM or
> + * For drawing or scanout operations, rsp. buffer objects have to be pinned
> + * in video RAM. Call drm_gem_vram_pin() with &DRM_GEM_VRAM_PL_FLAG_VRAM or
>   * &DRM_GEM_VRAM_PL_FLAG_SYSTEM to pin a buffer object in video RAM or system
>   * memory. Call drm_gem_vram_unpin() to release the pinned object afterwards.
>   *
> @@ -1177,12 +1170,16 @@ static void drm_vram_mm_cleanup(struct drm_vram_mm *vmm)
>   */
>  
>  /**
> - * drm_vram_helper_alloc_mm - Allocates a device's instance of \
> -	&struct drm_vram_mm
> + * drm_vram_helper_alloc_mm - Allocates a device's instance of
> + *                            &struct drm_vram_mm
>   * @dev:	the DRM device
>   * @vram_base:	the base address of the video memory
>   * @vram_size:	the size of the video memory in bytes
>   *
> + * The driver is responsible to clean-up the VRAM manager during
> + * device cleanup by calling drm_vram_helper_release_mm(). Use
> + * drmm_vram_helper_alloc_mm() if possible.
> + *
>   * Returns:
>   * The new instance of &struct drm_vram_mm on success, or
>   * an ERR_PTR()-encoded errno code otherwise.
drm_vram_helper_alloc_mm is deprecated so just delete the kernel-doc.
We do not want kernel-doc of deprecated functions.


> @@ -1228,6 +1225,43 @@ void drm_vram_helper_release_mm(struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_vram_helper_release_mm);
>  
> +static void drm_vram_mm_release(struct drm_device *dev, void *ptr)
> +{
> +	drm_vram_helper_release_mm(dev);
> +}
> +
> +/**
> + * drmm_vram_helper_alloc_mm - Allocates a device's managed instance of
> + *                             &struct drm_vram_mm
> + * @dev:	the DRM device
> + * @vram_base:	the base address of the video memory
> + * @vram_size:	the size of the video memory in bytes
> + *
> + * The returned instance of &struct drm_vram_mm is auto-managed and
> + * cleaned up as part of device cleanup.
This should document that dev->vram_mm is assigned the value of the
allocated drm_vram_mm is the allocation succeeds, otherwise set it to
NULL.

> + *
> + * Returns:
Some DRM doc uses "RETURNS" - I am not sure what it the most common way.

> + * The new instance of &struct drm_vram_mm on success, or
> + * an ERR_PTR()-encoded errno code otherwise.
> + */
> +struct drm_vram_mm *drmm_vram_helper_alloc_mm(struct drm_device *dev,
> +					      uint64_t vram_base,
> +					      size_t vram_size)
> +{
None of the users of drm_vram_helper_alloc_mm() uses the pointer, they
all uses PTR_ERR().
So the right interface would be to return an int as already documented
in the intro section.
I had a patch to convert the function to return an int - but it is
better to go direct to a managed interface.


> +	struct drm_vram_mm *vram_mm;
> +	int ret;
> +
> +	vram_mm = drm_vram_helper_alloc_mm(dev, vram_base, vram_size);
> +	if (IS_ERR(vram_mm))
> +		return vram_mm;
> +	ret = drmm_add_action_or_reset(dev, drm_vram_mm_release, NULL);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return vram_mm;
> +}
> +EXPORT_SYMBOL(drmm_vram_helper_alloc_mm);
> +
>  /*
>   * Mode-config helpers
>   */
> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> index b63bcd1b996d..a456a272968b 100644
> --- a/include/drm/drm_gem_vram_helper.h
> +++ b/include/drm/drm_gem_vram_helper.h
> @@ -206,6 +206,10 @@ struct drm_vram_mm *drm_vram_helper_alloc_mm(
>  	struct drm_device *dev, uint64_t vram_base, size_t vram_size);
>  void drm_vram_helper_release_mm(struct drm_device *dev);
>  
> +struct drm_vram_mm *drmm_vram_helper_alloc_mm(struct drm_device *dev,
> +					      uint64_t vram_base,
> +					      size_t vram_size);
> +
>  /*
>   * Mode-config helpers
>   */
> -- 
> 2.27.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm/vram-helper: Managed vram helpers

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 7592 bytes --]

Hi Sam

Am 08.07.20 um 19:19 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Wed, Jul 08, 2020 at 09:49:07AM +0200, Thomas Zimmermann wrote:
>> Calling drmm_vram_helper_alloc_mm() sets up a managed instance of
>> VRAM MM. Releasing the DRM device also frees the memory manager.
>>
>> The patch also updates the DRM documentation for VRAM helpers. The
>> tutorial now only describes the new managed interface. The older
>> interfaces are deprecated and should not be used in new code.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> A couple of comments...

These are all good points. I'll fix them in the next revision.

Best regards
Thomas

> 
> 
> 
>> ---
>>  drivers/gpu/drm/drm_gem_vram_helper.c | 68 ++++++++++++++++++++-------
>>  include/drm/drm_gem_vram_helper.h     |  4 ++
>>  2 files changed, 55 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
>> index ad096600d89f..af116999b193 100644
>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>> @@ -10,6 +10,7 @@
>>  #include <drm/drm_gem_framebuffer_helper.h>
>>  #include <drm/drm_gem_ttm_helper.h>
>>  #include <drm/drm_gem_vram_helper.h>
>> +#include <drm/drm_managed.h>
>>  #include <drm/drm_mode.h>
>>  #include <drm/drm_plane.h>
>>  #include <drm/drm_prime.h>
>> @@ -41,7 +42,7 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
>>   * left in VRAM, inactive GEM objects can be moved to system memory.
>>   *
>>   * The easiest way to use the VRAM helper library is to call
>> - * drm_vram_helper_alloc_mm(). The function allocates and initializes an
>> + * drmm_vram_helper_alloc_mm(). The function allocates and initializes an
>>   * instance of &struct drm_vram_mm in &struct drm_device.vram_mm . Use
>>   * &DRM_GEM_VRAM_DRIVER to initialize &struct drm_driver and
>>   * &DRM_VRAM_MM_FILE_OPERATIONS to initialize &struct file_operations;
>> @@ -69,7 +70,7 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
>>   *		// setup device, vram base and size
>>   *		// ...
>>   *
>> - *		ret = drm_vram_helper_alloc_mm(dev, vram_base, vram_size);
>> + *		ret = drmm_vram_helper_alloc_mm(dev, vram_base, vram_size);
>>   *		if (ret)
>>   *			return ret;
> This example is how this should be done - but we are not there yet..
> See below.
> 
>>   *		return 0;
>> @@ -81,20 +82,12 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
>>   * manages an area of video RAM with VRAM MM and provides GEM VRAM objects
>>   * to userspace.
>>   *
>> - * To clean up the VRAM memory management, call drm_vram_helper_release_mm()
>> - * in the driver's clean-up code.
>> + * You don't have to clean up the instance of VRAM MM.
>> + * drmm_vram_helper_alloc_mm() is a managed interface that installs a
>> + * clean-up handler to run during the DRM device's release.
>>   *
>> - * .. code-block:: c
>> - *
>> - *	void fini_drm_driver()
>> - *	{
>> - *		struct drm_device *dev = ...;
>> - *
>> - *		drm_vram_helper_release_mm(dev);
>> - *	}
>> - *
>> - * For drawing or scanout operations, buffer object have to be pinned in video
>> - * RAM. Call drm_gem_vram_pin() with &DRM_GEM_VRAM_PL_FLAG_VRAM or
>> + * For drawing or scanout operations, rsp. buffer objects have to be pinned
>> + * in video RAM. Call drm_gem_vram_pin() with &DRM_GEM_VRAM_PL_FLAG_VRAM or
>>   * &DRM_GEM_VRAM_PL_FLAG_SYSTEM to pin a buffer object in video RAM or system
>>   * memory. Call drm_gem_vram_unpin() to release the pinned object afterwards.
>>   *
>> @@ -1177,12 +1170,16 @@ static void drm_vram_mm_cleanup(struct drm_vram_mm *vmm)
>>   */
>>  
>>  /**
>> - * drm_vram_helper_alloc_mm - Allocates a device's instance of \
>> -	&struct drm_vram_mm
>> + * drm_vram_helper_alloc_mm - Allocates a device's instance of
>> + *                            &struct drm_vram_mm
>>   * @dev:	the DRM device
>>   * @vram_base:	the base address of the video memory
>>   * @vram_size:	the size of the video memory in bytes
>>   *
>> + * The driver is responsible to clean-up the VRAM manager during
>> + * device cleanup by calling drm_vram_helper_release_mm(). Use
>> + * drmm_vram_helper_alloc_mm() if possible.
>> + *
>>   * Returns:
>>   * The new instance of &struct drm_vram_mm on success, or
>>   * an ERR_PTR()-encoded errno code otherwise.
> drm_vram_helper_alloc_mm is deprecated so just delete the kernel-doc.
> We do not want kernel-doc of deprecated functions.
> 
> 
>> @@ -1228,6 +1225,43 @@ void drm_vram_helper_release_mm(struct drm_device *dev)
>>  }
>>  EXPORT_SYMBOL(drm_vram_helper_release_mm);
>>  
>> +static void drm_vram_mm_release(struct drm_device *dev, void *ptr)
>> +{
>> +	drm_vram_helper_release_mm(dev);
>> +}
>> +
>> +/**
>> + * drmm_vram_helper_alloc_mm - Allocates a device's managed instance of
>> + *                             &struct drm_vram_mm
>> + * @dev:	the DRM device
>> + * @vram_base:	the base address of the video memory
>> + * @vram_size:	the size of the video memory in bytes
>> + *
>> + * The returned instance of &struct drm_vram_mm is auto-managed and
>> + * cleaned up as part of device cleanup.
> This should document that dev->vram_mm is assigned the value of the
> allocated drm_vram_mm is the allocation succeeds, otherwise set it to
> NULL.
> 
>> + *
>> + * Returns:
> Some DRM doc uses "RETURNS" - I am not sure what it the most common way.
> 
>> + * The new instance of &struct drm_vram_mm on success, or
>> + * an ERR_PTR()-encoded errno code otherwise.
>> + */
>> +struct drm_vram_mm *drmm_vram_helper_alloc_mm(struct drm_device *dev,
>> +					      uint64_t vram_base,
>> +					      size_t vram_size)
>> +{
> None of the users of drm_vram_helper_alloc_mm() uses the pointer, they
> all uses PTR_ERR().
> So the right interface would be to return an int as already documented
> in the intro section.
> I had a patch to convert the function to return an int - but it is
> better to go direct to a managed interface.
> 
> 
>> +	struct drm_vram_mm *vram_mm;
>> +	int ret;
>> +
>> +	vram_mm = drm_vram_helper_alloc_mm(dev, vram_base, vram_size);
>> +	if (IS_ERR(vram_mm))
>> +		return vram_mm;
>> +	ret = drmm_add_action_or_reset(dev, drm_vram_mm_release, NULL);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	return vram_mm;
>> +}
>> +EXPORT_SYMBOL(drmm_vram_helper_alloc_mm);
>> +
>>  /*
>>   * Mode-config helpers
>>   */
>> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
>> index b63bcd1b996d..a456a272968b 100644
>> --- a/include/drm/drm_gem_vram_helper.h
>> +++ b/include/drm/drm_gem_vram_helper.h
>> @@ -206,6 +206,10 @@ struct drm_vram_mm *drm_vram_helper_alloc_mm(
>>  	struct drm_device *dev, uint64_t vram_base, size_t vram_size);
>>  void drm_vram_helper_release_mm(struct drm_device *dev);
>>  
>> +struct drm_vram_mm *drmm_vram_helper_alloc_mm(struct drm_device *dev,
>> +					      uint64_t vram_base,
>> +					      size_t vram_size);
>> +
>>  /*
>>   * Mode-config helpers
>>   */
>> -- 
>> 2.27.0
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH 2/6] drm/ast: Rename ast_ttm.c to ast_mm.c

[Thomas Zimmermann]

Although built upon TTM, the ast driver's VRAM MM helper does not
expose TTM to its users. Rename the related ast file to ast_mm.c.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/Makefile                | 2 +-
 drivers/gpu/drm/ast/{ast_ttm.c => ast_mm.c} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename drivers/gpu/drm/ast/{ast_ttm.c => ast_mm.c} (100%)

diff --git a/drivers/gpu/drm/ast/Makefile b/drivers/gpu/drm/ast/Makefile
index 6a5b3b247316..2265a8a624dd 100644
--- a/drivers/gpu/drm/ast/Makefile
+++ b/drivers/gpu/drm/ast/Makefile
@@ -3,7 +3,7 @@
 # Makefile for the drm device driver.  This driver provides support for the
 # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
 
-ast-y := ast_cursor.o ast_drv.o ast_main.o ast_mode.o ast_ttm.o ast_post.o \
+ast-y := ast_cursor.o ast_drv.o ast_main.o ast_mm.o ast_mode.o ast_post.o \
 	 ast_dp501.o
 
 obj-$(CONFIG_DRM_AST) := ast.o
diff --git a/drivers/gpu/drm/ast/ast_ttm.c b/drivers/gpu/drm/ast/ast_mm.c
similarity index 100%
rename from drivers/gpu/drm/ast/ast_ttm.c
rename to drivers/gpu/drm/ast/ast_mm.c
-- 
2.27.0

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

[PATCH 3/6] drm/ast: Use managed VRAM-helper initialization

[Thomas Zimmermann]

As a first step to managed MM code in ast, switch over VRAM MM helpers.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mm.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
index 9c3788a4c1c5..c0bbcfed9c43 100644
--- a/drivers/gpu/drm/ast/ast_mm.c
+++ b/drivers/gpu/drm/ast/ast_mm.c
@@ -39,9 +39,8 @@ int ast_mm_init(struct ast_private *ast)
 	int ret;
 	struct drm_device *dev = ast->dev;
 
-	vmm = drm_vram_helper_alloc_mm(
-		dev, pci_resource_start(dev->pdev, 0),
-		ast->vram_size);
+	vmm = drmm_vram_helper_alloc_mm(dev, pci_resource_start(dev->pdev, 0),
+					ast->vram_size);
 	if (IS_ERR(vmm)) {
 		ret = PTR_ERR(vmm);
 		drm_err(dev, "Error initializing VRAM MM; %d\n", ret);
@@ -60,8 +59,6 @@ void ast_mm_fini(struct ast_private *ast)
 {
 	struct drm_device *dev = ast->dev;
 
-	drm_vram_helper_release_mm(dev);
-
 	arch_phys_wc_del(ast->fb_mtrr);
 	arch_io_free_memtype_wc(pci_resource_start(dev->pdev, 0),
 				pci_resource_len(dev->pdev, 0));
-- 
2.27.0

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

Re: [PATCH 3/6] drm/ast: Use managed VRAM-helper initialization

[Sam Ravnborg]

On Wed, Jul 08, 2020 at 09:49:09AM +0200, Thomas Zimmermann wrote:
> As a first step to managed MM code in ast, switch over VRAM MM helpers.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/ast/ast_mm.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
> index 9c3788a4c1c5..c0bbcfed9c43 100644
> --- a/drivers/gpu/drm/ast/ast_mm.c
> +++ b/drivers/gpu/drm/ast/ast_mm.c
> @@ -39,9 +39,8 @@ int ast_mm_init(struct ast_private *ast)
>  	int ret;
>  	struct drm_device *dev = ast->dev;
>  
> -	vmm = drm_vram_helper_alloc_mm(
> -		dev, pci_resource_start(dev->pdev, 0),
> -		ast->vram_size);
> +	vmm = drmm_vram_helper_alloc_mm(dev, pci_resource_start(dev->pdev, 0),
> +					ast->vram_size);
>  	if (IS_ERR(vmm)) {
>  		ret = PTR_ERR(vmm);
>  		drm_err(dev, "Error initializing VRAM MM; %d\n", ret);

This would be a little simpler if drmm_vram_helper_alloc_mm() return an
int error code as suggested in previous patch.

	Sam

> @@ -60,8 +59,6 @@ void ast_mm_fini(struct ast_private *ast)
>  {
>  	struct drm_device *dev = ast->dev;
>  
> -	drm_vram_helper_release_mm(dev);
> -
>  	arch_phys_wc_del(ast->fb_mtrr);
>  	arch_io_free_memtype_wc(pci_resource_start(dev->pdev, 0),
>  				pci_resource_len(dev->pdev, 0));
> -- 
> 2.27.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] drm/ast: Move VRAM size detection to ast_mm.c

[Thomas Zimmermann]

VRAM size detection is only relevant to the memory management. Move
the code into ast_mm.c.

While at it, rename the function to ast_get_vram_size(). The function
argument's type is now struct ast_private. The result is stored in a
local variable and not in struct ast_private any longer.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  |  1 -
 drivers/gpu/drm/ast/ast_main.c | 38 ++--------------------------
 drivers/gpu/drm/ast/ast_mm.c   | 45 +++++++++++++++++++++++++++++++++-
 3 files changed, 46 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index c8c442e9efdc..9a770e5b36d1 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -110,7 +110,6 @@ struct ast_private {
 	uint32_t dram_bus_width;
 	uint32_t dram_type;
 	uint32_t mclk;
-	uint32_t vram_size;
 
 	int fb_mtrr;
 
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 860a43a64b31..b162cc82204d 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -378,38 +378,6 @@ static int ast_get_dram_info(struct drm_device *dev)
 	return 0;
 }
 
-static u32 ast_get_vram_info(struct drm_device *dev)
-{
-	struct ast_private *ast = to_ast_private(dev);
-	u8 jreg;
-	u32 vram_size;
-	ast_open_key(ast);
-
-	vram_size = AST_VIDMEM_DEFAULT_SIZE;
-	jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xaa, 0xff);
-	switch (jreg & 3) {
-	case 0: vram_size = AST_VIDMEM_SIZE_8M; break;
-	case 1: vram_size = AST_VIDMEM_SIZE_16M; break;
-	case 2: vram_size = AST_VIDMEM_SIZE_32M; break;
-	case 3: vram_size = AST_VIDMEM_SIZE_64M; break;
-	}
-
-	jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0x99, 0xff);
-	switch (jreg & 0x03) {
-	case 1:
-		vram_size -= 0x100000;
-		break;
-	case 2:
-		vram_size -= 0x200000;
-		break;
-	case 3:
-		vram_size -= 0x400000;
-		break;
-	}
-
-	return vram_size;
-}
-
 int ast_driver_load(struct drm_device *dev, unsigned long flags)
 {
 	struct ast_private *ast;
@@ -456,10 +424,8 @@ int ast_driver_load(struct drm_device *dev, unsigned long flags)
 	ret = ast_get_dram_info(dev);
 	if (ret)
 		goto out_free;
-	ast->vram_size = ast_get_vram_info(dev);
-	drm_info(dev, "dram MCLK=%u Mhz type=%d bus_width=%d size=%08x\n",
-		 ast->mclk, ast->dram_type,
-		 ast->dram_bus_width, ast->vram_size);
+	drm_info(dev, "dram MCLK=%u Mhz type=%d bus_width=%d\n",
+		 ast->mclk, ast->dram_type, ast->dram_bus_width);
 
 	ret = ast_mm_init(ast);
 	if (ret)
diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
index c0bbcfed9c43..4cf30bf6414a 100644
--- a/drivers/gpu/drm/ast/ast_mm.c
+++ b/drivers/gpu/drm/ast/ast_mm.c
@@ -33,14 +33,57 @@
 
 #include "ast_drv.h"
 
+static u32 ast_get_vram_size(struct ast_private *ast)
+{
+	u8 jreg;
+	u32 vram_size;
+
+	ast_open_key(ast);
+
+	vram_size = AST_VIDMEM_DEFAULT_SIZE;
+	jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xaa, 0xff);
+	switch (jreg & 3) {
+	case 0:
+		vram_size = AST_VIDMEM_SIZE_8M;
+		break;
+	case 1:
+		vram_size = AST_VIDMEM_SIZE_16M;
+		break;
+	case 2:
+		vram_size = AST_VIDMEM_SIZE_32M;
+		break;
+	case 3:
+		vram_size = AST_VIDMEM_SIZE_64M;
+		break;
+	}
+
+	jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0x99, 0xff);
+	switch (jreg & 0x03) {
+	case 1:
+		vram_size -= 0x100000;
+		break;
+	case 2:
+		vram_size -= 0x200000;
+		break;
+	case 3:
+		vram_size -= 0x400000;
+		break;
+	}
+
+	return vram_size;
+}
+
 int ast_mm_init(struct ast_private *ast)
 {
 	struct drm_vram_mm *vmm;
+	u32 vram_size;
 	int ret;
 	struct drm_device *dev = ast->dev;
 
+	vram_size = ast_get_vram_size(ast);
+
 	vmm = drmm_vram_helper_alloc_mm(dev, pci_resource_start(dev->pdev, 0),
-					ast->vram_size);
+					vram_size);
 	if (IS_ERR(vmm)) {
 		ret = PTR_ERR(vmm);
 		drm_err(dev, "Error initializing VRAM MM; %d\n", ret);
-- 
2.27.0

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

[PATCH 5/6] drm/ast: Initialize DRAM type before posting GPU

[Thomas Zimmermann]

Posting the GPU requires the correct DRAM type to be stored in
struct ast_private. Therefore first initialize the DRAM info and
then post the GPU. This restores the original order of instructions
in this function.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: bad09da6deab ("drm/ast: Fixed vram size incorrect issue on POWER")
Cc: Joel Stanley <joel@jms.id.au>
Cc: Y.C. Chen <yc_chen@aspeedtech.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
Cc: <stable@vger.kernel.org> # v4.11+
---
 drivers/gpu/drm/ast/ast_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index b162cc82204d..87e5baded2a7 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -418,15 +418,15 @@ int ast_driver_load(struct drm_device *dev, unsigned long flags)
 
 	ast_detect_chip(dev, &need_post);
 
-	if (need_post)
-		ast_post_gpu(dev);
-
 	ret = ast_get_dram_info(dev);
 	if (ret)
 		goto out_free;
 	drm_info(dev, "dram MCLK=%u Mhz type=%d bus_width=%d\n",
 		 ast->mclk, ast->dram_type, ast->dram_bus_width);
 
+	if (need_post)
+		ast_post_gpu(dev);
+
 	ret = ast_mm_init(ast);
 	if (ret)
 		goto out_free;
-- 
2.27.0

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

Re: [PATCH 5/6] drm/ast: Initialize DRAM type before posting GPU

[Benjamin Herrenschmidt]

On Wed, 2020-07-08 at 09:49 +0200, Thomas Zimmermann wrote:
> Posting the GPU requires the correct DRAM type to be stored in
> struct ast_private. Therefore first initialize the DRAM info and
> then post the GPU. This restores the original order of instructions
> in this function.

I no longer have access to the relevant test POWER systems,
however the patch looks good to me.

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: bad09da6deab ("drm/ast: Fixed vram size incorrect issue on
> POWER")
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Y.C. Chen <yc_chen@aspeedtech.com>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
> Cc: <stable@vger.kernel.org> # v4.11+
> ---
>  drivers/gpu/drm/ast/ast_main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_main.c
> b/drivers/gpu/drm/ast/ast_main.c
> index b162cc82204d..87e5baded2a7 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -418,15 +418,15 @@ int ast_driver_load(struct drm_device *dev,
> unsigned long flags)
>  
>  	ast_detect_chip(dev, &need_post);
>  
> -	if (need_post)
> -		ast_post_gpu(dev);
> -
>  	ret = ast_get_dram_info(dev);
>  	if (ret)
>  		goto out_free;
>  	drm_info(dev, "dram MCLK=%u Mhz type=%d bus_width=%d\n",
>  		 ast->mclk, ast->dram_type, ast->dram_bus_width);
>  
> +	if (need_post)
> +		ast_post_gpu(dev);
> +
>  	ret = ast_mm_init(ast);
>  	if (ret)
>  		goto out_free;

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

[PATCH 6/6] drm/ast: Use managed MM initialization

[Thomas Zimmermann]

Cleaning up ast's MM code with ast_mm_fini() resets the write-combine
flags on the VRAM I/O memory. Drop ast_mm_fini() in favor of an auto-
release callback. Releasing the device also executes the callback.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  |  1 -
 drivers/gpu/drm/ast/ast_main.c |  1 -
 drivers/gpu/drm/ast/ast_mm.c   | 23 ++++++++++++-----------
 3 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 9a770e5b36d1..e3a264ac7ee2 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -291,7 +291,6 @@ int ast_mode_config_init(struct ast_private *ast);
 #define AST_MM_ALIGN_MASK ((1 << AST_MM_ALIGN_SHIFT) - 1)
 
 int ast_mm_init(struct ast_private *ast);
-void ast_mm_fini(struct ast_private *ast);
 
 /* ast post */
 void ast_enable_vga(struct drm_device *dev);
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 87e5baded2a7..dd12b55d57a2 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -452,6 +452,5 @@ void ast_driver_unload(struct drm_device *dev)
 	ast_release_firmware(dev);
 	kfree(ast->dp501_fw_addr);
 
-	ast_mm_fini(ast);
 	kfree(ast);
 }
diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
index 4cf30bf6414a..52debea8d1c7 100644
--- a/drivers/gpu/drm/ast/ast_mm.c
+++ b/drivers/gpu/drm/ast/ast_mm.c
@@ -28,8 +28,9 @@
 
 #include <linux/pci.h>
 
-#include <drm/drm_print.h>
 #include <drm/drm_gem_vram_helper.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_print.h>
 
 #include "ast_drv.h"
 
@@ -73,6 +74,15 @@ static u32 ast_get_vram_size(struct ast_private *ast)
 	return vram_size;
 }
 
+static void ast_mm_release(struct drm_device *dev, void *ptr)
+{
+	struct ast_private *ast = to_ast_private(dev);
+
+	arch_phys_wc_del(ast->fb_mtrr);
+	arch_io_free_memtype_wc(pci_resource_start(dev->pdev, 0),
+				pci_resource_len(dev->pdev, 0));
+}
+
 int ast_mm_init(struct ast_private *ast)
 {
 	struct drm_vram_mm *vmm;
@@ -95,14 +105,5 @@ int ast_mm_init(struct ast_private *ast)
 	ast->fb_mtrr = arch_phys_wc_add(pci_resource_start(dev->pdev, 0),
 					pci_resource_len(dev->pdev, 0));
 
-	return 0;
-}
-
-void ast_mm_fini(struct ast_private *ast)
-{
-	struct drm_device *dev = ast->dev;
-
-	arch_phys_wc_del(ast->fb_mtrr);
-	arch_io_free_memtype_wc(pci_resource_start(dev->pdev, 0),
-				pci_resource_len(dev->pdev, 0));
+	return drmm_add_action_or_reset(dev, ast_mm_release, NULL);
 }
-- 
2.27.0

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