All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] drm/vram: Check display modes against available VRAM
@ 2020-02-01 12:27 Thomas Zimmermann
  2020-02-01 12:27 ` [PATCH 1/4] drm/vram: Add helpers to validate a display mode's memory requirements Thomas Zimmermann
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Thomas Zimmermann @ 2020-02-01 12:27 UTC (permalink / raw)
  To: airlied, daniel, kraxel, maarten.lankhorst, mripard,
	z.liuxinliang, zourongrong, kong.kongxinwei, puck.chen, hdegoede,
	sam
  Cc: Thomas Zimmermann, dri-devel

This adds drm_vram_helper_mode_valid(), which tests a display mode against
the available video memory. It's a helper function to sort out display
modes that cannot be used because of a lack of video memory.

The ast driver already implemented this test for a while. The patchset
converts ast over to the helper and adds the test to the over drivers
that use VRAM helpers; except mgag200. I left out mgag200 as it doesn't
have atomic modesetting yet and needs adjustments to memory management
first.

Thomas Zimmermann (4):
  drm/vram: Add helpers to validate a display mode's memory requirements
  drm/bochs: Implement struct drm_mode_config_funcs.mode_valid
  drm/hibmc: Implement struct drm_mode_config_funcs.mode_valid
  drm/vboxvideo: Implement struct drm_mode_config_funcs.mode_valid

 drivers/gpu/drm/ast/ast_main.c              | 24 +------
 drivers/gpu/drm/bochs/bochs_kms.c           |  1 +
 drivers/gpu/drm/drm_gem_vram_helper.c       | 76 +++++++++++++++++++++
 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c |  1 +
 drivers/gpu/drm/vboxvideo/vbox_mode.c       |  1 +
 include/drm/drm_gem_vram_helper.h           | 14 ++++
 6 files changed, 94 insertions(+), 23 deletions(-)

--
2.25.0

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/4] drm/vram: Add helpers to validate a display mode's memory requirements
  2020-02-01 12:27 [PATCH 0/4] drm/vram: Check display modes against available VRAM Thomas Zimmermann
@ 2020-02-01 12:27 ` Thomas Zimmermann
  2020-02-03  9:49   ` Daniel Vetter
  2020-02-01 12:27 ` [PATCH 2/4] drm/bochs: Implement struct drm_mode_config_funcs.mode_valid Thomas Zimmermann
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Thomas Zimmermann @ 2020-02-01 12:27 UTC (permalink / raw)
  To: airlied, daniel, kraxel, maarten.lankhorst, mripard,
	z.liuxinliang, zourongrong, kong.kongxinwei, puck.chen, hdegoede,
	sam
  Cc: Thomas Zimmermann, dri-devel

Devices with low amount of dedicated video memory may not be able
to use all possible display modes, as the framebuffers may not fit
into VRAM. The new helper function drm_vram_helper_mode_valid()
implements a simple test to sort out all display modes that can
not be used in any case. Drivers should call this function from
struct drm_mode_config_funcs.mode_valid.

Calling drm_vram_helper_mode_valid() works best for 32-bit framebuffers;
drivers with framebuffers of different color depth can use
drm_vram_helper_mode_valid_internal() instead.

The functionality was originally implemented by the ast driver, which
is being converted as well.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_main.c        | 24 +--------
 drivers/gpu/drm/drm_gem_vram_helper.c | 76 +++++++++++++++++++++++++++
 include/drm/drm_gem_vram_helper.h     | 14 +++++
 3 files changed, 91 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index b79f484e9bd2..18a0a4ce00f6 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -388,31 +388,9 @@ static int ast_get_dram_info(struct drm_device *dev)
 	return 0;
 }
 
-enum drm_mode_status ast_mode_config_mode_valid(struct drm_device *dev,
-						const struct drm_display_mode *mode)
-{
-	static const unsigned long max_bpp = 4; /* DRM_FORMAT_XRGBA8888 */
-
-	struct ast_private *ast = dev->dev_private;
-	unsigned long fbsize, fbpages, max_fbpages;
-
-	/* To support double buffering, a framebuffer may not
-	 * consume more than half of the available VRAM.
-	 */
-	max_fbpages = (ast->vram_size / 2) >> PAGE_SHIFT;
-
-	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
-	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
-
-	if (fbpages > max_fbpages)
-		return MODE_MEM;
-
-	return MODE_OK;
-}
-
 static const struct drm_mode_config_funcs ast_mode_funcs = {
 	.fb_create = drm_gem_fb_create,
-	.mode_valid = ast_mode_config_mode_valid,
+	.mode_valid = drm_vram_helper_mode_valid,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
 };
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index a4863326061a..89aebd500655 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -1141,3 +1141,79 @@ void drm_vram_helper_release_mm(struct drm_device *dev)
 	dev->vram_mm = NULL;
 }
 EXPORT_SYMBOL(drm_vram_helper_release_mm);
