[Intel-gfx] [PATCH] drm/i915: implement high-bpc + pipeconf-dither support for g4x/vlv

[Daniel Vetter]

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