Here's a couple of patches that fix some bpc (bits per component) computation issues with DisplayPort. The problem was that the DisplayPort code tried to figure out the 'current' bpc by looking at the bpp stored in an associated crtc, but that was never right (as described in the message for the first patch).
The first patch assumes that the display will run at 8bpc (24bpp) if the link has enough bandwidth, otherwise at 6bpc (18bpp). This is essentially what the existing code ends up doing at boot time; modes are computed before any crtc is assigned, so intel_dp_link_required would have used 24bpp for bandwidth computations.
The second patch allows for arbitrary bpc values, computing the display bpc in both intel_dp_mode_fixup and the two crtc_mode_set functions. Obviously doing the computation once would be nice, but there isn't an obvious place to stick the result between those two functions as the bpc computation is also needed for non-DP encoders.
This should fix problems where display port doesn't come back after resume for panels which need 6bpc modes.
-- keith.packard@intel.com
It is never correct to use intel_crtc->bpp in intel_dp_link_required, so instead pass an explicit bpp in to this function. This patch only supports 18bpp and 24bpp modes, which means that 10bpc modes will be computed incorrectly. Fixing that will require more extensive changes, and so must be addressed separately from this bugfix.
intel_dp_link_required is called from intel_dp_mode_valid and intel_dp_mode_fixup.
* intel_dp_mode_valid is called to list supported modes; in this case, the current crtc values cannot be relevant as the modes in question may never be selected. Thus, using intel_crtc->bpp is never right.
* intel_dp_mode_fixup is called during mode setting, but it is run well before ironlake_crtc_mode_set is called to set intel_crtc->bpp, so using intel_crtc-bpp in this path can only ever get a stale value.
Cc: Lubos Kolouch lubos.kolouch@gmail.com Cc: Adam Jackson ajax@redhat.com Signed-off-by: Keith Packard keithp@keithp.com --- drivers/gpu/drm/i915/intel_dp.c | 20 +++++--------------- 1 files changed, 5 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index db3b461..94f860c 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -208,17 +208,8 @@ intel_dp_link_clock(uint8_t link_bw) */
static int -intel_dp_link_required(struct intel_dp *intel_dp, int pixel_clock, int check_bpp) +intel_dp_link_required(int pixel_clock, int bpp) { - struct drm_crtc *crtc = intel_dp->base.base.crtc; - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - int bpp = 24; - - if (check_bpp) - bpp = check_bpp; - else if (intel_crtc) - bpp = intel_crtc->bpp; - return (pixel_clock * bpp + 9) / 10; }
@@ -245,12 +236,11 @@ intel_dp_mode_valid(struct drm_connector *connector, return MODE_PANEL; }
- mode_rate = intel_dp_link_required(intel_dp, mode->clock, 0); + mode_rate = intel_dp_link_required(mode->clock, 24); max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
if (mode_rate > max_rate) { - mode_rate = intel_dp_link_required(intel_dp, - mode->clock, 18); + mode_rate = intel_dp_link_required(mode->clock, 18); if (mode_rate > max_rate) return MODE_CLOCK_HIGH; else @@ -683,7 +673,7 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode, int lane_count, clock; int max_lane_count = intel_dp_max_lane_count(intel_dp); int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0; - int bpp = mode->private_flags & INTEL_MODE_DP_FORCE_6BPC ? 18 : 0; + int bpp = mode->private_flags & INTEL_MODE_DP_FORCE_6BPC ? 18 : 24; static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
if (is_edp(intel_dp) && intel_dp->panel_fixed_mode) { @@ -701,7 +691,7 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode, for (clock = 0; clock <= max_clock; clock++) { int link_avail = intel_dp_max_data_rate(intel_dp_link_clock(bws[clock]), lane_count);
- if (intel_dp_link_required(intel_dp, mode->clock, bpp) + if (intel_dp_link_required(mode->clock, bpp) <= link_avail) { intel_dp->link_bw = bws[clock]; intel_dp->lane_count = lane_count;
On Wed, 25 Jan 2012 08:16:25 -0800 Keith Packard keithp@keithp.com wrote:
It is never correct to use intel_crtc->bpp in intel_dp_link_required, so instead pass an explicit bpp in to this function. This patch only supports 18bpp and 24bpp modes, which means that 10bpc modes will be computed incorrectly. Fixing that will require more extensive changes, and so must be addressed separately from this bugfix.
intel_dp_link_required is called from intel_dp_mode_valid and intel_dp_mode_fixup.
intel_dp_mode_valid is called to list supported modes; in this case, the current crtc values cannot be relevant as the modes in question may never be selected. Thus, using intel_crtc->bpp is never right.
intel_dp_mode_fixup is called during mode setting, but it is run well before ironlake_crtc_mode_set is called to set intel_crtc->bpp, so using intel_crtc-bpp in this path can only ever get a stale value.
Yeah, looks like I got this wrong when I added the pipe bpp field. Wonder if there's a good way to catch this sort of bug with an assert so we don't get the order mixed up again...
The big downside here is that we'll be very pessimistic about the link bw requirements for say 16 or 8bpp modes.
I see you have another patch to address some of this, but I wonder if we have enough info to calculate the bpp at prepare time so it's set early on for use by both the crtc and encoder code?
On Wed, 25 Jan 2012 12:51:16 -0800, Jesse Barnes jbarnes@virtuousgeek.org wrote:
Yeah, looks like I got this wrong when I added the pipe bpp field. Wonder if there's a good way to catch this sort of bug with an assert so we don't get the order mixed up again...
Ideally, we'd figure out a way to compute this only once. I think the only relevant piece of data here is what bpc the pipe should run at, and that can be computed at any time during the mode set. We'd then be able to compare the pipe bpc with the frame buffer bpp to decide when to dither in the crtc mode set code.
The big downside here is that we'll be very pessimistic about the link bw requirements for say 16 or 8bpp modes.
Right, with this patch, we'll choose link parameters capable of supporting 8bpc, even for 16bpp video modes. Note that 8bpp video modes need 8bpc links as they have colormaps with 8bpc data in them.
I see you have another patch to address some of this, but I wonder if we have enough info to calculate the bpp at prepare time so it's set early on for use by both the crtc and encoder code?
fixup gets exactly the same info as mode set, so it should end up with exactly the same resulting bpc, which will set the link bandwidth to support 6bpc mode if that's all that is needed.
One problem with the patch is that the pre-PCH mode set path doesn't use intel_choose_pipe_bpp_dither, so the condition for 6bpc mode is duplicated in i9xx_crtc_mode_set and intel_dp_mode_fixup.
If crtc->fixup was called before encoder->fixup, we could have computed bpp in the crtc->fixup function and then used it in encoder->fixup.
Alternatively, crtc->fixup could call into the DP code to set the link_bw, lane_count and adjusted_mode->clock values after it sets the bpp, but that seems to break the abstraction even more.
On Wed, Jan 25, 2012 at 01:17:51PM -0800, Keith Packard wrote:
On Wed, 25 Jan 2012 12:51:16 -0800, Jesse Barnes jbarnes@virtuousgeek.org wrote:
Yeah, looks like I got this wrong when I added the pipe bpp field. Wonder if there's a good way to catch this sort of bug with an assert so we don't get the order mixed up again...
Ideally, we'd figure out a way to compute this only once. I think the only relevant piece of data here is what bpc the pipe should run at, and that can be computed at any time during the mode set. We'd then be able to compare the pipe bpc with the frame buffer bpp to decide when to dither in the crtc mode set code.
I think we could compute this in crtc->mode_fixup (crtc->prepare doesn't have the mode and adjusted_mode arguments). We could then store the computed bpc and dithering in one of the private fields. We'd still have to loop over all encoders, but alas ...
The big downside here is that we'll be very pessimistic about the link bw requirements for say 16 or 8bpp modes.
Right, with this patch, we'll choose link parameters capable of supporting 8bpc, even for 16bpp video modes. Note that 8bpp video modes need 8bpc links as they have colormaps with 8bpc data in them.
Afaics we'll still correctly fall back to 6bpc (undithered for 16bpp obviously) and hence things should keep on working.
I see you have another patch to address some of this, but I wonder if we have enough info to calculate the bpp at prepare time so it's set early on for use by both the crtc and encoder code?
fixup gets exactly the same info as mode set, so it should end up with exactly the same resulting bpc, which will set the link bandwidth to support 6bpc mode if that's all that is needed.
One problem with the patch is that the pre-PCH mode set path doesn't use intel_choose_pipe_bpp_dither, so the condition for 6bpc mode is duplicated in i9xx_crtc_mode_set and intel_dp_mode_fixup.
If crtc->fixup was called before encoder->fixup, we could have computed bpp in the crtc->fixup function and then used it in encoder->fixup.
Alternatively, crtc->fixup could call into the DP code to set the link_bw, lane_count and adjusted_mode->clock values after it sets the bpp, but that seems to break the abstraction even more.
Yeah, there are a few rough corners with the bpc computation in patch 2. I'll try to throw around a few ideas that crossed my mind while reading through it in a reply there. -Daniel
On Wed, 25 Jan 2012 22:50:55 +0100, Daniel Vetter daniel@ffwll.ch wrote:
I think we could compute this in crtc->mode_fixup (crtc->prepare doesn't have the mode and adjusted_mode arguments). We could then store the computed bpc and dithering in one of the private fields. We'd still have to loop over all encoders, but alas ...
Alas, intel_crtc_mode_fixup is called *after* the intel_dp_mode_fixup. So, we'd either need to change drm_crtc_helper, or have intel_crtc_mode_fixup call down into intel_dp.c to set the link parameters. In either case, ick.
Afaics we'll still correctly fall back to 6bpc (undithered for 16bpp obviously) and hence things should keep on working.
Right, the problem is that the DP link parameters will be set to support 24bpp color, so we'll use a higher clock/lane-count than strictly necessary as intel_dp_mode_fixup doesn't take the frame buffer format into consideration when computing the link values.
Yeah, there are a few rough corners with the bpc computation in patch 2. I'll try to throw around a few ideas that crossed my mind while reading through it in a reply there.
Thanks. I'm not happy with it either.
In short, I think we can (and should) apply the simple first patch to drm-intel-fixes so that at least displays work consistently, and then come up with a nicer patch that computes correct link parameters, and also supports 10bpc formats.
On Wed, Jan 25, 2012 at 08:16:25AM -0800, Keith Packard wrote:
It is never correct to use intel_crtc->bpp in intel_dp_link_required, so instead pass an explicit bpp in to this function. This patch only supports 18bpp and 24bpp modes, which means that 10bpc modes will be computed incorrectly. Fixing that will require more extensive changes, and so must be addressed separately from this bugfix.
intel_dp_link_required is called from intel_dp_mode_valid and intel_dp_mode_fixup.
intel_dp_mode_valid is called to list supported modes; in this case, the current crtc values cannot be relevant as the modes in question may never be selected. Thus, using intel_crtc->bpp is never right.
intel_dp_mode_fixup is called during mode setting, but it is run well before ironlake_crtc_mode_set is called to set intel_crtc->bpp, so using intel_crtc-bpp in this path can only ever get a stale value.
Cc: Lubos Kolouch lubos.kolouch@gmail.com Cc: Adam Jackson ajax@redhat.com Signed-off-by: Keith Packard keithp@keithp.com
Afaics this is correct and should fix quite a few "dp doesn't light up issue" (in combination with 6bpc auto-dither code that's already there). I think all the open issues are only about how to make this less pessimistic and more generic, i.e. patch 2.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
On Wed, Jan 25, 2012 at 08:16:25AM -0800, Keith Packard wrote:
It is never correct to use intel_crtc->bpp in intel_dp_link_required, so instead pass an explicit bpp in to this function. This patch only supports 18bpp and 24bpp modes, which means that 10bpc modes will be computed incorrectly. Fixing that will require more extensive changes, and so must be addressed separately from this bugfix.
intel_dp_link_required is called from intel_dp_mode_valid and intel_dp_mode_fixup.
intel_dp_mode_valid is called to list supported modes; in this case, the current crtc values cannot be relevant as the modes in question may never be selected. Thus, using intel_crtc->bpp is never right.
intel_dp_mode_fixup is called during mode setting, but it is run well before ironlake_crtc_mode_set is called to set intel_crtc->bpp, so using intel_crtc-bpp in this path can only ever get a stale value.
Cc: Lubos Kolouch lubos.kolouch@gmail.com Cc: Adam Jackson ajax@redhat.com Signed-off-by: Keith Packard keithp@keithp.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42263 Tested-by: camalot@picnicpark.org (Dell Latitude 6510)
Not the original reporter and might not exactly be this bug, but likely. -Daniel
On Fri, Jan 27, 2012 at 10:30 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Jan 25, 2012 at 08:16:25AM -0800, Keith Packard wrote:
It is never correct to use intel_crtc->bpp in intel_dp_link_required, so instead pass an explicit bpp in to this function. This patch only supports 18bpp and 24bpp modes, which means that 10bpc modes will be computed incorrectly. Fixing that will require more extensive changes, and so must be addressed separately from this bugfix.
intel_dp_link_required is called from intel_dp_mode_valid and intel_dp_mode_fixup.
- intel_dp_mode_valid is called to list supported modes; in this case,
the current crtc values cannot be relevant as the modes in question may never be selected. Thus, using intel_crtc->bpp is never right.
- intel_dp_mode_fixup is called during mode setting, but it is run
well before ironlake_crtc_mode_set is called to set intel_crtc->bpp, so using intel_crtc-bpp in this path can only ever get a stale value.
Cc: Lubos Kolouch lubos.kolouch@gmail.com Cc: Adam Jackson ajax@redhat.com Signed-off-by: Keith Packard keithp@keithp.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42263 Tested-by: camalot@picnicpark.org (Dell Latitude 6510)
Not the original reporter and might not exactly be this bug, but likely. -Daniel
Tested-by: Dave Airlie airlied@redhat.com
Without this patch the eDP panel on my HP 2540p won't resume.
Please get this into -fixes and too me asap.
Dave.
On Mon, Feb 06, 2012 at 12:12:16PM +0000, Dave Airlie wrote:
On Fri, Jan 27, 2012 at 10:30 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Jan 25, 2012 at 08:16:25AM -0800, Keith Packard wrote:
It is never correct to use intel_crtc->bpp in intel_dp_link_required, so instead pass an explicit bpp in to this function. This patch only supports 18bpp and 24bpp modes, which means that 10bpc modes will be computed incorrectly. Fixing that will require more extensive changes, and so must be addressed separately from this bugfix.
intel_dp_link_required is called from intel_dp_mode_valid and intel_dp_mode_fixup.
- intel_dp_mode_valid is called to list supported modes; in this case,
the current crtc values cannot be relevant as the modes in question may never be selected. Thus, using intel_crtc->bpp is never right.
- intel_dp_mode_fixup is called during mode setting, but it is run
well before ironlake_crtc_mode_set is called to set intel_crtc->bpp, so using intel_crtc-bpp in this path can only ever get a stale value.
Cc: Lubos Kolouch lubos.kolouch@gmail.com Cc: Adam Jackson ajax@redhat.com Signed-off-by: Keith Packard keithp@keithp.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42263 Tested-by: camalot@picnicpark.org (Dell Latitude 6510)
Not the original reporter and might not exactly be this bug, but likely. -Daniel
Tested-by: Dave Airlie airlied@redhat.com
Another bugzilla for this patch:
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44881 Tested-by: Roland Dreier roland@digitalvampire.org
-Daniel
On Mon, 6 Feb 2012 12:12:16 +0000, Dave Airlie airlied@gmail.com wrote:
Please get this into -fixes and too me asap.
It's in -fixes; will be on its way to you soon.
This patch is sufficient to make machines work again; the second patch serves to improve things a bit, and (as such) should probably wait in drm-intel-next for another release.
The DP configuration must match the pipe configuration, and until mode set we don't know what BPC will be used. Delay all decisions about BPC until mode set, computing the target BPC in both intel_dp_mode_fixup and either i9xx_crtc_mode_set or ironlake_crtc_mode_set. It might be slightly better to compute this only once, but storing the value between those two calls would be a pain.
Cc: Lubos Kolouch lubos.kolouch@gmail.com Cc: Adam Jackson ajax@redhat.com Signed-off-by: Keith Packard keithp@keithp.com --- drivers/gpu/drm/i915/intel_display.c | 27 +++++------- drivers/gpu/drm/i915/intel_dp.c | 77 +++++++++++++++++++++++++++------- drivers/gpu/drm/i915/intel_drv.h | 6 ++- 3 files changed, 78 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b3b51c4..d613676 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4850,9 +4850,9 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv) * Dithering requirement (i.e. false if display bpc and pipe bpc match, * true if they don't match). */ -static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc, - unsigned int *pipe_bpp, - struct drm_display_mode *mode) +bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc, + unsigned int *pipe_bpp, + struct drm_display_mode *mode) { struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; @@ -4883,13 +4883,13 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc, 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 (intel_encoder->type == INTEL_OUTPUT_EDP || + intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) { + unsigned int dp_bpc = intel_dp_max_bpp(&intel_encoder->base, mode);
- if (edp_bpc < display_bpc) { - DRM_DEBUG_KMS("clamping display bpc (was %d) to eDP (%d)\n", display_bpc, edp_bpc); - display_bpc = edp_bpc; + if (dp_bpc < display_bpc) { + DRM_DEBUG_KMS("clamping display bpc (was %d) to DP (%d)\n", display_bpc, dp_bpc); + display_bpc = dp_bpc; } continue; } @@ -4923,11 +4923,6 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc, } }
- if (mode->private_flags & INTEL_MODE_DP_FORCE_6BPC) { - DRM_DEBUG_KMS("Dithering DP to 6bpc\n"); - display_bpc = 6; - } - /* * We could just drive the pipe at the highest bpc all the time and * enable dithering as needed, but that costs bandwidth. So choose @@ -4990,6 +4985,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, int ret; u32 temp; u32 lvds_sync = 0; + int dp_max_bpp = 0;
list_for_each_entry(encoder, &mode_config->encoder_list, base.head) { if (encoder->base.crtc != crtc) @@ -5016,6 +5012,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, break; case INTEL_OUTPUT_DISPLAYPORT: is_dp = true; + dp_max_bpp = intel_dp_max_bpp(&encoder->base, mode); break; }
@@ -5192,7 +5189,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, /* default to 8bpc */ pipeconf &= ~(PIPECONF_BPP_MASK | PIPECONF_DITHER_EN); if (is_dp) { - if (mode->private_flags & INTEL_MODE_DP_FORCE_6BPC) { + if (dp_max_bpp <= 18) { pipeconf |= PIPECONF_BPP_6 | PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP; diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 94f860c..2482796 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -219,14 +219,51 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes) return (max_link_clock * max_lanes * 8) / 10; }
+/* + * For the specified encoder, return the maximum bpp that can be used + * for the specified mode. + */ + +static const int dp_supported_bpc[] = { 6, 8, 10, 12, 16 }; + +#define NUM_DP_SUPPORTED_BPC (sizeof dp_supported_bpc/sizeof dp_supported_bpc[0]) + +int +intel_dp_max_bpp(struct drm_encoder *encoder, struct drm_display_mode *mode) +{ + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); + int max_link_clock = intel_dp_link_clock(intel_dp_max_link_bw(intel_dp)); + int max_lanes = intel_dp_max_lane_count(intel_dp); + int max_rate, mode_rate; + int i; + + max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes); + for (i = NUM_DP_SUPPORTED_BPC - 1; i >= 0; i--) { + int bpp = dp_supported_bpc[i] * 3; /* 3 channels of data (RGB) */ + + mode_rate = intel_dp_link_required(mode->clock, bpp); + if (mode_rate <= max_rate) { + + /* Limit EDP bpp to panel ability */ + if (intel_dp->base.type == INTEL_OUTPUT_EDP) { + struct drm_device *dev = encoder->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + int edp_bpp = dev_priv->edp.bpp; + + if (bpp > edp_bpp) + bpp = edp_bpp; + } + return bpp; + } + } + return 0; +} + static int intel_dp_mode_valid(struct drm_connector *connector, struct drm_display_mode *mode) { struct intel_dp *intel_dp = intel_attached_dp(connector); - int max_link_clock = intel_dp_link_clock(intel_dp_max_link_bw(intel_dp)); - int max_lanes = intel_dp_max_lane_count(intel_dp); - int max_rate, mode_rate;
if (is_edp(intel_dp) && intel_dp->panel_fixed_mode) { if (mode->hdisplay > intel_dp->panel_fixed_mode->hdisplay) @@ -236,16 +273,8 @@ intel_dp_mode_valid(struct drm_connector *connector, return MODE_PANEL; }
- mode_rate = intel_dp_link_required(mode->clock, 24); - max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes); - - if (mode_rate > max_rate) { - mode_rate = intel_dp_link_required(mode->clock, 18); - if (mode_rate > max_rate) - return MODE_CLOCK_HIGH; - else - mode->private_flags |= INTEL_MODE_DP_FORCE_6BPC; - } + if (intel_dp_max_bpp(&intel_dp->base.base, mode) == 0) + return MODE_CLOCK_HIGH;
if (mode->clock < 10000) return MODE_CLOCK_LOW; @@ -673,9 +702,26 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode, int lane_count, clock; int max_lane_count = intel_dp_max_lane_count(intel_dp); int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0; - int bpp = mode->private_flags & INTEL_MODE_DP_FORCE_6BPC ? 18 : 24; + unsigned int bpp; static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
+ if (HAS_PCH_SPLIT(dev)) + (void) intel_choose_pipe_bpp_dither(intel_dp->base.base.crtc, &bpp, mode); + else { + bpp = intel_dp_max_bpp(encoder, mode); + if (bpp > 24) + bpp = 24; + } + + DRM_DEBUG_KMS("encoder assuming bpp %d (bpc %d)\n", + bpp, bpp / 3); + + if (bpp == 0) { + DRM_DEBUG_KMS("Display port cannot support requested clock %d\n", + mode->clock); + return false; + } + if (is_edp(intel_dp) && intel_dp->panel_fixed_mode) { intel_fixed_panel_mode(intel_dp->panel_fixed_mode, adjusted_mode); intel_pch_panel_fitting(dev, DRM_MODE_SCALE_FULLSCREEN, @@ -691,8 +737,7 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode, for (clock = 0; clock <= max_clock; clock++) { int link_avail = intel_dp_max_data_rate(intel_dp_link_clock(bws[clock]), lane_count);
- if (intel_dp_link_required(mode->clock, bpp) - <= link_avail) { + if (intel_dp_link_required(mode->clock, bpp) <= link_avail) { intel_dp->link_bw = bws[clock]; intel_dp->lane_count = lane_count; adjusted_mode->clock = intel_dp_link_clock(intel_dp->link_bw); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 1348705..03b4595 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -104,7 +104,6 @@ /* drm_display_mode->private_flags */ #define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0) #define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT) -#define INTEL_MODE_DP_FORCE_6BPC (0x10)
static inline void intel_mode_set_pixel_multiplier(struct drm_display_mode *mode, @@ -303,11 +302,16 @@ extern void intel_dp_init(struct drm_device *dev, int dp_reg); void intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode); +extern int intel_dp_max_bpp(struct drm_encoder *encoder, struct drm_display_mode *mode); extern bool intel_dpd_is_edp(struct drm_device *dev); extern void intel_edp_link_config(struct intel_encoder *, int *, int *); extern bool intel_encoder_is_pch_edp(struct drm_encoder *encoder); extern int intel_plane_init(struct drm_device *dev, enum pipe pipe);
+bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc, + unsigned int *pipe_bpp, + struct drm_display_mode *mode); + /* intel_panel.c */ extern void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode);
On Wed, Jan 25, 2012 at 08:16:26AM -0800, Keith Packard wrote:
The DP configuration must match the pipe configuration, and until mode set we don't know what BPC will be used. Delay all decisions about BPC until mode set, computing the target BPC in both intel_dp_mode_fixup and either i9xx_crtc_mode_set or ironlake_crtc_mode_set. It might be slightly better to compute this only once, but storing the value between those two calls would be a pain.
Cc: Lubos Kolouch lubos.kolouch@gmail.com Cc: Adam Jackson ajax@redhat.com Signed-off-by: Keith Packard keithp@keithp.com
drivers/gpu/drm/i915/intel_display.c | 27 +++++------- drivers/gpu/drm/i915/intel_dp.c | 77 +++++++++++++++++++++++++++------- drivers/gpu/drm/i915/intel_drv.h | 6 ++- 3 files changed, 78 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b3b51c4..d613676 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4850,9 +4850,9 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
- Dithering requirement (i.e. false if display bpc and pipe bpc match,
- true if they don't match).
*/ -static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
unsigned int *pipe_bpp,struct drm_display_mode *mode)+bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
unsigned int *pipe_bpp,struct drm_display_mode *mode){ struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; @@ -4883,13 +4883,13 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc, 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 (intel_encoder->type == INTEL_OUTPUT_EDP ||intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) {unsigned int dp_bpc = intel_dp_max_bpp(&intel_encoder->base, mode);
if (edp_bpc < display_bpc) {DRM_DEBUG_KMS("clamping display bpc (was %d) to eDP (%d)\n", display_bpc, edp_bpc);display_bpc = edp_bpc;
if (dp_bpc < display_bpc) {DRM_DEBUG_KMS("clamping display bpc (was %d) to DP (%d)\n", display_bpc, dp_bpc);display_bpc = dp_bpc; } continue;
I'm a bit unhappy how generic code in intel_display.c calls function out of intel_dp.c. And choose_pipe_bpp_dither already has special cases for quite a few other encoders ... Could we add an ->adjust_bpc function to intel_encoder to separate this in a cleaner fashion?
I know that this isn't really the only layering violation in intel_display.c, but functions in that file have an uncanny ability to grow without bounds ;-)
@@ -4923,11 +4923,6 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc, } }
- if (mode->private_flags & INTEL_MODE_DP_FORCE_6BPC) {
DRM_DEBUG_KMS("Dithering DP to 6bpc\n");display_bpc = 6;- }
- /*
- We could just drive the pipe at the highest bpc all the time and
- enable dithering as needed, but that costs bandwidth. So choose
@@ -4990,6 +4985,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, int ret; u32 temp; u32 lvds_sync = 0;
int dp_max_bpp = 0;
list_for_each_entry(encoder, &mode_config->encoder_list, base.head) { if (encoder->base.crtc != crtc)
@@ -5016,6 +5012,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, break; case INTEL_OUTPUT_DISPLAYPORT: is_dp = true;
dp_max_bpp = intel_dp_max_bpp(&encoder->base, mode); break;@@ -5192,7 +5189,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, /* default to 8bpc */ pipeconf &= ~(PIPECONF_BPP_MASK | PIPECONF_DITHER_EN); if (is_dp) {
if (mode->private_flags & INTEL_MODE_DP_FORCE_6BPC) {
if (dp_max_bpp <= 18) { pipeconf |= PIPECONF_BPP_6 | PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP;diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 94f860c..2482796 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -219,14 +219,51 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes) return (max_link_clock * max_lanes * 8) / 10; }
+/*
- For the specified encoder, return the maximum bpp that can be used
- for the specified mode.
- */
+static const int dp_supported_bpc[] = { 6, 8, 10, 12, 16 };
+#define NUM_DP_SUPPORTED_BPC (sizeof dp_supported_bpc/sizeof dp_supported_bpc[0])
+int +intel_dp_max_bpp(struct drm_encoder *encoder, struct drm_display_mode *mode) +{
- struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
- int max_link_clock = intel_dp_link_clock(intel_dp_max_link_bw(intel_dp));
- int max_lanes = intel_dp_max_lane_count(intel_dp);
- int max_rate, mode_rate;
- int i;
- max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
- for (i = NUM_DP_SUPPORTED_BPC - 1; i >= 0; i--) {
int bpp = dp_supported_bpc[i] * 3; /* 3 channels of data (RGB) */mode_rate = intel_dp_link_required(mode->clock, bpp);if (mode_rate <= max_rate) {/* Limit EDP bpp to panel ability */if (intel_dp->base.type == INTEL_OUTPUT_EDP) {struct drm_device *dev = encoder->dev;struct drm_i915_private *dev_priv = dev->dev_private;int edp_bpp = dev_priv->edp.bpp;if (bpp > edp_bpp)bpp = edp_bpp;}return bpp;}- }
- return 0;
+}
static int intel_dp_mode_valid(struct drm_connector *connector, struct drm_display_mode *mode) { struct intel_dp *intel_dp = intel_attached_dp(connector);
int max_link_clock = intel_dp_link_clock(intel_dp_max_link_bw(intel_dp));
int max_lanes = intel_dp_max_lane_count(intel_dp);
int max_rate, mode_rate;
if (is_edp(intel_dp) && intel_dp->panel_fixed_mode) { if (mode->hdisplay > intel_dp->panel_fixed_mode->hdisplay)
@@ -236,16 +273,8 @@ intel_dp_mode_valid(struct drm_connector *connector, return MODE_PANEL; }
- mode_rate = intel_dp_link_required(mode->clock, 24);
- max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
- if (mode_rate > max_rate) {
mode_rate = intel_dp_link_required(mode->clock, 18);if (mode_rate > max_rate)return MODE_CLOCK_HIGH;elsemode->private_flags |= INTEL_MODE_DP_FORCE_6BPC;- }
if (intel_dp_max_bpp(&intel_dp->base.base, mode) == 0)
return MODE_CLOCK_HIGH;if (mode->clock < 10000) return MODE_CLOCK_LOW;
@@ -673,9 +702,26 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode, int lane_count, clock; int max_lane_count = intel_dp_max_lane_count(intel_dp); int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0;
- int bpp = mode->private_flags & INTEL_MODE_DP_FORCE_6BPC ? 18 : 24;
unsigned int bpp; static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
if (HAS_PCH_SPLIT(dev))
(void) intel_choose_pipe_bpp_dither(intel_dp->base.base.crtc, &bpp, mode);else {
bpp = intel_dp_max_bpp(encoder, mode);if (bpp > 24)bpp = 24;}
As you've already said in another mail, this PCH_SPLIT here looks a bit strange. Could we unify these two paths here a bit?
Otherwise I couldn't poke holes into it. -Daniel
- DRM_DEBUG_KMS("encoder assuming bpp %d (bpc %d)\n",
bpp, bpp / 3);- if (bpp == 0) {
DRM_DEBUG_KMS("Display port cannot support requested clock %d\n",mode->clock);return false;- }
- if (is_edp(intel_dp) && intel_dp->panel_fixed_mode) { intel_fixed_panel_mode(intel_dp->panel_fixed_mode, adjusted_mode); intel_pch_panel_fitting(dev, DRM_MODE_SCALE_FULLSCREEN,
@@ -691,8 +737,7 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode, for (clock = 0; clock <= max_clock; clock++) { int link_avail = intel_dp_max_data_rate(intel_dp_link_clock(bws[clock]), lane_count);
if (intel_dp_link_required(mode->clock, bpp)<= link_avail) {
if (intel_dp_link_required(mode->clock, bpp) <= link_avail) { intel_dp->link_bw = bws[clock]; intel_dp->lane_count = lane_count; adjusted_mode->clock = intel_dp_link_clock(intel_dp->link_bw);diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 1348705..03b4595 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -104,7 +104,6 @@ /* drm_display_mode->private_flags */ #define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0) #define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT) -#define INTEL_MODE_DP_FORCE_6BPC (0x10)
static inline void intel_mode_set_pixel_multiplier(struct drm_display_mode *mode, @@ -303,11 +302,16 @@ extern void intel_dp_init(struct drm_device *dev, int dp_reg); void intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode); +extern int intel_dp_max_bpp(struct drm_encoder *encoder, struct drm_display_mode *mode); extern bool intel_dpd_is_edp(struct drm_device *dev); extern void intel_edp_link_config(struct intel_encoder *, int *, int *); extern bool intel_encoder_is_pch_edp(struct drm_encoder *encoder); extern int intel_plane_init(struct drm_device *dev, enum pipe pipe);
+bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
unsigned int *pipe_bpp,struct drm_display_mode *mode);/* intel_panel.c */ extern void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode); -- 1.7.8.3
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 25 Jan 2012 23:11:00 +0100, Daniel Vetter daniel@ffwll.ch wrote:
I'm a bit unhappy how generic code in intel_display.c calls function out of intel_dp.c. And choose_pipe_bpp_dither already has special cases for quite a few other encoders ... Could we add an ->adjust_bpc function to intel_encoder to separate this in a cleaner fashion?
Yeah, seems quite reasonable.
I can't find any reason why the lane count and link bw values are set in fixup_mode and not just in intel_dp_set_mode. If we moved that, we could use the bpp value computed in intel_display.c.
There's a weird mixture of code in ironlake_crtc_mode_set where it calls intel_edp_link_config and uses those values when setting the CPU M/N values for non-PCH eDP panels. That would also need fixing.
I know that this isn't really the only layering violation in intel_display.c, but functions in that file have an uncanny ability to grow without bounds ;-)
The more we clean things up, the easier fixing bugs is in the future.
As you've already said in another mail, this PCH_SPLIT here looks a bit strange. Could we unify these two paths here a bit?
The simple way to unify them would be to use intel_choose_pipe_bpp_dither from the i9xx_crtc_mode_set path. Perhaps that function could codify the currently simplistic rule used on i9xx?
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Dave Airlie -
Jesse Barnes -
Keith Packard