+
+/*
+ * Mode-config helpers
+ */
+
+/**
+ * drm_vram_helper_mode_valid_internal - Tests if a display mode's
+ *	framebuffer fits into the available video memory.
+ * @dev:	the DRM device
+ * @mode:	the mode to test
+ * @max_bpp:	the maximum number of bytes per pixel
+ * Returns:
+ *	MODE_OK the display mode is supported, or an error code of type
+ *	enum drm_mode_status otherwise.
+ *
+ * This function tests if enough video memory is available using the
+ * specified display mode. Atomic modesetting requires importing the
+ * designated framebuffer into video memory before evicting the active
+ * one. Hence, any framebuffer may consume at most half of the available
+ * VRAM. Display modes that require a larger framebuffer can not be used,
+ * even of the CRTC does support them.
+ *
+ * Drivers should call this function from
+ * struct drm_mode_config_funcs.mode_valid. See drm_vram_helper_mode_valid()
+ * for framebuffers that use at most 32-bit color depth.
+ *
+ * Note:
+ *	The function can only test if the display mode is supported in
+ *	general. If you have too many framebuffers pinned to video memory,
+ *	a display mode may still not be usable in practice.
+ */
+enum drm_mode_status
+drm_vram_helper_mode_valid_internal(struct drm_device *dev,
+				    const struct drm_display_mode *mode,
+				    unsigned long max_bpp)
+{
+	struct drm_vram_mm *vmm = dev->vram_mm;
+	unsigned long fbsize, fbpages, max_fbpages;
+
+	if (!dev->vram_mm)
+		return MODE_BAD;
+
+	max_fbpages = (vmm->vram_size / 2) >> PAGE_SHIFT;
+
+	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
+	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
+
+	if (fbpages > max_fbpages)
+		return MODE_MEM;
+
+	return MODE_OK;
+}
+EXPORT_SYMBOL(drm_vram_helper_mode_valid_internal);
+
+/**
+ * drm_vram_helper_mode_valid - Tests if a display mode's
+ *	framebuffer fits into the available video memory.
+ * @dev:	the DRM device
+ * @mode:	the mode to test
+ * Returns:
+ *	MODE_OK the display mode is supported, or an error code of type
+ *	enum drm_mode_status otherwise.
+ *
+ * This function is a variant of drm_vram_helper_mode_valid_internal()
+ * for framebuffers that use at most 32-bit color depth. It can be plugged
+ * directly into struct drm_mode_config_funcs.mode_valid.
+ */
+enum drm_mode_status
+drm_vram_helper_mode_valid(struct drm_device *dev,
+			   const struct drm_display_mode *mode)
+{
+	static const unsigned long max_bpp = 4; /* DRM_FORMAT_XRGBA8888 */
+
+	return drm_vram_helper_mode_valid_internal(dev, mode, max_bpp);
+}
+EXPORT_SYMBOL(drm_vram_helper_mode_valid);
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index 573e9fd109bf..e7d9144c79ad 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -6,6 +6,7 @@
 #include <drm/drm_file.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_ioctl.h>
+#include <drm/drm_modes.h>
 #include <drm/ttm/ttm_bo_api.h>
 #include <drm/ttm/ttm_bo_driver.h>
 #include <drm/ttm/ttm_placement.h>
@@ -205,4 +206,17 @@ 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);
 
+/*
+ * Mode-config helpers
+ */
+
+enum drm_mode_status
+drm_vram_helper_mode_valid_internal(struct drm_device *dev,
+				    const struct drm_display_mode *mode,
+				    unsigned long max_bpp);
+
+enum drm_mode_status
+drm_vram_helper_mode_valid(struct drm_device *dev,
+			   const struct drm_display_mode *mode);
+
 #endif
-- 
2.25.0

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/4] drm/bochs: Implement struct drm_mode_config_funcs.mode_valid
  2020-02-01 12:27 [PATCH 0/4] drm/vram: Check display modes against available VRAM Thomas Zimmermann
  2020-02-01 12:27 ` [PATCH 1/4] drm/vram: Add helpers to validate a display mode's memory requirements Thomas Zimmermann
@ 2020-02-01 12:27 ` Thomas Zimmermann
  2020-02-03  6:47   ` Gerd Hoffmann
  2020-02-01 12:27 ` [PATCH 3/4] drm/hibmc: " Thomas Zimmermann
  2020-02-01 12:27 ` [PATCH 4/4] drm/vboxvideo: " Thomas Zimmermann
  3 siblings, 1 reply; 13+ messages in thread
From: Thomas Zimmermann @ 2020-02-01 12:27 UTC (permalink / raw)
  To: airlied, daniel, kraxel, maarten.lankhorst, mripard,
	z.liuxinliang, zourongrong, kong.kongxinwei, puck.chen, hdegoede,
	sam
  Cc: Thomas Zimmermann, dri-devel

The implementation of struct drm_mode_config_funcs.mode_valid verifies
that enough video memory is available for a given display mode.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/bochs/bochs_kms.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
index ff275faee88d..ddb6c7e2a174 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -148,6 +148,7 @@ bochs_gem_fb_create(struct drm_device *dev, struct drm_file *file,
 
 const struct drm_mode_config_funcs bochs_mode_funcs = {
 	.fb_create = bochs_gem_fb_create,
+	.mode_valid = drm_vram_helper_mode_valid,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
 };
-- 
2.25.0

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/4] drm/hibmc: Implement struct drm_mode_config_funcs.mode_valid
  2020-02-01 12:27 [PATCH 0/4] drm/vram: Check display modes against available VRAM Thomas Zimmermann
  2020-02-01 12:27 ` [PATCH 1/4] drm/vram: Add helpers to validate a display mode's memory requirements Thomas Zimmermann
  2020-02-01 12:27 ` [PATCH 2/4] drm/bochs: Implement struct drm_mode_config_funcs.mode_valid Thomas Zimmermann
@ 2020-02-01 12:27 ` Thomas Zimmermann
  2020-02-03  9:50   ` Daniel Vetter
  2020-02-01 12:27 ` [PATCH 4/4] drm/vboxvideo: " Thomas Zimmermann
  3 siblings, 1 reply; 13+ messages in thread
From: Thomas Zimmermann @ 2020-02-01 12:27 UTC (permalink / raw)
  To: airlied, daniel, kraxel, maarten.lankhorst, mripard,
	z.liuxinliang, zourongrong, kong.kongxinwei, puck.chen, hdegoede,
	sam
  Cc: Thomas Zimmermann, dri-devel

The implementation of struct drm_mode_config_funcs.mode_valid verifies
that enough video memory is available for a given display mode.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
index 50b988fdd5cc..99397ac3b363 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
@@ -54,6 +54,7 @@ int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
 }
 
 const struct drm_mode_config_funcs hibmc_mode_funcs = {
+	.mode_valid = drm_vram_helper_mode_valid,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
 	.fb_create = drm_gem_fb_create,
-- 
2.25.0

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/4] drm/vboxvideo: Implement struct drm_mode_config_funcs.mode_valid
  2020-02-01 12:27 [PATCH 0/4] drm/vram: Check display modes against available VRAM Thomas Zimmermann
                   ` (2 preceding siblings ...)
  2020-02-01 12:27 ` [PATCH 3/4] drm/hibmc: " Thomas Zimmermann
@ 2020-02-01 12:27 ` Thomas Zimmermann
  2020-02-03  9:53   ` Daniel Vetter
  3 siblings, 1 reply; 13+ messages in thread
From: Thomas Zimmermann @ 2020-02-01 12:27 UTC (permalink / raw)
  To: airlied, daniel, kraxel, maarten.lankhorst, mripard,
	z.liuxinliang, zourongrong, kong.kongxinwei, puck.chen, hdegoede,
	sam
  Cc: Thomas Zimmermann, dri-devel

The implementation of struct drm_mode_config_funcs.mode_valid verifies
that enough video memory is available for a given display mode.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/vboxvideo/vbox_mode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c
index 8b7f005c4d20..0883a435e62b 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_mode.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c
@@ -826,6 +826,7 @@ static int vbox_connector_init(struct drm_device *dev,
 
 static const struct drm_mode_config_funcs vbox_mode_funcs = {
 	.fb_create = drm_gem_fb_create_with_dirty,
+	.mode_valid = drm_vram_helper_mode_valid,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
 };
-- 
2.25.0

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] drm/bochs: Implement struct drm_mode_config_funcs.mode_valid
  2020-02-01 12:27 ` [PATCH 2/4] drm/bochs: Implement struct drm_mode_config_funcs.mode_valid Thomas Zimmermann
@ 2020-02-03  6:47   ` Gerd Hoffmann
  2020-02-03  8:35     ` Thomas Zimmermann
  0 siblings, 1 reply; 13+ messages in thread
From: Gerd Hoffmann @ 2020-02-03  6:47 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: airlied, puck.chen, z.liuxinliang, hdegoede, kong.kongxinwei,
	dri-devel, zourongrong, sam

On Sat, Feb 01, 2020 at 01:27:42PM +0100, Thomas Zimmermann wrote:
> The implementation of struct drm_mode_config_funcs.mode_valid verifies
> that enough video memory is available for a given display mode.

There is bochs_connector_mode_valid() doing the same check,
you can drop it when hooking up drm_vram_helper_mode_valid.

cheers,
  Gerd

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] drm/bochs: Implement struct drm_mode_config_funcs.mode_valid
  2020-02-03  6:47   ` Gerd Hoffmann
@ 2020-02-03  8:35     ` Thomas Zimmermann
  2020-02-03 10:25       ` Gerd Hoffmann
  0 siblings, 1 reply; 13+ messages in thread
From: Thomas Zimmermann @ 2020-02-03  8:35 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: airlied, puck.chen, dri-devel, hdegoede, kong.kongxinwei,
	zourongrong, sam


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

Hi Gerd

Am 03.02.20 um 07:47 schrieb Gerd Hoffmann:
> On Sat, Feb 01, 2020 at 01:27:42PM +0100, Thomas Zimmermann wrote:
>> The implementation of struct drm_mode_config_funcs.mode_valid verifies
>> that enough video memory is available for a given display mode.
> 
> There is bochs_connector_mode_valid() doing the same check,
> you can drop it when hooking up drm_vram_helper_mode_valid.

Oh, didn't see it. That's more duplicated code to remove. :)

There's a helpful comment in bochs_connector_mode_valid(). Where shall I
move it?

Best regard
Thomas

