| Message ID | 1422559121-24477-1-git-send-email-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 29, 2015 at 2:18 PM, Sean Paul <seanpaul@chromium.org> wrote: > On 64-bit targets, tegra_gem_mmap doesn't return the > offset to userspace. As such, subsequent calls to mmap(2) > fail. Add a new tegra_gem_mmap2 ioctl to fix this. > > Signed-off-by: Sean Paul <seanpaul@chromium.org> Reviewed-by: Rob Clark <robdclark@gmail.com> > --- > drivers/gpu/drm/tegra/drm.c | 21 +++++++++++++++++++++ > include/uapi/drm/tegra_drm.h | 9 +++++++++ > 2 files changed, 30 insertions(+) > > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c > index d4f8275..be5dbe7 100644 > --- a/drivers/gpu/drm/tegra/drm.c > +++ b/drivers/gpu/drm/tegra/drm.c > @@ -343,6 +343,26 @@ static int tegra_gem_create(struct drm_device *drm, void *data, > return 0; > } > > +static int tegra_gem_mmap2(struct drm_device *drm, void *data, > + struct drm_file *file) > +{ > + struct drm_tegra_gem_mmap2 *args = data; > + struct drm_gem_object *gem; > + struct tegra_bo *bo; > + > + gem = drm_gem_object_lookup(drm, file, args->handle); > + if (!gem) > + return -EINVAL; > + > + bo = to_tegra_bo(gem); > + > + args->offset = drm_vma_node_offset_addr(&bo->gem.vma_node); > + > + drm_gem_object_unreference(gem); > + > + return 0; > +} > + > static int tegra_gem_mmap(struct drm_device *drm, void *data, > struct drm_file *file) > { > @@ -677,6 +697,7 @@ static const struct drm_ioctl_desc tegra_drm_ioctls[] = { > DRM_IOCTL_DEF_DRV(TEGRA_GEM_GET_TILING, tegra_gem_get_tiling, DRM_UNLOCKED), > DRM_IOCTL_DEF_DRV(TEGRA_GEM_SET_FLAGS, tegra_gem_set_flags, DRM_UNLOCKED), > DRM_IOCTL_DEF_DRV(TEGRA_GEM_GET_FLAGS, tegra_gem_get_flags, DRM_UNLOCKED), > + DRM_IOCTL_DEF_DRV(TEGRA_GEM_MMAP2, tegra_gem_mmap2, DRM_UNLOCKED), > #endif > }; > > diff --git a/include/uapi/drm/tegra_drm.h b/include/uapi/drm/tegra_drm.h > index c15d781..9057b0f 100644 > --- a/include/uapi/drm/tegra_drm.h > +++ b/include/uapi/drm/tegra_drm.h > @@ -167,6 +167,13 @@ struct drm_tegra_gem_get_flags { > __u32 flags; > }; > > +struct drm_tegra_gem_mmap2 { > + __u32 handle; > + __u32 pad; > + __u64 offset; > +}; > + > + > #define DRM_TEGRA_GEM_CREATE 0x00 > #define DRM_TEGRA_GEM_MMAP 0x01 > #define DRM_TEGRA_SYNCPT_READ 0x02 > @@ -181,6 +188,7 @@ struct drm_tegra_gem_get_flags { > #define DRM_TEGRA_GEM_GET_TILING 0x0b > #define DRM_TEGRA_GEM_SET_FLAGS 0x0c > #define DRM_TEGRA_GEM_GET_FLAGS 0x0d > +#define DRM_TEGRA_GEM_MMAP2 0x0e > > #define DRM_IOCTL_TEGRA_GEM_CREATE DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_CREATE, struct drm_tegra_gem_create) > #define DRM_IOCTL_TEGRA_GEM_MMAP DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_MMAP, struct drm_tegra_gem_mmap) > @@ -196,5 +204,6 @@ struct drm_tegra_gem_get_flags { > #define DRM_IOCTL_TEGRA_GEM_GET_TILING DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_GET_TILING, struct drm_tegra_gem_get_tiling) > #define DRM_IOCTL_TEGRA_GEM_SET_FLAGS DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_SET_FLAGS, struct drm_tegra_gem_set_flags) > #define DRM_IOCTL_TEGRA_GEM_GET_FLAGS DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_GET_FLAGS, struct drm_tegra_gem_get_flags) > +#define DRM_IOCTL_TEGRA_GEM_MMAP2 DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_MMAP2, struct drm_tegra_gem_mmap2) > > #endif > -- > 2.2.0.rc0.207.ga3a616c > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Jan 29, 2015 at 02:18:41PM -0500, Sean Paul wrote: > On 64-bit targets, tegra_gem_mmap doesn't return the > offset to userspace. As such, subsequent calls to mmap(2) > fail. Add a new tegra_gem_mmap2 ioctl to fix this. > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/tegra/drm.c | 21 +++++++++++++++++++++ > include/uapi/drm/tegra_drm.h | 9 +++++++++ > 2 files changed, 30 insertions(+) To be honest, I'd rather just fix the existing IOCTL to do the right thing on 64-bit. All IOCTLs are still protected by the DRM_TEGRA_STAGING Kconfig symbol which depends on STAGING. We originally did that precisely so we'd have some leeway in fixing things up. And we've done precisely that in the past. The only user of this IOCTL is libdrm and I don't think that has any users aside from a few projects that are still under heavy development (like grate or the xf86-video-opentegra driver). Cc'ing Erik, who's probably the only one that's ever worked with this, besides me. Thierry
On Fri, Jan 30, 2015 at 10:49 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, Jan 29, 2015 at 02:18:41PM -0500, Sean Paul wrote: >> On 64-bit targets, tegra_gem_mmap doesn't return the >> offset to userspace. As such, subsequent calls to mmap(2) >> fail. Add a new tegra_gem_mmap2 ioctl to fix this. >> >> Signed-off-by: Sean Paul <seanpaul@chromium.org> >> --- >> drivers/gpu/drm/tegra/drm.c | 21 +++++++++++++++++++++ >> include/uapi/drm/tegra_drm.h | 9 +++++++++ >> 2 files changed, 30 insertions(+) > > To be honest, I'd rather just fix the existing IOCTL to do the right > thing on 64-bit. All IOCTLs are still protected by the DRM_TEGRA_STAGING > Kconfig symbol which depends on STAGING. We originally did that > precisely so we'd have some leeway in fixing things up. And we've done > precisely that in the past. > > The only user of this IOCTL is libdrm and I don't think that has any > users aside from a few projects that are still under heavy development > (like grate or the xf86-video-opentegra driver). > > Cc'ing Erik, who's probably the only one that's ever worked with this, > besides me. I also saw the patch, and had the same reaction. I'm fine with changing the ABI, it was done already anyway (cbfbbabb89b37f6bad05f478d906a385149f288d, "drm/tegra: Remove gratuitous pad field"). And as you say, this is only in staging so nobody is really relying on it, except grate and libdrm (in which this is clearly marked as experimental). I'm fine with just changing it, and updating grate and libdrm accordingly.
On Fri, Jan 30, 2015 at 11:15:30AM +0100, Erik Faye-Lund wrote: > On Fri, Jan 30, 2015 at 10:49 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Thu, Jan 29, 2015 at 02:18:41PM -0500, Sean Paul wrote: > >> On 64-bit targets, tegra_gem_mmap doesn't return the > >> offset to userspace. As such, subsequent calls to mmap(2) > >> fail. Add a new tegra_gem_mmap2 ioctl to fix this. > >> > >> Signed-off-by: Sean Paul <seanpaul@chromium.org> > >> --- > >> drivers/gpu/drm/tegra/drm.c | 21 +++++++++++++++++++++ > >> include/uapi/drm/tegra_drm.h | 9 +++++++++ > >> 2 files changed, 30 insertions(+) > > > > To be honest, I'd rather just fix the existing IOCTL to do the right > > thing on 64-bit. All IOCTLs are still protected by the DRM_TEGRA_STAGING > > Kconfig symbol which depends on STAGING. We originally did that > > precisely so we'd have some leeway in fixing things up. And we've done > > precisely that in the past. > > > > The only user of this IOCTL is libdrm and I don't think that has any > > users aside from a few projects that are still under heavy development > > (like grate or the xf86-video-opentegra driver). > > > > Cc'ing Erik, who's probably the only one that's ever worked with this, > > besides me. > > I also saw the patch, and had the same reaction. I'm fine with > changing the ABI, it was done already anyway > (cbfbbabb89b37f6bad05f478d906a385149f288d, "drm/tegra: Remove > gratuitous pad field"). And as you say, this is only in staging so > nobody is really relying on it, except grate and libdrm (in which this > is clearly marked as experimental). I'm fine with just changing it, > and updating grate and libdrm accordingly. Okay, I can prepare a patch for libdrm and push it if we decide to go ahead with this plan. Rob, Sean, (anyone,) any objections to just changing the ABI? I made another pass through the list of IOCTL argument structures and couldn't spot any others that would have the same issue. Perhaps we're finally approaching a point where we can remove the dependency on STAGING? Thierry
On 30/01/15 10:21, Thierry Reding wrote: > On Fri, Jan 30, 2015 at 11:15:30AM +0100, Erik Faye-Lund wrote: >> On Fri, Jan 30, 2015 at 10:49 AM, Thierry Reding >> <thierry.reding@gmail.com> wrote: >>> On Thu, Jan 29, 2015 at 02:18:41PM -0500, Sean Paul wrote: >>>> On 64-bit targets, tegra_gem_mmap doesn't return the >>>> offset to userspace. As such, subsequent calls to mmap(2) >>>> fail. Add a new tegra_gem_mmap2 ioctl to fix this. >>>> >>>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>>> --- >>>> drivers/gpu/drm/tegra/drm.c | 21 +++++++++++++++++++++ >>>> include/uapi/drm/tegra_drm.h | 9 +++++++++ >>>> 2 files changed, 30 insertions(+) >>> >>> To be honest, I'd rather just fix the existing IOCTL to do the right >>> thing on 64-bit. All IOCTLs are still protected by the DRM_TEGRA_STAGING >>> Kconfig symbol which depends on STAGING. We originally did that >>> precisely so we'd have some leeway in fixing things up. And we've done >>> precisely that in the past. >>> >>> The only user of this IOCTL is libdrm and I don't think that has any >>> users aside from a few projects that are still under heavy development >>> (like grate or the xf86-video-opentegra driver). >>> >>> Cc'ing Erik, who's probably the only one that's ever worked with this, >>> besides me. >> >> I also saw the patch, and had the same reaction. I'm fine with >> changing the ABI, it was done already anyway >> (cbfbbabb89b37f6bad05f478d906a385149f288d, "drm/tegra: Remove >> gratuitous pad field"). And as you say, this is only in staging so >> nobody is really relying on it, except grate and libdrm (in which this >> is clearly marked as experimental). I'm fine with just changing it, >> and updating grate and libdrm accordingly. > > Okay, I can prepare a patch for libdrm and push it if we decide to go > ahead with this plan. > > Rob, Sean, (anyone,) any objections to just changing the ABI? I made > another pass through the list of IOCTL argument structures and couldn't > spot any others that would have the same issue. Perhaps we're finally > approaching a point where we can remove the dependency on STAGING? > Hi Thierry, Just a small tip - pahole is nice little helper wrt structure size/padding. Just scan the 32 & 64bit build and grep + diff the struct sizes. Cheers, Emil
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index d4f8275..be5dbe7 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -343,6 +343,26 @@ static int tegra_gem_create(struct drm_device *drm, void *data, return 0; } +static int tegra_gem_mmap2(struct drm_device *drm, void *data, + struct drm_file *file) +{ + struct drm_tegra_gem_mmap2 *args = data; + struct drm_gem_object *gem; + struct tegra_bo *bo; + + gem = drm_gem_object_lookup(drm, file, args->handle); + if (!gem) + return -EINVAL; + + bo = to_tegra_bo(gem); + + args->offset = drm_vma_node_offset_addr(&bo->gem.vma_node); + + drm_gem_object_unreference(gem); + + return 0; +} + static int tegra_gem_mmap(struct drm_device *drm, void *data, struct drm_file *file) { @@ -677,6 +697,7 @@ static const struct drm_ioctl_desc tegra_drm_ioctls[] = { DRM_IOCTL_DEF_DRV(TEGRA_GEM_GET_TILING, tegra_gem_get_tiling, DRM_UNLOCKED), DRM_IOCTL_DEF_DRV(TEGRA_GEM_SET_FLAGS, tegra_gem_set_flags, DRM_UNLOCKED), DRM_IOCTL_DEF_DRV(TEGRA_GEM_GET_FLAGS, tegra_gem_get_flags, DRM_UNLOCKED), + DRM_IOCTL_DEF_DRV(TEGRA_GEM_MMAP2, tegra_gem_mmap2, DRM_UNLOCKED), #endif }; diff --git a/include/uapi/drm/tegra_drm.h b/include/uapi/drm/tegra_drm.h index c15d781..9057b0f 100644 --- a/include/uapi/drm/tegra_drm.h +++ b/include/uapi/drm/tegra_drm.h @@ -167,6 +167,13 @@ struct drm_tegra_gem_get_flags { __u32 flags; }; +struct drm_tegra_gem_mmap2 { + __u32 handle; + __u32 pad; + __u64 offset; +}; + + #define DRM_TEGRA_GEM_CREATE 0x00 #define DRM_TEGRA_GEM_MMAP 0x01 #define DRM_TEGRA_SYNCPT_READ 0x02 @@ -181,6 +188,7 @@ struct drm_tegra_gem_get_flags { #define DRM_TEGRA_GEM_GET_TILING 0x0b #define DRM_TEGRA_GEM_SET_FLAGS 0x0c #define DRM_TEGRA_GEM_GET_FLAGS 0x0d +#define DRM_TEGRA_GEM_MMAP2 0x0e #define DRM_IOCTL_TEGRA_GEM_CREATE DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_CREATE, struct drm_tegra_gem_create) #define DRM_IOCTL_TEGRA_GEM_MMAP DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_MMAP, struct drm_tegra_gem_mmap) @@ -196,5 +204,6 @@ struct drm_tegra_gem_get_flags { #define DRM_IOCTL_TEGRA_GEM_GET_TILING DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_GET_TILING, struct drm_tegra_gem_get_tiling) #define DRM_IOCTL_TEGRA_GEM_SET_FLAGS DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_SET_FLAGS, struct drm_tegra_gem_set_flags) #define DRM_IOCTL_TEGRA_GEM_GET_FLAGS DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_GET_FLAGS, struct drm_tegra_gem_get_flags) +#define DRM_IOCTL_TEGRA_GEM_MMAP2 DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_MMAP2, struct drm_tegra_gem_mmap2) #endif
On 64-bit targets, tegra_gem_mmap doesn't return the offset to userspace. As such, subsequent calls to mmap(2) fail. Add a new tegra_gem_mmap2 ioctl to fix this. Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/tegra/drm.c | 21 +++++++++++++++++++++ include/uapi/drm/tegra_drm.h | 9 +++++++++ 2 files changed, 30 insertions(+)