Re: [PATCH 9/9] drm/virtio: Implement dumb_create_fbdev with GEM SHMEM helpers
On 3/9/22 09:52, Thomas Zimmermann wrote:
[snip]
>>> +struct drm_gem_object *virtio_gpu_create_object_fbdev(struct drm_device
>>> *dev,
>>> + size_t size)
>>> +{
>>> + return ERR_PTR(-ENOSYS);
>>> +}
>>
>> As mentioned, I believe this should be ERR_PTR(-ENOTSUPP) instead.
>
> I've been wondering about this as well. I finally went with the rules at
> [1]. All the variants of ENOTOP/ENOTSUPP seem to be for specific use
> cases, such as a certain feature is not implemented be a specific
> interface (e.g., sockets for EOPNOTSUPP). ENOSYS is the only general
> error that indicates that an entire interface is missing. Even though
> checkpatch.pl warns that it's only for system calls.
>
> Best regards
> Thomas
>
> [1] https://www.cs.helsinki.fi/linux/linux-kernel/2002-30/1135.html
>
Thanks for the link. It would be good to have an authoritative guideline
about this in the kernel documentation (and make checkpatch.pl aware).
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 9/9] drm/virtio: Implement dumb_create_fbdev with GEM SHMEM helpers
Hi
Am 08.03.22 um 20:37 schrieb Javier Martinez Canillas:
On 3/3/22 21:58, Thomas Zimmermann wrote:
Implement struct drm_driver.dumb_create_fbdev with the helpers
provided by GEM SHMEM. Fbdev deferred I/O will now work without
an intermediate shadow buffer for mmap.
As the virtio driver replaces several of the regular GEM SHMEM
functions with its own implementation, some additional code is
necessary make fbdev optimization work. Especially, the driver
has to provide buffer-object functions for fbdev. Add the hook
struct drm_driver.gem_create_object_fbdev, which is similar to
struct drm_driver.gem_create_object and allows for the creation
of dedicated fbdev buffer objects. Wire things up within GEM
SHMEM.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 7 +++-
drivers/gpu/drm/virtio/virtgpu_drv.c| 2 +
drivers/gpu/drm/virtio/virtgpu_drv.h| 2 +
drivers/gpu/drm/virtio/virtgpu_object.c | 54 +++--
include/drm/drm_drv.h | 10 +
5 files changed, 71 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index ab7cb7d896c3..225aa17895bd 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -71,7 +71,12 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size,
bool private, bool f
size = PAGE_ALIGN(size);
- if (dev->driver->gem_create_object) {
+ if (dev->driver->gem_create_object_fbdev && fbdev) {
Same comment here to check if fbdev emulation is enabled or maybe is not
worht it ? But I think this hints the compiler to optimize the if branch.
[snip]
+}
+#else
+struct drm_gem_object *virtio_gpu_create_object_fbdev(struct drm_device *dev,
+ size_t size)
+{
+ return ERR_PTR(-ENOSYS);
+}
As mentioned, I believe this should be ERR_PTR(-ENOTSUPP) instead.
I've been wondering about this as well. I finally went with the rules at
[1]. All the variants of ENOTOP/ENOTSUPP seem to be for specific use
cases, such as a certain feature is not implemented be a specific
interface (e.g., sockets for EOPNOTSUPP). ENOSYS is the only general
error that indicates that an entire interface is missing. Even though
checkpatch.pl warns that it's only for system calls.
Best regards
Thomas
[1] https://www.cs.helsinki.fi/linux/linux-kernel/2002-30/1135.html
Feel free to ignore all this nits if you consider that are not applicable.
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 9/9] drm/virtio: Implement dumb_create_fbdev with GEM SHMEM helpers
On 3/3/22 21:58, Thomas Zimmermann wrote:
> Implement struct drm_driver.dumb_create_fbdev with the helpers
> provided by GEM SHMEM. Fbdev deferred I/O will now work without
> an intermediate shadow buffer for mmap.
>
> As the virtio driver replaces several of the regular GEM SHMEM
> functions with its own implementation, some additional code is
> necessary make fbdev optimization work. Especially, the driver
> has to provide buffer-object functions for fbdev. Add the hook
> struct drm_driver.gem_create_object_fbdev, which is similar to
> struct drm_driver.gem_create_object and allows for the creation
> of dedicated fbdev buffer objects. Wire things up within GEM
> SHMEM.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c | 7 +++-
> drivers/gpu/drm/virtio/virtgpu_drv.c| 2 +
> drivers/gpu/drm/virtio/virtgpu_drv.h| 2 +
> drivers/gpu/drm/virtio/virtgpu_object.c | 54 +++--
> include/drm/drm_drv.h | 10 +
> 5 files changed, 71 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index ab7cb7d896c3..225aa17895bd 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -71,7 +71,12 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t
> size, bool private, bool f
>
> size = PAGE_ALIGN(size);
>
> - if (dev->driver->gem_create_object) {
> + if (dev->driver->gem_create_object_fbdev && fbdev) {
Same comment here to check if fbdev emulation is enabled or maybe is not
worht it ? But I think this hints the compiler to optimize the if branch.
[snip]
> +}
> +#else
> +struct drm_gem_object *virtio_gpu_create_object_fbdev(struct drm_device *dev,
> + size_t size)
> +{
> + return ERR_PTR(-ENOSYS);
> +}
As mentioned, I believe this should be ERR_PTR(-ENOTSUPP) instead.
Feel free to ignore all this nits if you consider that are not applicable.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
