Re: [PATCH 8/9] drm/gem-shmem: Implement fbdev dumb buffer and mmap helpers
On 3/9/22 09:47, Thomas Zimmermann wrote:
[snip]
>>>
>>> +static const struct drm_gem_object_funcs drm_gem_shmem_funcs_fbdev = {
>>> + .free = drm_gem_shmem_object_free,
>>> + .print_info = drm_gem_shmem_object_print_info,
>>> + .pin = drm_gem_shmem_object_pin,
>>> + .unpin = drm_gem_shmem_object_unpin,
>>> + .get_sg_table = drm_gem_shmem_object_get_sg_table,
>>> + .vmap = drm_gem_shmem_object_vmap,
>>> + .vunmap = drm_gem_shmem_object_vunmap,
>>> + .mmap = drm_gem_shmem_object_mmap_fbdev,
>>> + .vm_ops = _gem_shmem_vm_ops_fbdev,
>>
>> The drm_gem_shmem_funcs_fbdev is the same than drm_gem_shmem_funcs but
>> .mmap and .vm_ops callbacks. Maybe adding a macro to declare these two
>> struct drm_gem_object_funcs could make easier to maintain it long term ?
>
> I see you point, but I'm undecided about this. Such macros also add some
> amount of obfuscation. I'm not sure if it's worth it for an internal
> constant. And since the fbdev buffer is never exported, we could remove
> some of the callbacks. AFAICT we never call pin, unpin or get_sg_table.
>
Yeah, that's true too. It was just a suggestion, I'm OK with patch as is.
> Best regards
> Thomas
>
>>
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 8/9] drm/gem-shmem: Implement fbdev dumb buffer and mmap helpers
Hi
Am 08.03.22 um 20:29 schrieb Javier Martinez Canillas:
On 3/3/22 21:58, Thomas Zimmermann wrote:
Implement struct drm_driver.dumb_create_fbdev for GEM SHMEM. The
created buffer object returned by this function implements deferred
I/O for its mmap operation.
Add this feature to a number of drivers that use GEM SHMEM helpers
as shadow planes over their regular video memory. The new macro
DRM_GEM_SHMEM_DRIVER_OPS_WITH_SHADOW_PLANES sets the regular GEM
functions and dumb_create_fbdev in struct drm_driver. Fbdev emulation
on these drivers will now mmap the GEM SHMEM pages directly with
deferred I/O without an intermediate shadow buffer.
Signed-off-by: Thomas Zimmermann
---
[snip]
@@ -49,8 +50,20 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs
= {
.vm_ops = _gem_shmem_vm_ops,
};
+static const struct drm_gem_object_funcs drm_gem_shmem_funcs_fbdev = {
+ .free = drm_gem_shmem_object_free,
+ .print_info = drm_gem_shmem_object_print_info,
+ .pin = drm_gem_shmem_object_pin,
+ .unpin = drm_gem_shmem_object_unpin,
+ .get_sg_table = drm_gem_shmem_object_get_sg_table,
+ .vmap = drm_gem_shmem_object_vmap,
+ .vunmap = drm_gem_shmem_object_vunmap,
+ .mmap = drm_gem_shmem_object_mmap_fbdev,
+ .vm_ops = _gem_shmem_vm_ops_fbdev,
The drm_gem_shmem_funcs_fbdev is the same than drm_gem_shmem_funcs but
.mmap and .vm_ops callbacks. Maybe adding a macro to declare these two
struct drm_gem_object_funcs could make easier to maintain it long term ?
I see you point, but I'm undecided about this. Such macros also add some
amount of obfuscation. I'm not sure if it's worth it for an internal
constant. And since the fbdev buffer is never exported, we could remove
some of the callbacks. AFAICT we never call pin, unpin or get_sg_table.
Best regards
Thomas
+};
+
static struct drm_gem_shmem_object *
-__drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
+__drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private, bool
fbdev)
{
struct drm_gem_shmem_object *shmem;
struct drm_gem_object *obj;
@@ -70,8 +83,12 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size,
bool private)
obj = >base;
}
- if (!obj->funcs)
- obj->funcs = _gem_shmem_funcs;
+ if (!obj->funcs) {
+ if (fbdev)
Same question than in patch #6, maybe
if (defined(CONFIG_DRM_FBDEV_EMULATION) && fbdev) ?
[snip]
+ */
+int drm_gem_shmem_dumb_create_fbdev(struct drm_file *file, struct drm_device
*dev,
+ struct drm_mode_create_dumb *args)
+{
+#if defined(CONFIG_DRM_FBDEV_EMULATION)
+ return __drm_gem_shmem_dumb_create(file, dev, true, args);
+#else
+ return -ENOSYS;
+#endif
+}
I believe the correct errno code here should be -ENOTSUPP.
[snip]
+ /* We don't use vmf->pgoff since that has the fake offset */
+ page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
I see this (vmf->address - vma->vm_start) calculation in many places of
this series. Maybe we could add a macro to calculate the offset instead ?
Reviewed-by: Javier Martinez Canillas
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 8/9] drm/gem-shmem: Implement fbdev dumb buffer and mmap helpers
On 3/3/22 21:58, Thomas Zimmermann wrote:
> Implement struct drm_driver.dumb_create_fbdev for GEM SHMEM. The
> created buffer object returned by this function implements deferred
> I/O for its mmap operation.
>
> Add this feature to a number of drivers that use GEM SHMEM helpers
> as shadow planes over their regular video memory. The new macro
> DRM_GEM_SHMEM_DRIVER_OPS_WITH_SHADOW_PLANES sets the regular GEM
> functions and dumb_create_fbdev in struct drm_driver. Fbdev emulation
> on these drivers will now mmap the GEM SHMEM pages directly with
> deferred I/O without an intermediate shadow buffer.
>
> Signed-off-by: Thomas Zimmermann
> ---
[snip]
> @@ -49,8 +50,20 @@ static const struct drm_gem_object_funcs
> drm_gem_shmem_funcs = {
> .vm_ops = _gem_shmem_vm_ops,
> };
>
> +static const struct drm_gem_object_funcs drm_gem_shmem_funcs_fbdev = {
> + .free = drm_gem_shmem_object_free,
> + .print_info = drm_gem_shmem_object_print_info,
> + .pin = drm_gem_shmem_object_pin,
> + .unpin = drm_gem_shmem_object_unpin,
> + .get_sg_table = drm_gem_shmem_object_get_sg_table,
> + .vmap = drm_gem_shmem_object_vmap,
> + .vunmap = drm_gem_shmem_object_vunmap,
> + .mmap = drm_gem_shmem_object_mmap_fbdev,
> + .vm_ops = _gem_shmem_vm_ops_fbdev,
The drm_gem_shmem_funcs_fbdev is the same than drm_gem_shmem_funcs but
.mmap and .vm_ops callbacks. Maybe adding a macro to declare these two
struct drm_gem_object_funcs could make easier to maintain it long term ?
> +};
> +
> static struct drm_gem_shmem_object *
> -__drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
> +__drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private,
> bool fbdev)
> {
> struct drm_gem_shmem_object *shmem;
> struct drm_gem_object *obj;
> @@ -70,8 +83,12 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t
> size, bool private)
> obj = >base;
> }
>
> - if (!obj->funcs)
> - obj->funcs = _gem_shmem_funcs;
> + if (!obj->funcs) {
> + if (fbdev)
Same question than in patch #6, maybe
if (defined(CONFIG_DRM_FBDEV_EMULATION) && fbdev) ?
[snip]
> + */
> +int drm_gem_shmem_dumb_create_fbdev(struct drm_file *file, struct drm_device
> *dev,
> + struct drm_mode_create_dumb *args)
> +{
> +#if defined(CONFIG_DRM_FBDEV_EMULATION)
> + return __drm_gem_shmem_dumb_create(file, dev, true, args);
> +#else
> + return -ENOSYS;
> +#endif
> +}
I believe the correct errno code here should be -ENOTSUPP.
[snip]
> + /* We don't use vmf->pgoff since that has the fake offset */
> + page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
I see this (vmf->address - vma->vm_start) calculation in many places of
this series. Maybe we could add a macro to calculate the offset instead ?
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
[PATCH 8/9] drm/gem-shmem: Implement fbdev dumb buffer and mmap helpers
Implement struct drm_driver.dumb_create_fbdev for GEM SHMEM. The
created buffer object returned by this function implements deferred
I/O for its mmap operation.
Add this feature to a number of drivers that use GEM SHMEM helpers
as shadow planes over their regular video memory. The new macro
DRM_GEM_SHMEM_DRIVER_OPS_WITH_SHADOW_PLANES sets the regular GEM
functions and dumb_create_fbdev in struct drm_driver. Fbdev emulation
on these drivers will now mmap the GEM SHMEM pages directly with
deferred I/O without an intermediate shadow buffer.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 197 +---
drivers/gpu/drm/gud/gud_drv.c | 2 +-
drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 2 +-
drivers/gpu/drm/mgag200/mgag200_drv.c | 2 +-
drivers/gpu/drm/tiny/cirrus.c | 2 +-
drivers/gpu/drm/tiny/gm12u320.c | 2 +-
drivers/gpu/drm/tiny/simpledrm.c| 2 +-
drivers/gpu/drm/udl/udl_drv.c | 2 +-
drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
include/drm/drm_gem_shmem_helper.h | 63 +++-
10 files changed, 240 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 8ad0e02991ca..ab7cb7d896c3 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -49,8 +50,20 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs
= {
.vm_ops = _gem_shmem_vm_ops,
};
+static const struct drm_gem_object_funcs drm_gem_shmem_funcs_fbdev = {
+ .free = drm_gem_shmem_object_free,
+ .print_info = drm_gem_shmem_object_print_info,
+ .pin = drm_gem_shmem_object_pin,
+ .unpin = drm_gem_shmem_object_unpin,
+ .get_sg_table = drm_gem_shmem_object_get_sg_table,
+ .vmap = drm_gem_shmem_object_vmap,
+ .vunmap = drm_gem_shmem_object_vunmap,
+ .mmap = drm_gem_shmem_object_mmap_fbdev,
+ .vm_ops = _gem_shmem_vm_ops_fbdev,
+};
+
static struct drm_gem_shmem_object *
-__drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
+__drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private, bool
fbdev)
{
struct drm_gem_shmem_object *shmem;
struct drm_gem_object *obj;
@@ -70,8 +83,12 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size,
bool private)
obj = >base;
}
- if (!obj->funcs)
- obj->funcs = _gem_shmem_funcs;
+ if (!obj->funcs) {
+ if (fbdev)
+ obj->funcs = _gem_shmem_funcs_fbdev;
+ else
+ obj->funcs = _gem_shmem_funcs;
+ }
if (private) {
drm_gem_private_object_init(dev, obj, size);
@@ -124,7 +141,7 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size,
bool private)
*/
struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev,
size_t size)
{
- return __drm_gem_shmem_create(dev, size, false);
+ return __drm_gem_shmem_create(dev, size, false, false);
}
EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
@@ -415,12 +432,12 @@ EXPORT_SYMBOL(drm_gem_shmem_vunmap);
static struct drm_gem_shmem_object *
drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
struct drm_device *dev, size_t size,
-uint32_t *handle)
+bool fbdev, uint32_t *handle)
{
struct drm_gem_shmem_object *shmem;
int ret;
- shmem = drm_gem_shmem_create(dev, size);
+ shmem = __drm_gem_shmem_create(dev, size, false, fbdev);
if (IS_ERR(shmem))
return shmem;
@@ -496,6 +513,29 @@ bool drm_gem_shmem_purge(struct drm_gem_shmem_object
*shmem)
}
EXPORT_SYMBOL(drm_gem_shmem_purge);
+static int __drm_gem_shmem_dumb_create(struct drm_file *file, struct
drm_device *dev,
+ bool fbdev, struct drm_mode_create_dumb
*args)
+{
+ u32 min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+ struct drm_gem_shmem_object *shmem;
+
+ if (!args->pitch || !args->size) {
+ args->pitch = min_pitch;
+ args->size = PAGE_ALIGN(args->pitch * args->height);
+ } else {
+ /* ensure sane minimum values */
+ if (args->pitch < min_pitch)
+ args->pitch = min_pitch;
+ if (args->size < args->pitch * args->height)
+ args->size = PAGE_ALIGN(args->pitch * args->height);
+ }
+
+ shmem = drm_gem_shmem_create_with_handle(file, dev, args->size, fbdev,
+>handle);
+
+ return PTR_ERR_OR_ZERO(shmem);
+}
+
/**
* drm_gem_shmem_dumb_create - Create a dumb shmem buffer object
* @file: DRM file
