[Intel-gfx] [RFC 1/6] drm/i915: cover ioctls with runtime_get/put

[Paulo Zanoni]

2014/1/24 Naresh Kumar Kachhi <naresh.kumar.kachhi at intel.com>:
> On 01/22/2014 06:53 PM, Imre Deak wrote:
>
> On Wed, 2014-01-22 at 13:51 +0100, Daniel Vetter wrote:
>
> On Wed, Jan 22, 2014 at 05:34:17PM +0530, naresh.kumar.kachhi at intel.com
> wrote:
>
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi at intel.com>
>
> With runtime PM enabled, we need to make sure that all HW access
> are valid (i.e.  Gfx is in D0). Invalid accesses might end up in
> HW hangs. Ex. A hang is seen if display register is accessed on
> BYT while display power island is power gated.
>
> This patch is covering all the IOCTLs with get/put.
> TODO: limit runtime_get/put to IOCTLs that accesses HW
>
> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi at intel.com>
>
> Nack on the concept. We need to have get/put calls for the individual
> functional blocks of the hw, which then in turn (if it's not the top-level
> power domain) need to grab references to the next up power domain.
> Splattering unconditional get/puts over all driver entry points is bad
> style and imo also too fragile.
>
> Also, with Paulos final runtime pm/pc8 unification patches and Imre's
> display power well patches for byt we should have full coverage already.
> Have you looked at the work of these too?
>
> I'm still in debt with the BYT specific power domain patches, I want to
> post them (this week) after I sorted out spurious pipe stat IRQs we'd
> receive atm with the power well off. Until then there is only the WIP
> version at: https://github.com/ideak/linux/commits/powerwells
>
> But in practice, as you point out the plan was to only call
> modeset_update_power_wells() during modeset time and that will end up
> doing the proper RPM get/put as necessary. Similarly on some other code
> paths like connector detect and HW state read-out code, we'd ask only
> for the needed power domains which would end up doing the RPM get/put.
>
> --Imre
>
> Hi Imre,
> I tried to go through your and Paulo's patches
> (http://patchwork.freedesktop.org/patch/16952/).
> As per my understanding we are doing disp power well gate/ungate independent
> of we are in
> runtime_suspend/resume state we might face problem here as on BYT interrupts
> are routed through
> display power well, so we might have a situation where display power island
> is power gated,
> but render workload is still active. As we won't be receiving any interrupt
> __wait_seq_no will
> stall and we might end up in a TDR. We will not see the issue until display
> power gating is
> enabled. I will try to include a test to cover this test
>
> This can be avoided by adding display_get/put in runtime_resume/suspend.
> I am not sure if this scenario will be applicable for HSW.
>
> Need for covering ioclts:
> However there is one more problem for LP platforms. Once
> display/render/media power
> islands are power gated, system will enter into deeper sleep state causing
> Gunit to power
> gate. We will loose the state once Gunit resumes, so Gunit restore and ring
> initialization is
> required in runtime_resume. Even after adding these to
> runtime_suspend/resume, we will have
> to make sure all the ring activities, Gunit register accesses are covered
> with runtime_get/put.
> There are few places with current implementation where we might hit this
> problem.
> Example:
> i915_batchbuffer(ioctl)->i915_batchbuffer (gpu commands scheduled)
> i915_irq_emit(ioctl) -> i915_emit_irq (gpu commands scheduled)
> i915_cmdbuffer (ioctl) -> i915_dispatch_cmdbuffer-> i915_emit_cmds(gpu
> commands scheduled)
> There are few more.
> These ioclts not being covered with runtime_get/put will might cause invalid
> executions
> if we are resuming for deeper power state (gunit was powergated).

If possible, please write pm_pc8.c subtests that reproduce all these
problems you found :)


>
> solutions:
> From design perspective, we can cover all accesses at low level, but this
> means a lot of get/put
> and also will have to make sure that we cover future accesses as well. As an
> other solution we
> can cover these ioctls individually or can have a wrapper function (as
> purposed in this patch)
> to cover the accesses (TODO: make them conditional to do get/put on few
> ioclts only).
> I will try to upload refined patch with conditional get/put.
>
> Thanks,
> Naresh
>
>
>
>
>
>
>
>
>
>
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Paulo Zanoni