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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm: check user mode flags for validity
Date: Thu, 9 May 2013 01:25:59 +0300	[thread overview]
Message-ID: <20130508222559.GP14974@intel.com> (raw)
In-Reply-To: <1368046886-11932-1-git-send-email-jbarnes@virtuousgeek.org>

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 +.

> +
>  	out->clock = in->clock;
>  	out->hdisplay = in->hdisplay;
>  	out->hsync_start = in->hsync_start;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC