Re: [PATCH 4/4] drm/i915: Do some basic sanity adjustments on the user provided mode

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

On Tue, Oct 01, 2013 at 09:06:10PM +0200, Daniel Vetter wrote:
> On Tue, Oct 01, 2013 at 04:13:31PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Make sure the active, blanking and sync portions of the timings are in
> > the proper order. The contract with userspace basically is that we must
> > not increase the active portion. For the rest we're more or less free to
> > do as we please. A good rul IMO is that we only increase the non-active
> > portions of the timings.
> > 
> > We could do a lot more adjustment to make sure the timings meet all
> > the minimum requirements of the hardware. But as the mimimums may depend
> > on the output type and other details, we should perhaps do the adjustments
> > at some later point.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Do we actually get such modes?
> 
> I'd vote to just filter them from our ->get_modes callbacks and reject
> them here if at all possible ...

The user is free to feed us any kind of garbage. Doesn't matter which
modes we list in the connector's mode list.

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 58 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 7f61cfb..48cad99 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8458,6 +8458,55 @@ static bool check_encoder_cloning(struct drm_crtc *crtc)
> >  	return !(num_encoders > 1 && uncloneable_encoders);
> >  }
> >  
> > +static void adjust_timings(struct drm_display_mode *mode,
> > +			   int *val, int min, int clocks_per_unit)
> > +{
> > +	if (*val >= min)
> > +		return;
> > +
> > +	/* maintain the same refresh rate */
> > +	mode->clock += (min - *val) * clocks_per_unit;
> > +
> > +	*val = min;
> > +}
> > +
> > +static int intel_adjust_mode(struct drm_display_mode *mode)
> > +{
> > +	if (mode->clock == 0) {
> > +		DRM_DEBUG_KMS("bad pixel clock\n");
> > +		drm_mode_debug_printmodeline(mode);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (mode->hdisplay == 0 || mode->vdisplay == 0) {
> > +		DRM_DEBUG_KMS("bad hdisplay or vdisplay\n");
> > +		drm_mode_debug_printmodeline(mode);
> > +		return -EINVAL;
> > +	}
> > +
> > +	adjust_timings(mode, &mode->hsync_start, mode->hdisplay, 0);
> > +	adjust_timings(mode, &mode->hsync_end, mode->hsync_start, 0);
> > +	adjust_timings(mode, &mode->htotal, mode->hsync_end, mode->vtotal);
> > +
> > +	adjust_timings(mode, &mode->vsync_start, mode->vdisplay, 0);
> > +	adjust_timings(mode, &mode->vsync_end, mode->vsync_start, 0);
> > +
> > +	/*
> > +	 * We must not increase the size of the active
> > +	 * space area for frame packing modes.
> > +	 */
> > +	if ((mode->flags & DRM_MODE_FLAG_3D_MASK) == DRM_MODE_FLAG_3D_FRAME_PACKING) {
> > +		if (mode->vtotal <  mode->vsync_end) {
> > +			DRM_DEBUG_KMS("refusing to adjust vtotal for frame packing mode\n");
> > +			drm_mode_debug_printmodeline(mode);
> > +			return -EINVAL;
> > +		}
> > +	} else
> > +		adjust_timings(mode, &mode->vtotal, mode->vsync_end, mode->htotal);
> > +
> > +	return 0;
> > +}
> > +
> >  static struct intel_crtc_config *
> >  intel_modeset_pipe_config(struct drm_crtc *crtc,
> >  			  struct drm_framebuffer *fb,
> > @@ -8466,7 +8515,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> >  	struct drm_device *dev = crtc->dev;
> >  	struct intel_encoder *encoder;
> >  	struct intel_crtc_config *pipe_config;
> > -	int plane_bpp, ret = -EINVAL;
> > +	int plane_bpp, ret;
> >  	bool retry = true;
> >  
> >  	if (!check_encoder_cloning(crtc)) {
> > @@ -8498,14 +8547,20 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> >  	      (DRM_MODE_FLAG_PVSYNC | DRM_MODE_FLAG_NVSYNC)))
> >  		pipe_config->adjusted_mode.flags |= DRM_MODE_FLAG_NVSYNC;
> >  
> > +	ret = intel_adjust_mode(&pipe_config->adjusted_mode);
> > +	if (ret)
> > +		goto fail;
> > +
> >  	/* Compute a starting value for pipe_config->pipe_bpp taking the source
> >  	 * plane pixel format and any sink constraints into account. Returns the
> >  	 * source plane bpp so that dithering can be selected on mismatches
> >  	 * after encoders and crtc also have had their say. */
> >  	plane_bpp = compute_baseline_pipe_bpp(to_intel_crtc(crtc),
> >  					      fb, pipe_config);
> > -	if (plane_bpp < 0)
> > +	if (plane_bpp < 0) {
> > +		ret = -EINVAL;
> >  		goto fail;
> > +	}
> >  
> >  	/* Determine the real pipe dimensions */
> >  	drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, CRTC_STEREO_DOUBLE);
> > @@ -8532,6 +8587,7 @@ encoder_retry:
> >  
> >  		if (!(encoder->compute_config(encoder, pipe_config))) {
> >  			DRM_DEBUG_KMS("Encoder config failure\n");
> > +			ret = -EINVAL;
> >  			goto fail;
> >  		}
> >  	}
> > -- 
> > 1.8.1.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC