Re: [PATCH 1/4] drm/i915: Don't populate pipe_src_{w, h} multiple times

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 01, 2013 at 04:13:28PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If we ever end up doing the retry loop due to bandwidth constraints, we
> would rewrite pipe_src_{w,n} based on adjusted_mode timings. But by that
> time the encoder may have already replaced the adjusted_mode with a
> fixed panel mode, which would then corrupt pipe_src_{w,h}.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a91f20a..a695888 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8472,6 +8472,11 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  	if (plane_bpp < 0)
>  		goto fail;
>  
> +	/* Determine the real pipe dimensions */

I think it'll benefit clarity a bit if we'd use the requested mode
here (even though we never really care about it's crtc timings anywhere
else).

This needs more comment imo. What about the following instead?

"Determine the real pipe dimensions. Note that stereo modes can increase
the actual pipe size due to the frame doubling and insertion of additional
space for blanks between the frame. This is stored in the crtc timings. We
use the requested mode to do this computation to clearly distinguish it
from the adjusted mode, which can be changed by the connectors in the
below retry loop."

Cheers, Daniel

> +	drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, CRTC_STEREO_DOUBLE);
> +	pipe_config->pipe_src_w = pipe_config->adjusted_mode.crtc_hdisplay;
> +	pipe_config->pipe_src_h = pipe_config->adjusted_mode.crtc_vdisplay;
> +
>  encoder_retry:
>  	/* Ensure the port clock defaults are reset when retrying. */
>  	pipe_config->port_clock = 0;
> @@ -8480,10 +8485,6 @@ encoder_retry:
>  	/* Fill in default crtc timings, allow encoders to overwrite them. */
>  	drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, CRTC_STEREO_DOUBLE);
>  
> -	/* set_crtcinfo() may have adjusted hdisplay/vdisplay */
> -	pipe_config->pipe_src_w = pipe_config->adjusted_mode.crtc_hdisplay;
> -	pipe_config->pipe_src_h = pipe_config->adjusted_mode.crtc_vdisplay;
> -
>  	/* Pass our mode to the connectors and the CRTC to give them a chance to
>  	 * adjust it according to limitations or connector properties, and also
>  	 * a chance to reject the mode entirely.
> -- 
> 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