[Intel-gfx] [PATCH v2 07/32] drm/i915/dg1: add initial DG-1 definitions

Tue Jun 23 06:17:11 UTC 2020

On Tue, Jun 23, 2020 at 6:54 AM Dave Airlie <airlied at gmail.com> wrote:
>
> On Mon, 22 Jun 2020 at 19:55, Jani Nikula <jani.nikula at linux.intel.com> wrote:
> >
> > On Mon, 22 Jun 2020, Daniel Vetter <daniel at ffwll.ch> wrote:
> > > On Wed, Jun 17, 2020 at 05:42:15PM -0700, Lucas De Marchi wrote:
> > >> From: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
> > >>
> > >> Bspec: 33617, 33617
> > >>
> > >> Cc: José Roberto de Souza <jose.souza at intel.com>
> > >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > >> Cc: Stuart Summers <stuart.summers at intel.com>
> > >> Cc: Vanshidhar Konda <vanshidhar.r.konda at intel.com>
> > >> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > >> Cc: Aravind Iddamsetty <aravind.iddamsetty at intel.com>
> > >> Cc: Matt Roper <matthew.d.roper at intel.com>
> > >> Signed-off-by: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
> > >> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> > >> Reviewed-by: José Roberto de Souza <jose.souza at intel.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/i915_drv.h          |  7 +++++++
> > >>  drivers/gpu/drm/i915/i915_pci.c          | 12 ++++++++++++
> > >>  drivers/gpu/drm/i915/intel_device_info.c |  1 +
> > >>  drivers/gpu/drm/i915/intel_device_info.h |  1 +
> > >>  4 files changed, 21 insertions(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > >> index 2f8057a0b2280..f79c09257eb6b 100644
> > >> --- a/drivers/gpu/drm/i915/i915_drv.h
> > >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> > >> @@ -1428,6 +1428,7 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> > >>  #define IS_ELKHARTLAKE(dev_priv)     IS_PLATFORM(dev_priv, INTEL_ELKHARTLAKE)
> > >>  #define IS_TIGERLAKE(dev_priv)       IS_PLATFORM(dev_priv, INTEL_TIGERLAKE)
> > >>  #define IS_ROCKETLAKE(dev_priv)      IS_PLATFORM(dev_priv, INTEL_ROCKETLAKE)
> > >> +#define IS_DG1(dev_priv)        IS_PLATFORM(dev_priv, INTEL_DG1)
> > >>  #define IS_HSW_EARLY_SDV(dev_priv) (IS_HASWELL(dev_priv) && \
> > >>                                   (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0C00)
> > >>  #define IS_BDW_ULT(dev_priv) \
> > >> @@ -1556,6 +1557,12 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> > >>  #define IS_RKL_REVID(p, since, until) \
> > >>       (IS_ROCKETLAKE(p) && IS_REVID(p, since, until))
> > >>
> > >> +#define DG1_REVID_A0         0x0
> > >> +#define DG1_REVID_B0         0x1
> > >> +
> > >> +#define IS_DG1_REVID(p, since, until) \
> > >> +     (IS_DG1(p) && IS_REVID(p, since, until))
> > >> +
> > >>  #define IS_LP(dev_priv)      (INTEL_INFO(dev_priv)->is_lp)
> > >>  #define IS_GEN9_LP(dev_priv) (IS_GEN(dev_priv, 9) && IS_LP(dev_priv))
> > >>  #define IS_GEN9_BC(dev_priv) (IS_GEN(dev_priv, 9) && !IS_LP(dev_priv))
> > >> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> > >> index e5fdf17cd9cdd..58cceeaa0ffa5 100644
> > >> --- a/drivers/gpu/drm/i915/i915_pci.c
> > >> +++ b/drivers/gpu/drm/i915/i915_pci.c
> > >> @@ -896,8 +896,20 @@ static const struct intel_device_info rkl_info = {
> > >>
> > >>  #define GEN12_DGFX_FEATURES \
> > >>       GEN12_FEATURES, \
> > >> +     .memory_regions = REGION_SMEM | REGION_LMEM, \
> > >
> > > This has lmem, and we need a new uapi for that. Last year we discussed a
> > > plan to have that behind a very scary compile-time only option, to make
> > > absolutely sure no one will start relying on this broken state before we
> > > aligned everything. And we know the current status is wrong since this
> > > patch series doesn't include any of the lmem specific uapi that's being
> > > worked on.
> > >
> > > But now almost a year passed, so that original plan needs to be
> > > renegotiated. Personally I'm not sure the scary compile option makes
> > > sense any longer, we're way later, users will soon have real hw, and once
> > > they have that they will find ways to enable it and we're potentially
> > > screwed.  Discussed this also with Joonas last week or so in private, and
> > > he shares similar concerns.
> > >
> > > So I think best option here (since keeping patches out of tree is rarely
> > > best option) would be to merge just the display enabling for DG1, but also
> > > making sure that we have all the rendering support completely disabled.
> > > This would mean:
> > > - wedge gpu on startup, just to make sure
> > > - drm_driver->num_ioctls = 0 (plus ioctls = NULL)
> > > - no setting DRIVER_RENDER and DRIVER_SYNCOBJ, we'd be a pure
> > > display-only driver
> > >
> > > Adding Dave and drm-intel maintainers to quickly hash this out. Thoughts?
> >
> > I'll defer the decision on lmem to folks who actually know this stuff,
> > and focus on display. And from that perspective, I'd like to unblock
> > merging the display patches.
> >
>
> How does display work without LMEM, I'm assuming you have to scan out
> from VRAM on DG1, even if we only have internal non-uapi is this
> enough to bring up fbcon?

It doesn't. And I think merging completely non-functional code to
upstream would be rather pointless, no one (outside from intel) can
test it, so the usual Dave Airlie "how does this benefit upstream"
would get a "not really, just dead weight".

But we have some basic lmem code in upstream already, and afaik it's
not much code to wire this up into dumb_create and intel_fbdev.c.
Which would give us a working kms-only driver (more or less, still
early enabling), which can be tested with what's in upstream only (so
actually somewhat useful, and unblocks all the display upstreaming,
heck we could even do kms-only CI with igt to make sure it keeps
working). And as long as we remove the ioctls and DRIVER_RENDER flag
(wedging the gpu is a bit overkill) I don't think there's any risk for
uapi: As long as the i915 getparam ioctl doesn't work for reading the
chipset id, umds skip (might need to double-check that, worst case we
have to rename the driver to something like "intel-display" to make
sure no one matches).

But yeah I think this series here is not quite there yet.

And then once Joonas we can try and figure out a new plan for
upstreaming lmem refactoring and uapi. When we discussed this all late
last summer just upstreaming kms-only was the first planned step,
altough back then with the render uapi hidden behind a compile flag.
Joonas thinks that's a bit too risk now that we're a lot later.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch