Re: [PATCH 1/2] drm/i915:bxt: Enable Pooled EU support

From: Daniel Vetter <daniel@ffwll.ch>
To: Arun Siluvery <arun.siluvery@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, Mika Kuoppala <mika.kuoppala@intel.com>
Subject: Re: [PATCH 1/2] drm/i915:bxt: Enable Pooled EU support
Date: Tue, 17 May 2016 13:58:34 +0200	[thread overview]
Message-ID: <20160517115834.GD27098@phenom.ffwll.local> (raw)
In-Reply-To: <573B012C.90004@linux.intel.com>

On Tue, May 17, 2016 at 05:01:56PM +0530, Arun Siluvery wrote:
> On 17/05/2016 16:43, Daniel Vetter wrote:
> >On Tue, May 17, 2016 at 04:32:49PM +0530, Arun Siluvery wrote:
> >>On 17/05/2016 15:34, Daniel Vetter wrote:
> >>>On Thu, May 12, 2016 at 10:37:14AM +0100, Arun Siluvery wrote:
> >>>>This mode allows to assign EUs to pools which can process work collectively.
> >>>>The command to enable this mode should be issued as part of context initialization.
> >>>>
> >>>>The pooled mode is global, once enabled it has to stay the same across all
> >>>>contexts until HW reset hence this is sent in auxiliary golden context batch.
> >>>>Thanks to Mika for the preliminary review and comments.
> >>>>
> >>>>v2: explain why this is enabled in golden context, use feature flag while
> >>>>enabling the support (Chris)
> >>>>
> >>>>v3: Pooled EU support announced in userspace before enabling in kernel,
> >>>>to simplify include all changes in the same patch.
> >>>>
> >>>>User space clients need to know when the pooled EU feature is present
> >>>>and enabled on the hardware so that they can adapt work submissions.
> >>>>Create a new device info flag for this purpose, and create a new GETPARAM
> >>>>entry to allow user space to query its setting.
> >>>>
> >>>>Set has_pooled_eu to true in the Broxton static device info - Broxton
> >>>>supports the feature in hardware and the driver will enable it by
> >>>>default.
> >>>>
> >>>>Opensource users for this feature are mesa, libva and beignet.
> >>>
> >>>Link to those patches would be great. At least I haven't seen anything fly
> >>>by for mesa or libva, might have missed it though. Note that the patches
> >>>must be fully reviewed an ready for merging by respective userspace
> >>>upstream, before we can land the kernel side.
> >>
> >>Initially beignet is trying to use it and they are adding support to it. I
> >>will share links once they send them to the list, reviewed and ready to be
> >>merged.
> >
> >Then please don't say you have the userspace when it doesn't exist yet.
> 
> Hi Daniel,
> 
> I never claimed userspace exists for this feature, only mentioned who are
> the users and clearly stated in cover letter that Beignet team is currently
> adding userspace and libdrm support, sorry if I gave that impression.

Sorry for missing that, I just spotted the one line in the patch here and
assumed it's all there already.

> >The other question: Will existing userspace fall over if we enable this in
> >the kernel, or is it purely opt-in? Would be good to clarify in the commit
> >message.
> Yes it is an opt-in, userspace queries its availability and configures
> itself. We do have one WA affecting few revisions where it needs to be
> enabled (explained in cover letter). I will add a note to the commit msg as
> well.

For the places where we must enable it, might be good to just merge that
without the userspace flag. That way we don't block the wa on the
userspace/ABI question. Then just add the patch with the getparam later on
when we have the userspace reviewed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx