From mboxrd@z Thu Jan 1 00:00:00 1970 From: Naresh Kumar Kachhi Subject: Re: [RFC 1/6] drm/i915: cover ioctls with runtime_get/put Date: Mon, 24 Feb 2014 10:47:57 +0530 Message-ID: <530AD605.20807@intel.com> References: <1390392262-30937-1-git-send-email-naresh.kumar.kachhi@intel.com> <1390392262-30937-2-git-send-email-naresh.kumar.kachhi@intel.com> <20140122125122.GG9772@phenom.ffwll.local> <1390397008.12277.15.camel@intelbox> <52E2814B.4030305@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 94182FA8E8 for ; Sun, 23 Feb 2014 21:19:23 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Paulo Zanoni Cc: "Satyanantha, Rama Gopal M" , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On 01/24/2014 09:26 PM, Paulo Zanoni wrote: > 2014/1/24 Naresh Kumar Kachhi : >> 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 >> >> 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 >> >> 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 >> > >