[Intel-gfx] [PATCH 04/19] drm/i915: get/put PC8 when we get/put a CRTC

[Daniel Vetter]

On Thu, Dec 5, 2013 at 2:43 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
[snip]

> Just documenting what we discussed on IRC yesterday: I agree this is
> the way to go, but I think this should all be follow-up patches. We'll
> also probably need to add some new power domains: for example which
> one do we grab when someone submits a execbuf? POWER_DOMAIN_GT?

Imo Imre's infrastructure for display power wells doesn't need to be
extended for everything. Adding a POWER_DOMAIN_GT makes not much sense
to me - you're current approach with special get/put functions looks
sane.

To reiterate: My gripe isn't about the color of the get/put functions
at all, but that here we need to wrestle with 2 different power
domains: The pc8 stuff and the display power well stuff (as expressed
through imre's interface work). Which is redundant since if we ask for
the the CRTC_A/B/C power domain to be on that must always also forbid
pc8.

[snip]

> Ok, but assert_can_disable_lcpll is completely unrelated with this
> specific patch we're replying. We should fix this with a follow-up
> patch. Also, why aren't the implicit barriers good enough anymore?

Your commit message says that this patch fixes a race where the pc8
enable work can WARN while we concurrently set crtc->base.enabled.
Afaics that WARN is in assert_can_disable_lcpll (but your commit
message is a bit unclear what exactly is racing against which WARN).

Imo that WARN is bogus and needs to be fixed/removed, not worked
around like you do in this patch here.

[Snip]

> Since we don't support disabling LCPLL when the CRTC is in DPMS state,
> I still think that check is not wrong (not considering
> locking/concurrency issues). But if we want to only check what's
> strictly necessary, yeah we could change to crtc->active or even
> directly poke at the register.
>
>
> Anyway, given all this discussion, I don't think I should do any
> changes on this specific 04/19 patch since it's completely unrelated
> with what we're discussing. Please correct me if I'm wrong.

See above, I think the patch fixes the wrong part of the code and we
instead need to adjust the WARN check. If you want my bikeshed I'd go
with checking crtc->active, but I'm fine with either checking the
register directly or just ditching the check altogether, your call.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch