Re: [Intel-gfx] [PATCH 12/13] drm/i915: clean up pipe bpp confusion
On Wed, 27 Mar 2013 00:45:01 +0100
Daniel Vetter daniel.vet...@ffwll.ch wrote:
- gen4 and earlier (save for g4x) only really have a 8bpc pipe, with
the possibility to dither to 6bpc using the panel fitter
- g4x has hdmi, but no 12 bpc pipe ... !? Clamp hdmi accordingly.
- TV/SDVO out are the only connectors available on platforms with
a pipe bpp != 8, add code to force the pipe to 8bpc unconditionally.
rant
The dither handling on gmch platforms is one giant disaster. I'm hoping
somewhat that vlv enabling will fix this up, but given that the 6bpc
handling for edp was simply added with another quick hack, I don't have
high hopes ...
/rant
v2: Neither vlv nor g4x have 12bpc pipes. Still set pipe_bpp to 12*3,
but let the crtc code clamp things down to 10bpc on these platforms.
v3: Fix a bpc vs. bpp mixup in the gen4 and earlier pipe_bpp limiter
code.
v4: Drop the hunk in intel_hdmi.c about g4x/vlv 12bpc, it was wrong.
Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/intel_display.c | 8
drivers/gpu/drm/i915/intel_sdvo.c| 3 +++
drivers/gpu/drm/i915/intel_tv.c | 14 --
3 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index bbf31aa..8ab7520 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3954,6 +3954,14 @@ static bool intel_crtc_compute_config(struct drm_crtc
*crtc,
adjusted_mode-hsync_start == adjusted_mode-hdisplay)
return false;
+ if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) pipe_config-pipe_bpp 10) {
+ pipe_config-pipe_bpp = 10*3; /* 12bpc is gen5+ */
+ } else if (INTEL_INFO(dev)-gen = 4 pipe_config-pipe_bpp 8) {
+ /* only a 8bpc pipe, with 6bpc dither through the panel fitter
+ * for lvds. */
+ pipe_config-pipe_bpp = 8*3;
+ }
+
return true;
}
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
b/drivers/gpu/drm/i915/intel_sdvo.c
index c6fbfd1..80f8680 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1048,6 +1048,9 @@ static bool intel_sdvo_compute_config(struct
intel_encoder *encoder,
struct drm_display_mode *adjusted_mode = pipe_config-adjusted_mode;
struct drm_display_mode *mode = pipe_config-requested_mode;
+ DRM_DEBUG_KMS(forcing bpc to 8 for SDVO\n);
+ pipe_config-pipe_bpp = 8*3;
+
if (HAS_PCH_SPLIT(encoder-base.dev))
pipe_config-has_pch_encoder = true;
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index d808421..6673726 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -905,11 +905,10 @@ intel_tv_mode_valid(struct drm_connector *connector,
static bool
-intel_tv_mode_fixup(struct drm_encoder *encoder,
- const struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode)
+intel_tv_compute_config(struct intel_encoder *encoder,
+ struct intel_crtc_config *pipe_config)
{
- struct intel_tv *intel_tv = enc_to_intel_tv(encoder);
+ struct intel_tv *intel_tv = enc_to_intel_tv(encoder-base);
const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
if (!tv_mode)
@@ -918,7 +917,10 @@ intel_tv_mode_fixup(struct drm_encoder *encoder,
if (intel_encoder_check_is_cloned(intel_tv-base))
return false;
- adjusted_mode-clock = tv_mode-clock;
+ pipe_config-adjusted_mode.clock = tv_mode-clock;
+ DRM_DEBUG_KMS(forcing bpc to 8 for TV\n);
+ pipe_config-pipe_bpp = 8*3;
+
return true;
}
@@ -1485,7 +1487,6 @@ out:
}
static const struct drm_encoder_helper_funcs intel_tv_helper_funcs = {
- .mode_fixup = intel_tv_mode_fixup,
.mode_set = intel_tv_mode_set,
};
@@ -1620,6 +1621,7 @@ intel_tv_init(struct drm_device *dev)
drm_encoder_init(dev, intel_encoder-base, intel_tv_enc_funcs,
DRM_MODE_ENCODER_TVDAC);
+ intel_encoder-compute_config = intel_tv_compute_config;
intel_encoder-enable = intel_enable_tv;
intel_encoder-disable = intel_disable_tv;
intel_encoder-get_hw_state = intel_tv_get_hw_state;
I had to double check this against 9/13... I guess the order will be
clearer with the actual code as opposed to patches.
But won't these override the pipe_config bpp we set in
pipe_config_set_bpp()? Why bother setting it there if each of the
encoders will set it themselves, and the real comparison is against
the plane bpp? And doesn't that mean we'd need to set pipe_config-bpp
in the DP version too?
Maybe I've been looking at this too hard and I've just confused
myself...
--
Jesse Barnes, Intel Open Source
Re: [Intel-gfx] [PATCH 12/13] drm/i915: clean up pipe bpp confusion
On Wed, Mar 27, 2013 at 10:28 PM, Jesse Barnes jbar...@virtuousgeek.org wrote: I had to double check this against 9/13... I guess the order will be clearer with the actual code as opposed to patches. But won't these override the pipe_config bpp we set in pipe_config_set_bpp()? Why bother setting it there if each of the encoders will set it themselves, and the real comparison is against the plane bpp? And doesn't that mean we'd need to set pipe_config-bpp in the DP version too? The pipe_bpp we set from the planes is the proposed one, used when nothing else overrides it. Then encoders can come around and add in their opinion about what's possible. Note that encoders want to know which pipe_bpp is the proposed one (from looking just at the plane) to make their own decision. E.g. hdmi wants to updither 10bpc to 12bpc (if possible) since it doesn't support 10bpc natively. Whereas DP only ever down-dithers. This way we gain a notch more flexibility in handling bpp. My auto-fdi dither work which is based on top of this goes one step further and checks (after plane/pipe/encoder all had their say) whether it actually fits into the fdi link. If it doesn't fit it tries to dither down. If that's possible we'll restart the pipe_config arbitrage, but with the new proposed pipe_bpp plus a flag telling everyone that they'll get shot if they try to increase bw requirements. Maybe I've been looking at this too hard and I've just confused myself... Maybe it's a bit overdesigned ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 12/13] drm/i915: clean up pipe bpp confusion
On Wed, 27 Mar 2013 23:41:55 +0100 Daniel Vetter daniel.vet...@ffwll.ch wrote: On Wed, Mar 27, 2013 at 10:28 PM, Jesse Barnes jbar...@virtuousgeek.org wrote: I had to double check this against 9/13... I guess the order will be clearer with the actual code as opposed to patches. But won't these override the pipe_config bpp we set in pipe_config_set_bpp()? Why bother setting it there if each of the encoders will set it themselves, and the real comparison is against the plane bpp? And doesn't that mean we'd need to set pipe_config-bpp in the DP version too? The pipe_bpp we set from the planes is the proposed one, used when nothing else overrides it. Then encoders can come around and add in their opinion about what's possible. Note that encoders want to know which pipe_bpp is the proposed one (from looking just at the plane) to make their own decision. E.g. hdmi wants to updither 10bpc to 12bpc (if possible) since it doesn't support 10bpc natively. Whereas DP only ever down-dithers. This way we gain a notch more flexibility in handling bpp. My auto-fdi dither work which is based on top of this goes one step further and checks (after plane/pipe/encoder all had their say) whether it actually fits into the fdi link. If it doesn't fit it tries to dither down. If that's possible we'll restart the pipe_config arbitrage, but with the new proposed pipe_bpp plus a flag telling everyone that they'll get shot if they try to increase bw requirements. Maybe I've been looking at this too hard and I've just confused myself... Maybe it's a bit overdesigned ;-) Ok it makes some sense... though maybe if we passed down the plane bpp directly we'd be able to avoid some of the re-calc stuff in your FDI dither patch. We can always improve it after it lands and becomes clearer. Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 12/13] drm/i915: clean up pipe bpp confusion
On Wed, Mar 27, 2013 at 04:13:13PM -0700, Jesse Barnes wrote: On Wed, 27 Mar 2013 23:41:55 +0100 Daniel Vetter daniel.vet...@ffwll.ch wrote: On Wed, Mar 27, 2013 at 10:28 PM, Jesse Barnes jbar...@virtuousgeek.org wrote: I had to double check this against 9/13... I guess the order will be clearer with the actual code as opposed to patches. But won't these override the pipe_config bpp we set in pipe_config_set_bpp()? Why bother setting it there if each of the encoders will set it themselves, and the real comparison is against the plane bpp? And doesn't that mean we'd need to set pipe_config-bpp in the DP version too? The pipe_bpp we set from the planes is the proposed one, used when nothing else overrides it. Then encoders can come around and add in their opinion about what's possible. Note that encoders want to know which pipe_bpp is the proposed one (from looking just at the plane) to make their own decision. E.g. hdmi wants to updither 10bpc to 12bpc (if possible) since it doesn't support 10bpc natively. Whereas DP only ever down-dithers. This way we gain a notch more flexibility in handling bpp. My auto-fdi dither work which is based on top of this goes one step further and checks (after plane/pipe/encoder all had their say) whether it actually fits into the fdi link. If it doesn't fit it tries to dither down. If that's possible we'll restart the pipe_config arbitrage, but with the new proposed pipe_bpp plus a flag telling everyone that they'll get shot if they try to increase bw requirements. Maybe I've been looking at this too hard and I've just confused myself... Maybe it's a bit overdesigned ;-) Ok it makes some sense... though maybe if we passed down the plane bpp directly we'd be able to avoid some of the re-calc stuff in your FDI dither patch. We can always improve it after it lands and becomes clearer. Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org Slurped them all into dinq, thanks for the review. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 12/13] drm/i915: clean up pipe bpp confusion
- gen4 and earlier (save for g4x) only really have a 8bpc pipe, with
the possibility to dither to 6bpc using the panel fitter
- g4x has hdmi, but no 12 bpc pipe ... !? Clamp hdmi accordingly.
- TV/SDVO out are the only connectors available on platforms with
a pipe bpp != 8, add code to force the pipe to 8bpc unconditionally.
rant
The dither handling on gmch platforms is one giant disaster. I'm hoping
somewhat that vlv enabling will fix this up, but given that the 6bpc
handling for edp was simply added with another quick hack, I don't have
high hopes ...
/rant
v2: Neither vlv nor g4x have 12bpc pipes. Still set pipe_bpp to 12*3,
but let the crtc code clamp things down to 10bpc on these platforms.
v3: Fix a bpc vs. bpp mixup in the gen4 and earlier pipe_bpp limiter
code.
v4: Drop the hunk in intel_hdmi.c about g4x/vlv 12bpc, it was wrong.
Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/intel_display.c | 8
drivers/gpu/drm/i915/intel_sdvo.c| 3 +++
drivers/gpu/drm/i915/intel_tv.c | 14 --
3 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index bbf31aa..8ab7520 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3954,6 +3954,14 @@ static bool intel_crtc_compute_config(struct drm_crtc
*crtc,
adjusted_mode-hsync_start == adjusted_mode-hdisplay)
return false;
+ if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) pipe_config-pipe_bpp 10) {
+ pipe_config-pipe_bpp = 10*3; /* 12bpc is gen5+ */
+ } else if (INTEL_INFO(dev)-gen = 4 pipe_config-pipe_bpp 8) {
+ /* only a 8bpc pipe, with 6bpc dither through the panel fitter
+* for lvds. */
+ pipe_config-pipe_bpp = 8*3;
+ }
+
return true;
}
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
b/drivers/gpu/drm/i915/intel_sdvo.c
index c6fbfd1..80f8680 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1048,6 +1048,9 @@ static bool intel_sdvo_compute_config(struct
intel_encoder *encoder,
struct drm_display_mode *adjusted_mode = pipe_config-adjusted_mode;
struct drm_display_mode *mode = pipe_config-requested_mode;
+ DRM_DEBUG_KMS(forcing bpc to 8 for SDVO\n);
+ pipe_config-pipe_bpp = 8*3;
+
if (HAS_PCH_SPLIT(encoder-base.dev))
pipe_config-has_pch_encoder = true;
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index d808421..6673726 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -905,11 +905,10 @@ intel_tv_mode_valid(struct drm_connector *connector,
static bool
-intel_tv_mode_fixup(struct drm_encoder *encoder,
- const struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode)
+intel_tv_compute_config(struct intel_encoder *encoder,
+ struct intel_crtc_config *pipe_config)
{
- struct intel_tv *intel_tv = enc_to_intel_tv(encoder);
+ struct intel_tv *intel_tv = enc_to_intel_tv(encoder-base);
const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
if (!tv_mode)
@@ -918,7 +917,10 @@ intel_tv_mode_fixup(struct drm_encoder *encoder,
if (intel_encoder_check_is_cloned(intel_tv-base))
return false;
- adjusted_mode-clock = tv_mode-clock;
+ pipe_config-adjusted_mode.clock = tv_mode-clock;
+ DRM_DEBUG_KMS(forcing bpc to 8 for TV\n);
+ pipe_config-pipe_bpp = 8*3;
+
return true;
}
@@ -1485,7 +1487,6 @@ out:
}
static const struct drm_encoder_helper_funcs intel_tv_helper_funcs = {
- .mode_fixup = intel_tv_mode_fixup,
.mode_set = intel_tv_mode_set,
};
@@ -1620,6 +1621,7 @@ intel_tv_init(struct drm_device *dev)
drm_encoder_init(dev, intel_encoder-base, intel_tv_enc_funcs,
DRM_MODE_ENCODER_TVDAC);
+ intel_encoder-compute_config = intel_tv_compute_config;
intel_encoder-enable = intel_enable_tv;
intel_encoder-disable = intel_disable_tv;
intel_encoder-get_hw_state = intel_tv_get_hw_state;
--
1.7.11.7
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
