From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jindal, Sonika" Subject: Re: [PATCH 5/6] drm/i915: Add 180 degree primary plane rotation support Date: Tue, 12 Aug 2014 08:55:58 +0530 Message-ID: <53E98946.8010109@intel.com> 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> <20140811114201.GP8727@phenom.ffwll.local> <53E8B1F8.9030002@intel.com> <20140811122332.GA10500@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 3B7D76E49C for ; Mon, 11 Aug 2014 20:26:34 -0700 (PDT) In-Reply-To: <20140811122332.GA10500@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org, Sagar Kamble List-Id: intel-gfx@lists.freedesktop.org On 8/11/2014 5:53 PM, Daniel Vetter wrote: > On Mon, Aug 11, 2014 at 05:37:20PM +0530, Jindal, Sonika wrote: >> >> >> On 8/11/2014 5:12 PM, Daniel Vetter wrote: >>> 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 >>> >> But we are already doing dev_priv->display.update_primary_plane. Can you >> please elaborate? Last time we had a discussion on this same point, and we >> thought for some platforms more work might be needed but for current >> platforms this looks ok. Following was the discussion: > > That's a different cook used internally in the driver (mostly for > historical reasons nowadays). I'm talking about the > drm_plane->fops->update_plane hook, which is implemented for primary > planes by intel_primary_plane_setplane. > Ok, let me add that and post a new patch for this. -Sonika > The idea is to have the exact same code flow as for sprite planes, since > once we have atomic modesets that will be what we need. In the sprite > set_prop function you pretty directly call the update_plane hook, and I > think we should do the same here. Actually we might as well directly reuse > the intel_plane_restore function, it seems to exactly do what we want. > >> "> >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. >>>> >>> Can you please elaborate on this? What needs to be done? >> >> Apparently nothing, it turned out that on the platforms currently supported >> everything is fine with your patch. Damien promised to follow up with the >> details on internal mailing lists. > > Damien follow up internally. Since we're now allowed to talk about skl > I'll paste this here: > > "On SKL, for 90/270, we'll also need to update the watermarks and remap > the fb. > > "We already duplicate some of the logic: the FBC update, wait for > pending flips, ... > > "So maybe we should try to find a way to take the same path for all > updates. I'm not too sure how practical this is though." > > Damien also said that he doesn't want to still the series on this, but > since we now have universal plane support for the primary plane I think it > makes a lot of sense. > -Daniel > > >> -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 >>>>> >>> >