panfrost_ioctl_mmap_bo() contains a reimplementation of
drm_gem_map_offset() but with a bug - it allows mapping imported objects
(without going through the exporter). Fix this by switching to use the
newly renamed drm_gem_map_offset() function instead which has the bonus
of simplifying the code.
CC: Alyssa Rosenzweig <[email protected]>
Signed-off-by: Steven Price <[email protected]>
Reviewed-by: Alyssa Rosenzweig <[email protected]>
---
drivers/gpu/drm/panfrost/panfrost_drv.c | 16 ++--------------
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index d11e2281dde6..8be0cd5d6c7a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -255,26 +255,14 @@ static int panfrost_ioctl_mmap_bo(struct drm_device *dev, void *data,
struct drm_file *file_priv)
{
struct drm_panfrost_mmap_bo *args = data;
- struct drm_gem_object *gem_obj;
- int ret;
if (args->flags != 0) {
DRM_INFO("unknown mmap_bo flags: %d\n", args->flags);
return -EINVAL;
}
- gem_obj = drm_gem_object_lookup(file_priv, args->handle);
- if (!gem_obj) {
- DRM_DEBUG("Failed to look up GEM BO %d\n", args->handle);
- return -ENOENT;
- }
-
- ret = drm_gem_create_mmap_offset(gem_obj);
- if (ret == 0)
- args->offset = drm_vma_node_offset_addr(&gem_obj->vma_node);
- drm_gem_object_put_unlocked(gem_obj);
-
- return ret;
+ return drm_gem_map_offset(file_priv, dev, args->handle,
+ &args->offset);
}
static int panfrost_ioctl_get_bo_offset(struct drm_device *dev, void *data,
--
2.20.1
On Mon, May 20, 2019 at 4:23 AM Steven Price <[email protected]> wrote:
>
You forgot to update the subject. I can fixup when applying, but I'd
like an ack from Chris on patch 1.
> panfrost_ioctl_mmap_bo() contains a reimplementation of
> drm_gem_map_offset() but with a bug - it allows mapping imported objects
> (without going through the exporter). Fix this by switching to use the
> newly renamed drm_gem_map_offset() function instead which has the bonus
> of simplifying the code.
>
> CC: Alyssa Rosenzweig <[email protected]>
> Signed-off-by: Steven Price <[email protected]>
> Reviewed-by: Alyssa Rosenzweig <[email protected]>
Quoting Rob Herring (2019-05-21 16:24:27)
> On Mon, May 20, 2019 at 4:23 AM Steven Price <[email protected]> wrote:
> >
>
> You forgot to update the subject. I can fixup when applying, but I'd
> like an ack from Chris on patch 1.
I still think it is incorrect as the limitation is purely an issue with
the shmem backend and not a generic GEM limitation. It matters if you
have a history of passing names and want a consistent api across objects
when the apps themselves no longer can tell the difference, and
certainly do not have access to the dmabuf fd. i915 provides an indirect
mmap interface that uses the dma mapping regardless of source.
-Chris
On 21/05/2019 19:23, Chris Wilson wrote:
> Quoting Rob Herring (2019-05-21 16:24:27)
>> On Mon, May 20, 2019 at 4:23 AM Steven Price <[email protected]> wrote:
>>>
>>
>> You forgot to update the subject. I can fixup when applying, but I'd
>> like an ack from Chris on patch 1.
Sorry about that - I'll try to be more careful in the future.
> I still think it is incorrect as the limitation is purely an issue with
> the shmem backend and not a generic GEM limitation. It matters if you
Do you prefer the previous version of this series[1] with the shmem helper?
[1]
https://lore.kernel.org/lkml/[email protected]/
Although this isn't a generic GEM limitation it's currently the same
limitation that applies to the 'dumb' drivers as well as shmem backend,
so I'd prefer not implementing two identical functions purely because
this limitation could be removed in the future.
> have a history of passing names and want a consistent api across objects
> when the apps themselves no longer can tell the difference, and
> certainly do not have access to the dmabuf fd. i915 provides an indirect
> mmap interface that uses the dma mapping regardless of source.
I don't understand the i915 driver, but from a quick look at the source
of i915_gem_mmap_ioctl():
/* prime objects have no backing filp to GEM mmap
* pages from.
*/
if (!obj->base.filp) {
addr = -ENXIO;
goto err;
}
it looks to me that an attempt to map an imported dmabuf from user space
using the ioctl will fail. Am I missing something?
exynos_drm_gem_mmap() is the only (mainline[2]) instance I can see where
a transparent mapping of a dma_buf is supported.
[2] mali_kbase does this too - but I'm not convinced it was a good idea.
I could instead add support in shmem/panfrost for transparently passing
the request to the exporter (i.e. call dma_buf_mmap()) - but I'm not
sure we want to implement this if there isn't going to be a user of the
support.
Steve
On Wed, May 22, 2019 at 6:39 AM Steven Price <[email protected]> wrote:
>
> On 21/05/2019 19:23, Chris Wilson wrote:
> > Quoting Rob Herring (2019-05-21 16:24:27)
> >> On Mon, May 20, 2019 at 4:23 AM Steven Price <[email protected]> wrote:
> >>>
> >>
> >> You forgot to update the subject. I can fixup when applying, but I'd
> >> like an ack from Chris on patch 1.
>
> Sorry about that - I'll try to be more careful in the future.
>
> > I still think it is incorrect as the limitation is purely an issue with
> > the shmem backend and not a generic GEM limitation. It matters if you
>
> Do you prefer the previous version of this series[1] with the shmem helper?
>
> [1]
> https://lore.kernel.org/lkml/[email protected]/
>
> Although this isn't a generic GEM limitation it's currently the same
> limitation that applies to the 'dumb' drivers as well as shmem backend,
> so I'd prefer not implementing two identical functions purely because
> this limitation could be removed in the future.
In interest of moving this forward, how about some comments in
drm_gem_map_offset() explaining the limitations and when it is
appropriate to use the function.
Rob