| Message ID | 20180323134553.15993-4-daniels@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 23, 2018 at 01:45:52PM +0000, Daniel Stone wrote: > getfb2 allows us to pass multiple planes and modifiers, just like addfb2 > over addfb. > > Signed-off-by: Daniel Stone <daniels@collabora.com> > --- > drivers/gpu/drm/drm_crtc_internal.h | 2 + > drivers/gpu/drm/drm_framebuffer.c | 109 ++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_ioctl.c | 2 + > include/uapi/drm/drm.h | 1 + > 4 files changed, 114 insertions(+) > > diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h > index 3c2b82865ad2..0cd02f3d203d 100644 > --- a/drivers/gpu/drm/drm_crtc_internal.h > +++ b/drivers/gpu/drm/drm_crtc_internal.h > @@ -173,6 +173,8 @@ int drm_mode_rmfb(struct drm_device *dev, > void *data, struct drm_file *file_priv); > int drm_mode_getfb(struct drm_device *dev, > void *data, struct drm_file *file_priv); > +int drm_mode_getfb2_ioctl(struct drm_device *dev, > + void *data, struct drm_file *file_priv); > int drm_mode_dirtyfb_ioctl(struct drm_device *dev, > void *data, struct drm_file *file_priv); > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index 6d5ff541225a..f1cfb6ddc776 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -24,6 +24,7 @@ > #include <drm/drmP.h> > #include <drm/drm_auth.h> > #include <drm/drm_framebuffer.h> > +#include <drm/drm_gem.h> > #include <drm/drm_atomic.h> > #include <drm/drm_print.h> > > @@ -494,7 +495,115 @@ int drm_mode_getfb(struct drm_device *dev, > > out: > drm_framebuffer_put(fb); > + return ret; > +} > + > +/** > + * drm_mode_getfb2 - get extended FB info > + * @dev: drm device for the ioctl > + * @data: data pointer for the ioctl > + * @file_priv: drm file for the ioctl call > + * > + * Lookup the FB given its ID and return info about it. > + * > + * Called by the user via ioctl. > + * > + * Returns: > + * Zero on success, negative errno on failure. > + */ > +int drm_mode_getfb2_ioctl(struct drm_device *dev, > + void *data, struct drm_file *file_priv) > +{ > + struct drm_mode_fb_cmd2 *r = data; > + struct drm_framebuffer *fb; > + unsigned int i; > + int ret; > + > + if (!drm_core_check_feature(dev, DRIVER_MODESET)) > + return -EINVAL; > > + fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id); > + if (!fb) > + return -ENOENT; > + > + /* For multi-plane framebuffers, we require the driver to place the > + * GEM objects directly in the drm_framebuffer. For single-plane > + * framebuffers, we can fall back to create_handle. > + */ > + if (!fb->obj[0] && > + (fb->format->num_planes > 1 || !fb->funcs->create_handle)) { > + ret = -ENODEV; > + goto out; > + } > + > + r->height = fb->height; > + r->width = fb->width; > + r->pixel_format = fb->format->format; > + > + r->flags = 0; > + if (dev->mode_config.allow_fb_modifiers) > + r->flags |= DRM_MODE_FB_MODIFIERS; > + > + for (i = 0; i < ARRAY_SIZE(r->handles); i++) { > + r->handles[i] = 0; > + r->pitches[i] = 0; > + r->offsets[i] = 0; > + r->modifier[i] = DRM_FORMAT_MOD_INVALID; Don't we want to leave this zeroed too? For addfb2 we require any unused modifier to be 0, so if someone does 'getfb2(&cmd); addfb2(&cmd);' they would get an error from the addfb2(). > + } > + > + for (i = 0; i < fb->format->num_planes; i++) { > + int j; > + > + r->pitches[i] = fb->pitches[i]; > + r->offsets[i] = fb->offsets[i]; > + if (dev->mode_config.allow_fb_modifiers) > + r->modifier[i] = fb->modifier; > + > + /* If we reuse the same object for multiple planes, also > + * return the same handle. > + */ > + for (j = 0; j < i; j++) { > + if (fb->obj[i] == fb->obj[j]) { > + r->handles[i] = r->handles[j]; > + break; > + } > + } > + > + if (r->handles[i]) > + continue; > + > + if (fb->obj[i]) { > + ret = drm_gem_handle_create(file_priv, fb->obj[i], > + &r->handles[i]); > + } else { > + WARN_ON(i > 0); > + ret = fb->funcs->create_handle(fb, file_priv, > + &r->handles[i]); > + } > + > + if (ret != 0) > + goto out; > + } > + > +out: > + if (ret != 0) { > + /* Delete any previously-created handles on failure. */ > + for (i = 0; i < ARRAY_SIZE(r->handles); i++) { > + int j; > + > + if (r->handles[i]) > + drm_gem_handle_delete(file_priv, r->handles[i]); > + > + /* Zero out any handles identical to the one we just > + * deleted. */ > + for (j = i + 1; j < ARRAY_SIZE(r->handles); j++) { > + if (r->handles[j] == r->handles[i]) > + r->handles[j] = 0; > + } > + } > + } > + > + drm_framebuffer_put(fb); > return ret; > } > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index af782911c505..b5896e3615e5 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -669,6 +669,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = { > DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), > + > + DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB2, drm_mode_getfb2_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), > }; > > #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls ) > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index 6fdff5945c8a..9a33613394a9 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -892,6 +892,7 @@ extern "C" { > #define DRM_IOCTL_MODE_LIST_LESSEES DRM_IOWR(0xC7, struct drm_mode_list_lessees) > #define DRM_IOCTL_MODE_GET_LEASE DRM_IOWR(0xC8, struct drm_mode_get_lease) > #define DRM_IOCTL_MODE_REVOKE_LEASE DRM_IOWR(0xC9, struct drm_mode_revoke_lease) > +#define DRM_IOCTL_MODE_GETFB2 DRM_IOWR(0xCA, struct drm_mode_fb_cmd2) > > /** > * Device specific ioctls should only be in their respective headers > -- > 2.16.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, On 23 March 2018 at 14:49, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Mar 23, 2018 at 01:45:52PM +0000, Daniel Stone wrote: >> + for (i = 0; i < ARRAY_SIZE(r->handles); i++) { >> + r->handles[i] = 0; >> + r->pitches[i] = 0; >> + r->offsets[i] = 0; >> + r->modifier[i] = DRM_FORMAT_MOD_INVALID; > > Don't we want to leave this zeroed too? For addfb2 we require any unused > modifier to be 0, so if someone does 'getfb2(&cmd); addfb2(&cmd);' they > would get an error from the addfb2(). My thinking is that since the primary userspace for this doesn't have symmetry with add (args for add, struct for get), that it was better to feed in INVALID directly. This is going to change, e.g., X server to: modifier = (fb->flags ? DRM_MODE_FB_MODIFIERS) ? fb->modifier : DRM_FORMAT_MOD_INVALID; It's a good point about the symmetry though. Do you know of direct non-libdrm users? Apart from igt of course ;) Cheers, Daniel
On Fri, Mar 23, 2018 at 05:00:11PM +0000, Daniel Stone wrote: > Hi, > > On 23 March 2018 at 14:49, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Fri, Mar 23, 2018 at 01:45:52PM +0000, Daniel Stone wrote: > >> + for (i = 0; i < ARRAY_SIZE(r->handles); i++) { > >> + r->handles[i] = 0; > >> + r->pitches[i] = 0; > >> + r->offsets[i] = 0; > >> + r->modifier[i] = DRM_FORMAT_MOD_INVALID; > > > > Don't we want to leave this zeroed too? For addfb2 we require any unused > > modifier to be 0, so if someone does 'getfb2(&cmd); addfb2(&cmd);' they > > would get an error from the addfb2(). > > My thinking is that since the primary userspace for this doesn't have > symmetry with add (args for add, struct for get), that it was better > to feed in INVALID directly. This is going to change, e.g., X server > to: > modifier = (fb->flags ? DRM_MODE_FB_MODIFIERS) ? fb->modifier : > DRM_FORMAT_MOD_INVALID; > > It's a good point about the symmetry though. Do you know of direct > non-libdrm users? Apart from igt of course ;) Nope. Just thought that since both take the same struct it'd make some sense. And figured it could serve as a quick sanity check to make sure getfb outputs sane data. Or rather, if the driver accepts it back in it can't be all bad at least. But if you think it's not a particularly useful thing to do then I'm certainly willing to accept that.
On 23 March 2018 at 17:31, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Mar 23, 2018 at 05:00:11PM +0000, Daniel Stone wrote: >> On 23 March 2018 at 14:49, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> > On Fri, Mar 23, 2018 at 01:45:52PM +0000, Daniel Stone wrote: >> >> + for (i = 0; i < ARRAY_SIZE(r->handles); i++) { >> >> + r->handles[i] = 0; >> >> + r->pitches[i] = 0; >> >> + r->offsets[i] = 0; >> >> + r->modifier[i] = DRM_FORMAT_MOD_INVALID; >> > >> > Don't we want to leave this zeroed too? For addfb2 we require any unused >> > modifier to be 0, so if someone does 'getfb2(&cmd); addfb2(&cmd);' they >> > would get an error from the addfb2(). >> >> My thinking is that since the primary userspace for this doesn't have >> symmetry with add (args for add, struct for get), that it was better >> to feed in INVALID directly. This is going to change, e.g., X server >> to: >> modifier = (fb->flags ? DRM_MODE_FB_MODIFIERS) ? fb->modifier : >> DRM_FORMAT_MOD_INVALID; >> >> It's a good point about the symmetry though. Do you know of direct >> non-libdrm users? Apart from igt of course ;) > > Nope. Just thought that since both take the same struct it'd make some > sense. And figured it could serve as a quick sanity check to make sure > getfb outputs sane data. Or rather, if the driver accepts it back in > it can't be all bad at least. > > But if you think it's not a particularly useful thing to do then I'm > certainly willing to accept that. Makes sense. I'm on the fence; let's see if anyone else has any suggestions. Cheers, Daniel
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index 3c2b82865ad2..0cd02f3d203d 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -173,6 +173,8 @@ int drm_mode_rmfb(struct drm_device *dev, void *data, struct drm_file *file_priv); int drm_mode_getfb(struct drm_device *dev, void *data, struct drm_file *file_priv); +int drm_mode_getfb2_ioctl(struct drm_device *dev, + void *data, struct drm_file *file_priv); int drm_mode_dirtyfb_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 6d5ff541225a..f1cfb6ddc776 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -24,6 +24,7 @@ #include <drm/drmP.h> #include <drm/drm_auth.h> #include <drm/drm_framebuffer.h> +#include <drm/drm_gem.h> #include <drm/drm_atomic.h> #include <drm/drm_print.h> @@ -494,7 +495,115 @@ int drm_mode_getfb(struct drm_device *dev, out: drm_framebuffer_put(fb); + return ret; +} + +/** + * drm_mode_getfb2 - get extended FB info + * @dev: drm device for the ioctl + * @data: data pointer for the ioctl + * @file_priv: drm file for the ioctl call + * + * Lookup the FB given its ID and return info about it. + * + * Called by the user via ioctl. + * + * Returns: + * Zero on success, negative errno on failure. + */ +int drm_mode_getfb2_ioctl(struct drm_device *dev, + void *data, struct drm_file *file_priv) +{ + struct drm_mode_fb_cmd2 *r = data; + struct drm_framebuffer *fb; + unsigned int i; + int ret; + + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id); + if (!fb) + return -ENOENT; + + /* For multi-plane framebuffers, we require the driver to place the + * GEM objects directly in the drm_framebuffer. For single-plane + * framebuffers, we can fall back to create_handle. + */ + if (!fb->obj[0] && + (fb->format->num_planes > 1 || !fb->funcs->create_handle)) { + ret = -ENODEV; + goto out; + } + + r->height = fb->height; + r->width = fb->width; + r->pixel_format = fb->format->format; + + r->flags = 0; + if (dev->mode_config.allow_fb_modifiers) + r->flags |= DRM_MODE_FB_MODIFIERS; + + for (i = 0; i < ARRAY_SIZE(r->handles); i++) { + r->handles[i] = 0; + r->pitches[i] = 0; + r->offsets[i] = 0; + r->modifier[i] = DRM_FORMAT_MOD_INVALID; + } + + for (i = 0; i < fb->format->num_planes; i++) { + int j; + + r->pitches[i] = fb->pitches[i]; + r->offsets[i] = fb->offsets[i]; + if (dev->mode_config.allow_fb_modifiers) + r->modifier[i] = fb->modifier; + + /* If we reuse the same object for multiple planes, also + * return the same handle. + */ + for (j = 0; j < i; j++) { + if (fb->obj[i] == fb->obj[j]) { + r->handles[i] = r->handles[j]; + break; + } + } + + if (r->handles[i]) + continue; + + if (fb->obj[i]) { + ret = drm_gem_handle_create(file_priv, fb->obj[i], + &r->handles[i]); + } else { + WARN_ON(i > 0); + ret = fb->funcs->create_handle(fb, file_priv, + &r->handles[i]); + } + + if (ret != 0) + goto out; + } + +out: + if (ret != 0) { + /* Delete any previously-created handles on failure. */ + for (i = 0; i < ARRAY_SIZE(r->handles); i++) { + int j; + + if (r->handles[i]) + drm_gem_handle_delete(file_priv, r->handles[i]); + + /* Zero out any handles identical to the one we just + * deleted. */ + for (j = i + 1; j < ARRAY_SIZE(r->handles); j++) { + if (r->handles[j] == r->handles[i]) + r->handles[j] = 0; + } + } + } + + drm_framebuffer_put(fb); return ret; } diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index af782911c505..b5896e3615e5 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -669,6 +669,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), + + DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB2, drm_mode_getfb2_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), }; #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls ) diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 6fdff5945c8a..9a33613394a9 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -892,6 +892,7 @@ extern "C" { #define DRM_IOCTL_MODE_LIST_LESSEES DRM_IOWR(0xC7, struct drm_mode_list_lessees) #define DRM_IOCTL_MODE_GET_LEASE DRM_IOWR(0xC8, struct drm_mode_get_lease) #define DRM_IOCTL_MODE_REVOKE_LEASE DRM_IOWR(0xC9, struct drm_mode_revoke_lease) +#define DRM_IOCTL_MODE_GETFB2 DRM_IOWR(0xCA, struct drm_mode_fb_cmd2) /** * Device specific ioctls should only be in their respective headers
getfb2 allows us to pass multiple planes and modifiers, just like addfb2 over addfb. Signed-off-by: Daniel Stone <daniels@collabora.com> --- drivers/gpu/drm/drm_crtc_internal.h | 2 + drivers/gpu/drm/drm_framebuffer.c | 109 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_ioctl.c | 2 + include/uapi/drm/drm.h | 1 + 4 files changed, 114 insertions(+)