Re: [RFC 1/6] drm/i915: cover ioctls with runtime_get/put

[Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>]

On 01/24/2014 09:26 PM, Paulo Zanoni wrote:
> 2014/1/24 Naresh Kumar Kachhi <naresh.kumar.kachhi@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@intel.com
>> wrote:
>>
>> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@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@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 :)
>
Above scenarios are only valid for non-KMS mode. I missed it, so looks like
we should be fine with the current implementation and your outstanding patches

>> 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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>
>