> 
> cheers,
>   Gerd
> 
> _______________________________________________
> 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: 488 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] drm/vram: Add helpers to validate a display mode's memory requirements
  2020-02-01 12:27 ` [PATCH 1/4] drm/vram: Add helpers to validate a display mode's memory requirements Thomas Zimmermann
@ 2020-02-03  9:49   ` Daniel Vetter
  2020-02-03  9:53     ` Daniel Vetter
  0 siblings, 1 reply; 13+ messages in thread
From: Daniel Vetter @ 2020-02-03  9:49 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: airlied, puck.chen, dri-devel, z.liuxinliang, hdegoede,
	kong.kongxinwei, kraxel, zourongrong, sam

On Sat, Feb 01, 2020 at 01:27:41PM +0100, Thomas Zimmermann wrote:
> Devices with low amount of dedicated video memory may not be able
> to use all possible display modes, as the framebuffers may not fit
> into VRAM. The new helper function drm_vram_helper_mode_valid()
> implements a simple test to sort out all display modes that can
> not be used in any case. Drivers should call this function from
> struct drm_mode_config_funcs.mode_valid.
> 
> Calling drm_vram_helper_mode_valid() works best for 32-bit framebuffers;
> drivers with framebuffers of different color depth can use
> drm_vram_helper_mode_valid_internal() instead.
> 
> The functionality was originally implemented by the ast driver, which
> is being converted as well.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/ast/ast_main.c        | 24 +--------
>  drivers/gpu/drm/drm_gem_vram_helper.c | 76 +++++++++++++++++++++++++++
>  include/drm/drm_gem_vram_helper.h     | 14 +++++
>  3 files changed, 91 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index b79f484e9bd2..18a0a4ce00f6 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -388,31 +388,9 @@ static int ast_get_dram_info(struct drm_device *dev)
>  	return 0;
>  }
>  
> -enum drm_mode_status ast_mode_config_mode_valid(struct drm_device *dev,
> -						const struct drm_display_mode *mode)
> -{
> -	static const unsigned long max_bpp = 4; /* DRM_FORMAT_XRGBA8888 */
> -
> -	struct ast_private *ast = dev->dev_private;
> -	unsigned long fbsize, fbpages, max_fbpages;
> -
> -	/* To support double buffering, a framebuffer may not
> -	 * consume more than half of the available VRAM.
> -	 */
> -	max_fbpages = (ast->vram_size / 2) >> PAGE_SHIFT;
> -
> -	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
> -	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
> -
> -	if (fbpages > max_fbpages)
> -		return MODE_MEM;
> -
> -	return MODE_OK;
> -}
> -
>  static const struct drm_mode_config_funcs ast_mode_funcs = {
>  	.fb_create = drm_gem_fb_create,
> -	.mode_valid = ast_mode_config_mode_valid,
> +	.mode_valid = drm_vram_helper_mode_valid,
>  	.atomic_check = drm_atomic_helper_check,
>  	.atomic_commit = drm_atomic_helper_commit,
>  };
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index a4863326061a..89aebd500655 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -1141,3 +1141,79 @@ void drm_vram_helper_release_mm(struct drm_device *dev)
>  	dev->vram_mm = NULL;
>  }
>  EXPORT_SYMBOL(drm_vram_helper_release_mm);
> +
> +/*
> + * Mode-config helpers
> + */
> +
> +/**
> + * drm_vram_helper_mode_valid_internal - Tests if a display mode's
> + *	framebuffer fits into the available video memory.
> + * @dev:	the DRM device
> + * @mode:	the mode to test
> + * @max_bpp:	the maximum number of bytes per pixel

Does this render correctly? I thought an empty comment line is required
between comments and the free-form paragraphs ... Also usual style (in drm
at least) is that the Returns: block is more towards the end of the text,
and not indented.


> + * Returns:
> + *	MODE_OK the display mode is supported, or an error code of type
> + *	enum drm_mode_status otherwise.
> + *
> + * This function tests if enough video memory is available using the
> + * specified display mode. Atomic modesetting requires importing the
> + * designated framebuffer into video memory before evicting the active
> + * one. Hence, any framebuffer may consume at most half of the available
> + * VRAM. Display modes that require a larger framebuffer can not be used,
> + * even of the CRTC does support them.
> + *
> + * Drivers should call this function from
> + * struct drm_mode_config_funcs.mode_valid. See drm_vram_helper_mode_valid()
> + * for framebuffers that use at most 32-bit color depth.
> + *
> + * Note:
> + *	The function can only test if the display mode is supported in
> + *	general. If you have too many framebuffers pinned to video memory,
> + *	a display mode may still not be usable in practice.
> + */
> +enum drm_mode_status
> +drm_vram_helper_mode_valid_internal(struct drm_device *dev,
> +				    const struct drm_display_mode *mode,
> +				    unsigned long max_bpp)
> +{
> +	struct drm_vram_mm *vmm = dev->vram_mm;
> +	unsigned long fbsize, fbpages, max_fbpages;
> +
> +	if (!dev->vram_mm)

This is a driver bug imo (most likely of enabling hotplug/output detection
before the vram handling is set up), needs to be wrapped in a WARN_ON to
catch this.

> +		return MODE_BAD;
> +
> +	max_fbpages = (vmm->vram_size / 2) >> PAGE_SHIFT;
> +
> +	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
> +	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
> +
> +	if (fbpages > max_fbpages)
> +		return MODE_MEM;
> +
> +	return MODE_OK;
> +}
> +EXPORT_SYMBOL(drm_vram_helper_mode_valid_internal);

Anyway, patch looks good (with the nits addressed one way or the other):

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> +
> +/**
> + * drm_vram_helper_mode_valid - Tests if a display mode's
> + *	framebuffer fits into the available video memory.
> + * @dev:	the DRM device
> + * @mode:	the mode to test
> + * Returns:
> + *	MODE_OK the display mode is supported, or an error code of type
> + *	enum drm_mode_status otherwise.
> + *
> + * This function is a variant of drm_vram_helper_mode_valid_internal()
> + * for framebuffers that use at most 32-bit color depth. It can be plugged
> + * directly into struct drm_mode_config_funcs.mode_valid.
> + */
> +enum drm_mode_status
> +drm_vram_helper_mode_valid(struct drm_device *dev,
> +			   const struct drm_display_mode *mode)
> +{
> +	static const unsigned long max_bpp = 4; /* DRM_FORMAT_XRGBA8888 */
> +
> +	return drm_vram_helper_mode_valid_internal(dev, mode, max_bpp);
> +}
> +EXPORT_SYMBOL(drm_vram_helper_mode_valid);
> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> index 573e9fd109bf..e7d9144c79ad 100644
> --- a/include/drm/drm_gem_vram_helper.h
> +++ b/include/drm/drm_gem_vram_helper.h
> @@ -6,6 +6,7 @@
>  #include <drm/drm_file.h>
>  #include <drm/drm_gem.h>
>  #include <drm/drm_ioctl.h>
> +#include <drm/drm_modes.h>
>  #include <drm/ttm/ttm_bo_api.h>
>  #include <drm/ttm/ttm_bo_driver.h>
>  #include <drm/ttm/ttm_placement.h>
> @@ -205,4 +206,17 @@ 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);
>  
> +/*
> + * Mode-config helpers
> + */
> +
> +enum drm_mode_status
> +drm_vram_helper_mode_valid_internal(struct drm_device *dev,
> +				    const struct drm_display_mode *mode,
> +				    unsigned long max_bpp);
> +
> +enum drm_mode_status
> +drm_vram_helper_mode_valid(struct drm_device *dev,
> +			   const struct drm_display_mode *mode);
> +
>  #endif
> -- 
> 2.25.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] drm/hibmc: Implement struct drm_mode_config_funcs.mode_valid
  2020-02-01 12:27 ` [PATCH 3/4] drm/hibmc: " Thomas Zimmermann
