From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Add Baytrail PSR Support. Date: Tue, 4 Feb 2014 11:53:00 +0100 Message-ID: <20140204105259.GF17001@phenom.ffwll.local> References: <20140129145648.GE9454@intel.com> <1391010606-9206-1-git-send-email-rodrigo.vivi@gmail.com> <20140130130236.GI29091@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f177.google.com (mail-ea0-f177.google.com [209.85.215.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 3315FFC1A5 for ; Tue, 4 Feb 2014 02:53:05 -0800 (PST) Received: by mail-ea0-f177.google.com with SMTP id n15so4339163ead.36 for ; Tue, 04 Feb 2014 02:53:05 -0800 (PST) Content-Disposition: inline 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: Rodrigo Vivi Cc: intel-gfx List-Id: intel-gfx@lists.freedesktop.org On Thu, Jan 30, 2014 at 03:01:08PM -0200, Rodrigo Vivi wrote: > On Thu, Jan 30, 2014 at 11:02 AM, Chris Wilson wrote: > > On Wed, Jan 29, 2014 at 01:50:06PM -0200, Rodrigo Vivi wrote: > >> @@ -7501,6 +7501,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, > >> u32 base = 0, pos = 0; > >> bool visible; > >> > >> + if (IS_VALLEYVIEW(dev)) > >> + intel_edp_psr_inactivate(dev); > >> + > > > > Inactivate means that we turn off PSR for some period of time until idle > > again, right? > > Yes, right. > > > In the case of a moving cursor that means indefinitely. > That's true... So I think we really need a work queue delaying the enable. > Or do you have any better idea? Yeah, sounds like we need a delayed work-queue to re-enable psr, also for gtt mmap writes. See Chris' latest crazy example of what the X server is currently allowed to do. > > Also it sounds overkill to have to disable PSR just for the cursor > > sprite. Is there not some preferrable alternatives or hw that dtrt? > > Yeap, I totally agree. The other option implemented by other drivers > is to use PSR SW control mode where driver controls all entry and exit > flow, since Baytrail cannot do it right on HW mode. But even the force > exit on this case was using the PSR_RESET that I'm using on inactivate > function. and they exported that over ioctl. > So I preferred to let all this on kernel side using hardware as much > as possible and workarounding the cases where hardware buggy on screen > update detection, although it looks like an overkill for a single > cursor update. :/ Yeah, exporting psr to userspace is imo a complete no-go. We can give userspace hints and eventually go to an explicit damage tracking model, but it's ultimately the kernel's responsibility to shovel pixels to the screen. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch