| Message ID | 1378493845-476-7-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Fri, Sep 6, 2013 at 8:57 PM, Damien Lespiau <damien.lespiau@intel.com> wrote: > This capability allows user space to control the delivery of modes with > the 3D flags set. This is to not play games with current user space > users not knowing anything about stereo 3D flags and that could try > to set a mode with one or several of those bits set. > > So, the plan is to remove the stereo 3D flags from the user mode > modeinfo structure by default, and let them through if we are being told > otherwise. > > stereo_allowed is bound to the drm_file structure to make it a > per-client setting, not a global one. > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > --- > drivers/gpu/drm/drm_crtc.c | 16 +++++++++++++--- > drivers/gpu/drm/drm_ioctl.c | 14 +++++++++++++- > include/drm/drmP.h | 3 +++ > include/uapi/drm/drm.h | 9 +++++++++ > 4 files changed, 38 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index a691764..ff9646f 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -1257,12 +1257,14 @@ EXPORT_SYMBOL(drm_mode_group_init_legacy_group); > * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo > * @out: drm_mode_modeinfo struct to return to the user > * @in: drm_display_mode to use > + * @file_priv: drm file from the ioctl call > * > * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to > * the user. > */ > static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out, > - const struct drm_display_mode *in) > + const struct drm_display_mode *in, > + const struct drm_file *file_priv) > { > WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX || > in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX || > @@ -1287,6 +1289,13 @@ static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out, > out->type = in->type; > strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); > out->name[DRM_DISPLAY_MODE_LEN-1] = 0; > + > + /* > + * If user-space hasn't configured the driver to expose the stereo 3D > + * flags, clear them. > + */ > + if (!file_priv->stereo_allowed) > + out->flags &= ~DRM_MODE_FLAG_3D_MASK; So just to be clear: Whenever a mode is present with 3D flags, it is also a valid non-3D mode? Is this guaranteed? Don't you want to add a mode twice, once without 3D flags and once with? You could then just skip all 3D modes for clients that don't support it. I have no idea how the 3D flags are specified, just want to go sure this doesn't break. So whenever a mode with 3D flags is present on a device, it can be set by a client dropping the 3D flags and it will be a valid mono-mode? Cheers David > } > > /** > @@ -1556,7 +1565,8 @@ int drm_mode_getcrtc(struct drm_device *dev, > > if (crtc->enabled) { > > - drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->mode); > + drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->mode, > + file_priv); > crtc_resp->mode_valid = 1; > > } else { > @@ -1655,7 +1665,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > copied = 0; > mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr; > list_for_each_entry(mode, &connector->modes, head) { > - drm_crtc_convert_to_umode(&u_mode, mode); > + drm_crtc_convert_to_umode(&u_mode, mode, file_priv); > if (copy_to_user(mode_ptr + copied, > &u_mode, sizeof(u_mode))) { > ret = -EFAULT; > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index e471cd9..a716641 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -304,7 +304,19 @@ int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv) > */ > int drm_setcap(struct drm_device *dev, void *data, struct drm_file *file_priv) > { > - return -EINVAL; > + struct drm_set_cap *req = data; > + > + switch (req->capability) { > + case DRM_CAP_STEREO_3D: > + if (req->value > 1) > + return -EINVAL; > + file_priv->stereo_allowed = req->value; > + break; > + default: > + return -EINVAL; > + } > + > + return 0; > } > > /** > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index b9c321b..0df654c 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -431,6 +431,9 @@ struct drm_file { > struct drm_master *master; /* master this node is currently associated with > N.B. not always minor->master */ > > + /* true when the client has asked us to expose stereo 3D mode flags */ > + bool stereo_allowed; > + > /** > * fbs - List of framebuffers associated with this file. > * > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index d400e6f..23922b4 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -627,6 +627,15 @@ struct drm_get_cap { > __u64 value; > }; > > +/** > + * DRM_CAP_STEREO_3D > + * > + * if set to 1, the DRM core will expose the stereo 3D capabilities of the > + * monitor by advertising the supported 3D layouts in the flags of struct > + * drm_mode_modeinfo. > + */ > +#define DRM_CAP_STEREO_3D 1 > + > /** DRM_IOCTL_SET_CAP ioctl argument type */ > struct drm_set_cap { > __u64 capability; > -- > 1.8.3.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sun, Sep 08, 2013 at 03:50:29PM +0200, David Herrmann wrote: > Hi > > On Fri, Sep 6, 2013 at 8:57 PM, Damien Lespiau <damien.lespiau@intel.com> wrote: > > This capability allows user space to control the delivery of modes with > > the 3D flags set. This is to not play games with current user space > > users not knowing anything about stereo 3D flags and that could try > > to set a mode with one or several of those bits set. > > > > So, the plan is to remove the stereo 3D flags from the user mode > > modeinfo structure by default, and let them through if we are being told > > otherwise. > > > > stereo_allowed is bound to the drm_file structure to make it a > > per-client setting, not a global one. > > > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > > --- > > drivers/gpu/drm/drm_crtc.c | 16 +++++++++++++--- > > drivers/gpu/drm/drm_ioctl.c | 14 +++++++++++++- > > include/drm/drmP.h | 3 +++ > > include/uapi/drm/drm.h | 9 +++++++++ > > 4 files changed, 38 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index a691764..ff9646f 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -1257,12 +1257,14 @@ EXPORT_SYMBOL(drm_mode_group_init_legacy_group); > > * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo > > * @out: drm_mode_modeinfo struct to return to the user > > * @in: drm_display_mode to use > > + * @file_priv: drm file from the ioctl call > > * > > * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to > > * the user. > > */ > > static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out, > > - const struct drm_display_mode *in) > > + const struct drm_display_mode *in, > > + const struct drm_file *file_priv) > > { > > WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX || > > in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX || > > @@ -1287,6 +1289,13 @@ static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out, > > out->type = in->type; > > strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); > > out->name[DRM_DISPLAY_MODE_LEN-1] = 0; > > + > > + /* > > + * If user-space hasn't configured the driver to expose the stereo 3D > > + * flags, clear them. > > + */ > > + if (!file_priv->stereo_allowed) > > + out->flags &= ~DRM_MODE_FLAG_3D_MASK; > > So just to be clear: Whenever a mode is present with 3D flags, it is > also a valid non-3D mode? Is this guaranteed? Don't you want to add a > mode twice, once without 3D flags and once with? You could then just > skip all 3D modes for clients that don't support it. > > I have no idea how the 3D flags are specified, just want to go sure > this doesn't break. So whenever a mode with 3D flags is present on a > device, it can be set by a client dropping the 3D flags and it will be > a valid mono-mode? So yes, that's exactly what happens right now. I was actually thinking about a v4 of the series with what you said in the first paragraph: just add the 3D modes to the list, one mode per 3D layout, and drop those modes from the list given back to userspace when the stereo_allowed isn't set. That seems quite a bit better than the convoluted approach here.
David Herrmann <dh.herrmann <at> gmail.com> writes: > > So just to be clear: Whenever a mode is present with 3D flags, it is > also a valid non-3D mode? Is this guaranteed? > Well.. Some HDTV's will when they receive a frame packed mode (1080*2+45=2205 pixels high) . Display just the top part. The bottom part of that is not on screen. So while it will not display it as 3d, it will discard half of the image. /Joakim
On Fri, Sep 13, 2013 at 04:04:02PM +0000, Joakim Plate wrote: > David Herrmann <dh.herrmann <at> gmail.com> writes: > > > > > So just to be clear: Whenever a mode is present with 3D flags, it is > > also a valid non-3D mode? Is this guaranteed? > > > > Well.. Some HDTV's will when they receive a frame packed mode (1080*2+45=2205 > pixels high) . Display just the top part. The bottom part of that is not on > screen. I changed this part of the API so there's no mixing the 2d mode structure with the 3d flags. Now (v4 of the series) userspace receives one struct drm_mode_modeinfo for the 2D mode (like before) and one struct drm_mode_modeinfo for each 3D layout (and the rest of the timings are from the "underlying 2D mode" that userspace can use in conjonction with the layout bit to compute the stereo framebufer layout) HTH,
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index a691764..ff9646f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1257,12 +1257,14 @@ EXPORT_SYMBOL(drm_mode_group_init_legacy_group); * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo * @out: drm_mode_modeinfo struct to return to the user * @in: drm_display_mode to use + * @file_priv: drm file from the ioctl call * * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to * the user. */ static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out, - const struct drm_display_mode *in) + const struct drm_display_mode *in, + const struct drm_file *file_priv) { WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX || in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX || @@ -1287,6 +1289,13 @@ static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out, out->type = in->type; strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); out->name[DRM_DISPLAY_MODE_LEN-1] = 0; + + /* + * If user-space hasn't configured the driver to expose the stereo 3D + * flags, clear them. + */ + if (!file_priv->stereo_allowed) + out->flags &= ~DRM_MODE_FLAG_3D_MASK; } /** @@ -1556,7 +1565,8 @@ int drm_mode_getcrtc(struct drm_device *dev, if (crtc->enabled) { - drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->mode); + drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->mode, + file_priv); crtc_resp->mode_valid = 1; } else { @@ -1655,7 +1665,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, copied = 0; mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr; list_for_each_entry(mode, &connector->modes, head) { - drm_crtc_convert_to_umode(&u_mode, mode); + drm_crtc_convert_to_umode(&u_mode, mode, file_priv); if (copy_to_user(mode_ptr + copied, &u_mode, sizeof(u_mode))) { ret = -EFAULT; diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index e471cd9..a716641 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -304,7 +304,19 @@ int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv) */ int drm_setcap(struct drm_device *dev, void *data, struct drm_file *file_priv) { - return -EINVAL; + struct drm_set_cap *req = data; + + switch (req->capability) { + case DRM_CAP_STEREO_3D: + if (req->value > 1) + return -EINVAL; + file_priv->stereo_allowed = req->value; + break; + default: + return -EINVAL; + } + + return 0; } /** diff --git a/include/drm/drmP.h b/include/drm/drmP.h index b9c321b..0df654c 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -431,6 +431,9 @@ struct drm_file { struct drm_master *master; /* master this node is currently associated with N.B. not always minor->master */ + /* true when the client has asked us to expose stereo 3D mode flags */ + bool stereo_allowed; + /** * fbs - List of framebuffers associated with this file. * diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index d400e6f..23922b4 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -627,6 +627,15 @@ struct drm_get_cap { __u64 value; }; +/** + * DRM_CAP_STEREO_3D + * + * if set to 1, the DRM core will expose the stereo 3D capabilities of the + * monitor by advertising the supported 3D layouts in the flags of struct + * drm_mode_modeinfo. + */ +#define DRM_CAP_STEREO_3D 1 + /** DRM_IOCTL_SET_CAP ioctl argument type */ struct drm_set_cap { __u64 capability;
This capability allows user space to control the delivery of modes with the 3D flags set. This is to not play games with current user space users not knowing anything about stereo 3D flags and that could try to set a mode with one or several of those bits set. So, the plan is to remove the stereo 3D flags from the user mode modeinfo structure by default, and let them through if we are being told otherwise. stereo_allowed is bound to the drm_file structure to make it a per-client setting, not a global one. Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- drivers/gpu/drm/drm_crtc.c | 16 +++++++++++++--- drivers/gpu/drm/drm_ioctl.c | 14 +++++++++++++- include/drm/drmP.h | 3 +++ include/uapi/drm/drm.h | 9 +++++++++ 4 files changed, 38 insertions(+), 4 deletions(-)