| Message ID | 1438073609-32664-8-git-send-email-jy0922.shim@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2015? 07? 28? 17:53, Joonyoung Shim wrote: > Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If > not, it will call drm_gem_create_mmap_offset whenever user requests > DRM_IOCTL_MODE_MAP_DUMB ioctl. This patch makes drm_gem_create_mmap_offset to be called even in case of not using dumb* interfaces. I.e., exynos_drm_gem_create_ioctl -> exynos_drm_gem_mmap And drm_gem_create_mmap_offset checks if vma_node was already allocated or not so this patch doesn't make sense. Thanks, Inki Dae > > Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c > index 550d267..c76aa8a 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c > @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev, > return ERR_PTR(ret); > } > > + ret = drm_gem_create_mmap_offset(obj); > + if (ret < 0) { > + drm_gem_object_release(obj); > + kfree(exynos_gem_obj); > + return ERR_PTR(ret); > + } > + > DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp); > > return exynos_gem_obj; > @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv, > goto unlock; > } > > - ret = drm_gem_create_mmap_offset(obj); > - if (ret) > - goto out; > - > *offset = drm_vma_node_offset_addr(&obj->vma_node); > DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset); > > -out: > drm_gem_object_unreference(obj); > unlock: > mutex_unlock(&dev->struct_mutex); >
On 08/16/2015 01:50 PM, Inki Dae wrote: > On 2015? 07? 28? 17:53, Joonyoung Shim wrote: >> Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If >> not, it will call drm_gem_create_mmap_offset whenever user requests >> DRM_IOCTL_MODE_MAP_DUMB ioctl. > > This patch makes drm_gem_create_mmap_offset to be called even in case of > not using dumb* interfaces. I.e., > exynos_drm_gem_create_ioctl -> exynos_drm_gem_mmap > I know mmap() of exynos-drm also needs mmap offset in drm_gem_mmap(). > And drm_gem_create_mmap_offset checks if vma_node was already allocated > or not so this patch doesn't make sense. > OK, but it calls drm_gem_create_mmap_offset still and will be returned after checking node->allocated. It's not unnecessary to me. > Thanks, > Inki Dae > >> >> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c >> index 550d267..c76aa8a 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c >> @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev, >> return ERR_PTR(ret); >> } >> >> + ret = drm_gem_create_mmap_offset(obj); >> + if (ret < 0) { >> + drm_gem_object_release(obj); >> + kfree(exynos_gem_obj); >> + return ERR_PTR(ret); >> + } >> + >> DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp); >> >> return exynos_gem_obj; >> @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv, >> goto unlock; >> } >> >> - ret = drm_gem_create_mmap_offset(obj); >> - if (ret) >> - goto out; >> - >> *offset = drm_vma_node_offset_addr(&obj->vma_node); >> DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset); >> >> -out: >> drm_gem_object_unreference(obj); >> unlock: >> mutex_unlock(&dev->struct_mutex); >> > >
On 2015? 08? 17? 14:29, Joonyoung Shim wrote: > On 08/16/2015 01:50 PM, Inki Dae wrote: >> On 2015? 07? 28? 17:53, Joonyoung Shim wrote: >>> Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If >>> not, it will call drm_gem_create_mmap_offset whenever user requests >>> DRM_IOCTL_MODE_MAP_DUMB ioctl. >> >> This patch makes drm_gem_create_mmap_offset to be called even in case of >> not using dumb* interfaces. I.e., >> exynos_drm_gem_create_ioctl -> exynos_drm_gem_mmap >> > > I know mmap() of exynos-drm also needs mmap offset in drm_gem_mmap(). Ah, sorry. We already removed Exynos specific mmap ioctl. So we could never use direct mapping ioctl anymore. I confused that. > >> And drm_gem_create_mmap_offset checks if vma_node was already allocated >> or not so this patch doesn't make sense. >> > > OK, but it calls drm_gem_create_mmap_offset still and will be returned > after checking node->allocated. It's not unnecessary to me. > >> Thanks, >> Inki Dae >> >>> >>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com> >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++----- >>> 1 file changed, 7 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c >>> index 550d267..c76aa8a 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c >>> @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev, >>> return ERR_PTR(ret); >>> } >>> >>> + ret = drm_gem_create_mmap_offset(obj); >>> + if (ret < 0) { >>> + drm_gem_object_release(obj); >>> + kfree(exynos_gem_obj); >>> + return ERR_PTR(ret); >>> + } >>> + >>> DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp); >>> >>> return exynos_gem_obj; >>> @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv, >>> goto unlock; >>> } >>> >>> - ret = drm_gem_create_mmap_offset(obj); >>> - if (ret) >>> - goto out; >>> - >>> *offset = drm_vma_node_offset_addr(&obj->vma_node); >>> DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset); >>> >>> -out: >>> drm_gem_object_unreference(obj); >>> unlock: >>> mutex_unlock(&dev->struct_mutex); >>> >> >> > >
On Tue, Jul 28, 2015 at 05:53:23PM +0900, Joonyoung Shim wrote: > Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If > not, it will call drm_gem_create_mmap_offset whenever user requests > DRM_IOCTL_MODE_MAP_DUMB ioctl. > > Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c > index 550d267..c76aa8a 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c > @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev, > return ERR_PTR(ret); > } > > + ret = drm_gem_create_mmap_offset(obj); > + if (ret < 0) { > + drm_gem_object_release(obj); > + kfree(exynos_gem_obj); > + return ERR_PTR(ret); > + } > + > DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp); > > return exynos_gem_obj; > @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv, > goto unlock; > } > > - ret = drm_gem_create_mmap_offset(obj); drm_gem_create_mmap_offset internally checks whether it's been already (protected by locks), so this code is perfectly fine. I don't see any justification for this change (but only noticed it because rockchip cargo-culted this change). -Daniel > - if (ret) > - goto out; > - > *offset = drm_vma_node_offset_addr(&obj->vma_node); > DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset); > > -out: > drm_gem_object_unreference(obj); > unlock: > mutex_unlock(&dev->struct_mutex); > -- > 1.9.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Nov 16, 2015 at 05:22:42PM +0100, Daniel Vetter wrote: > On Tue, Jul 28, 2015 at 05:53:23PM +0900, Joonyoung Shim wrote: > > Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If > > not, it will call drm_gem_create_mmap_offset whenever user requests > > DRM_IOCTL_MODE_MAP_DUMB ioctl. > > > > Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com> > > --- > > drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c > > index 550d267..c76aa8a 100644 > > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c > > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c > > @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev, > > return ERR_PTR(ret); > > } > > > > + ret = drm_gem_create_mmap_offset(obj); > > + if (ret < 0) { > > + drm_gem_object_release(obj); > > + kfree(exynos_gem_obj); > > + return ERR_PTR(ret); > > + } > > + > > DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp); > > > > return exynos_gem_obj; > > @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv, > > goto unlock; > > } > > > > - ret = drm_gem_create_mmap_offset(obj); > > drm_gem_create_mmap_offset internally checks whether it's been already > (protected by locks), so this code is perfectly fine. I don't see any > justification for this change (but only noticed it because rockchip > cargo-culted this change). I think it'd be good to revert this to stay consistent with cma helpers and other drivers. -Daniel > -Daniel > > > - if (ret) > > - goto out; > > - > > *offset = drm_vma_node_offset_addr(&obj->vma_node); > > DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset); > > > > -out: > > drm_gem_object_unreference(obj); > > unlock: > > mutex_unlock(&dev->struct_mutex); > > -- > > 1.9.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
2015? 11? 17? 01:23? Daniel Vetter ?(?) ? ?: > On Mon, Nov 16, 2015 at 05:22:42PM +0100, Daniel Vetter wrote: >> On Tue, Jul 28, 2015 at 05:53:23PM +0900, Joonyoung Shim wrote: >>> Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If >>> not, it will call drm_gem_create_mmap_offset whenever user requests >>> DRM_IOCTL_MODE_MAP_DUMB ioctl. >>> >>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com> >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++----- >>> 1 file changed, 7 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c >>> index 550d267..c76aa8a 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c >>> @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev, >>> return ERR_PTR(ret); >>> } >>> >>> + ret = drm_gem_create_mmap_offset(obj); >>> + if (ret < 0) { >>> + drm_gem_object_release(obj); >>> + kfree(exynos_gem_obj); >>> + return ERR_PTR(ret); >>> + } >>> + >>> DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp); >>> >>> return exynos_gem_obj; >>> @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv, >>> goto unlock; >>> } >>> >>> - ret = drm_gem_create_mmap_offset(obj); >> >> drm_gem_create_mmap_offset internally checks whether it's been already >> (protected by locks), so this code is perfectly fine. I don't see any >> justification for this change (but only noticed it because rockchip >> cargo-culted this change). > > I think it'd be good to revert this to stay consistent with cma helpers > and other drivers. At least, seems cma halper doesn't also call drm_gem_create_mmap_offset function at drm_gem_cma_dumb_map_offset function and calls it at cma creation instead. So I think now Exynos drm keeps consistent with cma helper. Thanks, Inki Dae > -Daniel > >> -Daniel >> >>> - if (ret) >>> - goto out; >>> - >>> *offset = drm_vma_node_offset_addr(&obj->vma_node); >>> DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset); >>> >>> -out: >>> drm_gem_object_unreference(obj); >>> unlock: >>> mutex_unlock(&dev->struct_mutex); >>> -- >>> 1.9.1 >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch >
On Tue, Nov 17, 2015 at 11:53:28AM +0900, Inki Dae wrote: > > > 2015? 11? 17? 01:23? Daniel Vetter ?(?) ? ?: > > On Mon, Nov 16, 2015 at 05:22:42PM +0100, Daniel Vetter wrote: > >> On Tue, Jul 28, 2015 at 05:53:23PM +0900, Joonyoung Shim wrote: > >>> Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If > >>> not, it will call drm_gem_create_mmap_offset whenever user requests > >>> DRM_IOCTL_MODE_MAP_DUMB ioctl. > >>> > >>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com> > >>> --- > >>> drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++----- > >>> 1 file changed, 7 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c > >>> index 550d267..c76aa8a 100644 > >>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c > >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c > >>> @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev, > >>> return ERR_PTR(ret); > >>> } > >>> > >>> + ret = drm_gem_create_mmap_offset(obj); > >>> + if (ret < 0) { > >>> + drm_gem_object_release(obj); > >>> + kfree(exynos_gem_obj); > >>> + return ERR_PTR(ret); > >>> + } > >>> + > >>> DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp); > >>> > >>> return exynos_gem_obj; > >>> @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv, > >>> goto unlock; > >>> } > >>> > >>> - ret = drm_gem_create_mmap_offset(obj); > >> > >> drm_gem_create_mmap_offset internally checks whether it's been already > >> (protected by locks), so this code is perfectly fine. I don't see any > >> justification for this change (but only noticed it because rockchip > >> cargo-culted this change). > > > > I think it'd be good to revert this to stay consistent with cma helpers > > and other drivers. > > At least, seems cma halper doesn't also call drm_gem_create_mmap_offset function > at drm_gem_cma_dumb_map_offset function and calls it at cma creation instead. > So I think now Exynos drm keeps consistent with cma helper. Indeed, common style is to create the offset at alloc time - I checked a few other drivers too. Never mind my comment, sorry for the noise. -Daniel
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index 550d267..c76aa8a 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev, return ERR_PTR(ret); } + ret = drm_gem_create_mmap_offset(obj); + if (ret < 0) { + drm_gem_object_release(obj); + kfree(exynos_gem_obj); + return ERR_PTR(ret); + } + DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp); return exynos_gem_obj; @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv, goto unlock; } - ret = drm_gem_create_mmap_offset(obj); - if (ret) - goto out; - *offset = drm_vma_node_offset_addr(&obj->vma_node); DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset); -out: drm_gem_object_unreference(obj); unlock: mutex_unlock(&dev->struct_mutex);
Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If not, it will call drm_gem_create_mmap_offset whenever user requests DRM_IOCTL_MODE_MAP_DUMB ioctl. Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com> --- drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)