On Fri, Apr 19, 2013 at 8:39 PM, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
> On Fri, 19 Apr 2013 20:17:10 +0200
> Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 8c36376..7f1ab8b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4605,22 +4605,29 @@ static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc)
>> pipeconf &= ~PIPECONF_DOUBLE_WIDE;
>> }
>>
>> - /* default to 8bpc */
>> - pipeconf &= ~(PIPECONF_BPC_MASK | PIPECONF_DITHER_EN);
>> - if (intel_crtc->config.has_dp_encoder) {
>> - if (intel_crtc->config.dither) {
>> - pipeconf |= PIPECONF_6BPC |
>> - PIPECONF_DITHER_EN |
>> + /* only g4x and later have fancy bpc/dither controls */
>> + if (IS_G4X(dev) || IS_VALLEYVIEW(dev)) {
>> + pipeconf &= ~(PIPECONF_BPC_MASK | PIPECONF_DITHER_EN);
>> +
>> + /* Bspec claims that we can't use dithering for 30bpp pipes. */
>> + if (intel_crtc->config.dither && intel_crtc->config.pipe_bpp != 30)
>> + pipeconf |= PIPECONF_DITHER_EN |
>> PIPECONF_DITHER_TYPE_SP;
>> - }
>> - }
>>
>> - if (IS_VALLEYVIEW(dev) && intel_pipe_has_type(&intel_crtc->base,
>> - INTEL_OUTPUT_EDP)) {
>> - if (intel_crtc->config.dither) {
>> - pipeconf |= PIPECONF_6BPC |
>> - PIPECONF_ENABLE |
>> - I965_PIPECONF_ACTIVE;
>> + pipeconf &= ~PIPECONF_BPC_MASK;
>> + switch (intel_crtc->config.pipe_bpp) {
>> + case 18:
>> + pipeconf |= PIPECONF_6BPC;
>> + break;
>> + case 24:
>> + pipeconf |= PIPECONF_8BPC;
>> + break;
>> + case 30:
>> + pipeconf |= PIPECONF_10BPC;
>> + break;
>> + default:
>> + /* Case prevented by intel_choose_pipe_bpp_dither. */
>> + BUG();
>
> Am I misreading here? It looks like we may set the 10bpc pipeconf bit,
> but never enable it. Should there be another G4x check somewhere? Or
> should the 10bpc case be dropped from the switch?
Previous patches fixed up the pipe_bpp computation for vlv, and other
patches in this series allow us to actually come up with a 10bpp
config (e.g. dp unconditionally goes with a 24bpp limit thus far ...).
So I don't see exactly where you see an issue.
>> }
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
>> index 58a98ff..094f3c5 100644
>> --- a/drivers/gpu/drm/i915/intel_lvds.c
>> +++ b/drivers/gpu/drm/i915/intel_lvds.c
>> @@ -135,7 +135,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
>> /* Set the dithering flag on LVDS as needed, note that there is no
>> * special lvds dither control bit on pch-split platforms, dithering is
>> * only controlled through the PIPECONF reg. */
>> - if (INTEL_INFO(dev)->gen == 4) {
>> + if (INTEL_INFO(dev)->gen == 4 && !IS_G4X(dev)) {
>> if (intel_crtc->config.dither)
>> temp |= LVDS_ENABLE_DITHER;
>> else
>
> Looks ok otherwise, though I'd still like to see LVDS dither compared
> with pipe dither.
I've figued since we want dither for DP (e.g. cheap vga dongles which
only work with 6bpc on high-res modes) we should switch to pipe
dithering unconditionally.
> And what happens now if a 30bpp config gets chosen for an LVDS or eDP
> panel? Do we just output it anyway and get junk on the screen?
encoder->compute_config will save the day. If you read up to the fdi
auto-dither code you can watch a pretty elaborate negotiation going on
between encoders and the pipe about which bpp things should run at ;-)
So should all just magically work. In the end (once everything
interesting is converted over) the mode_set/enable functions just grab
the parameters from pipe_conf and smash it into hw registers. All
interesting stuff is done up-front in the pipe/encoder compute_config
stage. Of course that means for a given hw feature you need to check
individual encoders and the pipe to figure out what exactly pops out
at the end.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
