| Message ID | 1303240361-6057-5-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 19 Apr 2011 12:12:38 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > Figuring out which pipe bpp to use is a bit painful. It depends on both > the encoder and display configuration attached to a pipe. For instance, > to drive a 24bpp framebuffer out to an 18bpp panel, we need to use 6bpc > on the pipe but also enable dithering. But driving that same > framebuffer to a DisplayPort output on another pipe means using 8bpc and > no dithering. > > So split out and enhance the code to handle the various cases, returning > an appropriate pipe bpp as well as whether dithering should be enabled. > > Save the resulting pipe bpp in the intel_crtc struct for use by encoders > in calculating bandwidth requirements (defaults to 24bpp on pre-ILK). > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_display.c | 181 ++++++++++++++++++++++++++-------- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 140 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 8ef0c39..e81e418 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4232,6 +4232,120 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv) > return dev_priv->lvds_use_ssc && i915_panel_use_ssc; > } > > +/** > + * intel_choose_pipe_bpp - figure out what color depth the pipe should send > + * @crtc: CRTC structure > + * > + * A pipe may be connected to one or more outputs. Based on the depth of the > + * attached framebuffer, choose a good color depth to use on the pipe. > + * > + * If possible, match the pipe depth to the fb depth. In some cases, this > + * isn't ideal, because the connected output supports a lesser or restricted > + * set of depths. Resolve that here: > + * LVDS typically supports only 6bpc, so clamp down in that case > + * HDMI supports only 8bpc or 12bpc, so clamp to 8bpc with dither for 10bpc > + * Displays may support a restricted set as well, check EDID and clamp as > + * appropriate. > + * > + * RETURNS: > + * Dithering requirement (i.e. false if display bpc and pipe bpc match, > + * true if they don't match). > + */ > +static bool intel_choose_pipe_bpp(struct drm_crtc *crtc, unsigned int > *pipe_bpp) Should mention dithering in the name, perhaps choose_pipe_bpp_and_dithering or some such? Perhaps returning the dithering in another by-reference parameter? > +{ > + struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_encoder *encoder; > + struct drm_connector *connector; > + unsigned int display_bpc = UINT_MAX, fb_bpc, bpc; > + > + fb_bpc = crtc->fb->depth / 3; > + > + /* Walk the encoders & connectors on this crtc, get min bpc */ Don't you want the max bpc? > + > + /* > + * We could just drive the pipe at the highest bpc all the time and > + * enable dithering as needed, but that costs bandwidth. So choose > + * the minimum value that expresses the full color range of the fb but > + * also stays within the max display bpc discovered above. > + */ > + > + switch (crtc->fb->depth) { > + case 8: > + case 15: > + case 16: > + bpc = 6; /* min is 18bpp */ > + break; > + case 24: > + bpc = min((unsigned int)8, display_bpc); > + break; > + case 30: > + bpc = min((unsigned int)10, display_bpc); > + break; > + case 48: > + bpc = min((unsigned int)12, display_bpc); > + break; > + default: > + DRM_DEBUG("unsupported depth, assuming 24 bits\n"); > + bpc = min((unsigned int)8, display_bpc); > + break; > + } Hrm. 8-bit through a colormap should probably use 8 bpc? We can probably ignore 15/16-bit direct color (which might want 8bpc as well). The simple fix would be to use 8bpc on 8bpp and otherwise leave things alone.
On Tue, 10 May 2011 14:23:18 -0700 Keith Packard <keithp@keithp.com> wrote: > On Tue, 19 Apr 2011 12:12:38 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > Figuring out which pipe bpp to use is a bit painful. It depends on both > > the encoder and display configuration attached to a pipe. For instance, > > to drive a 24bpp framebuffer out to an 18bpp panel, we need to use 6bpc > > on the pipe but also enable dithering. But driving that same > > framebuffer to a DisplayPort output on another pipe means using 8bpc and > > no dithering. > > > > So split out and enhance the code to handle the various cases, returning > > an appropriate pipe bpp as well as whether dithering should be enabled. > > > > Save the resulting pipe bpp in the intel_crtc struct for use by encoders > > in calculating bandwidth requirements (defaults to 24bpp on pre-ILK). > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/intel_display.c | 181 ++++++++++++++++++++++++++-------- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > 2 files changed, 140 insertions(+), 42 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 8ef0c39..e81e418 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4232,6 +4232,120 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv) > > return dev_priv->lvds_use_ssc && i915_panel_use_ssc; > > } > > > > +/** > > + * intel_choose_pipe_bpp - figure out what color depth the pipe should send > > + * @crtc: CRTC structure > > + * > > + * A pipe may be connected to one or more outputs. Based on the depth of the > > + * attached framebuffer, choose a good color depth to use on the pipe. > > + * > > + * If possible, match the pipe depth to the fb depth. In some cases, this > > + * isn't ideal, because the connected output supports a lesser or restricted > > + * set of depths. Resolve that here: > > + * LVDS typically supports only 6bpc, so clamp down in that case > > + * HDMI supports only 8bpc or 12bpc, so clamp to 8bpc with dither for 10bpc > > + * Displays may support a restricted set as well, check EDID and clamp as > > + * appropriate. > > + * > > + * RETURNS: > > + * Dithering requirement (i.e. false if display bpc and pipe bpc match, > > + * true if they don't match). > > + */ > > +static bool intel_choose_pipe_bpp(struct drm_crtc *crtc, unsigned int > > *pipe_bpp) > > Should mention dithering in the name, perhaps > choose_pipe_bpp_and_dithering or some such? Perhaps returning the > dithering in another by-reference parameter? Ok fixed, just added _dither to the name. > > > +{ > > + struct drm_device *dev = crtc->dev; > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + struct drm_encoder *encoder; > > + struct drm_connector *connector; > > + unsigned int display_bpc = UINT_MAX, fb_bpc, bpc; > > + > > + fb_bpc = crtc->fb->depth / 3; > > + > > + /* Walk the encoders & connectors on this crtc, get min bpc */ > > Don't you want the max bpc? If the comment is confusing, I can remove it. Or maybe I need to expand it. What we're trying to do is find a common bpc for all the displays attached to the pipe. But we need to use the minimum of all attached in order to set the pipe's dithering correctly, since dithering occurs at the pipe level, not the output (at least on ILK+). > > + > > + /* > > + * We could just drive the pipe at the highest bpc all the time and > > + * enable dithering as needed, but that costs bandwidth. So choose > > + * the minimum value that expresses the full color range of the fb but > > + * also stays within the max display bpc discovered above. > > + */ > > + > > + switch (crtc->fb->depth) { > > + case 8: > > + case 15: > > + case 16: > > + bpc = 6; /* min is 18bpp */ > > + break; > > + case 24: > > + bpc = min((unsigned int)8, display_bpc); > > + break; > > + case 30: > > + bpc = min((unsigned int)10, display_bpc); > > + break; > > + case 48: > > + bpc = min((unsigned int)12, display_bpc); > > + break; > > + default: > > + DRM_DEBUG("unsupported depth, assuming 24 bits\n"); > > + bpc = min((unsigned int)8, display_bpc); > > + break; > > + } > > Hrm. 8-bit through a colormap should probably use 8 bpc? We can probably > ignore 15/16-bit direct color (which might want 8bpc as well). > > The simple fix would be to use 8bpc on 8bpp and otherwise leave things alone. Ok, fixed. New patches on their way.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 8ef0c39..e81e418 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4232,6 +4232,120 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv) return dev_priv->lvds_use_ssc && i915_panel_use_ssc; } +/** + * intel_choose_pipe_bpp - figure out what color depth the pipe should send + * @crtc: CRTC structure + * + * A pipe may be connected to one or more outputs. Based on the depth of the + * attached framebuffer, choose a good color depth to use on the pipe. + * + * If possible, match the pipe depth to the fb depth. In some cases, this + * isn't ideal, because the connected output supports a lesser or restricted + * set of depths. Resolve that here: + * LVDS typically supports only 6bpc, so clamp down in that case + * HDMI supports only 8bpc or 12bpc, so clamp to 8bpc with dither for 10bpc + * Displays may support a restricted set as well, check EDID and clamp as + * appropriate. + * + * RETURNS: + * Dithering requirement (i.e. false if display bpc and pipe bpc match, + * true if they don't match). + */ +static bool intel_choose_pipe_bpp(struct drm_crtc *crtc, unsigned int *pipe_bpp) +{ + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_encoder *encoder; + struct drm_connector *connector; + unsigned int display_bpc = UINT_MAX, fb_bpc, bpc; + + fb_bpc = crtc->fb->depth / 3; + + /* Walk the encoders & connectors on this crtc, get min bpc */ + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + struct intel_encoder *intel_encoder = to_intel_encoder(encoder); + + if (encoder->crtc != crtc) + continue; + + if (intel_encoder->type == INTEL_OUTPUT_LVDS) { + unsigned int lvds_bpc; + + if ((I915_READ(PCH_LVDS) & LVDS_A3_POWER_MASK) == + LVDS_A3_POWER_UP) + lvds_bpc = 8; + else + lvds_bpc = 6; + + if (lvds_bpc < display_bpc) + display_bpc = lvds_bpc; + continue; + } + + if (intel_encoder->type == INTEL_OUTPUT_EDP) { + /* Use VBT settings if we have an eDP panel */ + unsigned int edp_bpc = dev_priv->edp.bpp / 3; + + if (edp_bpc < display_bpc) + display_bpc = edp_bpc; + continue; + } + + /* Not one of the known troublemakers, check the EDID */ + list_for_each_entry(connector, &dev->mode_config.connector_list, + head) { + if (connector->encoder != encoder) + continue; + + if (connector->display_info.bpc < display_bpc) + display_bpc = connector->display_info.bpc; + } + + /* + * HDMI is either 12 or 8, so if the display let 10bpc sneak + * through, clamp it down. (Note: >12bpc will be caught below.) + */ + if (intel_encoder->type == INTEL_OUTPUT_HDMI) { + if (display_bpc > 8 && display_bpc < 12) + display_bpc = 12; + else + display_bpc = 8; + } + } + + /* + * We could just drive the pipe at the highest bpc all the time and + * enable dithering as needed, but that costs bandwidth. So choose + * the minimum value that expresses the full color range of the fb but + * also stays within the max display bpc discovered above. + */ + + switch (crtc->fb->depth) { + case 8: + case 15: + case 16: + bpc = 6; /* min is 18bpp */ + break; + case 24: + bpc = min((unsigned int)8, display_bpc); + break; + case 30: + bpc = min((unsigned int)10, display_bpc); + break; + case 48: + bpc = min((unsigned int)12, display_bpc); + break; + default: + DRM_DEBUG("unsupported depth, assuming 24 bits\n"); + bpc = min((unsigned int)8, display_bpc); + break; + } + + *pipe_bpp = bpc * 3; + + return display_bpc != bpc; +} + static int i9xx_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode, @@ -4644,7 +4758,9 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, struct fdi_m_n m_n = {0}; u32 temp; u32 lvds_sync = 0; - int target_clock, pixel_multiplier, lane, link_bw, bpp, factor; + int target_clock, pixel_multiplier, lane, link_bw, factor; + unsigned int pipe_bpp; + bool dither; list_for_each_entry(encoder, &mode_config->encoder_list, base.head) { if (encoder->base.crtc != crtc) @@ -4771,56 +4887,37 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, /* determine panel color depth */ temp = I915_READ(PIPECONF(pipe)); temp &= ~PIPE_BPC_MASK; - if (is_lvds) { - /* the BPC will be 6 if it is 18-bit LVDS panel */ - if ((I915_READ(PCH_LVDS) & LVDS_A3_POWER_MASK) == LVDS_A3_POWER_UP) - temp |= PIPE_8BPC; - else - temp |= PIPE_6BPC; - } else if (has_edp_encoder) { - switch (dev_priv->edp.bpp/3) { - case 8: - temp |= PIPE_8BPC; - break; - case 10: - temp |= PIPE_10BPC; - break; - case 6: - temp |= PIPE_6BPC; - break; - case 12: - temp |= PIPE_12BPC; - break; - } - } else - temp |= PIPE_8BPC; - I915_WRITE(PIPECONF(pipe), temp); - - switch (temp & PIPE_BPC_MASK) { - case PIPE_8BPC: - bpp = 24; + dither = intel_choose_pipe_bpp(crtc, &pipe_bpp); + switch (pipe_bpp) { + case 18: + temp |= PIPE_6BPC; break; - case PIPE_10BPC: - bpp = 30; + case 24: + temp |= PIPE_8BPC; break; - case PIPE_6BPC: - bpp = 18; + case 30: + temp |= PIPE_10BPC; break; - case PIPE_12BPC: - bpp = 36; + case 36: + temp |= PIPE_12BPC; break; default: - DRM_ERROR("unknown pipe bpc value\n"); - bpp = 24; + WARN(1, "intel_choose_pipe_bpp returned invalid value\n"); + temp |= PIPE_8BPC; + pipe_bpp = 24; + break; } + intel_crtc->bpp = pipe_bpp; + I915_WRITE(PIPECONF(pipe), temp); + if (!lane) { /* * Account for spread spectrum to avoid * oversubscribing the link. Max center spread * is 2.5%; use 5% for safety's sake. */ - u32 bps = target_clock * bpp * 21 / 20; + u32 bps = target_clock * intel_crtc->bpp * 21 / 20; lane = bps / (link_bw * 8) + 1; } @@ -4828,7 +4925,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, if (pixel_multiplier > 1) link_bw *= pixel_multiplier; - ironlake_compute_m_n(bpp, lane, target_clock, link_bw, &m_n); + ironlake_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw, + &m_n); /* Ironlake: try to setup display ref clock before DPLL * enabling. This is only under driver's control after @@ -5031,14 +5129,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, I915_WRITE(PCH_LVDS, temp); } - /* set the dithering flag and clear for anything other than a panel. */ pipeconf &= ~PIPECONF_DITHER_EN; pipeconf &= ~PIPECONF_DITHER_TYPE_MASK; - if (dev_priv->lvds_dither && (is_lvds || has_edp_encoder)) { + if ((is_lvds && dev_priv->lvds_dither) || dither) { pipeconf |= PIPECONF_DITHER_EN; pipeconf |= PIPECONF_DITHER_TYPE_ST1; } - if (is_dp || intel_encoder_is_pch_edp(&has_edp_encoder->base)) { intel_dp_set_m_n(crtc, mode, adjusted_mode); } else { @@ -6330,6 +6426,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) intel_crtc_reset(&intel_crtc->base); intel_crtc->active = true; /* force the pipe off on setup_init_config */ + intel_crtc->bpp = 24; /* default for pre-Ironlake */ if (HAS_PCH_SPLIT(dev)) { intel_helper_funcs.prepare = ironlake_crtc_prepare; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index f5b0d83..051345c 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -171,6 +171,7 @@ struct intel_crtc { int16_t cursor_x, cursor_y; int16_t cursor_width, cursor_height; bool cursor_visible; + unsigned int bpp; }; #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
Figuring out which pipe bpp to use is a bit painful. It depends on both the encoder and display configuration attached to a pipe. For instance, to drive a 24bpp framebuffer out to an 18bpp panel, we need to use 6bpc on the pipe but also enable dithering. But driving that same framebuffer to a DisplayPort output on another pipe means using 8bpc and no dithering. So split out and enhance the code to handle the various cases, returning an appropriate pipe bpp as well as whether dithering should be enabled. Save the resulting pipe bpp in the intel_crtc struct for use by encoders in calculating bandwidth requirements (defaults to 24bpp on pre-ILK). Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/intel_display.c | 181 ++++++++++++++++++++++++++-------- drivers/gpu/drm/i915/intel_drv.h | 1 + 2 files changed, 140 insertions(+), 42 deletions(-)