[PATCH 15/15] drm/i915: implement fdi auto-dithering

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Apr 29, 2013 at 05:02:20PM +0300, Imre Deak wrote:
> On Fri, 2013-04-19 at 11:24 +0200, Daniel Vetter wrote:
> > So on a bunch of setups we only have 2 fdi lanes available, e.g. hsw
> > VGA or 3 pipes on ivb. And seemingly a lot of modes don't quite fit
> > into this, among them the default 1080p mode.
> > 
> > The solution is to dither down the pipe a bit so that everything fits,
> > which this patch implements.
> > 
> > But ports compute their state under the assumption that the bpp they
> > pick will be the one selected, e.g. the display port bw computations
> > won't work otherwise. Now we could adjust our code to again up-dither
> > to the computed DP link parameters, but that's pointless.
> > 
> > So instead when the pipe needs to adjust parameters we need to retry
> > the pipe_config computation at the encoder stage. Furthermore we need
> > to inform encoders that they should not increase bandwidth
> > requirements if possible. This is required for the hdmi code, which
> > prefers the pipe to up-dither to either of the two possible hdmi bpc
> > values.
> > 
> > LVDS has a similar requirement, although that's probably only
> > theoretical in nature: It's unlikely that we'll ever see an 8bpc
> > high-res lvds panel (which is required to hit the 2 fdi lane limit).
> > 
> > v2: Rebased on top of a bikeshed from Paulo.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> >
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 56 ++++++++++++++++++++++++++++--------
> >  drivers/gpu/drm/i915/intel_drv.h     |  7 +++++
> >  drivers/gpu/drm/i915/intel_hdmi.c    | 14 ++++++---
> >  drivers/gpu/drm/i915/intel_lvds.c    |  2 +-
> >  4 files changed, 62 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index c25dbdd..6d35ccd 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4021,13 +4021,16 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
> >  	}
> >  }
> >  
> > -static bool ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> > -					struct intel_crtc_config *pipe_config)
> > +#define RETRY 1
> > +static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> > +				       struct intel_crtc_config *pipe_config)
> >  {
> >  	struct drm_device *dev = intel_crtc->base.dev;
> >  	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> >  	int target_clock, lane, link_bw;
> > +	bool setup_ok, needs_recompute = false;
> >  
> > +retry:
> >  	/* FDI is a binary signal running at ~2.7GHz, encoding
> >  	 * each output octet as 10 bits. The actual frequency
> >  	 * is stored as a divider into a 100MHz clock, and the
> > @@ -4052,12 +4055,26 @@ static bool ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> >  	intel_link_compute_m_n(pipe_config->pipe_bpp, lane, target_clock,
> >  			       link_bw, &pipe_config->fdi_m_n);
> >  
> > -	return ironlake_check_fdi_lanes(intel_crtc->base.dev,
> > -					intel_crtc->pipe, pipe_config);
> > +	setup_ok = ironlake_check_fdi_lanes(intel_crtc->base.dev,
> > +					    intel_crtc->pipe, pipe_config);
> > +	if (!setup_ok && pipe_config->pipe_bpp > 6*3) {
> > +		pipe_config->pipe_bpp -= 2*3;
> > +		DRM_DEBUG_KMS("fdi link bw constraint, reducing pipe bpp to %i\n",
> > +			      pipe_config->pipe_bpp);
> > +		needs_recompute = true;
> > +		pipe_config->bw_constrained = true;
> > +
> > +		goto retry;
> > +	}
> > +
> > +	if (needs_recompute)
> > +		return RETRY;
> > +
> > +	return setup_ok ? 0 : -EINVAL;
> >  }
> >  
> > -static bool intel_crtc_compute_config(struct drm_crtc *crtc,
> > -				      struct intel_crtc_config *pipe_config)
> > +static int intel_crtc_compute_config(struct drm_crtc *crtc,
> > +				     struct intel_crtc_config *pipe_config)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> > @@ -4066,7 +4083,7 @@ static bool intel_crtc_compute_config(struct drm_crtc *crtc,
> >  		/* FDI link clock is fixed at 2.7G */
> >  		if (pipe_config->requested_mode.clock * 3
> >  		    > IRONLAKE_FDI_FREQ * 4)
> > -			return false;
> > +			return -EINVAL;
> >  	}
> >  
> >  	/* All interlaced capable intel hw wants timings in frames. Note though
> > @@ -4080,7 +4097,7 @@ static bool intel_crtc_compute_config(struct drm_crtc *crtc,
> >  	 */
> >  	if ((INTEL_INFO(dev)->gen > 4 || IS_G4X(dev)) &&
> >  		adjusted_mode->hsync_start == adjusted_mode->hdisplay)
> > -		return false;
> > +		return -EINVAL;
> >  
> >  	if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) && pipe_config->pipe_bpp > 10*3) {
> >  		pipe_config->pipe_bpp = 10*3; /* 12bpc is gen5+ */
> > @@ -4093,7 +4110,7 @@ static bool intel_crtc_compute_config(struct drm_crtc *crtc,
> >  	if (pipe_config->has_pch_encoder)
> >  		return ironlake_fdi_compute_config(to_intel_crtc(crtc), pipe_config);
> >  
> > -	return true;
> > +	return 0;
> >  }
> >  
> >  static int valleyview_get_display_clock_speed(struct drm_device *dev)
> > @@ -7673,7 +7690,8 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> >  	struct drm_encoder_helper_funcs *encoder_funcs;
> >  	struct intel_encoder *encoder;
> >  	struct intel_crtc_config *pipe_config;
> > -	int plane_bpp;
> > +	int plane_bpp, ret = -EINVAL;
> > +	bool retry = true;
> >  
> >  	pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL);
> >  	if (!pipe_config)
> > @@ -7686,6 +7704,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> >  	if (plane_bpp < 0)
> >  		goto fail;
> >  
> > +encoder_retry:
> >  	/* 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.
> > @@ -7714,10 +7733,23 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> >  		}
> >  	}
> >  
> > -	if (!(intel_crtc_compute_config(crtc, pipe_config))) {
> > +	ret = intel_crtc_compute_config(crtc, pipe_config);
> > +	if (ret < 0) {
> >  		DRM_DEBUG_KMS("CRTC fixup failed\n");
> >  		goto fail;
> >  	}
> > +
> > +	if (ret == RETRY) {
> > +		if (WARN(!retry, "loop in pipe configuration computation\n")) {
> > +			ret = -EINVAL;
> > +			goto fail;
> 
> Isn't it possible that intel_dp_compute_config increases pipe_bpp when
> it forces pipe_bpp to what the firmware has set? In that case could hit
> this WARN.

Yeah, but that would mean that the firmware asks us for a configuration
for which we simply do not have enough bandwidth. So I don't think that
we'll actually hit this in reality.

If we do I guess we need to add a check into intel_dp_compute_config to
see whether we're in the a) retry loop and b) try to increase bpp and then
just fail the compute_config stage. But as long as the entire eDP mess is
a bit unclear I'd like to avoid such complexity until we have a proven
need.

Want me to add something like the above to the commit message?

Cheers, Daniel

> 
> > +		}
> > +
> > +		DRM_DEBUG_KMS("CRTC bw constrained, retrying\n");
> > +		retry = false;
> > +		goto encoder_retry;
> > +	}
> > +
> >  	DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
> >  
> >  	pipe_config->dither = pipe_config->pipe_bpp != plane_bpp;
> > @@ -7727,7 +7759,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> >  	return pipe_config;
> >  fail:
> >  	kfree(pipe_config);
> > -	return ERR_PTR(-EINVAL);
> > +	return ERR_PTR(ret);
> >  }
> >  
> >  /* Computes which crtcs are affected and sets the relevant bits in the mask. For
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index f40b43f..c14afc6 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -211,6 +211,13 @@ struct intel_crtc_config {
> >  	/* Controls for the clock computation, to override various stages. */
> >  	bool clock_set;
> >  
> > +	/*
> > +	 * crtc bandwidth limit, don't increase pipe bpp or clock if not really
> > +	 * required. This is set in the 2nd loop of calling encoder's
> > +	 * ->compute_config if the first pick doesn't work out.
> > +	 */
> > +	bool bw_constrained;
> > +
> >  	/* Settings for the intel dpll used on pretty much everything but
> >  	 * haswell. */
> >  	struct dpll {
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index a8273c7..3942041 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -784,6 +784,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >  	struct drm_device *dev = encoder->base.dev;
> >  	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> >  	int clock_12bpc = pipe_config->requested_mode.clock * 3 / 2;
> > +	int desired_bpp;
> >  
> >  	if (intel_hdmi->color_range_auto) {
> >  		/* See CEA-861-E - 5.1 Default Encoding Parameters */
> > @@ -808,16 +809,21 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >  	 */
> >  	if (pipe_config->pipe_bpp > 8*3 && clock_12bpc < 225000
> >  	    && HAS_PCH_SPLIT(dev)) {
> > -		DRM_DEBUG_KMS("forcing bpc to 12 for HDMI\n");
> > -		pipe_config->pipe_bpp = 12*3;
> > +		DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n");
> > +		desired_bpp = 12*3;
> >  
> >  		/* Need to adjust the port link by 1.5x for 12bpc. */
> >  		adjusted_mode->clock = clock_12bpc;
> >  		pipe_config->pixel_target_clock =
> >  			pipe_config->requested_mode.clock;
> >  	} else {
> > -		DRM_DEBUG_KMS("forcing bpc to 8 for HDMI\n");
> > -		pipe_config->pipe_bpp = 8*3;
> > +		DRM_DEBUG_KMS("picking bpc to 8 for HDMI output\n");
> > +		desired_bpp = 8*3;
> > +	}
> > +
> > +	if (!pipe_config->bw_constrained) {
> > +		DRM_DEBUG_KMS("forcing pipe bpc to %i for HDMI\n", desired_bpp);
> > +		pipe_config->pipe_bpp = desired_bpp;
> >  	}
> >  
> >  	if (adjusted_mode->clock > 225000) {
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 094f3c5..c426581 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -331,7 +331,7 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
> >  	else
> >  		lvds_bpp = 6*3;
> >  
> > -	if (lvds_bpp != pipe_config->pipe_bpp) {
> > +	if (lvds_bpp != pipe_config->pipe_bpp && !pipe_config->bw_constrained) {
> >  		DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n",
> >  			      pipe_config->pipe_bpp, lvds_bpp);
> >  		pipe_config->pipe_bpp = lvds_bpp;
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux