On Wed, Oct 24, 2012 at 6:33 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
> 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.
This needs to be in a comment somewhere. I think the long version here
in the commit message, and a short one in the code.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
