[Intel-gfx] [PATCH 05/18] drm/i915: add TRANSCODER_EDP

[Paulo Zanoni]

Hi

2012/10/24 Lespiau, Damien <damien.lespiau at intel.com>:
> On Tue, Oct 23, 2012 at 9:29 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
>>  static void haswell_crtc_off(struct drm_crtc *crtc)
>>  {
>> +       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> +
>> +       intel_crtc->cpu_transcoder = intel_crtc->pipe;
>>         intel_ddi_put_crtc_pll(crtc);
>>  }
>>
>
> I can't find the reason why you would set the cpu_transcoder in the
> off() function, would you mind explaining why? (or maybe the clue is
> in a later patch, which might mean this hunk belongs to a later patch
> as well).

The very first version I wrote for this patch did not have this line
too, until I discovered I needed it. Fasten your seatbelts...

Because TRANSCODER_EDP can be used by any CRTC, so when you stop using
it you have to stop saying you're using it, otherwise you may have at
some point 2 crtcs claiming they're using TRANSCODER_EDP (a disable
crtc and an enabled one), then the HW state readout code will get
completely confused.

In other words:

Imagine the following case:
- xrandr --output eDP1 --auto --crtc 0
- xrandr --output eDP1 --off
- xrandr --output eDP1 --auto --crtc 2

After the last command you will get a nice "pipe A assertion failure
(expected off, current on)" because crtc 0 still claims it's using
transcoder_edp, so the hw state readout function will read it (through
pipeconf) and expect it to be off, when it is actually on because it's
being used by crtc 2.

So when we make "intel_crtc->cpu_transcoder = intel_crtc->pipe" we
make sure we're pointing to our own original crtc which is certainly
not used by any other CRTC.

>
> --
> Damien



-- 
Paulo Zanoni