Re: [PATCH 6/9] drm: Add a DRM_CAP_STEREO_3D capability for SET_CAP ioctl

[Damien Lespiau <damien.lespiau@intel.com>]

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.

-- 
Damien