On Mon, 2013-04-29 at 16:43 +0200, Daniel Vetter wrote:
> 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?
Ok, after the IRC discussion things are clearer now. Yea, would be good
to have have it in the commit message that eDP is a non-issue. With
that:
Reviewed-by: Imre Deak <imre.deak at intel.com>
--Imre
>
> 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;
> >
> >
>
