On Thu, Dec 5, 2013 at 2:43 PM, Paulo Zanoni <przanoni@xxxxxxxxx> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
