From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 5/6] drm/i915: Add 180 degree primary plane rotation support Date: Mon, 11 Aug 2014 13:42:01 +0200 Message-ID: <20140811114201.GP8727@phenom.ffwll.local> References: <1405413629-4266-1-git-send-email-sonika.jindal@intel.com> <1405413629-4266-6-git-send-email-sonika.jindal@intel.com> <20140715091137.GS15237@phenom.ffwll.local> <20140807114531.GB8727@phenom.ffwll.local> <20140807121132.GC8727@phenom.ffwll.local> <53E8A3D4.4060101@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f180.google.com (mail-wi0-f180.google.com [209.85.212.180]) by gabe.freedesktop.org (Postfix) with ESMTP id 6A6966E04A for ; Mon, 11 Aug 2014 04:41:49 -0700 (PDT) Received: by mail-wi0-f180.google.com with SMTP id n3so4033862wiv.1 for ; Mon, 11 Aug 2014 04:41:48 -0700 (PDT) Content-Disposition: inline In-Reply-To: <53E8A3D4.4060101@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Jindal, Sonika" Cc: intel-gfx@lists.freedesktop.org, Sagar Kamble List-Id: intel-gfx@lists.freedesktop.org On Mon, Aug 11, 2014 at 04:37:00PM +0530, Jindal, Sonika wrote: > > Hi Daniel, > Are you taking this patch back in drm-intel? We should still call the primary_plane->update hook directly instead of open-coding it. -Daniel > > -Sonika > > On 8/7/2014 5:41 PM, Daniel Vetter wrote: > >On Thu, Aug 07, 2014 at 01:45:31PM +0200, Daniel Vetter wrote: > >>On Tue, Jul 15, 2014 at 11:11:37AM +0200, Daniel Vetter wrote: > >>>On Tue, Jul 15, 2014 at 02:10:28PM +0530, sonika.jindal@intel.com wrote: > >>>>+ /* FBC does not work on some platforms for rotated planes */ > >>>>+ if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev)) { > >>>>+ if (dev_priv->fbc.plane == intel_crtc->plane && > >>>>+ intel_plane->rotation != BIT(DRM_ROTATE_0)) > >>>>+ intel_disable_fbc(dev); > >>>>+ /* If rotation was set earlier and new rotation is 0, > >>>>+ we might have disabled fbc earlier. So update it now */ > >>>>+ else if (intel_plane->rotation == BIT(DRM_ROTATE_0) > >>>>+ && old_val != BIT(DRM_ROTATE_0)) { > >>>>+ mutex_lock(&dev->struct_mutex); > >>>>+ intel_update_fbc(dev); > >>>>+ mutex_unlock(&dev->struct_mutex); > >>>>+ } > >>>>+ } > >>> > >>>Indentation is screwed up here. Also if we convert some of the checks into > >>>early bails we could de-indent this by one level. > >>> > >>>Also Chris mentioned that on some platforms this won't work and it's more > >>>future-proof to just do a full modeset until we have the proper > >>>infrastructure. > >> > >>Apparently this review here was never addressed, as Chris just pointed out > >>on irc. I've dropped the patch again. > >> > >>I think we need: > >>- The same sequence as with the sprite set_property function, i.e. we need > >> to call the update_plane function (not the raw low-level one, the > >> high-level with all the checks). > >>- The fbc check is wrong and will miss updates when the crtc is off. We > >> need to move this into the general list of checks in intel_update_fbc. > >>- Since this seems to be buggy I want added testcases to combine fbc > >> correctness with screen rotation. Probably best to reuse the existing > >> fbc testcase and add a bunch or rotated tests. > > > >Ok, the check in update_fbc is there, I've been blind. Sorry about all the > >confusion. So just amounts of calling the higher level function and we can > >forgo the fbc testing. > >-Daniel > > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch