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