@ 2020-02-03  9:50   ` Daniel Vetter
  0 siblings, 0 replies; 13+ messages in thread
From: Daniel Vetter @ 2020-02-03  9:50 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: airlied, puck.chen, dri-devel, z.liuxinliang, hdegoede,
	kong.kongxinwei, kraxel, zourongrong, sam

On Sat, Feb 01, 2020 at 01:27:43PM +0100, Thomas Zimmermann wrote:
> The implementation of struct drm_mode_config_funcs.mode_valid verifies
> that enough video memory is available for a given display mode.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

lgtm, Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

While browsing I realized that hibmc has a bunch of dummy mode_valid
functions that we could garbage collect. And the kirin driver has a
hand-rolled version of "must call crtc validation funcs for
connector->mode_valid" (because that driver predates the addition of that
support in the probe helpers). In case you're bored :-)

Cheers, Daniel

> ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> index 50b988fdd5cc..99397ac3b363 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> @@ -54,6 +54,7 @@ int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>  }
>  
>  const struct drm_mode_config_funcs hibmc_mode_funcs = {
> +	.mode_valid = drm_vram_helper_mode_valid,
>  	.atomic_check = drm_atomic_helper_check,
>  	.atomic_commit = drm_atomic_helper_commit,
>  	.fb_create = drm_gem_fb_create,
> -- 
> 2.25.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] drm/vram: Add helpers to validate a display mode's memory requirements
  2020-02-03  9:49   ` Daniel Vetter
@ 2020-02-03  9:53     ` Daniel Vetter
  2020-02-03 10:25       ` Thomas Zimmermann
  0 siblings, 1 reply; 13+ messages in thread
From: Daniel Vetter @ 2020-02-03  9:53 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: airlied, puck.chen, dri-devel, z.liuxinliang, hdegoede,
	kong.kongxinwei, kraxel, zourongrong, sam

On Mon, Feb 03, 2020 at 10:49:30AM +0100, Daniel Vetter wrote:
> On Sat, Feb 01, 2020 at 01:27:41PM +0100, Thomas Zimmermann wrote:
> > Devices with low amount of dedicated video memory may not be able
> > to use all possible display modes, as the framebuffers may not fit
> > into VRAM. The new helper function drm_vram_helper_mode_valid()
> > implements a simple test to sort out all display modes that can
> > not be used in any case. Drivers should call this function from
> > struct drm_mode_config_funcs.mode_valid.
> > 
> > Calling drm_vram_helper_mode_valid() works best for 32-bit framebuffers;
> > drivers with framebuffers of different color depth can use
> > drm_vram_helper_mode_valid_internal() instead.
> > 
> > The functionality was originally implemented by the ast driver, which
> > is being converted as well.
> > 
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > ---
> >  drivers/gpu/drm/ast/ast_main.c        | 24 +--------
> >  drivers/gpu/drm/drm_gem_vram_helper.c | 76 +++++++++++++++++++++++++++
> >  include/drm/drm_gem_vram_helper.h     | 14 +++++
> >  3 files changed, 91 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> > index b79f484e9bd2..18a0a4ce00f6 100644
> > --- a/drivers/gpu/drm/ast/ast_main.c
> > +++ b/drivers/gpu/drm/ast/ast_main.c
> > @@ -388,31 +388,9 @@ static int ast_get_dram_info(struct drm_device *dev)
> >  	return 0;
> >  }
> >  
> > -enum drm_mode_status ast_mode_config_mode_valid(struct drm_device *dev,
> > -						const struct drm_display_mode *mode)
> > -{
> > -	static const unsigned long max_bpp = 4; /* DRM_FORMAT_XRGBA8888 */
> > -
> > -	struct ast_private *ast = dev->dev_private;
> > -	unsigned long fbsize, fbpages, max_fbpages;
> > -
> > -	/* To support double buffering, a framebuffer may not
> > -	 * consume more than half of the available VRAM.
> > -	 */
> > -	max_fbpages = (ast->vram_size / 2) >> PAGE_SHIFT;
> > -
> > -	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
> > -	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
> > -
> > -	if (fbpages > max_fbpages)
> > -		return MODE_MEM;
> > -
> > -	return MODE_OK;
> > -}
> > -
> >  static const struct drm_mode_config_funcs ast_mode_funcs = {
> >  	.fb_create = drm_gem_fb_create,
> > -	.mode_valid = ast_mode_config_mode_valid,
> > +	.mode_valid = drm_vram_helper_mode_valid,
> >  	.atomic_check = drm_atomic_helper_check,
> >  	.atomic_commit = drm_atomic_helper_commit,
> >  };
> > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> > index a4863326061a..89aebd500655 100644
> > --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> > @@ -1141,3 +1141,79 @@ void drm_vram_helper_release_mm(struct drm_device *dev)
> >  	dev->vram_mm = NULL;
> >  }
> >  EXPORT_SYMBOL(drm_vram_helper_release_mm);
> > +
> > +/*
> > + * Mode-config helpers
> > + */
> > +
> > +/**
> > + * drm_vram_helper_mode_valid_internal - Tests if a display mode's
> > + *	framebuffer fits into the available video memory.
> > + * @dev:	the DRM device
> > + * @mode:	the mode to test
> > + * @max_bpp:	the maximum number of bytes per pixel
> 
> Does this render correctly? I thought an empty comment line is required
> between comments and the free-form paragraphs ... Also usual style (in drm
> at least) is that the Returns: block is more towards the end of the text,
> and not indented.
> 
> 
> > + * Returns:
> > + *	MODE_OK the display mode is supported, or an error code of type
> > + *	enum drm_mode_status otherwise.
> > + *
> > + * This function tests if enough video memory is available using the
> > + * specified display mode. Atomic modesetting requires importing the
> > + * designated framebuffer into video memory before evicting the active
> > + * one. Hence, any framebuffer may consume at most half of the available
> > + * VRAM. Display modes that require a larger framebuffer can not be used,
> > + * even of the CRTC does support them.
> > + *
> > + * Drivers should call this function from
> > + * struct drm_mode_config_funcs.mode_valid. See drm_vram_helper_mode_valid()
> > + * for framebuffers that use at most 32-bit color depth.
> > + *
> > + * Note:
> > + *	The function can only test if the display mode is supported in
> > + *	general. If you have too many framebuffers pinned to video memory,
> > + *	a display mode may still not be usable in practice.
> > + */
> > +enum drm_mode_status
> > +drm_vram_helper_mode_valid_internal(struct drm_device *dev,
> > +				    const struct drm_display_mode *mode,
> > +				    unsigned long max_bpp)
> > +{
> > +	struct drm_vram_mm *vmm = dev->vram_mm;
> > +	unsigned long fbsize, fbpages, max_fbpages;
> > +
> > +	if (!dev->vram_mm)
> 
> This is a driver bug imo (most likely of enabling hotplug/output detection
> before the vram handling is set up), needs to be wrapped in a WARN_ON to
> catch this.
> 
> > +		return MODE_BAD;
> > +
> > +	max_fbpages = (vmm->vram_size / 2) >> PAGE_SHIFT;
> > +
> > +	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
> > +	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
> > +
> > +	if (fbpages > max_fbpages)
> > +		return MODE_MEM;
> > +
> > +	return MODE_OK;
> > +}
> > +EXPORT_SYMBOL(drm_vram_helper_mode_valid_internal);
> 
> Anyway, patch looks good (with the nits addressed one way or the other):
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > +
> > +/**
> > + * drm_vram_helper_mode_valid - Tests if a display mode's
> > + *	framebuffer fits into the available video memory.
> > + * @dev:	the DRM device
> > + * @mode:	the mode to test
> > + * Returns:
> > + *	MODE_OK the display mode is supported, or an error code of type
> > + *	enum drm_mode_status otherwise.
> > + *
> > + * This function is a variant of drm_vram_helper_mode_valid_internal()
> > + * for framebuffers that use at most 32-bit color depth. It can be plugged
> > + * directly into struct drm_mode_config_funcs.mode_valid.
> > + */
> > +enum drm_mode_status
> > +drm_vram_helper_mode_valid(struct drm_device *dev,
> > +			   const struct drm_display_mode *mode)
> > +{
> > +	static const unsigned long max_bpp = 4; /* DRM_FORMAT_XRGBA8888 */
> > +
> > +	return drm_vram_helper_mode_valid_internal(dev, mode, max_bpp);

Since you don't use the _internal() version anywhere yet ... What about
unexporting that and instead using the preferred_bpp setting to compute
valid modes? I suspect that should dtrt almost anywhere ...
-Daniel

> > +}
> > +EXPORT_SYMBOL(drm_vram_helper_mode_valid);
> > diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> > index 573e9fd109bf..e7d9144c79ad 100644
> > --- a/include/drm/drm_gem_vram_helper.h
> > +++ b/include/drm/drm_gem_vram_helper.h
> > @@ -6,6 +6,7 @@
> >  #include <drm/drm_file.h>
> >  #include <drm/drm_gem.h>
> >  #include <drm/drm_ioctl.h>
> > +#include <drm/drm_modes.h>
> >  #include <drm/ttm/ttm_bo_api.h>
> >  #include <drm/ttm/ttm_bo_driver.h>
> >  #include <drm/ttm/ttm_placement.h>
> > @@ -205,4 +206,17 @@ 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);
> >  
> > +/*
> > + * Mode-config helpers
> > + */
> > +
> > +enum drm_mode_status
> > +drm_vram_helper_mode_valid_internal(struct drm_device *dev,
> > +				    const struct drm_display_mode *mode,
> > +				    unsigned long max_bpp);
> > +
> > +enum drm_mode_status
> > +drm_vram_helper_mode_valid(struct drm_device *dev,
> > +			   const struct drm_display_mode *mode);
> > +
> >  #endif
> > -- 
> > 2.25.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] drm/vboxvideo: Implement struct drm_mode_config_funcs.mode_valid
  2020-02-01 12:27 ` [PATCH 4/4] drm/vboxvideo: " Thomas Zimmermann
@ 2020-02-03  9:53   ` Daniel Vetter
  0 siblings, 0 replies; 13+ messages in thread
From: Daniel Vetter @ 2020-02-03  9:53 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: airlied, puck.chen, dri-devel, z.liuxinliang, hdegoede,
	kong.kongxinwei, kraxel, zourongrong, sam

On Sat, Feb 01, 2020 at 01:27:44PM +0100, Thomas Zimmermann wrote:
> The implementation of struct drm_mode_config_funcs.mode_valid verifies
> that enough video memory is available for a given display mode.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/vboxvideo/vbox_mode.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c
> index 8b7f005c4d20..0883a435e62b 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_mode.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c
> @@ -826,6 +826,7 @@ static int vbox_connector_init(struct drm_device *dev,
>  
>  static const struct drm_mode_config_funcs vbox_mode_funcs = {
>  	.fb_create = drm_gem_fb_create_with_dirty,
> +	.mode_valid = drm_vram_helper_mode_valid,
>  	.atomic_check = drm_atomic_helper_check,
>  	.atomic_commit = drm_atomic_helper_commit,
>  };
> -- 
> 2.25.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] drm/vram: Add helpers to validate a display mode's memory requirements
  2020-02-03  9:53     ` Daniel Vetter
@ 2020-02-03 10:25       ` Thomas Zimmermann
  0 siblings, 0 replies; 13+ messages in thread
From: Thomas Zimmermann @ 2020-02-03 10:25 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: airlied, puck.chen, dri-devel, z.liuxinliang, hdegoede,
	kong.kongxinwei, kraxel, zourongrong, sam


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

