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

[Daniel Vetter <daniel@ffwll.ch>]

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