GEM helper libraries use struct drm_driver.gem_create_object to let drivers override GEM object allocation. On failure, the call returns NULL.
Change the semantics to make the calls return a pointer-encoded error. This aligns the callback with its callers. Fixes the ingenic driver, which already returns an error pointer.
Also update the callers to handle the involved types more strictly.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- There is an alternative patch at [1] that updates the value returned by ingenics' gem_create_object to NULL. Fixing the interface to return an errno code is more consistent with the rest of the GEM functions.
[1] https://lore.kernel.org/dri-devel/20211118111522.GD1147@kili/ --- drivers/gpu/drm/drm_gem_cma_helper.c | 17 ++++++++++------- drivers/gpu/drm/drm_gem_shmem_helper.c | 17 ++++++++++------- drivers/gpu/drm/drm_gem_vram_helper.c | 4 ++-- drivers/gpu/drm/lima/lima_gem.c | 2 +- drivers/gpu/drm/panfrost/panfrost_gem.c | 2 +- drivers/gpu/drm/v3d/v3d_bo.c | 4 ++-- drivers/gpu/drm/vc4/vc4_bo.c | 2 +- drivers/gpu/drm/vgem/vgem_drv.c | 2 +- drivers/gpu/drm/virtio/virtgpu_object.c | 2 +- include/drm/drm_drv.h | 5 +++-- 10 files changed, 32 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 7d4895de9e0d..cefd0cbf9deb 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -67,18 +67,21 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size, bool private) struct drm_gem_object *gem_obj; int ret = 0;
- if (drm->driver->gem_create_object) + if (drm->driver->gem_create_object) { gem_obj = drm->driver->gem_create_object(drm, size); - else - gem_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL); - if (!gem_obj) - return ERR_PTR(-ENOMEM); + if (IS_ERR(gem_obj)) + return ERR_CAST(gem_obj); + cma_obj = to_drm_gem_cma_obj(gem_obj); + } else { + cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL); + if (!cma_obj) + return ERR_PTR(-ENOMEM); + gem_obj = &cma_obj->base; + }
if (!gem_obj->funcs) gem_obj->funcs = &drm_gem_cma_default_funcs;
- cma_obj = container_of(gem_obj, struct drm_gem_cma_object, base); - if (private) { drm_gem_private_object_init(drm, gem_obj, size);
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 0eeda1012364..7915047cb041 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -56,14 +56,17 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
size = PAGE_ALIGN(size);
- if (dev->driver->gem_create_object) + if (dev->driver->gem_create_object) { obj = dev->driver->gem_create_object(dev, size); - else - obj = kzalloc(sizeof(*shmem), GFP_KERNEL); - if (!obj) - return ERR_PTR(-ENOMEM); - - shmem = to_drm_gem_shmem_obj(obj); + if (IS_ERR(obj)) + return ERR_CAST(obj); + shmem = to_drm_gem_shmem_obj(obj); + } else { + shmem = kzalloc(sizeof(*shmem), GFP_KERNEL); + if (!shmem) + return ERR_PTR(-ENOMEM); + obj = &shmem->base; + }
if (!obj->funcs) obj->funcs = &drm_gem_shmem_funcs; diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index bfa386b98134..3f00192215d1 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -197,8 +197,8 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev,
if (dev->driver->gem_create_object) { gem = dev->driver->gem_create_object(dev, size); - if (!gem) - return ERR_PTR(-ENOMEM); + if (IS_ERR(gem)) + return ERR_CAST(gem); gbo = drm_gem_vram_of_gem(gem); } else { gbo = kzalloc(sizeof(*gbo), GFP_KERNEL); diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c index 2723d333c608..f9a9198ef198 100644 --- a/drivers/gpu/drm/lima/lima_gem.c +++ b/drivers/gpu/drm/lima/lima_gem.c @@ -221,7 +221,7 @@ struct drm_gem_object *lima_gem_create_object(struct drm_device *dev, size_t siz
bo = kzalloc(sizeof(*bo), GFP_KERNEL); if (!bo) - return NULL; + return ERR_PTR(-ENOMEM);
mutex_init(&bo->lock); INIT_LIST_HEAD(&bo->va); diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c index 6d9bdb9180cb..ead65f5fa2bc 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c @@ -223,7 +223,7 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t
obj = kzalloc(sizeof(*obj), GFP_KERNEL); if (!obj) - return NULL; + return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(&obj->mappings.list); mutex_init(&obj->mappings.lock); diff --git a/drivers/gpu/drm/v3d/v3d_bo.c b/drivers/gpu/drm/v3d/v3d_bo.c index 0d9af62f69ad..6e3113f419f4 100644 --- a/drivers/gpu/drm/v3d/v3d_bo.c +++ b/drivers/gpu/drm/v3d/v3d_bo.c @@ -70,11 +70,11 @@ struct drm_gem_object *v3d_create_object(struct drm_device *dev, size_t size) struct drm_gem_object *obj;
if (size == 0) - return NULL; + return ERR_PTR(-EINVAL);
bo = kzalloc(sizeof(*bo), GFP_KERNEL); if (!bo) - return NULL; + return ERR_PTR(-ENOMEM); obj = &bo->base.base;
obj->funcs = &v3d_gem_funcs; diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c index 8520e440dbd1..6d1281a343e9 100644 --- a/drivers/gpu/drm/vc4/vc4_bo.c +++ b/drivers/gpu/drm/vc4/vc4_bo.c @@ -391,7 +391,7 @@ struct drm_gem_object *vc4_create_object(struct drm_device *dev, size_t size)
bo = kzalloc(sizeof(*bo), GFP_KERNEL); if (!bo) - return NULL; + return ERR_PTR(-ENOMEM);
bo->madv = VC4_MADV_WILLNEED; refcount_set(&bo->usecnt, 0); diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index a87eafa89e9f..c5e3e5457737 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -97,7 +97,7 @@ static struct drm_gem_object *vgem_gem_create_object(struct drm_device *dev, siz
obj = kzalloc(sizeof(*obj), GFP_KERNEL); if (!obj) - return NULL; + return ERR_PTR(-ENOMEM);
/* * vgem doesn't have any begin/end cpu access ioctls, therefore must use diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c index 187e10da2f17..baef2c5f2aaf 100644 --- a/drivers/gpu/drm/virtio/virtgpu_object.c +++ b/drivers/gpu/drm/virtio/virtgpu_object.c @@ -139,7 +139,7 @@ struct drm_gem_object *virtio_gpu_create_object(struct drm_device *dev,
shmem = kzalloc(sizeof(*shmem), GFP_KERNEL); if (!shmem) - return NULL; + return ERR_PTR(-ENOMEM);
dshmem = &shmem->base.base; dshmem->base.funcs = &virtio_gpu_shmem_funcs; diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index da0c836fe8e1..f6159acb8856 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -291,8 +291,9 @@ struct drm_driver { /** * @gem_create_object: constructor for gem objects * - * Hook for allocating the GEM object struct, for use by the CMA and - * SHMEM GEM helpers. + * Hook for allocating the GEM object struct, for use by the CMA + * and SHMEM GEM helpers. Returns a GEM object on success, or an + * ERR_PTR()-encoded error code otherwise. */ struct drm_gem_object *(*gem_create_object)(struct drm_device *dev, size_t size); -- 2.34.0
On Tue, Nov 30, 2021 at 10:52:55AM +0100, Thomas Zimmermann wrote:
GEM helper libraries use struct drm_driver.gem_create_object to let drivers override GEM object allocation. On failure, the call returns NULL.
Change the semantics to make the calls return a pointer-encoded error. This aligns the callback with its callers. Fixes the ingenic driver, which already returns an error pointer.
Also update the callers to handle the involved types more strictly.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
There is an alternative patch at [1] that updates the value returned by ingenics' gem_create_object to NULL. Fixing the interface to return an errno code is more consistent with the rest of the GEM functions.
[1] https://lore.kernel.org/dri-devel/20211118111522.GD1147@kili/
Acked-by: Maxime Ripard maxime@cerno.tech
Maxime
On 30/11/2021 09:52, Thomas Zimmermann wrote:
GEM helper libraries use struct drm_driver.gem_create_object to let drivers override GEM object allocation. On failure, the call returns NULL.
Change the semantics to make the calls return a pointer-encoded error. This aligns the callback with its callers. Fixes the ingenic driver, which already returns an error pointer.
Also update the callers to handle the involved types more strictly.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
Reviewed-by: Steven Price steven.price@arm.com
There is an alternative patch at [1] that updates the value returned by ingenics' gem_create_object to NULL. Fixing the interface to return an errno code is more consistent with the rest of the GEM functions.
[1] https://lore.kernel.org/dri-devel/20211118111522.GD1147@kili/
drivers/gpu/drm/drm_gem_cma_helper.c | 17 ++++++++++------- drivers/gpu/drm/drm_gem_shmem_helper.c | 17 ++++++++++------- drivers/gpu/drm/drm_gem_vram_helper.c | 4 ++-- drivers/gpu/drm/lima/lima_gem.c | 2 +- drivers/gpu/drm/panfrost/panfrost_gem.c | 2 +- drivers/gpu/drm/v3d/v3d_bo.c | 4 ++-- drivers/gpu/drm/vc4/vc4_bo.c | 2 +- drivers/gpu/drm/vgem/vgem_drv.c | 2 +- drivers/gpu/drm/virtio/virtgpu_object.c | 2 +- include/drm/drm_drv.h | 5 +++-- 10 files changed, 32 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 7d4895de9e0d..cefd0cbf9deb 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -67,18 +67,21 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size, bool private) struct drm_gem_object *gem_obj; int ret = 0;
- if (drm->driver->gem_create_object)
- if (drm->driver->gem_create_object) { gem_obj = drm->driver->gem_create_object(drm, size);
- else
gem_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL);- if (!gem_obj)
return ERR_PTR(-ENOMEM);
if (IS_ERR(gem_obj))return ERR_CAST(gem_obj);cma_obj = to_drm_gem_cma_obj(gem_obj);} else {
cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL);if (!cma_obj)return ERR_PTR(-ENOMEM);gem_obj = &cma_obj->base;}
if (!gem_obj->funcs) gem_obj->funcs = &drm_gem_cma_default_funcs;
- cma_obj = container_of(gem_obj, struct drm_gem_cma_object, base);
- if (private) { drm_gem_private_object_init(drm, gem_obj, size);
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 0eeda1012364..7915047cb041 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -56,14 +56,17 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
size = PAGE_ALIGN(size);
- if (dev->driver->gem_create_object)
- if (dev->driver->gem_create_object) { obj = dev->driver->gem_create_object(dev, size);
- else
obj = kzalloc(sizeof(*shmem), GFP_KERNEL);- if (!obj)
return ERR_PTR(-ENOMEM);- shmem = to_drm_gem_shmem_obj(obj);
if (IS_ERR(obj))return ERR_CAST(obj);shmem = to_drm_gem_shmem_obj(obj);} else {
shmem = kzalloc(sizeof(*shmem), GFP_KERNEL);if (!shmem)return ERR_PTR(-ENOMEM);obj = &shmem->base;}
if (!obj->funcs) obj->funcs = &drm_gem_shmem_funcs;
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index bfa386b98134..3f00192215d1 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -197,8 +197,8 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev,
if (dev->driver->gem_create_object) { gem = dev->driver->gem_create_object(dev, size);
if (!gem)return ERR_PTR(-ENOMEM);
if (IS_ERR(gem))return ERR_CAST(gem);diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c index 2723d333c608..f9a9198ef198 100644 --- a/drivers/gpu/drm/lima/lima_gem.c +++ b/drivers/gpu/drm/lima/lima_gem.c @@ -221,7 +221,7 @@ struct drm_gem_object *lima_gem_create_object(struct drm_device *dev, size_t siz
bo = kzalloc(sizeof(*bo), GFP_KERNEL); if (!bo)
return NULL;
return ERR_PTR(-ENOMEM);mutex_init(&bo->lock); INIT_LIST_HEAD(&bo->va);
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c index 6d9bdb9180cb..ead65f5fa2bc 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c @@ -223,7 +223,7 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t
obj = kzalloc(sizeof(*obj), GFP_KERNEL); if (!obj)
return NULL;
return ERR_PTR(-ENOMEM);INIT_LIST_HEAD(&obj->mappings.list); mutex_init(&obj->mappings.lock);
diff --git a/drivers/gpu/drm/v3d/v3d_bo.c b/drivers/gpu/drm/v3d/v3d_bo.c index 0d9af62f69ad..6e3113f419f4 100644 --- a/drivers/gpu/drm/v3d/v3d_bo.c +++ b/drivers/gpu/drm/v3d/v3d_bo.c @@ -70,11 +70,11 @@ struct drm_gem_object *v3d_create_object(struct drm_device *dev, size_t size) struct drm_gem_object *obj;
if (size == 0)
return NULL;
return ERR_PTR(-EINVAL);bo = kzalloc(sizeof(*bo), GFP_KERNEL); if (!bo)
return NULL;
return ERR_PTR(-ENOMEM);obj = &bo->base.base;
obj->funcs = &v3d_gem_funcs;
diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c index 8520e440dbd1..6d1281a343e9 100644 --- a/drivers/gpu/drm/vc4/vc4_bo.c +++ b/drivers/gpu/drm/vc4/vc4_bo.c @@ -391,7 +391,7 @@ struct drm_gem_object *vc4_create_object(struct drm_device *dev, size_t size)
bo = kzalloc(sizeof(*bo), GFP_KERNEL); if (!bo)
return NULL;
return ERR_PTR(-ENOMEM);bo->madv = VC4_MADV_WILLNEED; refcount_set(&bo->usecnt, 0);
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index a87eafa89e9f..c5e3e5457737 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -97,7 +97,7 @@ static struct drm_gem_object *vgem_gem_create_object(struct drm_device *dev, siz
obj = kzalloc(sizeof(*obj), GFP_KERNEL); if (!obj)
return NULL;
return ERR_PTR(-ENOMEM);/*
- vgem doesn't have any begin/end cpu access ioctls, therefore must use
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c index 187e10da2f17..baef2c5f2aaf 100644 --- a/drivers/gpu/drm/virtio/virtgpu_object.c +++ b/drivers/gpu/drm/virtio/virtgpu_object.c @@ -139,7 +139,7 @@ struct drm_gem_object *virtio_gpu_create_object(struct drm_device *dev,
shmem = kzalloc(sizeof(*shmem), GFP_KERNEL); if (!shmem)
return NULL;
return ERR_PTR(-ENOMEM);dshmem = &shmem->base.base; dshmem->base.funcs = &virtio_gpu_shmem_funcs;
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index da0c836fe8e1..f6159acb8856 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -291,8 +291,9 @@ struct drm_driver { /** * @gem_create_object: constructor for gem objects *
* Hook for allocating the GEM object struct, for use by the CMA and* SHMEM GEM helpers.
* Hook for allocating the GEM object struct, for use by the CMA* and SHMEM GEM helpers. Returns a GEM object on success, or an* ERR_PTR()-encoded error code otherwise.-- 2.34.0
On Tue, Nov 30, 2021 at 10:52:55AM +0100, Thomas Zimmermann wrote:
GEM helper libraries use struct drm_driver.gem_create_object to let drivers override GEM object allocation. On failure, the call returns NULL.
Change the semantics to make the calls return a pointer-encoded error. This aligns the callback with its callers. Fixes the ingenic driver, which already returns an error pointer.
Also update the callers to handle the involved types more strictly.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
There is an alternative patch at [1] that updates the value returned by ingenics' gem_create_object to NULL. Fixing the interface to return an errno code is more consistent with the rest of the GEM functions.
[1] https://lore.kernel.org/dri-devel/20211118111522.GD1147@kili/
My fix was already applied and backported to -stable etc... Your patch is not developed against a current tree so you broke it.
That's the tricky thing with changing the API because say people wrote their code last week where returning NULL was correct. When they submit their driver upstream, everything will merge and build but it will break at runtime.
For now, it's only vc4_create_object() which is broken.
regards, dan carpenter
Hi
Am 06.12.21 um 11:42 schrieb Dan Carpenter:
On Tue, Nov 30, 2021 at 10:52:55AM +0100, Thomas Zimmermann wrote:
GEM helper libraries use struct drm_driver.gem_create_object to let drivers override GEM object allocation. On failure, the call returns NULL.
Change the semantics to make the calls return a pointer-encoded error. This aligns the callback with its callers. Fixes the ingenic driver, which already returns an error pointer.
Also update the callers to handle the involved types more strictly.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
There is an alternative patch at [1] that updates the value returned by ingenics' gem_create_object to NULL. Fixing the interface to return an errno code is more consistent with the rest of the GEM functions.
[1] https://lore.kernel.org/dri-devel/20211118111522.GD1147@kili/
My fix was already applied and backported to -stable etc... Your patch is not developed against a current tree so you broke it.
Do you have a specific link? I just checked the stable tree at [1] and there no trace of your patch.
Patches for DRM should go through through DRM trees; drm-misc-fixes in this case. Exceptions should at least be announce on dri-devel. Neither is the case here.
Best regards Thomas
[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/driver...
That's the tricky thing with changing the API because say people wrote their code last week where returning NULL was correct. When they submit their driver upstream, everything will merge and build but it will break at runtime.
For now, it's only vc4_create_object() which is broken.
regards, dan carpenter
On Mon, Dec 06, 2021 at 12:16:24PM +0100, Thomas Zimmermann wrote:
Hi
Am 06.12.21 um 11:42 schrieb Dan Carpenter:
On Tue, Nov 30, 2021 at 10:52:55AM +0100, Thomas Zimmermann wrote:
GEM helper libraries use struct drm_driver.gem_create_object to let drivers override GEM object allocation. On failure, the call returns NULL.
Change the semantics to make the calls return a pointer-encoded error. This aligns the callback with its callers. Fixes the ingenic driver, which already returns an error pointer.
Also update the callers to handle the involved types more strictly.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
There is an alternative patch at [1] that updates the value returned by ingenics' gem_create_object to NULL. Fixing the interface to return an errno code is more consistent with the rest of the GEM functions.
[1] https://lore.kernel.org/dri-devel/20211118111522.GD1147@kili/
My fix was already applied and backported to -stable etc... Your patch is not developed against a current tree so you broke it.
Do you have a specific link? I just checked the stable tree at [1] and there no trace of your patch.
It's in 5.15.6 and probably all the other supported -stable trees.
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/driver...
Patches for DRM should go through through DRM trees; drm-misc-fixes in this case. Exceptions should at least be announce on dri-devel. Neither is the case here.
Yeah. That's a good question. I don't know, because I just work against linux-next...
regards, dan carpenter
Hi
Am 06.12.21 um 15:40 schrieb Dan Carpenter:
On Mon, Dec 06, 2021 at 12:16:24PM +0100, Thomas Zimmermann wrote:
Hi
Am 06.12.21 um 11:42 schrieb Dan Carpenter:
On Tue, Nov 30, 2021 at 10:52:55AM +0100, Thomas Zimmermann wrote:
GEM helper libraries use struct drm_driver.gem_create_object to let drivers override GEM object allocation. On failure, the call returns NULL.
Change the semantics to make the calls return a pointer-encoded error. This aligns the callback with its callers. Fixes the ingenic driver, which already returns an error pointer.
Also update the callers to handle the involved types more strictly.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
There is an alternative patch at [1] that updates the value returned by ingenics' gem_create_object to NULL. Fixing the interface to return an errno code is more consistent with the rest of the GEM functions.
[1] https://lore.kernel.org/dri-devel/20211118111522.GD1147@kili/
My fix was already applied and backported to -stable etc... Your patch is not developed against a current tree so you broke it.
Do you have a specific link? I just checked the stable tree at [1] and there no trace of your patch.
It's in 5.15.6 and probably all the other supported -stable trees.
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/driver...
I'm not sure that I understand the problem.
The URL points to vc4, but my link was to your patch for ingenic. vc4 is being updated here as well to ERR_PTR. The ingenic patch never made it into any tree. It actually was the reason to fix the interface.
When the current patch makes it through the trees, it should fix all the affected drivers.
Best regards Thomas
Patches for DRM should go through through DRM trees; drm-misc-fixes in this case. Exceptions should at least be announce on dri-devel. Neither is the case here.
Yeah. That's a good question. I don't know, because I just work against linux-next...
regards, dan carpenter
I appologize. This thread has been really frustrating. I got mixed up because I recently sent patches for ingenic and vc4. Also we are working against different trees so maybe that is part of the problem?
I'm looking at today's linux-next. Your patch has been applied.
Yes. You updated all the drivers. But somehow the vc4 chunk from your patch was dropped. It was *NOT* dropped by Stephen Rothwell. It got dropped earlier. I am including the `git format-patch -1 <hash>` output from the commit.
regards, dan carpenter
From 4ff22f487f8c26b99cbe1678344595734c001a39 Mon Sep 17 00:00:00 2001
From: Thomas Zimmermann tzimmermann@suse.de Date: Tue, 30 Nov 2021 10:52:55 +0100 Subject: [PATCH] drm: Return error codes from struct drm_driver.gem_create_object
GEM helper libraries use struct drm_driver.gem_create_object to let drivers override GEM object allocation. On failure, the call returns NULL.
Change the semantics to make the calls return a pointer-encoded error. This aligns the callback with its callers. Fixes the ingenic driver, which already returns an error pointer.
Also update the callers to handle the involved types more strictly.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Reviewed-by: Steven Price steven.price@arm.com Acked-by: Maxime Ripard maxime@cerno.tech Link: https://patchwork.freedesktop.org/patch/msgid/20211130095255.26710-1-tzimmer... --- drivers/gpu/drm/drm_gem_cma_helper.c | 17 ++++++++++------- drivers/gpu/drm/drm_gem_shmem_helper.c | 17 ++++++++++------- drivers/gpu/drm/drm_gem_vram_helper.c | 4 ++-- drivers/gpu/drm/lima/lima_gem.c | 2 +- drivers/gpu/drm/panfrost/panfrost_gem.c | 2 +- drivers/gpu/drm/v3d/v3d_bo.c | 4 ++-- drivers/gpu/drm/vgem/vgem_drv.c | 2 +- drivers/gpu/drm/virtio/virtgpu_object.c | 2 +- include/drm/drm_drv.h | 5 +++-- 9 files changed, 31 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 5f42e44b2ab3..6f18f143dd30 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -67,18 +67,21 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size, bool private) struct drm_gem_object *gem_obj; int ret = 0;
- if (drm->driver->gem_create_object) + if (drm->driver->gem_create_object) { gem_obj = drm->driver->gem_create_object(drm, size); - else - gem_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL); - if (!gem_obj) - return ERR_PTR(-ENOMEM); + if (IS_ERR(gem_obj)) + return ERR_CAST(gem_obj); + cma_obj = to_drm_gem_cma_obj(gem_obj); + } else { + cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL); + if (!cma_obj) + return ERR_PTR(-ENOMEM); + gem_obj = &cma_obj->base; + }
if (!gem_obj->funcs) gem_obj->funcs = &drm_gem_cma_default_funcs;
- cma_obj = container_of(gem_obj, struct drm_gem_cma_object, base); - if (private) { drm_gem_private_object_init(drm, gem_obj, size);
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 0eeda1012364..7915047cb041 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -56,14 +56,17 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
size = PAGE_ALIGN(size);
- if (dev->driver->gem_create_object) + if (dev->driver->gem_create_object) { obj = dev->driver->gem_create_object(dev, size); - else - obj = kzalloc(sizeof(*shmem), GFP_KERNEL); - if (!obj) - return ERR_PTR(-ENOMEM); - - shmem = to_drm_gem_shmem_obj(obj); + if (IS_ERR(obj)) + return ERR_CAST(obj); + shmem = to_drm_gem_shmem_obj(obj); + } else { + shmem = kzalloc(sizeof(*shmem), GFP_KERNEL); + if (!shmem) + return ERR_PTR(-ENOMEM); + obj = &shmem->base; + }
if (!obj->funcs) obj->funcs = &drm_gem_shmem_funcs; diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index bfa386b98134..3f00192215d1 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -197,8 +197,8 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev,
if (dev->driver->gem_create_object) { gem = dev->driver->gem_create_object(dev, size); - if (!gem) - return ERR_PTR(-ENOMEM); + if (IS_ERR(gem)) + return ERR_CAST(gem); gbo = drm_gem_vram_of_gem(gem); } else { gbo = kzalloc(sizeof(*gbo), GFP_KERNEL); diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c index 2723d333c608..f9a9198ef198 100644 --- a/drivers/gpu/drm/lima/lima_gem.c +++ b/drivers/gpu/drm/lima/lima_gem.c @@ -221,7 +221,7 @@ struct drm_gem_object *lima_gem_create_object(struct drm_device *dev, size_t siz
bo = kzalloc(sizeof(*bo), GFP_KERNEL); if (!bo) - return NULL; + return ERR_PTR(-ENOMEM);
mutex_init(&bo->lock); INIT_LIST_HEAD(&bo->va); diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c index 6d9bdb9180cb..ead65f5fa2bc 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c @@ -223,7 +223,7 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t
obj = kzalloc(sizeof(*obj), GFP_KERNEL); if (!obj) - return NULL; + return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(&obj->mappings.list); mutex_init(&obj->mappings.lock); diff --git a/drivers/gpu/drm/v3d/v3d_bo.c b/drivers/gpu/drm/v3d/v3d_bo.c index 0d9af62f69ad..6e3113f419f4 100644 --- a/drivers/gpu/drm/v3d/v3d_bo.c +++ b/drivers/gpu/drm/v3d/v3d_bo.c @@ -70,11 +70,11 @@ struct drm_gem_object *v3d_create_object(struct drm_device *dev, size_t size) struct drm_gem_object *obj;
if (size == 0) - return NULL; + return ERR_PTR(-EINVAL);
bo = kzalloc(sizeof(*bo), GFP_KERNEL); if (!bo) - return NULL; + return ERR_PTR(-ENOMEM); obj = &bo->base.base;
obj->funcs = &v3d_gem_funcs; diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index a87eafa89e9f..c5e3e5457737 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -97,7 +97,7 @@ static struct drm_gem_object *vgem_gem_create_object(struct drm_device *dev, siz
obj = kzalloc(sizeof(*obj), GFP_KERNEL); if (!obj) - return NULL; + return ERR_PTR(-ENOMEM);
/* * vgem doesn't have any begin/end cpu access ioctls, therefore must use diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c index 187e10da2f17..baef2c5f2aaf 100644 --- a/drivers/gpu/drm/virtio/virtgpu_object.c +++ b/drivers/gpu/drm/virtio/virtgpu_object.c @@ -139,7 +139,7 @@ struct drm_gem_object *virtio_gpu_create_object(struct drm_device *dev,
shmem = kzalloc(sizeof(*shmem), GFP_KERNEL); if (!shmem) - return NULL; + return ERR_PTR(-ENOMEM);
dshmem = &shmem->base.base; dshmem->base.funcs = &virtio_gpu_shmem_funcs; diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index da0c836fe8e1..f6159acb8856 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -291,8 +291,9 @@ struct drm_driver { /** * @gem_create_object: constructor for gem objects * - * Hook for allocating the GEM object struct, for use by the CMA and - * SHMEM GEM helpers. + * Hook for allocating the GEM object struct, for use by the CMA + * and SHMEM GEM helpers. Returns a GEM object on success, or an + * ERR_PTR()-encoded error code otherwise. */ struct drm_gem_object *(*gem_create_object)(struct drm_device *dev, size_t size);
Hi
Am 07.12.21 um 09:55 schrieb Dan Carpenter:
I appologize. This thread has been really frustrating. I got mixed up because I recently sent patches for ingenic and vc4. Also we are working against different trees so maybe that is part of the problem?
I'm looking at today's linux-next. Your patch has been applied.
Yes. You updated all the drivers. But somehow the vc4 chunk from your patch was dropped. It was *NOT* dropped by Stephen Rothwell. It got dropped earlier. I am including the `git format-patch -1 <hash>` output from the commit.
My vc4 change is in drm-misc-next, [1] but not in drm-tip. [2] Your vc4 patch went through drm-misc-fixes.
drm-tip is build automatically by our DRM tools from the various trees. The tools took my patch from drm-misc-next and your patch from drm-misc-fixes and the result was broken.
Thanks for bringing this up. I'll see what I can do about it.
Best regards Thomas
[1] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/vc4/vc4_bo.c#... [2] https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/vc4/vc4_bo.c
Hi
Am 07.12.21 um 10:24 schrieb Thomas Zimmermann:
Hi
Am 07.12.21 um 09:55 schrieb Dan Carpenter:
I appologize. This thread has been really frustrating. I got mixed up because I recently sent patches for ingenic and vc4. Also we are working against different trees so maybe that is part of the problem?
I'm looking at today's linux-next. Your patch has been applied.
Yes. You updated all the drivers. But somehow the vc4 chunk from your patch was dropped. It was *NOT* dropped by Stephen Rothwell. It got dropped earlier. I am including the `git format-patch -1 <hash>` output from the commit.
My vc4 change is in drm-misc-next, [1] but not in drm-tip. [2] Your vc4 patch went through drm-misc-fixes.
drm-tip is build automatically by our DRM tools from the various trees. The tools took my patch from drm-misc-next and your patch from drm-misc-fixes and the result was broken.
Thanks for bringing this up. I'll see what I can do about it.
FYI I fixed drm-tip to return a pointer-encoded error in vc4. [1]
Best regards Thomas
[1] https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/vc4/vc4_bo.c?i...
Best regards Thomas
[1] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/vc4/vc4_bo.c#...
[2] https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/vc4/vc4_bo.c
dri-devel@lists.freedesktop.org
-
Dan Carpenter -
Maxime Ripard -
Steven Price -
Thomas Zimmermann