Hi,
On 2023/6/27 17:00, Thomas Zimmermann wrote:
Hi
Am 26.06.23 um 14:32 schrieb Maxime Ripard:
Hi,
On Tue, Jun 20, 2023 at 06:18:31PM +0200, Thomas Zimmermann wrote:
Am 20.06.23 um 18:06 schrieb Sui Jingfeng:
Hi,
On 2023/6/20 22:43, Thomas Zimmermann wrote:
Hi
Am 20.06.23 um 06:08 schrieb Sui Jingfeng:
ping ?
On 2023/3/14 20:53, Sui Jingfeng wrote:
else is not generally useful after return
No indention please.
OK, will be fixed at the next version.
Signed-off-by: Sui Jingfeng <15330273260@xxxxxx>
---
drivers/gpu/drm/drm_gem.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index a6208e2c089b..364e3733af98 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1150,8 +1150,8 @@ int drm_gem_pin(struct drm_gem_object *obj)
{
if (obj->funcs->pin)
return obj->funcs->pin(obj);
- else
- return 0;
+
+ return 0;
This change is ok.
}
void drm_gem_unpin(struct drm_gem_object *obj)
@@ -1172,7 +1172,8 @@ int drm_gem_vmap(struct drm_gem_object
*obj, struct iosys_map *map)
ret = obj->funcs->vmap(obj, map);
if (ret)
return ret;
- else if (iosys_map_is_null(map))
+
+ if (iosys_map_is_null(map))
return -ENOMEM;
This is not correct. Calling iosys_map_is_null() is part of handling
the return value from vmap, so the else is fine.
Are you serious ?
1. Before apply this patch:
If the 'ret' is 0, it stand for obj->funcs->vmap() is successful,
then
if (iosys_map_is_null(map)) will be run.
If the 'ret' is NOT 0, then it return immediately.
2. After apply this patch:
If the 'ret' is NOT 0, it stand for obj->funcs->vmap() failed, then it
return immediately.
If the 'ret' is 0, it stand for obj->funcs->vmap() is successful, then
the check if (iosys_map_is_null(map))
will be run!
I feel strange about the core here, I think the check ' if
(iosys_map_is_null(map))' is not needed,
the implement should responsible to handle all of possible errors.
The ->vmap() callback can succeed with ret=0, but we still have no
memory.
Then we return -ENOMEM. The call to _is_null(map) is part of the error
handling for ->vmap(). That is a bit strange, but it as always
worked like
that. Keeping all error handling in the same if-else block make all
this
more obvious.
Reading that patch, it wasn't obvious to me at all and could have made
the same patch.
The vmap callback could return any errno code; plus a 0 with a NULL
pointer means -ENOMEM. Doing this here simplifies the callers of
drm_gem_vmap and makes them more robust. We'd otherwise duplicate the
test for NULL in each caller.
Could we add a comment maybe to make it more obvious?
A one-liner that states the given rational might make sense.
Yeah, I'm agree.
But I think this is a short-term solution.
We probably should duplicate the test for NULL in each driver in a long
term.
Because we should to keep the align with TTM and SHMEM.
I means that TTM and SHMEM memory manager are good example to follow.
In TTM, Nearly all mapping related function will return -ENOMEM.
```
int ttm_bo_vmap(struct ttm_buffer_object *bo, struct iosys_map *map)
{
// ...
if (!vaddr_iomem)
return -ENOMEM;
iosys_map_set_vaddr_iomem(map, vaddr_iomem);
} else {
// ...
vaddr = vmap(ttm->pages, ttm->num_pages, 0, prot);
if (!vaddr)
return -ENOMEM;
iosys_map_set_vaddr(map, vaddr);
}
return 0;
}
```
The drm_gem_shmem_vmap() function in the SHMEM helper,
also return -ENOMEM.
After reading the code, It seems that
this is necessary to consolidate the standard behavior,
avoid the various device drivers violate each other.
Best regards
Thomas
Maxime
--
Jingfeng