Hi

Am 03.02.20 um 10:53 schrieb Daniel Vetter:
> On Mon, Feb 03, 2020 at 10:49:30AM +0100, Daniel Vetter wrote:
>> On Sat, Feb 01, 2020 at 01:27:41PM +0100, Thomas Zimmermann wrote:
>>> Devices with low amount of dedicated video memory may not be able
>>> to use all possible display modes, as the framebuffers may not fit
>>> into VRAM. The new helper function drm_vram_helper_mode_valid()
>>> implements a simple test to sort out all display modes that can
>>> not be used in any case. Drivers should call this function from
>>> struct drm_mode_config_funcs.mode_valid.
>>>
>>> Calling drm_vram_helper_mode_valid() works best for 32-bit framebuffers;
>>> drivers with framebuffers of different color depth can use
>>> drm_vram_helper_mode_valid_internal() instead.
>>>
>>> The functionality was originally implemented by the ast driver, which
>>> is being converted as well.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>  drivers/gpu/drm/ast/ast_main.c        | 24 +--------
>>>  drivers/gpu/drm/drm_gem_vram_helper.c | 76 +++++++++++++++++++++++++++
>>>  include/drm/drm_gem_vram_helper.h     | 14 +++++
>>>  3 files changed, 91 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
>>> index b79f484e9bd2..18a0a4ce00f6 100644
>>> --- a/drivers/gpu/drm/ast/ast_main.c
>>> +++ b/drivers/gpu/drm/ast/ast_main.c
>>> @@ -388,31 +388,9 @@ static int ast_get_dram_info(struct drm_device *dev)
>>>  	return 0;
>>>  }
>>>  
>>> -enum drm_mode_status ast_mode_config_mode_valid(struct drm_device *dev,
>>> -						const struct drm_display_mode *mode)
>>> -{
>>> -	static const unsigned long max_bpp = 4; /* DRM_FORMAT_XRGBA8888 */
>>> -
>>> -	struct ast_private *ast = dev->dev_private;
>>> -	unsigned long fbsize, fbpages, max_fbpages;
>>> -
>>> -	/* To support double buffering, a framebuffer may not
>>> -	 * consume more than half of the available VRAM.
>>> -	 */
>>> -	max_fbpages = (ast->vram_size / 2) >> PAGE_SHIFT;
>>> -
>>> -	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
>>> -	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
>>> -
>>> -	if (fbpages > max_fbpages)
>>> -		return MODE_MEM;
>>> -
>>> -	return MODE_OK;
>>> -}
>>> -
>>>  static const struct drm_mode_config_funcs ast_mode_funcs = {
>>>  	.fb_create = drm_gem_fb_create,
>>> -	.mode_valid = ast_mode_config_mode_valid,
>>> +	.mode_valid = drm_vram_helper_mode_valid,
>>>  	.atomic_check = drm_atomic_helper_check,
>>>  	.atomic_commit = drm_atomic_helper_commit,
>>>  };
>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
>>> index a4863326061a..89aebd500655 100644
>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>>> @@ -1141,3 +1141,79 @@ void drm_vram_helper_release_mm(struct drm_device *dev)
>>>  	dev->vram_mm = NULL;
>>>  }
>>>  EXPORT_SYMBOL(drm_vram_helper_release_mm);
>>> +
>>> +/*
>>> + * Mode-config helpers
>>> + */
>>> +
>>> +/**
>>> + * drm_vram_helper_mode_valid_internal - Tests if a display mode's
>>> + *	framebuffer fits into the available video memory.
>>> + * @dev:	the DRM device
>>> + * @mode:	the mode to test
>>> + * @max_bpp:	the maximum number of bytes per pixel
>>
>> Does this render correctly? I thought an empty comment line is required
>> between comments and the free-form paragraphs ... Also usual style (in drm
>> at least) is that the Returns: block is more towards the end of the text,
>> and not indented.

Will be checked and possibly fixed for v2.

>>
>>
>>> + * Returns:
>>> + *	MODE_OK the display mode is supported, or an error code of type
>>> + *	enum drm_mode_status otherwise.
>>> + *
>>> + * This function tests if enough video memory is available using the
>>> + * specified display mode. Atomic modesetting requires importing the
>>> + * designated framebuffer into video memory before evicting the active
>>> + * one. Hence, any framebuffer may consume at most half of the available
>>> + * VRAM. Display modes that require a larger framebuffer can not be used,
>>> + * even of the CRTC does support them.
>>> + *
>>> + * Drivers should call this function from
>>> + * struct drm_mode_config_funcs.mode_valid. See drm_vram_helper_mode_valid()
>>> + * for framebuffers that use at most 32-bit color depth.
>>> + *
>>> + * Note:
>>> + *	The function can only test if the display mode is supported in
>>> + *	general. If you have too many framebuffers pinned to video memory,
>>> + *	a display mode may still not be usable in practice.
>>> + */
>>> +enum drm_mode_status
>>> +drm_vram_helper_mode_valid_internal(struct drm_device *dev,
>>> +				    const struct drm_display_mode *mode,
>>> +				    unsigned long max_bpp)
>>> +{
>>> +	struct drm_vram_mm *vmm = dev->vram_mm;
>>> +	unsigned long fbsize, fbpages, max_fbpages;
>>> +
>>> +	if (!dev->vram_mm)
>>
>> This is a driver bug imo (most likely of enabling hotplug/output detection
>> before the vram handling is set up), needs to be wrapped in a WARN_ON to
>> catch this.
>>
>>> +		return MODE_BAD;
>>> +
>>> +	max_fbpages = (vmm->vram_size / 2) >> PAGE_SHIFT;
>>> +
>>> +	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
>>> +	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
>>> +
>>> +	if (fbpages > max_fbpages)
>>> +		return MODE_MEM;
>>> +
>>> +	return MODE_OK;
>>> +}
>>> +EXPORT_SYMBOL(drm_vram_helper_mode_valid_internal);
>>
>> Anyway, patch looks good (with the nits addressed one way or the other):
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> +
>>> +/**
>>> + * drm_vram_helper_mode_valid - Tests if a display mode's
>>> + *	framebuffer fits into the available video memory.
>>> + * @dev:	the DRM device
>>> + * @mode:	the mode to test
>>> + * Returns:
>>> + *	MODE_OK the display mode is supported, or an error code of type
>>> + *	enum drm_mode_status otherwise.
>>> + *
>>> + * This function is a variant of drm_vram_helper_mode_valid_internal()
>>> + * for framebuffers that use at most 32-bit color depth. It can be plugged
>>> + * directly into struct drm_mode_config_funcs.mode_valid.
>>> + */
>>> +enum drm_mode_status
>>> +drm_vram_helper_mode_valid(struct drm_device *dev,
>>> +			   const struct drm_display_mode *mode)
>>> +{
>>> +	static const unsigned long max_bpp = 4; /* DRM_FORMAT_XRGBA8888 */
>>> +
>>> +	return drm_vram_helper_mode_valid_internal(dev, mode, max_bpp);
> 
> Since you don't use the _internal() version anywhere yet ... What about
> unexporting that and instead using the preferred_bpp setting to compute
> valid modes? I suspect that should dtrt almost anywhere ...

Even though that might work, it's asks for problems. ast sets a
preferred depth of 24, when it actually wants 32 for most buffers. I'd
rather just unexport the _internal function until we need it and go with
the max_bpp of 4.

To me, preferred_bpp looks like a fossil from the early days of DRM. (?)
If we want to do something meaningful to auto-detect max_bpp, we should
walk over primary planes and compute the value from the planes' formats
arrays. But for now, the result will always be 4, so there's really no need.

Best regard
Thomas

> -Daniel
> 
>>> +}
>>> +EXPORT_SYMBOL(drm_vram_helper_mode_valid);
>>> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
>>> index 573e9fd109bf..e7d9144c79ad 100644
>>> --- a/include/drm/drm_gem_vram_helper.h
>>> +++ b/include/drm/drm_gem_vram_helper.h
>>> @@ -6,6 +6,7 @@
>>>  #include <drm/drm_file.h>
>>>  #include <drm/drm_gem.h>
>>>  #include <drm/drm_ioctl.h>
>>> +#include <drm/drm_modes.h>
>>>  #include <drm/ttm/ttm_bo_api.h>
>>>  #include <drm/ttm/ttm_bo_driver.h>
>>>  #include <drm/ttm/ttm_placement.h>
>>> @@ -205,4 +206,17 @@ 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);
>>>  
>>> +/*
>>> + * Mode-config helpers
>>> + */
>>> +
>>> +enum drm_mode_status
>>> +drm_vram_helper_mode_valid_internal(struct drm_device *dev,
>>> +				    const struct drm_display_mode *mode,
>>> +				    unsigned long max_bpp);
>>> +
>>> +enum drm_mode_status
>>> +drm_vram_helper_mode_valid(struct drm_device *dev,
>>> +			   const struct drm_display_mode *mode);
>>> +
>>>  #endif
>>> -- 
>>> 2.25.0
>>>
>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
> 

