Re: [PATCH] drm: check user mode flags for validity

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, May 10, 2013 at 09:08:08AM -0700, Jesse Barnes wrote:
> On Thu, 9 May 2013 01:25:59 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Wed, May 08, 2013 at 02:01:25PM -0700, Jesse Barnes wrote:
> > > Requested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/drm_crtc.c |   12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > index 792c3e3..72ae33a 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -1318,6 +1318,18 @@ static int drm_crtc_convert_umode(struct drm_display_mode *out,
> > >  	if (in->clock > INT_MAX || in->vrefresh > INT_MAX)
> > >  		return -ERANGE;
> > >  
> > > +	/* Reject modes with invalid h/vsync */
> > > +	if (!(in->flags & (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NHSYNC)))
> > > +		return -EINVAL;
> > > +	if ((in->flags & DRM_MODE_FLAG_PHSYNC) &&
> > > +	    (in->flags & DRM_MODE_FLAG_NHSYNC))
> > > +		return -EINVAL;
> > > +	if (!(in->flags & (DRM_MODE_FLAG_PVSYNC | DRM_MODE_FLAG_NVSYNC)))
> > > +		return -EINVAL;
> > > +	if ((in->flags & DRM_MODE_FLAG_PVSYNC) &&
> > > +	    (in->flags & DRM_MODE_FLAG_NVSYNC))
> > > +		return -EINVAL;
> > 
> > That might be a bit too drastic. Well I suppose making sure that both
> > flags are not enabled at the same time could be OK. But having neither
> > flag set could be perfectly legal (the user could be asking for composite
> > sync instead for example).
> > 
> > 
> > So my less drastic suggestion would be doing something like this in i915
> > specific code:
> > 
> >  adjusted_mode->flags = 0;
> >  if (requested_mode->flags & NHSYNC)
> >  	adjusted_mode->flags |= NHSYNC;
> >  else
> >  	adjusted_mode->flags |= PHSYNC;
> > 
> > It would gurantee that we end up picking exactly one of the flags in
> > every case. If both are set, we pick -, of neither is set we pick +.
> 
> Ah yeah I knew I was forgetting something... I'll drop the checks for
> no flags.
> 
> You really think it would be better to do this in i915?  I guess it's
> probably safe, but it seems nicer to filter this out where it might
> occur (the EDID quirks should already deal with bogus flags for kernel
> generated mode lists).

I was thinking that we're generally quite relaxed about modesetting. The
user may specify something, but we might not really honor it due to
various mode fixups. Heck, if we have an LVDS or some other fixed mode
display, we ignore everything but the hdisp/vdisp from the user mode.

The same goes with the sync flags. For example if we're doing composite
or component output, it's always going to be composite sync, no matter
what the user asked for. And if we're cloning it at the same time, we
may be outputting both composite sync and separate h/v syncs at the same
time. Also the actual sync polarities may be limited by the hardware, so
not all cloned outputs might be using the same polarities.

We have no spec for how these flags should work, and we've been very
relaxed about them thus far. I'm mostly worried that if we add strict
checks, we may break some setup that works. So my idea would be to treat
that stuff as just hints.

-- 
Ville Syrjälä
Intel OTC