-- 
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: 488 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] drm/bochs: Implement struct drm_mode_config_funcs.mode_valid
  2020-02-03  8:35     ` Thomas Zimmermann
@ 2020-02-03 10:25       ` Gerd Hoffmann
  0 siblings, 0 replies; 13+ messages in thread
From: Gerd Hoffmann @ 2020-02-03 10:25 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: airlied, puck.chen, dri-devel, hdegoede, kong.kongxinwei,
	zourongrong, sam

On Mon, Feb 03, 2020 at 09:35:54AM +0100, Thomas Zimmermann wrote:
> Hi Gerd
> 
> Am 03.02.20 um 07:47 schrieb Gerd Hoffmann:
> > On Sat, Feb 01, 2020 at 01:27:42PM +0100, Thomas Zimmermann wrote:
> >> The implementation of struct drm_mode_config_funcs.mode_valid verifies
> >> that enough video memory is available for a given display mode.
> > 
> > There is bochs_connector_mode_valid() doing the same check,
> > you can drop it when hooking up drm_vram_helper_mode_valid.
> 
> Oh, didn't see it. That's more duplicated code to remove. :)
> 
> There's a helpful comment in bochs_connector_mode_valid(). Where shall I
> move it?

I'd say it can be dropped.  linux kernel source code isn't exactly the
place where you expect documentation about configuring qemu ;)

cheers,
  Gerd

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-02-03 10:26 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-01 12:27 [PATCH 0/4] drm/vram: Check display modes against available VRAM Thomas Zimmermann
2020-02-01 12:27 ` [PATCH 1/4] drm/vram: Add helpers to validate a display mode's memory requirements Thomas Zimmermann
2020-02-03  9:49   ` Daniel Vetter
2020-02-03  9:53     ` Daniel Vetter
2020-02-03 10:25       ` Thomas Zimmermann
2020-02-01 12:27 ` [PATCH 2/4] drm/bochs: Implement struct drm_mode_config_funcs.mode_valid Thomas Zimmermann
2020-02-03  6:47   ` Gerd Hoffmann
2020-02-03  8:35     ` Thomas Zimmermann
2020-02-03 10:25       ` Gerd Hoffmann
2020-02-01 12:27 ` [PATCH 3/4] drm/hibmc: " Thomas Zimmermann
2020-02-03  9:50   ` Daniel Vetter
2020-02-01 12:27 ` [PATCH 4/4] drm/vboxvideo: " Thomas Zimmermann
2020-02-03  9:53   ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.