date:20210902

https://bugzilla.kernel.org/show_bug.cgi?id=211277

--- Comment #44 from James Zhu (jam...@amd.com) ---
Created attachment 298651
  --> https://bugzilla.kernel.org/attachment.cgi?id=298651=edit
A workaround for suspend/resume hung issue

The VCN block passed all ring tests, usually the vcn will get into idle within
1 sec. Somehow it affected later amd iommu device resume which is controlled by
kfd resume. This workaround is to gate vcn block immediately when ring test
passed.
It can fix the suspend/resume hung issue.

Hi kolAflash,
Please help check the WA in your setup. I will continue working on root cause.
thanks!
James

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

Re: [Intel-gfx] [PATCH 01/11] drm/i915: Release i915_gem_context from a worker

On Thu, Sep 2, 2021 at 6:20 PM Tvrtko Ursulin
 wrote:
> On 02/09/2021 16:05, Daniel Vetter wrote:
> > On Thu, Sep 2, 2021 at 2:42 PM Tvrtko Ursulin
> >  wrote:
> >>
> >>
> >> On 13/08/2021 21:30, Daniel Vetter wrote:
> >>> The only reason for this really is the i915_gem_engines->fence
> >>> callback engines_notify(), which exists purely as a fairly funky
> >>> reference counting scheme for that. Otherwise all other callers are
> >>> from process context, and generally fairly benign locking context.
> >>
> >> There is reset which definitely isn't process context.
> >
> > gpu reset runs in process context. The tasklet context is the
> > engines_notify I'm talking about above.
>
> I haven't looked very deeply but please double check the path from
> execlists_submission_tasklet -> execlists_reset -> intel_engine_reset ->
> __intel_engine_reset -> execlists_reset_rewind -> execlists_reset_csb ->
> execlists_reset_active -> __i915_request_reset -> mark_guilty ->
> i915_gem_context_put.

Thanks for pointing this out, I'll add it to the commit message.

More stuff to fix, yay.

> >> Otherwise I did not really get from the commit message is this patch
> >> fixing an existing problem or preparing something for the future. If the
> >> former then as I wrote above - I am pretty sure there are call sites
> >> from the tasklet already.
> >>
> >> Regards,
> >>
> >> Tvrtko
> >>
> >>> Unfortunately untangling that requires some major surgery, and we have
> >>> a few i915_gem_context reference counting bugs that need fixing, and
> >>> they blow in the current hardirq calling context, so we need a
> >>> stop-gap measure.
> >
> > I guess this para wasn't clear, but subsequent patches fix the
> > refcount bugs and need this prep patch here.
>
> So up to where in the series are those fixes and where other stuff
> follows? Worth spliting and having cover letters perhaps? Is the fixing
> part applicable to the existing code or only comes to play with the
> syncobj single timeline changes?

There's Fixes: lines. One is timeline syncobj, the other is 2 years old.
-Daniel

>
> Regards,
>
> Tvrtko
>
> > -Daniel
> >
> >>>
> >>> Put a FIXME comment in when this should be removable again.
> >>>
> >>> Signed-off-by: Daniel Vetter 
> >>> Cc: Jon Bloomfield 
> >>> Cc: Chris Wilson 
> >>> Cc: Maarten Lankhorst 
> >>> Cc: Joonas Lahtinen 
> >>> Cc: Daniel Vetter 
> >>> Cc: "Thomas Hellström" 
> >>> Cc: Matthew Auld 
> >>> Cc: Lionel Landwerlin 
> >>> Cc: Dave Airlie 
> >>> Cc: Jason Ekstrand 
> >>> ---
> >>>drivers/gpu/drm/i915/gem/i915_gem_context.c   | 13 +++--
> >>>drivers/gpu/drm/i915/gem/i915_gem_context_types.h | 12 
> >>>2 files changed, 23 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
> >>> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>> index fd169cf2f75a..051bc357ff65 100644
> >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>> @@ -986,9 +986,10 @@ static struct i915_gem_engines *user_engines(struct 
> >>> i915_gem_context *ctx,
> >>>return err;
> >>>}
> >>>
> >>> -void i915_gem_context_release(struct kref *ref)
> >>> +static void i915_gem_context_release_work(struct work_struct *work)
> >>>{
> >>> - struct i915_gem_context *ctx = container_of(ref, typeof(*ctx), ref);
> >>> + struct i915_gem_context *ctx = container_of(work, typeof(*ctx),
> >>> + release_work);
> >>>
> >>>trace_i915_context_free(ctx);
> >>>GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
> >>> @@ -1002,6 +1003,13 @@ void i915_gem_context_release(struct kref *ref)
> >>>kfree_rcu(ctx, rcu);
> >>>}
> >>>
> >>> +void i915_gem_context_release(struct kref *ref)
> >>> +{
> >>> + struct i915_gem_context *ctx = container_of(ref, typeof(*ctx), ref);
> >>> +
> >>> + queue_work(ctx->i915->wq, >release_work);
> >>> +}
> >>> +
> >>>static inline struct i915_gem_engines *
> >>>__context_engines_static(const struct i915_gem_context *ctx)
> >>>{
> >>> @@ -1303,6 +1311,7 @@ i915_gem_create_context(struct drm_i915_private 
> >>> *i915,
> >>>ctx->sched = pc->sched;
> >>>mutex_init(>mutex);
> >>>INIT_LIST_HEAD(>link);
> >>> + INIT_WORK(>release_work, i915_gem_context_release_work);
> >>>
> >>>spin_lock_init(>stale.lock);
> >>>INIT_LIST_HEAD(>stale.engines);
> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h 
> >>> b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> >>> index 94c03a97cb77..0c38789bd4a8 100644
> >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> >>> @@ -288,6 +288,18 @@ struct i915_gem_context {
> >>> */
> >>>struct kref ref;
> >>>
> >>> + /**
> >>> +  * @release_work:
> >>> +  *
> >>> +  * Work item for deferred cleanup, since

Re: [Intel-gfx] [PATCH 07/11] drm/i915: Add i915_gem_context_is_full_ppgtt

On Thu, Sep 02, 2021 at 05:05:05PM +, Bloomfield, Jon wrote:
> > -Original Message-
> > From: Tvrtko Ursulin 
> > Sent: Thursday, September 2, 2021 9:42 AM
> > To: Daniel Vetter 
> > Cc: Daniel Vetter ; DRI Development  > de...@lists.freedesktop.org>; Intel Graphics Development  > g...@lists.freedesktop.org>; Maarten Lankhorst
> > ; Vetter, Daniel
> > ; Bloomfield, Jon ;
> > Chris Wilson ; Joonas Lahtinen
> > ; Thomas Hellström
> > ; Auld, Matthew
> > ; Landwerlin, Lionel G
> > ; Dave Airlie ; Jason
> > Ekstrand 
> > Subject: Re: [Intel-gfx] [PATCH 07/11] drm/i915: Add
> > i915_gem_context_is_full_ppgtt
> > 
> > 
> > On 02/09/2021 16:22, Daniel Vetter wrote:
> > > On Thu, Sep 02, 2021 at 03:54:36PM +0100, Tvrtko Ursulin wrote:
> > >> On 02/09/2021 15:20, Daniel Vetter wrote:
> > >>> And use it anywhere we have open-coded checks for ctx->vm that really
> > >>> only check for full ppgtt.
> > >>>
> > >>> Plus for paranoia add a GEM_BUG_ON that checks it's really only set
> > >>> when we have full ppgtt, just in case. gem_context->vm is different
> > >>> since it's NULL in ggtt mode, unlike intel_context->vm or gt->vm,
> > >>> which is always set.
> > >>>
> > >>> v2: 0day found a testcase that I missed.
> > >>>
> > >>> Reviewed-by: Maarten Lankhorst 
> > >>> Signed-off-by: Daniel Vetter 
> > >>> Cc: Jon Bloomfield 
> > >>> Cc: Chris Wilson 
> > >>> Cc: Maarten Lankhorst 
> > >>> Cc: Joonas Lahtinen 
> > >>> Cc: Daniel Vetter 
> > >>> Cc: "Thomas Hellström" 
> > >>> Cc: Matthew Auld 
> > >>> Cc: Lionel Landwerlin 
> > >>> Cc: Dave Airlie 
> > >>> Cc: Jason Ekstrand 
> > >>> ---
> > >>>drivers/gpu/drm/i915/gem/i915_gem_context.c   | 2 +-
> > >>>drivers/gpu/drm/i915/gem/i915_gem_context.h   | 7 +++
> > >>>drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +-
> > >>>drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 6 +++---
> > >>>4 files changed, 12 insertions(+), 5 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > >>> index 7a566fb7cca4..1eec85944c1f 100644
> > >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > >>> @@ -1566,7 +1566,7 @@ static int get_ppgtt(struct
> > drm_i915_file_private *file_priv,
> > >>> int err;
> > >>> u32 id;
> > >>> -   if (!rcu_access_pointer(ctx->vm))
> > >>> +   if (!i915_gem_context_is_full_ppgtt(ctx))
> > >>
> > >> It reads a bit wrong because GEM context cannot *be* full ppggt. It can
> > be
> > >> associated with a VM which is or isn't full ppgtt. So a test on a VM
> > >> retrieved from a context is semnntically more correct. Perhaps you want
> > to
> > >> consider adding a helper to that effect instead? It could mean splitting
> > >> into two helpers (getter + test) or maybe just renaming would work. Like
> > >> i915_gem_context_has_full_ppgtt_vm(ctx)?
> > >
> > > The pointer isn't set when the driver/context isn't running in full ppgtt
> > > mode. This is why I've added the GEM_BUG_ON to check we're not
> > breaking
> > > any invariants. So yeah it is a full ppgtt context or it's not, that is
> > > indeed the question here.
> > >
> > > I'm happy to bikeshed the naming, but I don't see how your suggestion is
> > > an improvement.
> > 
> > I think the pointer being set or not is implementation detail, for
> > instance we could have it always set just like it is in intel_context.
> > 
> > I simply think GEM context *isn't* full ppgtt, but the VM is. And since
> > GEM context *points* to a VM, *has* is the right verb in my mind. You
> > did not write why do you not see has as more correct than is so I don't
> > want to be guessing too much.
> 
> FWIW, I agree with Tvrtko. i915_gem_context_is_full_ppgtt is incorrect
> grammar. It IS a bike shed, but, hey it'll live for a while.

Generally all our feature checks are of the various is_foo variety.
HAS_FULL_PPGTT is one of the very rare exceptions. So it's a question of
"is gem_ctx foo", not "has gem_ctx foo". The fact that we implement the
check by looking at a pointer doesn't matter. And yes if it is full ppgtt,
it also has it's own private vm, but we don't care about that part at all.
It's just a distraction.

Anyway I'll repaint, least because the HAS_FULL_PPGTT thing is almost a
decade old by now :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 2/2] clk: qcom: gcc-sdm660: Remove transient global "xo" clock

2021-09-02 Thread Stephen Boyd

Quoting Marijn Suijten (2021-09-02 06:05:34)
> On 2021-09-01 20:46:34, Stephen Boyd wrote:
> > Quoting Marijn Suijten (2021-09-01 01:57:15)
> > > On 2021-08-31 22:35:56, Stephen Boyd wrote:
> > > > Quoting Marijn Suijten (2021-08-30 11:24:45)
> > > > > The DSI PHY/PLL was relying on a global "xo" clock to be found, but 
> > > > > the
> > > > > real clock is named "xo_board" in the DT.  The standard nowadays is to
> > > > > never use global clock names anymore but require the firmware (DT) to
> > > > > provide every clock binding explicitly with .fw_name.  The DSI PLLs 
> > > > > have
> > > > > since been converted to this mechanism (specifically 14nm for SDM660)
> > > > > and this transient clock can now be removed.
> > > > > 
> > > > > This issue was originally discovered in:
> > > > > https://lore.kernel.org/linux-arm-msm/386db1a6-a1cd-3c7d-a88e-dc83f8a1b...@somainline.org/
> > > > > and prevented the removal of "xo" at that time.
> > > > > 
> > > > > Signed-off-by: Marijn Suijten 
> > > > > ---
> > > > 
> > > > Presumably this wants to go with the first one.
> > > 
> > > What are you referring to with "the first one"?  This patch can only go
> > > in after patch 1/2 of this series, unless you are suggesting to squash
> > > it with Bjorns cleanup and making sure that lands after the fix in the
> > > DSI?
> > 
> > The first patch in this series.
> 
> Are you suggesting to squash this patch into the first patch in this
> series?  Note that the first patch touches drm/msm/dsi, the second
> (this) patch touches clk/qcom.

No.

Re: [PATCH v5 10/25] drm/i915/guc: Copy whole golden context, set engine state size of subset

2021-09-02 Thread John Harrison


On 9/1/2021 17:50, Daniele Ceraolo Spurio wrote:

From: Matthew Brost 

When the GuC does a media reset, it copies a golden context state back
into the corrupted context's state. The address of the golden context
and the size of the engine state restore are passed in via the GuC ADS.
The i915 had a bug where it passed in the whole size of the golden
context, not the size of the engine state to restore resulting in a
memory corruption.

Also copy the entire golden context on init rather than just the engine
state that is restored.

v2 (Daniele): use defines to avoid duplicated const variables (John).

Fixes: 481d458caede ("drm/i915/guc: Add golden context to GuC ADS")
Signed-off-by: Matthew Brost 
Signed-off-by: Daniele Ceraolo Spurio 
Cc: John Harrison 
---
  drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c | 26 ++
  1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
index 6926919bcac6..2c6ea64af7ec 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
@@ -349,6 +349,8 @@ static void fill_engine_enable_masks(struct intel_gt *gt,
info->engine_enabled_masks[GUC_VIDEOENHANCE_CLASS] = VEBOX_MASK(gt);
  }
  
+#define LR_HW_CONTEXT_SIZE (80 * sizeof(u32))

+#define LRC_SKIP_SIZE (LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE)
Personally, I would have preferred to turn it into a function. 
Especially as it gets more complex with the later platforms that are now 
being pushed upstream. Not a blocker though.


Reviewed-by: John Harrison 


  static int guc_prep_golden_context(struct intel_guc *guc,
   struct __guc_ads_blob *blob)
  {
@@ -396,7 +398,18 @@ static int guc_prep_golden_context(struct intel_guc *guc,
if (!blob)
continue;
  
-		blob->ads.eng_state_size[guc_class] = real_size;

+   /*
+* This interface is slightly confusing. We need to pass the
+* base address of the full golden context and the size of just
+* the engine state, which is the section of the context image
+* that starts after the execlists context. This is required to
+* allow the GuC to restore just the engine state when a
+* watchdog reset occurs.
+* We calculate the engine state size by removing the size of
+* what comes before it in the context image (which is identical
+* on all engines).
+*/
+   blob->ads.eng_state_size[guc_class] = real_size - LRC_SKIP_SIZE;
blob->ads.golden_context_lrca[guc_class] = addr_ggtt;
addr_ggtt += alloc_size;
}
@@ -436,11 +449,6 @@ static void guc_init_golden_context(struct intel_guc *guc)
u8 engine_class, guc_class;
u8 *ptr;
  
-	/* Skip execlist and PPGTT registers + HWSP */

-   const u32 lr_hw_context_size = 80 * sizeof(u32);
-   const u32 skip_size = LRC_PPHWSP_SZ * PAGE_SIZE +
-   lr_hw_context_size;
-
if (!intel_uc_uses_guc_submission(>uc))
return;
  
@@ -476,12 +484,12 @@ static void guc_init_golden_context(struct intel_guc *guc)

continue;
}
  
-		GEM_BUG_ON(blob->ads.eng_state_size[guc_class] != real_size);

+   GEM_BUG_ON(blob->ads.eng_state_size[guc_class] !=
+  real_size - LRC_SKIP_SIZE);
GEM_BUG_ON(blob->ads.golden_context_lrca[guc_class] != 
addr_ggtt);
addr_ggtt += alloc_size;
  
-		shmem_read(engine->default_state, skip_size, ptr + skip_size,

-  real_size - skip_size);
+   shmem_read(engine->default_state, 0, ptr, real_size);
ptr += alloc_size;
}

Re: [PATCH] drm/msm: remove unneeded variable

2021-09-02 Thread Lyude Paul

Reviewed-by: Lyude Paul 

Do you need me to push this?

On Tue, 2021-08-31 at 04:51 -0700, cgel@gmail.com wrote:
> From: Chi Minghao 
> 
> Fix the following coccicheck REVIEW:
> ./drivers/gpu/drm/msm/edp/edp_ctrl.c:1245:5-8 Unneeded variable
> 
> Reported-by: Zeal Robot 
> Signed-off-by: Chi Minghao 
> ---
>  drivers/gpu/drm/msm/edp/edp_ctrl.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/edp/edp_ctrl.c
> b/drivers/gpu/drm/msm/edp/edp_ctrl.c
> index 4fb397ee7c84..3610e26e62fa 100644
> --- a/drivers/gpu/drm/msm/edp/edp_ctrl.c
> +++ b/drivers/gpu/drm/msm/edp/edp_ctrl.c
> @@ -1242,8 +1242,6 @@ bool msm_edp_ctrl_panel_connected(struct edp_ctrl
> *ctrl)
>  int msm_edp_ctrl_get_panel_info(struct edp_ctrl *ctrl,
> struct drm_connector *connector, struct edid **edid)
>  {
> -   int ret = 0;
> -
> mutex_lock(>dev_mutex);
>  
> if (ctrl->edid) {
> @@ -1278,7 +1276,7 @@ int msm_edp_ctrl_get_panel_info(struct edp_ctrl *ctrl,
> }
>  unlock_ret:
> mutex_unlock(>dev_mutex);
> -   return ret;
> +   return 0;
>  }
>  
>  int msm_edp_ctrl_timing_cfg(struct edp_ctrl *ctrl,

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [PATCH v1 03/14] mm: add iomem vma selection for memory migration

2021-09-02 Thread Dan Williams

On Thu, Sep 2, 2021 at 1:18 AM Christoph Hellwig  wrote:
>
> On Wed, Sep 01, 2021 at 11:40:43AM -0400, Felix Kuehling wrote:
> > >>> It looks like I'm totally misunderstanding what you are adding here
> > >>> then.  Why do we need any special treatment at all for memory that
> > >>> has normal struct pages and is part of the direct kernel map?
> > >> The pages are like normal memory for purposes of mapping them in CPU
> > >> page tables and for coherent access from the CPU.
> > > That's the user page tables.  What about the kernel direct map?
> > > If there is a normal kernel struct page backing there really should
> > > be no need for the pgmap.
> >
> > I'm not sure. The physical address ranges are in the UEFI system address
> > map as special-purpose memory. Does Linux create the struct pages and
> > kernel direct map for that without a pgmap call? I didn't see that last
> > time I went digging through that code.
>
> So doing some googling finds a patch from Dan that claims to hand EFI
> special purpose memory to the device dax driver.  But when I try to
> follow the version that got merged it looks it is treated simply as an
> MMIO region to be claimed by drivers, which would not get a struct page.
>
> Dan, did I misunderstand how E820_TYPE_SOFT_RESERVED works?

The original implementation of "soft reserve" support depended on the
combination of the EFI special purpose memory type and the ACPI HMAT
to define the device ranges. The requirement for ACPI HMAT was relaxed
later with commit:

5ccac54f3e12 ACPI: HMAT: attach a device for each soft-reserved range

The expectation is that system software policy can then either use the
device interface, assign a portion of the reservation back to the page
allocator, ignore the reservation altogether. Is this discussion
asking for a way to assign this memory to the GPU driver to manage?
device-dax already knows how to hand off to the page-allocator, seems
reasonable for it to be able to hand-off to another driver.

Re: [PATCH v5 25/25] drm/i915/guc: Add GuC kernel doc

2021-09-02 Thread Daniele Ceraolo Spurio





On 9/2/2021 10:01 AM, John Harrison wrote:

On 9/1/2021 17:50, Daniele Ceraolo Spurio wrote:

From: Matthew Brost 

Add GuC kernel doc for all structures added thus far for GuC submission
and update the main GuC submission section with the new interface
details.

v2:
  - Drop guc_active.lock DOC
v3:
  - Fixup a few kernel doc comments (Daniele)
v4 (Daniele):
  - Implement doc suggestions from John
  - Add kerneldoc for all members of the GuC structure and pull the file
    in i915.rst

Signed-off-by: Matthew Brost 
Signed-off-by: Daniele Ceraolo Spurio 
Cc: John Harrison 
---
  Documentation/gpu/i915.rst    |   2 +
  drivers/gpu/drm/i915/gt/intel_context_types.h |  43 +---
  drivers/gpu/drm/i915/gt/uc/intel_guc.h    |  68 +---
  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 100 ++
  drivers/gpu/drm/i915/i915_request.h   |  21 ++--
  5 files changed, 174 insertions(+), 60 deletions(-)

diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
index 101dde3eb1ea..311e10400708 100644
--- a/Documentation/gpu/i915.rst
+++ b/Documentation/gpu/i915.rst
@@ -495,6 +495,8 @@ GuC
  .. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_guc.c
 :doc: GuC
  +.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_guc.h
+
  GuC Firmware Layout
  ~~~
  diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h 
b/drivers/gpu/drm/i915/gt/intel_context_types.h

index 5285d660eacf..930569a1a01f 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -156,40 +156,51 @@ struct intel_context {
  u8 wa_bb_page; /* if set, page num reserved for context 
workarounds */

    struct {
-    /** lock: protects everything in guc_state */
+    /** @lock: protects everything in guc_state */
  spinlock_t lock;
  /**
- * sched_state: scheduling state of this context using GuC
+ * @sched_state: scheduling state of this context using GuC
   * submission
   */
  u32 sched_state;
  /*
- * fences: maintains of list of requests that have a submit
- * fence related to GuC submission
+ * @fences: maintains a list of requests that are currently
+ * being fenced until a GuC operation completes
   */
  struct list_head fences;
-    /* GuC context blocked fence */
+    /**
+ * @blocked: fence used to signal when the blocking of a
+ * context's submissions is complete.
+ */
  struct i915_sw_fence blocked;
-    /* GuC committed requests */
+    /** @number_committed_requests: number of committed requests */
  int number_committed_requests;
-    /** requests: active requests on this context */
+    /** @requests: list of active requests on this context */
  struct list_head requests;
-    /*
- * GuC priority management
- */
+    /** @prio: the context's current guc priority */
  u8 prio;
+    /**
+ * @prio_count: a counter of the number requests in flight in
+ * each priority bucket
+ */
  u32 prio_count[GUC_CLIENT_PRIORITY_NUM];
  } guc_state;
    struct {
-    /* GuC LRC descriptor ID */
+    /**
+ * @id: handle which is used to uniquely identify this context
+ * with the GuC, protected by guc->contexts_lock
+ */
  u16 id;
-
-    /* GuC LRC descriptor reference count */
+    /**
+ * @ref: the number of references to the guc_id, when
+ * transitioning in and out of zero protected by
+ * guc->contexts_lock
+ */
  atomic_t ref;
-
-    /*
- * GuC ID link - in list when unpinned but guc_id still 
valid in GuC

+    /**
+ * @link: in guc->guc_id_list when the guc_id has no refs 
but is

+ * still valid, protected by guc->contexts_lock
   */
  struct list_head link;
  } guc_id;

Not part of this patch, but shouldn't guc_id be inside guc_state?

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h

index 2e27fe59786b..8427cd590087 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -22,74 +22,114 @@
    struct __guc_ads_blob;
  -/*
- * Top level structure of GuC. It handles firmware loading and 
manages client
- * pool. intel_guc owns a intel_guc_client to replace the legacy 
ExecList

- * submission.
+/**
+ * struct intel_guc - Top level structure of GuC.
+ *
+ * It handles firmware loading and manages client pool. intel_guc 
owns an

+ * i915_sched_engine to replace the legacy ExecList submission.
Not sure if this is referring to how the GuC code used to be or if it 
is just meaning that the execlist backend as a whole is 'legacy'? 
Either way, seems like it would be simpler to just say 'manages client 
pool, owns an

RE: [Intel-gfx] [PATCH 07/11] drm/i915: Add i915_gem_context_is_full_ppgtt

2021-09-02 Thread Bloomfield, Jon

> -Original Message-
> From: Tvrtko Ursulin 
> Sent: Thursday, September 2, 2021 9:42 AM
> To: Daniel Vetter 
> Cc: Daniel Vetter ; DRI Development  de...@lists.freedesktop.org>; Intel Graphics Development  g...@lists.freedesktop.org>; Maarten Lankhorst
> ; Vetter, Daniel
> ; Bloomfield, Jon ;
> Chris Wilson ; Joonas Lahtinen
> ; Thomas Hellström
> ; Auld, Matthew
> ; Landwerlin, Lionel G
> ; Dave Airlie ; Jason
> Ekstrand 
> Subject: Re: [Intel-gfx] [PATCH 07/11] drm/i915: Add
> i915_gem_context_is_full_ppgtt
> 
> 
> On 02/09/2021 16:22, Daniel Vetter wrote:
> > On Thu, Sep 02, 2021 at 03:54:36PM +0100, Tvrtko Ursulin wrote:
> >> On 02/09/2021 15:20, Daniel Vetter wrote:
> >>> And use it anywhere we have open-coded checks for ctx->vm that really
> >>> only check for full ppgtt.
> >>>
> >>> Plus for paranoia add a GEM_BUG_ON that checks it's really only set
> >>> when we have full ppgtt, just in case. gem_context->vm is different
> >>> since it's NULL in ggtt mode, unlike intel_context->vm or gt->vm,
> >>> which is always set.
> >>>
> >>> v2: 0day found a testcase that I missed.
> >>>
> >>> Reviewed-by: Maarten Lankhorst 
> >>> Signed-off-by: Daniel Vetter 
> >>> Cc: Jon Bloomfield 
> >>> Cc: Chris Wilson 
> >>> Cc: Maarten Lankhorst 
> >>> Cc: Joonas Lahtinen 
> >>> Cc: Daniel Vetter 
> >>> Cc: "Thomas Hellström" 
> >>> Cc: Matthew Auld 
> >>> Cc: Lionel Landwerlin 
> >>> Cc: Dave Airlie 
> >>> Cc: Jason Ekstrand 
> >>> ---
> >>>drivers/gpu/drm/i915/gem/i915_gem_context.c   | 2 +-
> >>>drivers/gpu/drm/i915/gem/i915_gem_context.h   | 7 +++
> >>>drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +-
> >>>drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 6 +++---
> >>>4 files changed, 12 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>> index 7a566fb7cca4..1eec85944c1f 100644
> >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>> @@ -1566,7 +1566,7 @@ static int get_ppgtt(struct
> drm_i915_file_private *file_priv,
> >>>   int err;
> >>>   u32 id;
> >>> - if (!rcu_access_pointer(ctx->vm))
> >>> + if (!i915_gem_context_is_full_ppgtt(ctx))
> >>
> >> It reads a bit wrong because GEM context cannot *be* full ppggt. It can
> be
> >> associated with a VM which is or isn't full ppgtt. So a test on a VM
> >> retrieved from a context is semnntically more correct. Perhaps you want
> to
> >> consider adding a helper to that effect instead? It could mean splitting
> >> into two helpers (getter + test) or maybe just renaming would work. Like
> >> i915_gem_context_has_full_ppgtt_vm(ctx)?
> >
> > The pointer isn't set when the driver/context isn't running in full ppgtt
> > mode. This is why I've added the GEM_BUG_ON to check we're not
> breaking
> > any invariants. So yeah it is a full ppgtt context or it's not, that is
> > indeed the question here.
> >
> > I'm happy to bikeshed the naming, but I don't see how your suggestion is
> > an improvement.
> 
> I think the pointer being set or not is implementation detail, for
> instance we could have it always set just like it is in intel_context.
> 
> I simply think GEM context *isn't* full ppgtt, but the VM is. And since
> GEM context *points* to a VM, *has* is the right verb in my mind. You
> did not write why do you not see has as more correct than is so I don't
> want to be guessing too much.

FWIW, I agree with Tvrtko. i915_gem_context_is_full_ppgtt is incorrect grammar. 
It IS a bike shed, but, hey it'll live for a while.

Re: [PATCH v5 25/25] drm/i915/guc: Add GuC kernel doc

2021-09-02 Thread John Harrison


On 9/1/2021 17:50, Daniele Ceraolo Spurio wrote:

From: Matthew Brost 

Add GuC kernel doc for all structures added thus far for GuC submission
and update the main GuC submission section with the new interface
details.

v2:
  - Drop guc_active.lock DOC
v3:
  - Fixup a few kernel doc comments (Daniele)
v4 (Daniele):
  - Implement doc suggestions from John
  - Add kerneldoc for all members of the GuC structure and pull the file
in i915.rst

Signed-off-by: Matthew Brost 
Signed-off-by: Daniele Ceraolo Spurio 
Cc: John Harrison 
---
  Documentation/gpu/i915.rst|   2 +
  drivers/gpu/drm/i915/gt/intel_context_types.h |  43 +---
  drivers/gpu/drm/i915/gt/uc/intel_guc.h|  68 +---
  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 100 ++
  drivers/gpu/drm/i915/i915_request.h   |  21 ++--
  5 files changed, 174 insertions(+), 60 deletions(-)

diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
index 101dde3eb1ea..311e10400708 100644
--- a/Documentation/gpu/i915.rst
+++ b/Documentation/gpu/i915.rst
@@ -495,6 +495,8 @@ GuC
  .. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_guc.c
 :doc: GuC
  
+.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_guc.h

+
  GuC Firmware Layout
  ~~~
  
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h

index 5285d660eacf..930569a1a01f 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -156,40 +156,51 @@ struct intel_context {
u8 wa_bb_page; /* if set, page num reserved for context workarounds */
  
  	struct {

-   /** lock: protects everything in guc_state */
+   /** @lock: protects everything in guc_state */
spinlock_t lock;
/**
-* sched_state: scheduling state of this context using GuC
+* @sched_state: scheduling state of this context using GuC
 * submission
 */
u32 sched_state;
/*
-* fences: maintains of list of requests that have a submit
-* fence related to GuC submission
+* @fences: maintains a list of requests that are currently
+* being fenced until a GuC operation completes
 */
struct list_head fences;
-   /* GuC context blocked fence */
+   /**
+* @blocked: fence used to signal when the blocking of a
+* context's submissions is complete.
+*/
struct i915_sw_fence blocked;
-   /* GuC committed requests */
+   /** @number_committed_requests: number of committed requests */
int number_committed_requests;
-   /** requests: active requests on this context */
+   /** @requests: list of active requests on this context */
struct list_head requests;
-   /*
-* GuC priority management
-*/
+   /** @prio: the context's current guc priority */
u8 prio;
+   /**
+* @prio_count: a counter of the number requests in flight in
+* each priority bucket
+*/
u32 prio_count[GUC_CLIENT_PRIORITY_NUM];
} guc_state;
  
  	struct {

-   /* GuC LRC descriptor ID */
+   /**
+* @id: handle which is used to uniquely identify this context
+* with the GuC, protected by guc->contexts_lock
+*/
u16 id;
-
-   /* GuC LRC descriptor reference count */
+   /**
+* @ref: the number of references to the guc_id, when
+* transitioning in and out of zero protected by
+* guc->contexts_lock
+*/
atomic_t ref;
-
-   /*
-* GuC ID link - in list when unpinned but guc_id still valid 
in GuC
+   /**
+* @link: in guc->guc_id_list when the guc_id has no refs but is
+* still valid, protected by guc->contexts_lock
 */
struct list_head link;
} guc_id;

Not part of this patch, but shouldn't guc_id be inside guc_state?


diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 2e27fe59786b..8427cd590087 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -22,74 +22,114 @@
  
  struct __guc_ads_blob;
  
-/*

- * Top level structure of GuC. It handles firmware loading and manages client
- * pool. intel_guc owns a intel_guc_client to replace the legacy ExecList
- * submission.
+/**
+ * struct intel_guc - Top level structure of GuC.
+ *
+ * It handles firmware loading

Re: [diagnostic TDR mode patches] unify our solution opinions/suggestions in one thread

2021-09-02 Thread Daniel Stone

Hi Monk,

On Thu, 2 Sept 2021 at 06:52, Liu, Monk  wrote:
> I didn't mean your changes on AMD driver need my personal approval or review 
> ... and  I'm totally already get used that our driver is not 100% under 
> control by AMDers,
> but supposedly any one from community (including you) who tend to change 
> AMD's driver need at least to get approvement from someone in AMD, e.g.: 
> AlexD or Christian, doesn't that reasonable?
> just like we need your approve if we try to modify DRM-sched, or need 
> panfrost's approval if we need to change panfrost code ...
>
> by only CC AMD's engineers looks not quite properly, how do you know if your 
> changes (on AMD code part) are conflicting with AMD's on-going internal 
> features/refactoring or not ?

Looking at the patches in question, they were (at least mostly) CCed
both to the amd-gfx@ mailing list and also to ckoenig. Unfortunately
it is not possible for every single patch to get mandatory signoff
from every single stakeholder - e.g. if every AMD patch which touched
the scheduler required explicit approval from Etnaviv, Freedreno,
Lima, Panfrost, and V3D teams, it would become very difficult for AMD
to merge any code.

So the approach is that patches are sent for approval, they are CCed
to people who should be interested, and after some time with no
comments, they may be merged if it seems like a reasonable thing to
do.

The problem with internal work is that, well, it's internal. If the
community sends patches to amd-gfx@, there is no comment from AMD, and
then months later we are told that it should not have happened because
it conflicts with development that AMD has been doing - how should the
rest of the community have known about this? So unfortunately this is
the compromise: if you decide to do private development, not inform
anyone about your plans, and not join in any common discussion, then
it is your responsibility to deal with any changes or conflicts that
happen whilst you are developing privately.

The only way we can successfully have support in the same ecosystem
for AMD, Arm, Broadcom, Intel, NVIDIA, Qualcomm, and VeriSilicon, is
that we are all working together openly. If community development had
to stop because each of these vendors had been doing internal
development for several months without even informing the community of
their plans, any kind of shared development is clearly impossible.

Cheers,
Daniel

Re: [Intel-gfx] [PATCH 07/11] drm/i915: Add i915_gem_context_is_full_ppgtt




On 02/09/2021 16:22, Daniel Vetter wrote:

On Thu, Sep 02, 2021 at 03:54:36PM +0100, Tvrtko Ursulin wrote:

On 02/09/2021 15:20, Daniel Vetter wrote:

And use it anywhere we have open-coded checks for ctx->vm that really
only check for full ppgtt.

Plus for paranoia add a GEM_BUG_ON that checks it's really only set
when we have full ppgtt, just in case. gem_context->vm is different
since it's NULL in ggtt mode, unlike intel_context->vm or gt->vm,
which is always set.

v2: 0day found a testcase that I missed.

Reviewed-by: Maarten Lankhorst 
Signed-off-by: Daniel Vetter 
Cc: Jon Bloomfield 
Cc: Chris Wilson 
Cc: Maarten Lankhorst 
Cc: Joonas Lahtinen 
Cc: Daniel Vetter 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Cc: Lionel Landwerlin 
Cc: Dave Airlie 
Cc: Jason Ekstrand 
---
   drivers/gpu/drm/i915/gem/i915_gem_context.c   | 2 +-
   drivers/gpu/drm/i915/gem/i915_gem_context.h   | 7 +++
   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +-
   drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 6 +++---
   4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 7a566fb7cca4..1eec85944c1f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1566,7 +1566,7 @@ static int get_ppgtt(struct drm_i915_file_private 
*file_priv,
int err;
u32 id;
-   if (!rcu_access_pointer(ctx->vm))
+   if (!i915_gem_context_is_full_ppgtt(ctx))


It reads a bit wrong because GEM context cannot *be* full ppggt. It can be
associated with a VM which is or isn't full ppgtt. So a test on a VM
retrieved from a context is semnntically more correct. Perhaps you want to
consider adding a helper to that effect instead? It could mean splitting
into two helpers (getter + test) or maybe just renaming would work. Like
i915_gem_context_has_full_ppgtt_vm(ctx)?


The pointer isn't set when the driver/context isn't running in full ppgtt
mode. This is why I've added the GEM_BUG_ON to check we're not breaking
any invariants. So yeah it is a full ppgtt context or it's not, that is
indeed the question here.

I'm happy to bikeshed the naming, but I don't see how your suggestion is
an improvement.


I think the pointer being set or not is implementation detail, for 
instance we could have it always set just like it is in intel_context.


I simply think GEM context *isn't* full ppgtt, but the VM is. And since 
GEM context *points* to a VM, *has* is the right verb in my mind. You 
did not write why do you not see has as more correct than is so I don't 
want to be guessing too much.


Regards,

Tvrtko



Regards,

Tvrtko


return -ENODEV;
rcu_read_lock();
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h 
b/drivers/gpu/drm/i915/gem/i915_gem_context.h
index da6e8b506d96..37536a260e6e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
@@ -154,6 +154,13 @@ i915_gem_context_vm(struct i915_gem_context *ctx)
return rcu_dereference_protected(ctx->vm, lockdep_is_held(>mutex));
   }
+static inline bool i915_gem_context_is_full_ppgtt(struct i915_gem_context *ctx)
+{
+   GEM_BUG_ON(!!rcu_access_pointer(ctx->vm) != HAS_FULL_PPGTT(ctx->i915));
+
+   return !!rcu_access_pointer(ctx->vm);
+}
+
   static inline struct i915_address_space *
   i915_gem_context_get_eb_vm(struct i915_gem_context *ctx)
   {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c 
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 905b1cbd22d5..40f08948f0b2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -749,7 +749,7 @@ static int eb_select_context(struct i915_execbuffer *eb)
return PTR_ERR(ctx);
eb->gem_context = ctx;
-   if (rcu_access_pointer(ctx->vm))
+   if (i915_gem_context_is_full_ppgtt(ctx))
eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
return 0;
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
index fc7fb33a3a52..947154e445a7 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
@@ -704,7 +704,7 @@ static int igt_ctx_exec(void *arg)
pr_err("Failed to fill dword %lu [%lu/%lu] with gpu 
(%s) [full-ppgtt? %s], err=%d\n",
   ndwords, dw, max_dwords(obj),
   engine->name,
-  yesno(!!rcu_access_pointer(ctx->vm)),
+  
yesno(i915_gem_context_is_full_ppgtt(ctx)),
   err);
intel_context_put(ce);
kernel_context_close(ctx);
@@ -838,7

[Bug 214289] amdgpu Msg issuing pre-check failed and SMU may be not in the right state!

https://bugzilla.kernel.org/show_bug.cgi?id=214289

--- Comment #3 from Michal Przybylowicz (michal.przybylow...@gmail.com) ---
Just one thing that I have noticed it looks like these messages appear when I
do some interactions on webpages like clicking dropdown... 

I am using Vivaldi: 4.1.2369.21 (Stable channel) stable (64-bit)

And it also spams its log with these errors (maybe this all is related):

[1542:1542:0902/180420.608750:ERROR:shared_context_state.cc(73)] : Skia shader
compilation error  Errors:
[1542:1542:0902/180425.551298:ERROR:gles2_cmd_decoder.cc(18958)] :
[.RendererMainThread-0x3b5002c41a00]GL ERROR :GL_INVALID_OPERATION :
DoBeginSharedImageAccessCHROMIUM: bound texture is not a shared image
[1542:1542:0902/180425.552365:ERROR:gles2_cmd_decoder.cc(18987)] :
[.RendererMainThread-0x3b5002c41a00]GL ERROR :GL_INVALID_OPERATION :
DoEndSharedImageAccessCHROMIUM: bound texture is not a shared image
[1542:1542:0902/180539.574319:ERROR:shared_image_manager.cc(214)] :
SharedImageManager::ProduceSkia: Trying to Produce a Skia representation from a
non-existent mailbox.
[1542:1542:0902/180539.574446:ERROR:shared_image_manager.cc(214)] :
SharedImageManager::ProduceSkia: Trying to Produce a Skia representation from a
non-existent mailbox.
[1542:1542:0902/180606.529719:ERROR:shared_context_state.cc(73)] : Skia shader
compilation error  Errors:
[1542:1542:0902/180606.557709:ERROR:shared_context_state.cc(73)] : Skia shader
compilation error  Errors:
[1542:1542:0902/183031.153537:ERROR:shared_image_manager.cc(214)] :
SharedImageManager::ProduceSkia: Trying to Produce a Skia representation from a
non-existent mailbox.
[1542:1542:0902/183031.153664:ERROR:shared_image_manager.cc(214)] :
SharedImageManager::ProduceSkia: Trying to Produce a Skia representation from a
non-existent mailbox.
[1542:1542:0902/183143.069374:ERROR:shared_context_state.cc(73)] : Skia shader
compilation error  Errors:
[1542:1542:0902/183213.030240:ERROR:shared_context_state.cc(73)] : Skia shader
compilation error  Errors:

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 214289] amdgpu Msg issuing pre-check failed and SMU may be not in the right state!

https://bugzilla.kernel.org/show_bug.cgi?id=214289

--- Comment #2 from Michal Przybylowicz (michal.przybylow...@gmail.com) ---
Created attachment 298645
  --> https://bugzilla.kernel.org/attachment.cgi?id=298645=edit
dmesg

full dmesg extraced using:

$ journalctl -k -b -1 --no-pager > ~/Downloads/dmesg.txt

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

Re: [PATCH v6, 00/15] Using component framework to support multi hardware decode

2021-09-02 Thread Ezequiel Garcia

On Wed, 1 Sept 2021 at 05:32, Yunfei Dong  wrote:
>
> This series adds support for multi hardware decode into mtk-vcodec, by first
> adding component framework to manage each hardware information: interrupt,
> clock, register bases and power. Secondly add core thread to deal with core
> hardware message, at the same time, add msg queue for different hardware
> share messages. Lastly, the architecture of different specs are not the same,
> using specs type to separate them.
>
> This series has been tested with both MT8183 and MT8173. Decoding was working
> for both chips.
>
> Patches 1~3 rewrite get register bases and power on/off interface.
>
> Patch 4 add component framework to support multi hardware.
>
> Patch 5 separate video encoder and decoder document
>
> Patches 6-15 add interfaces to support core hardware.
> 
> This patch dependents on : "media: mtk-vcodec: support for MT8183 decoder"[1] 
> and
> "Mediatek MT8192 clock support"[2].
>
> 1: Multi hardware decode is based on stateless decoder, MT8183 is the first 
> time
> to add stateless decoder. Otherwise it will cause conflict. This patch will be
> accepted in 5.15[1].
>
> 2: The definition of decoder clocks are in mt8192-clk.h, this patch already 
> in clk tree[2].
>
> [1]https://patchwork.linuxtv.org/project/linux-media/list/?series=5826
> [2]https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/?h=clk-next=f35f1a23e0e12e3173e9e9dedbc150d139027189
> 
> Changes compared with v5:
> -Add decoder hardware block diagram for patch 13/15
>

The discussion on v5 was still on-going, so sending this v6
is not helpful. The context for v5's discussion is now harder to find.

Please avoid sending a new version without properly
discussing all the feedback, and without reaching consensus.
This is very important, please keep it in mind.

Specifically, the feedback on v5 was NAK, with the request to avoid
using any async framework, and instead try to find a simpler solution.

For instance, you can model things with a bus-like pattern,
which ties all the devices together, under a parent node.
This pattern is common in the kernel, the parent
node can use of_platform_populate or similar
(git grep of_platform_populate, you will see plenty of examples).

You will still have to do some work to have the proper
regs resources, but this is doable. Each child is a device,
so it can have its own resources (clocks, interrupts, iommus).

You don't need any async framework.

vcodec_dec: vcodec_dec@1600 {
compatible = "mediatek,mt8192-vcodec-dec";
reg = ;
mediatek,scp = <>;
iommus = < M4U_PORT_L4_VDEC_MC_EXT>;
dma-ranges = <0x1 0x0 0x0 0x4000 0x0 0xfff0>;

vcodec_lat@0x1 {
compatible = "mediatek,mtk-vcodec-lat";
reg = <0x1 0x800>;  /* VDEC_MISC */
interrupts = ;
// etc
};

vcodec_core@0x25000 {
   compatible = "mediatek,mtk-vcodec-core";
   reg = <0x25000 0x1000>;  /* VDEC_CORE_MISC */
   interrupts = ;
   // etc
};
};

Thanks,
Ezequiel

[PATCH 1/2] drm/msm/dsi: Add phy configuration for MSM8953

2021-09-02 Thread Sireesh Kodali

From: Vladimir Lypak 

Add phy configuration for 14nm dsi phy found on MSM8953 SoC. Only
difference from existing configurations are io_start addresses.

Signed-off-by: Vladimir Lypak 
Signed-off-by: Sireesh Kodali 
---
 .../bindings/display/msm/dsi-phy-14nm.yaml|  1 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c |  2 ++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h |  1 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c| 21 +++
 4 files changed, 25 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml 
b/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml
index 72a00cce0147..7527fb299caa 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml
@@ -17,6 +17,7 @@ properties:
 oneOf:
   - const: qcom,dsi-phy-14nm
   - const: qcom,dsi-phy-14nm-660
+  - const: qcom,dsi-phy-14nm-8953
 
   reg:
 items:
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 8c65ef6968ca..9842e04b5858 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -627,6 +627,8 @@ static const struct of_device_id dsi_phy_dt_match[] = {
  .data = _phy_14nm_cfgs },
{ .compatible = "qcom,dsi-phy-14nm-660",
  .data = _phy_14nm_660_cfgs },
+   { .compatible = "qcom,dsi-phy-14nm-8953",
+ .data = _phy_14nm_8953_cfgs },
 #endif
 #ifdef CONFIG_DRM_MSM_DSI_10NM_PHY
{ .compatible = "qcom,dsi-phy-10nm",
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
index b91303ada74f..4c8257581bfc 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
@@ -48,6 +48,7 @@ extern const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs;
 extern const struct msm_dsi_phy_cfg dsi_phy_28nm_8960_cfgs;
 extern const struct msm_dsi_phy_cfg dsi_phy_14nm_cfgs;
 extern const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs;
+extern const struct msm_dsi_phy_cfg dsi_phy_14nm_8953_cfgs;
 extern const struct msm_dsi_phy_cfg dsi_phy_10nm_cfgs;
 extern const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs;
 extern const struct msm_dsi_phy_cfg dsi_phy_7nm_cfgs;
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
index d13552b2213b..9a6b1f0cbbaf 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
@@ -1065,3 +1065,24 @@ const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs = {
.io_start = { 0xc994400, 0xc996000 },
.num_dsi_phy = 2,
 };
+
+const struct msm_dsi_phy_cfg dsi_phy_14nm_8953_cfgs = {
+   .has_phy_lane = true,
+   .reg_cfg = {
+   .num = 1,
+   .regs = {
+   {"vcca", 17000, 32},
+   },
+   },
+   .ops = {
+   .enable = dsi_14nm_phy_enable,
+   .disable = dsi_14nm_phy_disable,
+   .pll_init = dsi_pll_14nm_init,
+   .save_pll_state = dsi_14nm_pll_save_state,
+   .restore_pll_state = dsi_14nm_pll_restore_state,
+   },
+   .min_pll_rate = VCO_MIN_RATE,
+   .max_pll_rate = VCO_MAX_RATE,
+   .io_start = { 0x1a94400, 0x1a96400 },
+   .num_dsi_phy = 2,
+};
-- 
2.33.0

[PATCH 0/2] MSM8953 MDP/DSI PHY enablement

2021-09-02 Thread Sireesh Kodali

This patch series adds support for the MDP and DSI PHY as found on the
MSM8953 platform (APQ8053, SDM450, SDA450, SDM625, SDM626). All the SoCs
on this platform use the adreno 506 GPU.

Vladimir Lypak (2):
  drm/msm/dsi: Add phy configuration for MSM8953
  drm/msm/mdp5: Add configuration for MDP v1.16

 .../bindings/display/msm/dsi-phy-14nm.yaml|  1 +
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c  | 89 +++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c |  2 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h |  1 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c| 21 +
 5 files changed, 114 insertions(+)

-- 
2.33.0

[PATCH 2/2] drm/msm/mdp5: Add configuration for MDP v1.16

2021-09-02 Thread Sireesh Kodali

From: Vladimir Lypak 

MDP version v1.16 is almost identical to v1.15 with most significant
difference being presence of second DSI interface. MDP v1.16 is found on
SoCs such as MSM8x53, SDM450, SDM632 (All with Adreno 506).

Signed-off-by: Vladimir Lypak 
Signed-off-by: Sireesh Kodali 
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 89 
 1 file changed, 89 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c 
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
index 9741544ffc35..0d28c8ff4009 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
@@ -752,6 +752,94 @@ const struct mdp5_cfg_hw msm8x76_config = {
.max_clk = 36000,
 };
 
+static const struct mdp5_cfg_hw msm8x53_config = {
+   .name = "msm8x53",
+   .mdp = {
+   .count = 1,
+   .caps = MDP_CAP_CDM |
+   MDP_CAP_SRC_SPLIT,
+   },
+   .ctl = {
+   .count = 3,
+   .base = { 0x01000, 0x01200, 0x01400 },
+   .flush_hw_mask = 0x,
+   },
+   .pipe_vig = {
+   .count = 1,
+   .base = { 0x04000 },
+   .caps = MDP_PIPE_CAP_HFLIP  |
+   MDP_PIPE_CAP_VFLIP  |
+   MDP_PIPE_CAP_SCALE  |
+   MDP_PIPE_CAP_CSC|
+   MDP_PIPE_CAP_DECIMATION |
+   MDP_PIPE_CAP_SW_PIX_EXT |
+   0,
+   },
+   .pipe_rgb = {
+   .count = 2,
+   .base = { 0x14000, 0x16000 },
+   .caps = MDP_PIPE_CAP_HFLIP  |
+   MDP_PIPE_CAP_VFLIP  |
+   MDP_PIPE_CAP_DECIMATION |
+   MDP_PIPE_CAP_SW_PIX_EXT |
+   0,
+   },
+   .pipe_dma = {
+   .count = 1,
+   .base = { 0x24000 },
+   .caps = MDP_PIPE_CAP_HFLIP  |
+   MDP_PIPE_CAP_VFLIP  |
+   MDP_PIPE_CAP_SW_PIX_EXT |
+   0,
+   },
+   .pipe_cursor = {
+   .count = 1,
+   .base = { 0x34000 },
+   .caps = MDP_PIPE_CAP_HFLIP  |
+   MDP_PIPE_CAP_VFLIP  |
+   MDP_PIPE_CAP_SW_PIX_EXT |
+   MDP_PIPE_CAP_CURSOR |
+   0,
+   },
+
+   .lm = {
+   .count = 3,
+   .base = { 0x44000, 0x45000 },
+   .instances = {
+   { .id = 0, .pp = 0, .dspp = 0,
+ .caps = MDP_LM_CAP_DISPLAY |
+ MDP_LM_CAP_PAIR },
+   { .id = 1, .pp = 1, .dspp = -1,
+ .caps = MDP_LM_CAP_DISPLAY },
+},
+   .nb_stages = 5,
+   .max_width = 2048,
+   .max_height = 0x,
+   },
+   .dspp = {
+   .count = 1,
+   .base = { 0x54000 },
+
+   },
+   .pp = {
+   .count = 2,
+   .base = { 0x7, 0x70800 },
+   },
+   .cdm = {
+   .count = 1,
+   .base = { 0x79200 },
+   },
+   .intf = {
+   .base = { 0x6a000, 0x6a800, 0x6b000 },
+   .connect = {
+   [0] = INTF_DISABLED,
+   [1] = INTF_DSI,
+   [2] = INTF_DSI,
+   },
+   },
+   .max_clk = 4,
+};
+
 static const struct mdp5_cfg_hw msm8917_config = {
.name = "msm8917",
.mdp = {
@@ -1151,6 +1239,7 @@ static const struct mdp5_cfg_handler cfg_handlers_v1[] = {
{ .revision = 7, .config = { .hw = _config } },
{ .revision = 11, .config = { .hw = _config } },
{ .revision = 15, .config = { .hw = _config } },
+   { .revision = 16, .config = { .hw = _config } },
 };
 
 static const struct mdp5_cfg_handler cfg_handlers_v3[] = {
-- 
2.33.0

Re: [Intel-gfx] [PATCH 01/11] drm/i915: Release i915_gem_context from a worker




On 02/09/2021 16:05, Daniel Vetter wrote:

On Thu, Sep 2, 2021 at 2:42 PM Tvrtko Ursulin
 wrote:



On 13/08/2021 21:30, Daniel Vetter wrote:

The only reason for this really is the i915_gem_engines->fence
callback engines_notify(), which exists purely as a fairly funky
reference counting scheme for that. Otherwise all other callers are
from process context, and generally fairly benign locking context.


There is reset which definitely isn't process context.


gpu reset runs in process context. The tasklet context is the
engines_notify I'm talking about above.


I haven't looked very deeply but please double check the path from 
execlists_submission_tasklet -> execlists_reset -> intel_engine_reset -> 
__intel_engine_reset -> execlists_reset_rewind -> execlists_reset_csb -> 
execlists_reset_active -> __i915_request_reset -> mark_guilty -> 
i915_gem_context_put.



Otherwise I did not really get from the commit message is this patch
fixing an existing problem or preparing something for the future. If the
former then as I wrote above - I am pretty sure there are call sites
from the tasklet already.

Regards,

Tvrtko


Unfortunately untangling that requires some major surgery, and we have
a few i915_gem_context reference counting bugs that need fixing, and
they blow in the current hardirq calling context, so we need a
stop-gap measure.


I guess this para wasn't clear, but subsequent patches fix the
refcount bugs and need this prep patch here.


So up to where in the series are those fixes and where other stuff 
follows? Worth spliting and having cover letters perhaps? Is the fixing 
part applicable to the existing code or only comes to play with the 
syncobj single timeline changes?


Regards,

Tvrtko


-Daniel



Put a FIXME comment in when this should be removable again.

Signed-off-by: Daniel Vetter 
Cc: Jon Bloomfield 
Cc: Chris Wilson 
Cc: Maarten Lankhorst 
Cc: Joonas Lahtinen 
Cc: Daniel Vetter 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Cc: Lionel Landwerlin 
Cc: Dave Airlie 
Cc: Jason Ekstrand 
---
   drivers/gpu/drm/i915/gem/i915_gem_context.c   | 13 +++--
   drivers/gpu/drm/i915/gem/i915_gem_context_types.h | 12 
   2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index fd169cf2f75a..051bc357ff65 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -986,9 +986,10 @@ static struct i915_gem_engines *user_engines(struct 
i915_gem_context *ctx,
   return err;
   }

-void i915_gem_context_release(struct kref *ref)
+static void i915_gem_context_release_work(struct work_struct *work)
   {
- struct i915_gem_context *ctx = container_of(ref, typeof(*ctx), ref);
+ struct i915_gem_context *ctx = container_of(work, typeof(*ctx),
+ release_work);

   trace_i915_context_free(ctx);
   GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
@@ -1002,6 +1003,13 @@ void i915_gem_context_release(struct kref *ref)
   kfree_rcu(ctx, rcu);
   }

+void i915_gem_context_release(struct kref *ref)
+{
+ struct i915_gem_context *ctx = container_of(ref, typeof(*ctx), ref);
+
+ queue_work(ctx->i915->wq, >release_work);
+}
+
   static inline struct i915_gem_engines *
   __context_engines_static(const struct i915_gem_context *ctx)
   {
@@ -1303,6 +1311,7 @@ i915_gem_create_context(struct drm_i915_private *i915,
   ctx->sched = pc->sched;
   mutex_init(>mutex);
   INIT_LIST_HEAD(>link);
+ INIT_WORK(>release_work, i915_gem_context_release_work);

   spin_lock_init(>stale.lock);
   INIT_LIST_HEAD(>stale.engines);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h 
b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
index 94c03a97cb77..0c38789bd4a8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
@@ -288,6 +288,18 @@ struct i915_gem_context {
*/
   struct kref ref;

+ /**
+  * @release_work:
+  *
+  * Work item for deferred cleanup, since i915_gem_context_put() tends to
+  * be called from hardirq context.
+  *
+  * FIXME: The only real reason for this is _gem_engines.fence, all
+  * other callers are from process context and need at most some mild
+  * shuffling to pull the i915_gem_context_put() call out of a spinlock.
+  */
+ struct work_struct release_work;
+
   /**
* @rcu: rcu_head for deferred freeing.
*/

Re: [diagnostic TDR mode patches] unify our solution opinions/suggestions in one thread

On Thu, Sep 2, 2021 at 1:00 PM Christian König  wrote:
>
> Hi Monk,
>
> Am 02.09.21 um 07:52 schrieb Liu, Monk:
> > [AMD Official Use Only]
> >
> > I'm not sure I can add much to help this along, I'm sure Alex has some 
> > internal training,
> > Once your driver is upstream, it belongs to upstream, you can maintain it, 
> > but you no longer control it 100%, it's a tradeoff, it's not one companies 
> > always understand.
> > Usually people are fine developing away internally, but once interaction 
> > with other parts of the kernel/subsystem is required they have the 
> > realisation that they needed to work upstream 6 months earlier.
> > The best time to interact with upstream was 6 months ago, the second best 
> > time is now.
> > <<<
> >
> > Daniel/AlexD
> >
> > I didn't mean your changes on AMD driver need my personal approval or 
> > review ... and  I'm totally already get used that our driver is not 100% 
> > under control by AMDers,
> > but supposedly any one from community (including you) who tend to change 
> > AMD's driver need at least to get approvement from someone in AMD, e.g.: 
> > AlexD or Christian, doesn't that reasonable?
>
> I'm fearing that just repeating what Alex said, but to make it clear
> once more: That is *not* necessary!
>
> The shared repository is owned by upstream maintainers and they are
> usually free to do restructuring work without getting acknowledge from
> every single driver maintainer.
>
> Anybody can of course technically object to upstream design decisions,
> but that means that you need to pay attention to the mailing lists in
> the first place.
>
> > just like we need your approve if we try to modify DRM-sched, or need 
> > panfrost's approval if we need to change panfrost code ...
> >
> > by only CC AMD's engineers looks not quite properly, how do you know if 
> > your changes (on AMD code part) are conflicting with AMD's on-going 
> > internal features/refactoring or not ?
>
> Well because AMD is supposed to work in public as much as possible and
> ask upstream before doing changes to the code base.
>
> Additional to that design decisions are supposed to be discussed on the
> mailing list and *not* internally.

Yeah I'm honestly really surprised about the course of this discussion
here. With Alex, Christian and others amd has a lot of folks with
years/decades of experience in how to collaborate in upstream, when to
pull in others proactively and when that's not needed, and in general
how to plan upstream work with the lest amount of risk and surprises.

I think step zero here needs to be some training at amd and then
re-planning this effort, before we get back to technical stuff.
Otherwise we'll just get bogged down in pain because expectations
about the process don't pan out.
-Daniel

>
> Regards,
> Christian.
>
> >
> > Thanks
> >
> > --
> > Monk Liu | Cloud-GPU Core team
> > --
> >
> > -Original Message-
> > From: Dave Airlie 
> > Sent: Thursday, September 2, 2021 2:51 AM
> > To: Alex Deucher 
> > Cc: Liu, Monk ; Daniel Vetter ; Koenig, 
> > Christian ; Grodzovsky, Andrey 
> > ; Chen, JingWen ; DRI 
> > Development ; amd-...@lists.freedesktop.org
> > Subject: Re: [diagnostic TDR mode patches] unify our solution 
> > opinions/suggestions in one thread
> >
> > On Thu, 2 Sept 2021 at 01:20, Alex Deucher  wrote:
> >> On Wed, Sep 1, 2021 at 6:19 AM Liu, Monk  wrote:
> >>> [AMD Official Use Only]
> >>>
> >>> Daniel
> >>>
> >>>  From the link you share it looks you(or someone else) have quite a bunch 
> >>> patches that changes DRM_SCHED or even amdgpu, by that case before they 
> >>> are merged to kernel tree I'm wondering if any AMD develop reviewed them ?
> >>>
> >>> They looks to me somehow conflicting with what we changed in our repo
> >>>
> >>> It is really a chaos for AMDer if someone else out side of AMD changes 
> >>> our kernel driver (or/and scheduler) without reviewed by AMDer, just like 
> >>> we are requiring your review if we tend to change scheduler's logic here 
> >>> 
> >>>
> >>> This one changes AMD's code:
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
> >>> re.kernel.org%2Fdri-devel%2F20210625133327.2598825-2-boris.brezillon
> >>> %40collabora.com%2Fdata=04%7C01%7CMonk.Liu%40amd.com%7C6c507d18
> >>> d65341ef53bb08d96d7976e6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%
> >>> 7C637661190727875969%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> >>> QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=BWJSkKN
> >>> y2%2BwjxbQrfxGPzuJ5PBpBwB4aV0ZH6QoJGEg%3Dreserved=0
> >>> And I didn't see any reviewed-by from AMDers ...
> >>>
> >>> This one also touches AMD's code:
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
> >>> re.kernel.org%2Fdri-devel%2F20200604081224.863494-12-daniel.vetter%4
> >>> 0ffwll.ch%2Fdata=04%7C01%7CMonk.Liu%40amd.com%7C6c507d18d65341e
> >>>

[Bug 214289] amdgpu Msg issuing pre-check failed and SMU may be not in the right state!

https://bugzilla.kernel.org/show_bug.cgi?id=214289

Alex Deucher (alexdeuc...@gmail.com) changed:

   What|Removed |Added

 CC||alexdeuc...@gmail.com

--- Comment #1 from Alex Deucher (alexdeuc...@gmail.com) ---
Please include your full dmesg output.  Does the behavior change if you remove
all of the extra amdgpu module parameters you are applying?

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."

2021-09-02 Thread Andrey Grodzovsky




On 2021-09-02 10:28 a.m., Daniel Vetter wrote:

On Tue, Aug 31, 2021 at 02:24:52PM -0400, Andrey Grodzovsky wrote:

On 2021-08-31 9:11 a.m., Daniel Vetter wrote:

On Thu, Aug 26, 2021 at 11:04:14AM +0200, Daniel Vetter wrote:

On Thu, Aug 19, 2021 at 11:25:09AM -0400, Andrey Grodzovsky wrote:

On 2021-08-19 5:30 a.m., Daniel Vetter wrote:

On Wed, Aug 18, 2021 at 10:51:00AM -0400, Andrey Grodzovsky wrote:

On 2021-08-18 10:42 a.m., Daniel Vetter wrote:

On Wed, Aug 18, 2021 at 10:36:32AM -0400, Andrey Grodzovsky wrote:

On 2021-08-18 10:32 a.m., Daniel Vetter wrote:

On Wed, Aug 18, 2021 at 10:26:25AM -0400, Andrey Grodzovsky wrote:

On 2021-08-18 10:02 a.m., Alex Deucher wrote:


+ dri-devel

Since scheduler is a shared component, please add dri-devel on all
scheduler patches.

On Wed, Aug 18, 2021 at 7:21 AM Jingwen Chen  wrote:

[Why]
for bailing job, this commit will delete it from pending list thus the
bailing job will never have a chance to be resubmitted even in advance
tdr mode.

[How]
after embeded hw_fence into amdgpu_job is done, the race condition that
this commit tries to work around is completely solved.So revert this
commit.
This reverts commit 135517d3565b48f4def3b1b82008bc17eb5d1c90.
v2:
add dma_fence_get/put() around timedout_job to avoid concurrent delete
during processing timedout_job

Signed-off-by: Jingwen Chen 
---
   drivers/gpu/drm/scheduler/sched_main.c | 23 +--
   1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index a2a953693b45..f9b9b3aefc4a 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -314,6 +314,7 @@ static void drm_sched_job_timedout(struct work_struct *work)
   {
  struct drm_gpu_scheduler *sched;
  struct drm_sched_job *job;
+   struct dma_fence *fence;
  enum drm_gpu_sched_stat status = DRM_GPU_SCHED_STAT_NOMINAL;

  sched = container_of(work, struct drm_gpu_scheduler, 
work_tdr.work);
@@ -325,11 +326,10 @@ static void drm_sched_job_timedout(struct work_struct 
*work)

  if (job) {
  /*
-* Remove the bad job so it cannot be freed by concurrent
-* drm_sched_cleanup_jobs. It will be reinserted back after 
sched->thread
-* is parked at which point it's safe.
+* Get job->s_fence->parent here to avoid concurrent delete 
during
+* processing timedout_job
   */
-   list_del_init(>list);
+   fence = dma_fence_get(job->s_fence->parent);

While this is true for amdgpu, it has no meaning for other drivers for whom
we haven't
done the refactoring of embedding HW fence (parent) into the job structure.
In fact thinking
about it, unless you do the HW fence embedding for all the drivers using the
scheduler you cannot
revert this patch or you will just break them.

btw, why did you do that embedding? I do still have my patches with
dma_fence annotations floating around, but my idea at least was to fix
that issue with a mempool, not with embeddeding. What was the motivation
for embedding the wh fence?
-Daniel

The motivation was 2 fold, avoid memory allocation during jobs submissions
(HW fence allocation) because as Christian explained this leads to deadlock
with
mm code during evictions due to memory pressure (Christian can clarify if I
messed

Yeah that's the exact same thing I've chased with my dma_fence
annotations, but thus far zero to none interested in getting it sorted. I
think it'd be good to have some cross-driver agreement on how this should
be solved before someone just charges ahead ...


this explanation). Second is to exactly revert this patch because while it
solved the issue
described in the patch it created another with drivers who baildc out early
during TDR handling
for various reason and the job would just leak because it was already
removed form pending list.

Can't we reinsert it before we restart the scheduler thread? It might need
a separate list for that due to the lockless queue tricks. Or am I
thinking about the wrong kind of "we lost the job"?
-Danile

If you look at the original patch it would reinsert it even earlier - right
after stopping the  SW scheduler thread, and even then it was to late for
some drivers as they would decide to return back from their TDR handler even
before that. It is solvable but in an ugly way as far as I see, you need to
require each driver in his code to put the job back in the list if they do
it before reaching the place where scheduler framework does it. Kind of
spaghetti code seems to me.

Hm yeah I didn't realize this all happens before we stop the scheduler
thread.

Why can't we stop the scheduler thread first, so that there's guaranteed
no race? I've recently had a lot of discussions with panfrost folks about
their reset that spawns

[PATCH] drm/stm: ltdc: attach immutable zpos property to planes

2021-09-02 Thread Raphael GALLAIS-POU - foss

Defines plane ordering by hard-coding an immutable Z position from the
first plane, used as primary layer, to the next ones as overlay in order
of instantiation.

This zpos is only an information as it is not possible to modify it,
blending operations are still applied from the top to the bottom layer.

This patch helps to remove a warning message from the Android
Hardware Composer.

Signed-off-by: Raphael Gallais-Pou 
---
 drivers/gpu/drm/stm/ltdc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 195de30eb90c..bd603ef5e935 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -1024,6 +1024,8 @@ static int ltdc_crtc_init(struct drm_device *ddev, struct 
drm_crtc *crtc)
return -EINVAL;
}
 
+   drm_plane_create_zpos_immutable_property(primary, 0);
+
ret = drm_crtc_init_with_planes(ddev, crtc, primary, NULL,
_crtc_funcs, NULL);
if (ret) {
@@ -1046,6 +1048,7 @@ static int ltdc_crtc_init(struct drm_device *ddev, struct 
drm_crtc *crtc)
DRM_ERROR("Can not create overlay plane %d\n", i);
goto cleanup;
}
+   drm_plane_create_zpos_immutable_property(overlay, i);
}
 
return 0;
-- 
2.17.1

Re: [Intel-gfx] [PATCH 07/11] drm/i915: Add i915_gem_context_is_full_ppgtt

On Thu, Sep 02, 2021 at 03:54:36PM +0100, Tvrtko Ursulin wrote:
> On 02/09/2021 15:20, Daniel Vetter wrote:
> > And use it anywhere we have open-coded checks for ctx->vm that really
> > only check for full ppgtt.
> > 
> > Plus for paranoia add a GEM_BUG_ON that checks it's really only set
> > when we have full ppgtt, just in case. gem_context->vm is different
> > since it's NULL in ggtt mode, unlike intel_context->vm or gt->vm,
> > which is always set.
> > 
> > v2: 0day found a testcase that I missed.
> > 
> > Reviewed-by: Maarten Lankhorst 
> > Signed-off-by: Daniel Vetter 
> > Cc: Jon Bloomfield 
> > Cc: Chris Wilson 
> > Cc: Maarten Lankhorst 
> > Cc: Joonas Lahtinen 
> > Cc: Daniel Vetter 
> > Cc: "Thomas Hellström" 
> > Cc: Matthew Auld 
> > Cc: Lionel Landwerlin 
> > Cc: Dave Airlie 
> > Cc: Jason Ekstrand 
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_context.c   | 2 +-
> >   drivers/gpu/drm/i915/gem/i915_gem_context.h   | 7 +++
> >   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +-
> >   drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 6 +++---
> >   4 files changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
> > b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index 7a566fb7cca4..1eec85944c1f 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -1566,7 +1566,7 @@ static int get_ppgtt(struct drm_i915_file_private 
> > *file_priv,
> > int err;
> > u32 id;
> > -   if (!rcu_access_pointer(ctx->vm))
> > +   if (!i915_gem_context_is_full_ppgtt(ctx))
> 
> It reads a bit wrong because GEM context cannot *be* full ppggt. It can be
> associated with a VM which is or isn't full ppgtt. So a test on a VM
> retrieved from a context is semnntically more correct. Perhaps you want to
> consider adding a helper to that effect instead? It could mean splitting
> into two helpers (getter + test) or maybe just renaming would work. Like
> i915_gem_context_has_full_ppgtt_vm(ctx)?

The pointer isn't set when the driver/context isn't running in full ppgtt
mode. This is why I've added the GEM_BUG_ON to check we're not breaking
any invariants. So yeah it is a full ppgtt context or it's not, that is
indeed the question here.

I'm happy to bikeshed the naming, but I don't see how your suggestion is
an improvement.
-Daniel


> 
> Regards,
> 
> Tvrtko
> 
> > return -ENODEV;
> > rcu_read_lock();
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h 
> > b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> > index da6e8b506d96..37536a260e6e 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> > @@ -154,6 +154,13 @@ i915_gem_context_vm(struct i915_gem_context *ctx)
> > return rcu_dereference_protected(ctx->vm, lockdep_is_held(>mutex));
> >   }
> > +static inline bool i915_gem_context_is_full_ppgtt(struct i915_gem_context 
> > *ctx)
> > +{
> > +   GEM_BUG_ON(!!rcu_access_pointer(ctx->vm) != HAS_FULL_PPGTT(ctx->i915));
> > +
> > +   return !!rcu_access_pointer(ctx->vm);
> > +}
> > +
> >   static inline struct i915_address_space *
> >   i915_gem_context_get_eb_vm(struct i915_gem_context *ctx)
> >   {
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c 
> > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > index 905b1cbd22d5..40f08948f0b2 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > @@ -749,7 +749,7 @@ static int eb_select_context(struct i915_execbuffer *eb)
> > return PTR_ERR(ctx);
> > eb->gem_context = ctx;
> > -   if (rcu_access_pointer(ctx->vm))
> > +   if (i915_gem_context_is_full_ppgtt(ctx))
> > eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
> > return 0;
> > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c 
> > b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> > index fc7fb33a3a52..947154e445a7 100644
> > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> > @@ -704,7 +704,7 @@ static int igt_ctx_exec(void *arg)
> > pr_err("Failed to fill dword %lu [%lu/%lu] with 
> > gpu (%s) [full-ppgtt? %s], err=%d\n",
> >ndwords, dw, max_dwords(obj),
> >engine->name,
> > -  yesno(!!rcu_access_pointer(ctx->vm)),
> > +  
> > yesno(i915_gem_context_is_full_ppgtt(ctx)),
> >err);
> > intel_context_put(ce);
> > kernel_context_close(ctx);
> > @@ -838,7 +838,7 @@ static int igt_shared_ctx_exec(void *arg)
> > pr_err("Failed to fill dword %lu [%lu/%lu] with 
> > gpu (%s) [full-ppgtt? %s],

Re: [Intel-gfx] [PATCH 01/11] drm/i915: Release i915_gem_context from a worker

On Thu, Sep 2, 2021 at 2:42 PM Tvrtko Ursulin
 wrote:
>
>
> On 13/08/2021 21:30, Daniel Vetter wrote:
> > The only reason for this really is the i915_gem_engines->fence
> > callback engines_notify(), which exists purely as a fairly funky
> > reference counting scheme for that. Otherwise all other callers are
> > from process context, and generally fairly benign locking context.
>
> There is reset which definitely isn't process context.

gpu reset runs in process context. The tasklet context is the
engines_notify I'm talking about above.

> Otherwise I did not really get from the commit message is this patch
> fixing an existing problem or preparing something for the future. If the
> former then as I wrote above - I am pretty sure there are call sites
> from the tasklet already.
>
> Regards,
>
> Tvrtko
>
> > Unfortunately untangling that requires some major surgery, and we have
> > a few i915_gem_context reference counting bugs that need fixing, and
> > they blow in the current hardirq calling context, so we need a
> > stop-gap measure.

I guess this para wasn't clear, but subsequent patches fix the
refcount bugs and need this prep patch here.
-Daniel

> >
> > Put a FIXME comment in when this should be removable again.
> >
> > Signed-off-by: Daniel Vetter 
> > Cc: Jon Bloomfield 
> > Cc: Chris Wilson 
> > Cc: Maarten Lankhorst 
> > Cc: Joonas Lahtinen 
> > Cc: Daniel Vetter 
> > Cc: "Thomas Hellström" 
> > Cc: Matthew Auld 
> > Cc: Lionel Landwerlin 
> > Cc: Dave Airlie 
> > Cc: Jason Ekstrand 
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_context.c   | 13 +++--
> >   drivers/gpu/drm/i915/gem/i915_gem_context_types.h | 12 
> >   2 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
> > b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index fd169cf2f75a..051bc357ff65 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -986,9 +986,10 @@ static struct i915_gem_engines *user_engines(struct 
> > i915_gem_context *ctx,
> >   return err;
> >   }
> >
> > -void i915_gem_context_release(struct kref *ref)
> > +static void i915_gem_context_release_work(struct work_struct *work)
> >   {
> > - struct i915_gem_context *ctx = container_of(ref, typeof(*ctx), ref);
> > + struct i915_gem_context *ctx = container_of(work, typeof(*ctx),
> > + release_work);
> >
> >   trace_i915_context_free(ctx);
> >   GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
> > @@ -1002,6 +1003,13 @@ void i915_gem_context_release(struct kref *ref)
> >   kfree_rcu(ctx, rcu);
> >   }
> >
> > +void i915_gem_context_release(struct kref *ref)
> > +{
> > + struct i915_gem_context *ctx = container_of(ref, typeof(*ctx), ref);
> > +
> > + queue_work(ctx->i915->wq, >release_work);
> > +}
> > +
> >   static inline struct i915_gem_engines *
> >   __context_engines_static(const struct i915_gem_context *ctx)
> >   {
> > @@ -1303,6 +1311,7 @@ i915_gem_create_context(struct drm_i915_private *i915,
> >   ctx->sched = pc->sched;
> >   mutex_init(>mutex);
> >   INIT_LIST_HEAD(>link);
> > + INIT_WORK(>release_work, i915_gem_context_release_work);
> >
> >   spin_lock_init(>stale.lock);
> >   INIT_LIST_HEAD(>stale.engines);
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h 
> > b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> > index 94c03a97cb77..0c38789bd4a8 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> > @@ -288,6 +288,18 @@ struct i915_gem_context {
> >*/
> >   struct kref ref;
> >
> > + /**
> > +  * @release_work:
> > +  *
> > +  * Work item for deferred cleanup, since i915_gem_context_put() tends 
> > to
> > +  * be called from hardirq context.
> > +  *
> > +  * FIXME: The only real reason for this is _gem_engines.fence, 
> > all
> > +  * other callers are from process context and need at most some mild
> > +  * shuffling to pull the i915_gem_context_put() call out of a 
> > spinlock.
> > +  */
> > + struct work_struct release_work;
> > +
> >   /**
> >* @rcu: rcu_head for deferred freeing.
> >*/
> >



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH] drm/panel: otm8009a: add a 60 fps mode

2021-09-02 Thread Raphael GALLAIS-POU - foss

This patch adds a 60 fps mode to the Orisetech OTM8009A panel.
The 50 fps mode is left as preferred.

Signed-off-by: Raphael Gallais-Pou 
---
 .../gpu/drm/panel/panel-orisetech-otm8009a.c  | 85 ---
 1 file changed, 56 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c 
b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
index f80b44a8a700..dfb43b1374e7 100644
--- a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
+++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
@@ -60,6 +60,9 @@
 #define MCS_CMD2_ENA1  0xFF00  /* Enable Access Command2 "CMD2" */
 #define MCS_CMD2_ENA2  0xFF80  /* Enable Access Orise Command2 */
 
+#define OTM8009A_HDISPLAY  480
+#define OTM8009A_VDISPLAY  800
+
 struct otm8009a {
struct device *dev;
struct drm_panel panel;
@@ -70,19 +73,35 @@ struct otm8009a {
bool enabled;
 };
 
-static const struct drm_display_mode default_mode = {
-   .clock = 29700,
-   .hdisplay = 480,
-   .hsync_start = 480 + 98,
-   .hsync_end = 480 + 98 + 32,
-   .htotal = 480 + 98 + 32 + 98,
-   .vdisplay = 800,
-   .vsync_start = 800 + 15,
-   .vsync_end = 800 + 15 + 10,
-   .vtotal = 800 + 15 + 10 + 14,
-   .flags = 0,
-   .width_mm = 52,
-   .height_mm = 86,
+static const struct drm_display_mode modes[] = {
+   { /* 50 Hz, preferred */
+   .clock = 29700,
+   .hdisplay = 480,
+   .hsync_start = 480 + 98,
+   .hsync_end = 480 + 98 + 32,
+   .htotal = 480 + 98 + 32 + 98,
+   .vdisplay = 800,
+   .vsync_start = 800 + 15,
+   .vsync_end = 800 + 15 + 10,
+   .vtotal = 800 + 15 + 10 + 14,
+   .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+   .width_mm = 52,
+   .height_mm = 86,
+   },
+   { /* 60 Hz */
+   .clock = 33000,
+   .hdisplay = 480,
+   .hsync_start = 480 + 70,
+   .hsync_end = 480 + 70 + 32,
+   .htotal = 480 + 70 + 32 + 72,
+   .vdisplay = 800,
+   .vsync_start = 800 + 15,
+   .vsync_end = 800 + 15 + 10,
+   .vtotal = 800 + 15 + 10 + 16,
+   .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+   .width_mm = 52,
+   .height_mm = 86,
+   },
 };
 
 static inline struct otm8009a *panel_to_otm8009a(struct drm_panel *panel)
@@ -208,12 +227,11 @@ static int otm8009a_init_sequence(struct otm8009a *ctx)
/* Default portrait 480x800 rgb24 */
dcs_write_seq(ctx, MIPI_DCS_SET_ADDRESS_MODE, 0x00);
 
-   ret = mipi_dsi_dcs_set_column_address(dsi, 0,
- default_mode.hdisplay - 1);
+   ret = mipi_dsi_dcs_set_column_address(dsi, 0, OTM8009A_HDISPLAY - 1);
if (ret)
return ret;
 
-   ret = mipi_dsi_dcs_set_page_address(dsi, 0, default_mode.vdisplay - 1);
+   ret = mipi_dsi_dcs_set_page_address(dsi, 0, OTM8009A_VDISPLAY - 1);
if (ret)
return ret;
 
@@ -337,24 +355,33 @@ static int otm8009a_get_modes(struct drm_panel *panel,
  struct drm_connector *connector)
 {
struct drm_display_mode *mode;
-
-   mode = drm_mode_duplicate(connector->dev, _mode);
-   if (!mode) {
-   dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
-   default_mode.hdisplay, default_mode.vdisplay,
-   drm_mode_vrefresh(_mode));
-   return -ENOMEM;
+   unsigned int num_modes = ARRAY_SIZE(modes);
+   unsigned int i;
+
+   for (i = 0; i < num_modes; i++) {
+   mode = drm_mode_duplicate(connector->dev, [i]);
+   if (!mode) {
+   dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
+   modes[i].hdisplay,
+   modes[i].vdisplay,
+   drm_mode_vrefresh([i]));
+   return -ENOMEM;
+   }
+
+   mode->type = DRM_MODE_TYPE_DRIVER;
+
+   /* Setting first mode as preferred */
+   if (!i)
+   mode->type |=  DRM_MODE_TYPE_PREFERRED;
+
+   drm_mode_set_name(mode);
+   drm_mode_probed_add(connector, mode);
}
 
-   drm_mode_set_name(mode);
-
-   mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
-   drm_mode_probed_add(connector, mode);
-
connector->display_info.width_mm = mode->width_mm;
connector->display_info.height_mm = mode->height_mm;
 
-   return 1;
+   return num_modes;
 }
 
 static const struct drm_panel_funcs otm8009a_drm_funcs = {
-- 
2.17.1

Re: [Intel-gfx] [PATCH v2] drm/i915: Handle Intel igfx + Intel dgfx hybrid graphics setup




On 02/09/2021 15:33, Daniel Vetter wrote:

On Tue, Aug 31, 2021 at 02:18:15PM +0100, Tvrtko Ursulin wrote:


On 31/08/2021 13:43, Daniel Vetter wrote:

On Tue, Aug 31, 2021 at 10:15:03AM +0100, Tvrtko Ursulin wrote:


On 30/08/2021 09:26, Daniel Vetter wrote:

On Fri, Aug 27, 2021 at 03:44:42PM +0100, Tvrtko Ursulin wrote:


On 27/08/2021 15:39, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin 

In short this makes i915 work for hybrid setups (DRI_PRIME=1 with Mesa)
when rendering is done on Intel dgfx and scanout/composition on Intel
igfx.

Before this patch the driver was not quite ready for that setup, mainly
because it was able to emit a semaphore wait between the two GPUs, which
results in deadlocks because semaphore target location in HWSP is neither
shared between the two, nor mapped in both GGTT spaces.

To fix it the patch adds an additional check to a couple of relevant code
paths in order to prevent using semaphores for inter-engine
synchronisation between different driver instances.

Patch also moves singly used i915_gem_object_last_write_engine to be
private in its only calling unit (debugfs), while modifying it to only
show activity belonging to the respective driver instance.

What remains in this problem space is the question of the GEM busy ioctl.
We have a somewhat ambigous comment there saying only status of native
fences will be reported, which could be interpreted as either i915, or
native to the drm fd. For now I have decided to leave that as is, meaning
any i915 instance activity continues to be reported.

v2:
 * Avoid adding rq->i915. (Chris)

Signed-off-by: Tvrtko Ursulin 


Can't we just delete semaphore code and done?
- GuC won't have it
- media team benchmarked on top of softpin media driver, found no
 difference


You have S-curve for saturated workloads or something else? How thorough and
which media team I guess.

  From memory it was a nice win for some benchmarks (non-saturated ones), but
as I have told you previously, we haven't been putting numbers in commit
messages since it wasn't allowed. I may be able to dig out some more details
if I went trawling through GEM channel IRC logs, although probably not the
actual numbers since those were usually on pastebin. Or you go an talk with
Chris since he probably remembers more details. Or you just decide you don't
care and remove it. I wouldn't do that without putting the complete story in
writing, but it's your call after all.


Media has also changed, they're not using relocations anymore.


Meaning you think it changes the benchmarking story? When coupled with
removal of GPU relocations then possibly yes.


Unless there's solid data performance tuning of any kind that gets in the
way simply needs to be removed. Yes this is radical, but the codebase is
in a state to require this.

So either way we'd need to rebenchmark this if it's really required. Also


Therefore can you share what benchmarks have been done or is it secret?  As
said, I think the non-saturated case was the more interesting one here.


if we really need this code still someone needs to fix the design, the
current code is making layering violations an art form.


Anyway, without the debugfs churn it is more or less two line patch to fix
igfx + dgfx hybrid setup. So while mulling it over this could go in. I'd
just refine it to use a GGTT check instead of GT. And unless DG1 ends up
being GuC only.


The minimal robust fix here is imo to stop us from upcasting dma_fence to
i915_request if it's not for our device. Not sprinkle code here into the
semaphore code. We shouldn't even get this far with foreign fences.


Device check does not work for multi-tile. It was one of my earlier attempts
before I realized the problem. You'll see v3 which I think handles all the
cases.


There is no hw semaphores on multi-tile.


You mean because of GuC? Okay, there may not be after bringup has been 
done. In which case an assert is needed somewhere just in case, if you 
are adamant not to accept this fix. It may indeed not matter hugely 
outside of the current transition period since I spotted patches to 
enable GuC on DG1. But then again it is trivial and fixes current pains 
for more than just me.



But there _is_ a lot more going on than just hw semaphores that spawn
driver instances. Like priority boosting, clock boosting, and all kinds of
other things. I really dont' think it's very robust if we play
whack-a-mole here with things leaking.


You mean span not spawn? I audited those and they looks good to me. 
AFAIR scheduling was in fact designed with a global lock just so that 
works. Plus the cases you mention end up not holding pointers to 
"foreign" instances anyway, they just do priority inheritance. Which is 
probably nice not to lose if not unavoidable.



You also forgot to comment on the question lower in the email. I'll just
send a patch which removes that anyway so you can comment there.


:(

Regards,

Tvrtko




Regards,

Tvrtko


-Daniel




- pre-gen8

[Bug 214289] New: amdgpu Msg issuing pre-check failed and SMU may be not in the right state!

https://bugzilla.kernel.org/show_bug.cgi?id=214289

Bug ID: 214289
   Summary: amdgpu Msg issuing pre-check failed and SMU may be not
in the right state!
   Product: Drivers
   Version: 2.5
Kernel Version: 5.13.13
  Hardware: Intel
OS: Linux
  Tree: Mainline
Status: NEW
  Severity: normal
  Priority: P1
 Component: Video(DRI - non Intel)
  Assignee: drivers_video-...@kernel-bugs.osdl.org
  Reporter: michal.przybylow...@gmail.com
Regression: No

I am using kernel 5.13.13 with amdgpu drivers. My graphics card is: PowerColor
RX 6800 XT (connected through riser cable - if that means something).

I don't have issues when using card with windows 10 (playing games etc.)

$ inxi -Gx
Graphics:  Device-1: Advanced Micro Devices [AMD/ATI] Navi 21 [Radeon RX
6800/6800 XT / 6900 XT] vendor: Tul 
   driver: amdgpu v: kernel bus-ID: 03:00.0 
   Display: server: X.Org 1.20.11 driver: loaded: amdgpu resolution:
3440x1440~144Hz 
   OpenGL: renderer: AMD Radeon RX 6800 XT (SIENNA_CICHLID DRM 3.41.0
5.13.13-051313-lowlatency LLVM 12.0.1) 
   v: 4.6 Mesa 21.3.0-devel (git-dfef775 2021-09-02 hirsute-oibaf-ppa)
direct render: Yes

Almost everyday during normal browser usage (watching youtube videos or twitch)
I see these messages in the journalctl:

ep 02 16:39:13 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing pre-check
failed and SMU may be not in the right state!
Sep 02 16:39:13 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:20 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:20 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:20 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:20 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:21 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:21 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:22 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:22 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:24 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:24 dagon kernel: amdgpu :03:00.0: amdgpu: Failed to power gate
VCN!
Sep 02 16:39:24 dagon kernel: [drm:amdgpu_dpm_enable_uvd [amdgpu]] *ERROR* Dpm
disable uvd failed, ret = -5. 
Sep 02 16:39:24 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:24 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:24 dagon kernel: amdgpu :03:00.0: amdgpu: Failed to enable
gfxoff!
Sep 02 16:39:26 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:26 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:26 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:27 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:27 dagon kernel: amdgpu :03:00.0: amdgpu: Failed to power gate
VCN!
Sep 02 16:39:27 dagon kernel: [drm:amdgpu_dpm_enable_uvd [amdgpu]] *ERROR* Dpm
disable uvd failed, ret = -5. 
Sep 02 16:39:27 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:27 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:27 dagon kernel: amdgpu :03:00.0: amdgpu: Failed to enable
gfxoff!
Sep 02 16:39:29 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:29 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:30 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!
Sep 02 16:39:30 dagon kernel: amdgpu :03:00.0: amdgpu: Failed to enable
gfxoff!
Sep 02 16:39:31 dagon kernel: amdgpu :03:00.0: amdgpu: Msg issuing
pre-check failed and SMU may be not in the right state!

Re: [Intel-gfx] [PATCH 07/11] drm/i915: Add i915_gem_context_is_full_ppgtt




On 02/09/2021 15:20, Daniel Vetter wrote:

And use it anywhere we have open-coded checks for ctx->vm that really
only check for full ppgtt.

Plus for paranoia add a GEM_BUG_ON that checks it's really only set
when we have full ppgtt, just in case. gem_context->vm is different
since it's NULL in ggtt mode, unlike intel_context->vm or gt->vm,
which is always set.

v2: 0day found a testcase that I missed.

Reviewed-by: Maarten Lankhorst 
Signed-off-by: Daniel Vetter 
Cc: Jon Bloomfield 
Cc: Chris Wilson 
Cc: Maarten Lankhorst 
Cc: Joonas Lahtinen 
Cc: Daniel Vetter 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Cc: Lionel Landwerlin 
Cc: Dave Airlie 
Cc: Jason Ekstrand 
---
  drivers/gpu/drm/i915/gem/i915_gem_context.c   | 2 +-
  drivers/gpu/drm/i915/gem/i915_gem_context.h   | 7 +++
  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +-
  drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 6 +++---
  4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 7a566fb7cca4..1eec85944c1f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1566,7 +1566,7 @@ static int get_ppgtt(struct drm_i915_file_private 
*file_priv,
int err;
u32 id;
  
-	if (!rcu_access_pointer(ctx->vm))

+   if (!i915_gem_context_is_full_ppgtt(ctx))


It reads a bit wrong because GEM context cannot *be* full ppggt. It can 
be associated with a VM which is or isn't full ppgtt. So a test on a VM 
retrieved from a context is semnntically more correct. Perhaps you want 
to consider adding a helper to that effect instead? It could mean 
splitting into two helpers (getter + test) or maybe just renaming would 
work. Like i915_gem_context_has_full_ppgtt_vm(ctx)?


Regards,

Tvrtko


return -ENODEV;
  
  	rcu_read_lock();

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h 
b/drivers/gpu/drm/i915/gem/i915_gem_context.h
index da6e8b506d96..37536a260e6e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
@@ -154,6 +154,13 @@ i915_gem_context_vm(struct i915_gem_context *ctx)
return rcu_dereference_protected(ctx->vm, lockdep_is_held(>mutex));
  }
  
+static inline bool i915_gem_context_is_full_ppgtt(struct i915_gem_context *ctx)

+{
+   GEM_BUG_ON(!!rcu_access_pointer(ctx->vm) != HAS_FULL_PPGTT(ctx->i915));
+
+   return !!rcu_access_pointer(ctx->vm);
+}
+
  static inline struct i915_address_space *
  i915_gem_context_get_eb_vm(struct i915_gem_context *ctx)
  {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c 
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 905b1cbd22d5..40f08948f0b2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -749,7 +749,7 @@ static int eb_select_context(struct i915_execbuffer *eb)
return PTR_ERR(ctx);
  
  	eb->gem_context = ctx;

-   if (rcu_access_pointer(ctx->vm))
+   if (i915_gem_context_is_full_ppgtt(ctx))
eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
  
  	return 0;

diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
index fc7fb33a3a52..947154e445a7 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
@@ -704,7 +704,7 @@ static int igt_ctx_exec(void *arg)
pr_err("Failed to fill dword %lu [%lu/%lu] with gpu 
(%s) [full-ppgtt? %s], err=%d\n",
   ndwords, dw, max_dwords(obj),
   engine->name,
-  yesno(!!rcu_access_pointer(ctx->vm)),
+  
yesno(i915_gem_context_is_full_ppgtt(ctx)),
   err);
intel_context_put(ce);
kernel_context_close(ctx);
@@ -838,7 +838,7 @@ static int igt_shared_ctx_exec(void *arg)
pr_err("Failed to fill dword %lu [%lu/%lu] with gpu 
(%s) [full-ppgtt? %s], err=%d\n",
   ndwords, dw, max_dwords(obj),
   engine->name,
-  yesno(!!rcu_access_pointer(ctx->vm)),
+  
yesno(i915_gem_context_is_full_ppgtt(ctx)),
   err);
intel_context_put(ce);
kernel_context_close(ctx);
@@ -1417,7 +1417,7 @@ static int igt_ctx_readonly(void *arg)
pr_err("Failed to fill dword %lu [%lu/%lu] with gpu 
(%s) [full-ppgtt? %s], err=%d\n",
   ndwords, dw, max_dwords(obj),

Re: [PATCH] drm/lease: allow empty leases

On Thu, Sep 02, 2021 at 05:28:10PM +0300, Pekka Paalanen wrote:
> On Thu, 02 Sep 2021 09:11:40 +
> Simon Ser  wrote:
> 
> > This can be used to create a separate DRM file description, thus
> > creating a new GEM handle namespace. See [1].
> > 
> > Example usage in wlroots is available at [2].
> > 
> > [1]: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/110
> > [2]: https://github.com/swaywm/wlroots/pull/3158
> > 
> 
> Hi Simon,
> 
> I have a feeling that this is a good idea, but could you explain in
> this commit message some real use cases where one needs a new GEM
> handle namespace? Not just "when you share a DRM fd between processes"
> but *why* you shared a DRM device fd between processes.
> 
> If I have trouble remembering or figuring that out from those links,
> then I'm sure others have too.

Also please document the uapi headers and explain the use-case there and
why and all that.

I'd like that we officiate all uapi we intentionally create in the docs as
much as possible.

Also igt testcase patch for this too pls.
-Daniel

> 
> 
> Thanks,
> pq
> 
> > Signed-off-by: Simon Ser 
> > Cc: Daniel Vetter 
> > Cc: Daniel Stone 
> > Cc: Pekka Paalanen 
> > Cc: Michel Dänzer 
> > Cc: Emil Velikov 
> > Cc: Keith Packard 
> > Cc: Boris Brezillon 
> > Cc: Dave Airlie 
> > ---
> >  drivers/gpu/drm/drm_lease.c | 39 +
> >  1 file changed, 18 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> > index dee4f24a1808..d72c2fac0ff1 100644
> > --- a/drivers/gpu/drm/drm_lease.c
> > +++ b/drivers/gpu/drm/drm_lease.c
> > @@ -489,12 +489,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
> > if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > return -EOPNOTSUPP;
> >  
> > -   /* need some objects */
> > -   if (cl->object_count == 0) {
> > -   DRM_DEBUG_LEASE("no objects in lease\n");
> > -   return -EINVAL;
> > -   }
> > -
> > if (cl->flags && (cl->flags & ~(O_CLOEXEC | O_NONBLOCK))) {
> > DRM_DEBUG_LEASE("invalid flags\n");
> > return -EINVAL;
> > @@ -510,23 +504,26 @@ int drm_mode_create_lease_ioctl(struct drm_device 
> > *dev,
> >  
> > object_count = cl->object_count;
> >  
> > -   object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
> > -   array_size(object_count, sizeof(__u32)));
> > -   if (IS_ERR(object_ids)) {
> > -   ret = PTR_ERR(object_ids);
> > -   goto out_lessor;
> > -   }
> > -
> > +   /* Handle leased objects, if any */
> > idr_init();
> > +   if (object_count != 0) {
> > +   object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
> > +array_size(object_count, 
> > sizeof(__u32)));
> > +   if (IS_ERR(object_ids)) {
> > +   ret = PTR_ERR(object_ids);
> > +   idr_destroy();
> > +   goto out_lessor;
> > +   }
> >  
> > -   /* fill and validate the object idr */
> > -   ret = fill_object_idr(dev, lessor_priv, ,
> > - object_count, object_ids);
> > -   kfree(object_ids);
> > -   if (ret) {
> > -   DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret);
> > -   idr_destroy();
> > -   goto out_lessor;
> > +   /* fill and validate the object idr */
> > +   ret = fill_object_idr(dev, lessor_priv, ,
> > + object_count, object_ids);
> > +   kfree(object_ids);
> > +   if (ret) {
> > +   DRM_DEBUG_LEASE("lease object lookup failed: %i\n", 
> > ret);
> > +   idr_destroy();
> > +   goto out_lessor;
> > +   }
> > }
> >  
> > /* Allocate a file descriptor for the lease */
> 



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/ttm: provide default page protection for UML

On Thu, Sep 02, 2021 at 07:19:01AM +0100, Anton Ivanov wrote:
> On 02/09/2021 06:52, Randy Dunlap wrote:
> > On 9/1/21 10:48 PM, Anton Ivanov wrote:
> > > On 02/09/2021 03:01, Randy Dunlap wrote:
> > > > boot_cpu_data [struct cpuinfo_um (on UML)] does not have a struct
> > > > member named 'x86', so provide a default page protection mode
> > > > for CONFIG_UML.
> > > > 
> > > > Mends this build error:
> > > > ../drivers/gpu/drm/ttm/ttm_module.c: In function
> > > > ‘ttm_prot_from_caching’:
> > > > ../drivers/gpu/drm/ttm/ttm_module.c:59:24: error: ‘struct
> > > > cpuinfo_um’ has no member named ‘x86’
> > > >    else if (boot_cpu_data.x86 > 3)
> > > >  ^
> > > > 
> > > > Fixes: 3bf3710e3718 ("drm/ttm: Add a generic TTM memcpy move for
> > > > page-based iomem")
> > > > Signed-off-by: Randy Dunlap 
> > > > Cc: Thomas Hellström 
> > > > Cc: Christian König 
> > > > Cc: Huang Rui 
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > Cc: Jeff Dike 
> > > > Cc: Richard Weinberger 
> > > > Cc: Anton Ivanov 
> > > > Cc: linux...@lists.infradead.org
> > > > Cc: David Airlie 
> > > > Cc: Daniel Vetter 
> > > > ---
> > > >   drivers/gpu/drm/ttm/ttm_module.c |    4 
> > > >   1 file changed, 4 insertions(+)
> > > > 
> > > > --- linux-next-20210901.orig/drivers/gpu/drm/ttm/ttm_module.c
> > > > +++ linux-next-20210901/drivers/gpu/drm/ttm/ttm_module.c
> > > > @@ -53,6 +53,9 @@ pgprot_t ttm_prot_from_caching(enum ttm_
> > > >   if (caching == ttm_cached)
> > > >   return tmp;
> > > > +#ifdef CONFIG_UML
> > > > +    tmp = pgprot_noncached(tmp);
> > > > +#else
> > > >   #if defined(__i386__) || defined(__x86_64__)
> > > >   if (caching == ttm_write_combined)
> > > >   tmp = pgprot_writecombine(tmp);
> > > > @@ -69,6 +72,7 @@ pgprot_t ttm_prot_from_caching(enum ttm_
> > > >   #if defined(__sparc__)
> > > >   tmp = pgprot_noncached(tmp);
> > > >   #endif
> > > > +#endif
> > > >   return tmp;
> > > >   }
> > > 
> > > Patch looks OK.
> > > 
> > > I have a question though - why all of DRM is not !UML in config. Not
> > > like we can use them.
> > 
> > I have no idea about that.
> > Hopefully one of the (other) UML maintainers can answer you.
> 
> Touche.
> 
> We will discuss that and possibly push a patch to !UML that part of the
> tree. IMHO it is not applicable.

I thought kunit is based on top of uml, and we do want to eventually adopt
that. Especially for helper libraries like ttm.

But also that's quite a bit in the future.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2] dma-buf: clarify dma_fence_add_callback documentation

On Wed, Sep 01, 2021 at 02:02:40PM +0200, Christian König wrote:
> That the caller doesn't need to keep a reference is rather
> risky and not defensive at all.
> 
> Especially dma_buf_poll got that horrible wrong, so better
> remove that sentence and also clarify that the callback
> might be called in atomic or interrupt context.
> 
> Signed-off-by: Christian König 

Still on the fence between documenting the precise rules and documenting
the safe rules, but this is tricky enough that you got me convinced. Plus
shorter, simpler, clearer kerneldoc has much better chances of being read,
understood and followed.

Reviewed-by: Daniel Vetter 

> ---
>  drivers/dma-buf/dma-fence.c | 13 +
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index ce0f5eff575d..1e82ecd443fa 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -616,20 +616,17 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
>   * @cb: the callback to register
>   * @func: the function to call
>   *
> + * Add a software callback to the fence. The caller should keep a reference 
> to
> + * the fence.
> + *
>   * @cb will be initialized by dma_fence_add_callback(), no initialization
>   * by the caller is required. Any number of callbacks can be registered
>   * to a fence, but a callback can only be registered to one fence at a time.
>   *
> - * Note that the callback can be called from an atomic context.  If
> - * fence is already signaled, this function will return -ENOENT (and
> + * If fence is already signaled, this function will return -ENOENT (and
>   * *not* call the callback).
>   *
> - * Add a software callback to the fence. Same restrictions apply to
> - * refcount as it does to dma_fence_wait(), however the caller doesn't need 
> to
> - * keep a refcount to fence afterward dma_fence_add_callback() has returned:
> - * when software access is enabled, the creator of the fence is required to 
> keep
> - * the fence alive until after it signals with dma_fence_signal(). The 
> callback
> - * itself can be called from irq context.
> + * Note that the callback can be called from an atomic context or irq 
> context.
>   *
>   * Returns 0 in case of success, -ENOENT if the fence is already signaled
>   * and -EINVAL in case of error.
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] dma-buf: clarify dma_fence_ops->wait documentation

On Wed, Sep 01, 2021 at 02:02:39PM +0200, Christian König wrote:
> This callback is pretty much deprecated and should not be used by new 
> implementations.
> 
> Clarify that in the documentation as well.
> 
> Signed-off-by: Christian König 

Reviewed-by: Daniel Vetter 


> ---
>  include/linux/dma-fence.h | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 6ffb4b2c6371..a44e42b86c2a 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -214,19 +214,15 @@ struct dma_fence_ops {
>* Custom wait implementation, defaults to dma_fence_default_wait() if
>* not set.
>*
> -  * The dma_fence_default_wait implementation should work for any fence, 
> as long
> -  * as @enable_signaling works correctly. This hook allows drivers to
> -  * have an optimized version for the case where a process context is
> -  * already available, e.g. if @enable_signaling for the general case
> -  * needs to set up a worker thread.
> +  * Deprecated and should not be used by new implementations. Only used
> +  * by existing implementations which need special handling for their
> +  * hardware reset procedure.
>*
>* Must return -ERESTARTSYS if the wait is intr = true and the wait was
>* interrupted, and remaining jiffies if fence has signaled, or 0 if 
> wait
>* timed out. Can also return other error values on custom 
> implementations,
>* which should be treated as if the fence is signaled. For example a 
> hardware
>* lockup could be reported like that.
> -  *
> -  * This callback is optional.
>*/
>   signed long (*wait)(struct dma_fence *fence,
>   bool intr, signed long timeout);
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH v2] drm/i915: Handle Intel igfx + Intel dgfx hybrid graphics setup

On Tue, Aug 31, 2021 at 02:18:15PM +0100, Tvrtko Ursulin wrote:
> 
> On 31/08/2021 13:43, Daniel Vetter wrote:
> > On Tue, Aug 31, 2021 at 10:15:03AM +0100, Tvrtko Ursulin wrote:
> > > 
> > > On 30/08/2021 09:26, Daniel Vetter wrote:
> > > > On Fri, Aug 27, 2021 at 03:44:42PM +0100, Tvrtko Ursulin wrote:
> > > > > 
> > > > > On 27/08/2021 15:39, Tvrtko Ursulin wrote:
> > > > > > From: Tvrtko Ursulin 
> > > > > > 
> > > > > > In short this makes i915 work for hybrid setups (DRI_PRIME=1 with 
> > > > > > Mesa)
> > > > > > when rendering is done on Intel dgfx and scanout/composition on 
> > > > > > Intel
> > > > > > igfx.
> > > > > > 
> > > > > > Before this patch the driver was not quite ready for that setup, 
> > > > > > mainly
> > > > > > because it was able to emit a semaphore wait between the two GPUs, 
> > > > > > which
> > > > > > results in deadlocks because semaphore target location in HWSP is 
> > > > > > neither
> > > > > > shared between the two, nor mapped in both GGTT spaces.
> > > > > > 
> > > > > > To fix it the patch adds an additional check to a couple of 
> > > > > > relevant code
> > > > > > paths in order to prevent using semaphores for inter-engine
> > > > > > synchronisation between different driver instances.
> > > > > > 
> > > > > > Patch also moves singly used i915_gem_object_last_write_engine to be
> > > > > > private in its only calling unit (debugfs), while modifying it to 
> > > > > > only
> > > > > > show activity belonging to the respective driver instance.
> > > > > > 
> > > > > > What remains in this problem space is the question of the GEM busy 
> > > > > > ioctl.
> > > > > > We have a somewhat ambigous comment there saying only status of 
> > > > > > native
> > > > > > fences will be reported, which could be interpreted as either i915, 
> > > > > > or
> > > > > > native to the drm fd. For now I have decided to leave that as is, 
> > > > > > meaning
> > > > > > any i915 instance activity continues to be reported.
> > > > > > 
> > > > > > v2:
> > > > > > * Avoid adding rq->i915. (Chris)
> > > > > > 
> > > > > > Signed-off-by: Tvrtko Ursulin 
> > > > 
> > > > Can't we just delete semaphore code and done?
> > > > - GuC won't have it
> > > > - media team benchmarked on top of softpin media driver, found no
> > > > difference
> > > 
> > > You have S-curve for saturated workloads or something else? How thorough 
> > > and
> > > which media team I guess.
> > > 
> > >  From memory it was a nice win for some benchmarks (non-saturated ones), 
> > > but
> > > as I have told you previously, we haven't been putting numbers in commit
> > > messages since it wasn't allowed. I may be able to dig out some more 
> > > details
> > > if I went trawling through GEM channel IRC logs, although probably not the
> > > actual numbers since those were usually on pastebin. Or you go an talk 
> > > with
> > > Chris since he probably remembers more details. Or you just decide you 
> > > don't
> > > care and remove it. I wouldn't do that without putting the complete story 
> > > in
> > > writing, but it's your call after all.
> > 
> > Media has also changed, they're not using relocations anymore.
> 
> Meaning you think it changes the benchmarking story? When coupled with
> removal of GPU relocations then possibly yes.
> 
> > Unless there's solid data performance tuning of any kind that gets in the
> > way simply needs to be removed. Yes this is radical, but the codebase is
> > in a state to require this.
> > 
> > So either way we'd need to rebenchmark this if it's really required. Also
> 
> Therefore can you share what benchmarks have been done or is it secret?  As
> said, I think the non-saturated case was the more interesting one here.
> 
> > if we really need this code still someone needs to fix the design, the
> > current code is making layering violations an art form.
> >
> > > Anyway, without the debugfs churn it is more or less two line patch to fix
> > > igfx + dgfx hybrid setup. So while mulling it over this could go in. I'd
> > > just refine it to use a GGTT check instead of GT. And unless DG1 ends up
> > > being GuC only.
> > 
> > The minimal robust fix here is imo to stop us from upcasting dma_fence to
> > i915_request if it's not for our device. Not sprinkle code here into the
> > semaphore code. We shouldn't even get this far with foreign fences.
> 
> Device check does not work for multi-tile. It was one of my earlier attempts
> before I realized the problem. You'll see v3 which I think handles all the
> cases.

There is no hw semaphores on multi-tile.

But there _is_ a lot more going on than just hw semaphores that spawn
driver instances. Like priority boosting, clock boosting, and all kinds of
other things. I really dont' think it's very robust if we play
whack-a-mole here with things leaking.
-Daniel

> You also forgot to comment on the question lower in the email. I'll just
> send a patch which removes that anyway so you can comment there.
> 
> Regards,
> 
> Tvrtko
> 
> >

Re: [RFC PATCH 0/4] Allow to use DRM fbdev emulation layer with CONFIG_FB disabled

On Wed, Sep 01, 2021 at 11:08:10AM +0200, Javier Martinez Canillas wrote:
> On 8/31/21 2:35 PM, Daniel Vetter wrote:
> > On Sat, Aug 28, 2021 at 12:02:21AM +0200, Javier Martinez Canillas wrote:
> 
> [snip]
> 
> >>
> >> We talked about a drmcon with Peter Robinson as well but then decided that 
> >> a
> >> way to disable CONFIG_FB but still having the DRM fbdev emulation could be 
> >> a
> >> intermediary step, hence these RFC patches.
> >>
> >> But yes, I agree that a drmcon would be the proper approach for this, to 
> >> not
> >> need any fbdev support at all. We will just keep the explicit disable for 
> >> the
> >> fbdev drivers then in the meantime.
> > 
> > I think the only intermediate step would be to disable the fbdev uapi
> > (char node and anything in sysfs), while still registering against the
> > fbcon layer so you have a console.
> >
> 
> Right, $subject disabled the sysfs interface but left the fbdev chardev. I can
> try to do a v2 that also disables that interface but just keep the fbcon part.
>  
> > But looking at the things syzbot finds the really problematic code is all
> > in the fbcon and console layer in general, and /dev/fb0 seems pretty
> > solid.
> >
> 
> Yes, but still would be an improvement in the sense that no legacy fbdev uAPI
> will be exposed and so user-space would only depend on the DRM/KMS interface.
> 
> > I think for a substantial improvement here in robustness what you really
> > want is
> > - kmscon in userspace
> > - disable FB layer
> > - ideally also disable console/vt layer in the kernel
> 
> Earlier in the thread it was mentioned that an in-kernel drmcon could be used
> instead. My worry with kmscon is that moving something as critical as console
> output to user-space might make harder to troubleshoot early booting issues.
> 
> And also that will require user-space changes. An in-kernel drmcon could be a
> drop-in replacement though.

The drmcon wouldn't be a full console, but just an emergency log renderer.
See Sam's reply, he found the series again.

The real attack surface reduction is in getting rid of the console/vt uapi
implementation from the kernel.
-Daniel

> > - have a minimal emergency/boot-up log thing in drm, patches for that
> >   floated around a few times
> >
> 
> Interesting. Do you have any pointers for this? My search-fu failed me when
> trying to find these patches.
> 
> Best regards,
> -- 
> Javier Martinez Canillas
> Linux Engineering
> Red Hat
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/lease: allow empty leases

2021-09-02 Thread Pekka Paalanen

On Thu, 02 Sep 2021 09:11:40 +
Simon Ser  wrote:

> This can be used to create a separate DRM file description, thus
> creating a new GEM handle namespace. See [1].
> 
> Example usage in wlroots is available at [2].
> 
> [1]: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/110
> [2]: https://github.com/swaywm/wlroots/pull/3158
> 

Hi Simon,

I have a feeling that this is a good idea, but could you explain in
this commit message some real use cases where one needs a new GEM
handle namespace? Not just "when you share a DRM fd between processes"
but *why* you shared a DRM device fd between processes.

If I have trouble remembering or figuring that out from those links,
then I'm sure others have too.


Thanks,
pq

> Signed-off-by: Simon Ser 
> Cc: Daniel Vetter 
> Cc: Daniel Stone 
> Cc: Pekka Paalanen 
> Cc: Michel Dänzer 
> Cc: Emil Velikov 
> Cc: Keith Packard 
> Cc: Boris Brezillon 
> Cc: Dave Airlie 
> ---
>  drivers/gpu/drm/drm_lease.c | 39 +
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index dee4f24a1808..d72c2fac0ff1 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -489,12 +489,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>   if (!drm_core_check_feature(dev, DRIVER_MODESET))
>   return -EOPNOTSUPP;
>  
> - /* need some objects */
> - if (cl->object_count == 0) {
> - DRM_DEBUG_LEASE("no objects in lease\n");
> - return -EINVAL;
> - }
> -
>   if (cl->flags && (cl->flags & ~(O_CLOEXEC | O_NONBLOCK))) {
>   DRM_DEBUG_LEASE("invalid flags\n");
>   return -EINVAL;
> @@ -510,23 +504,26 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>  
>   object_count = cl->object_count;
>  
> - object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
> - array_size(object_count, sizeof(__u32)));
> - if (IS_ERR(object_ids)) {
> - ret = PTR_ERR(object_ids);
> - goto out_lessor;
> - }
> -
> + /* Handle leased objects, if any */
>   idr_init();
> + if (object_count != 0) {
> + object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
> +  array_size(object_count, 
> sizeof(__u32)));
> + if (IS_ERR(object_ids)) {
> + ret = PTR_ERR(object_ids);
> + idr_destroy();
> + goto out_lessor;
> + }
>  
> - /* fill and validate the object idr */
> - ret = fill_object_idr(dev, lessor_priv, ,
> -   object_count, object_ids);
> - kfree(object_ids);
> - if (ret) {
> - DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret);
> - idr_destroy();
> - goto out_lessor;
> + /* fill and validate the object idr */
> + ret = fill_object_idr(dev, lessor_priv, ,
> +   object_count, object_ids);
> + kfree(object_ids);
> + if (ret) {
> + DRM_DEBUG_LEASE("lease object lookup failed: %i\n", 
> ret);
> + idr_destroy();
> + goto out_lessor;
> + }
>   }
>  
>   /* Allocate a file descriptor for the lease */



pgpJcKMpfX8nB.pgp
Description: OpenPGP digital signature

Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."

On Tue, Aug 31, 2021 at 02:24:52PM -0400, Andrey Grodzovsky wrote:
> 
> On 2021-08-31 9:11 a.m., Daniel Vetter wrote:
> > On Thu, Aug 26, 2021 at 11:04:14AM +0200, Daniel Vetter wrote:
> > > On Thu, Aug 19, 2021 at 11:25:09AM -0400, Andrey Grodzovsky wrote:
> > > > On 2021-08-19 5:30 a.m., Daniel Vetter wrote:
> > > > > On Wed, Aug 18, 2021 at 10:51:00AM -0400, Andrey Grodzovsky wrote:
> > > > > > On 2021-08-18 10:42 a.m., Daniel Vetter wrote:
> > > > > > > On Wed, Aug 18, 2021 at 10:36:32AM -0400, Andrey Grodzovsky wrote:
> > > > > > > > On 2021-08-18 10:32 a.m., Daniel Vetter wrote:
> > > > > > > > > On Wed, Aug 18, 2021 at 10:26:25AM -0400, Andrey Grodzovsky 
> > > > > > > > > wrote:
> > > > > > > > > > On 2021-08-18 10:02 a.m., Alex Deucher wrote:
> > > > > > > > > > 
> > > > > > > > > > > + dri-devel
> > > > > > > > > > > 
> > > > > > > > > > > Since scheduler is a shared component, please add 
> > > > > > > > > > > dri-devel on all
> > > > > > > > > > > scheduler patches.
> > > > > > > > > > > 
> > > > > > > > > > > On Wed, Aug 18, 2021 at 7:21 AM Jingwen Chen 
> > > > > > > > > > >  wrote:
> > > > > > > > > > > > [Why]
> > > > > > > > > > > > for bailing job, this commit will delete it from 
> > > > > > > > > > > > pending list thus the
> > > > > > > > > > > > bailing job will never have a chance to be resubmitted 
> > > > > > > > > > > > even in advance
> > > > > > > > > > > > tdr mode.
> > > > > > > > > > > > 
> > > > > > > > > > > > [How]
> > > > > > > > > > > > after embeded hw_fence into amdgpu_job is done, the 
> > > > > > > > > > > > race condition that
> > > > > > > > > > > > this commit tries to work around is completely 
> > > > > > > > > > > > solved.So revert this
> > > > > > > > > > > > commit.
> > > > > > > > > > > > This reverts commit 
> > > > > > > > > > > > 135517d3565b48f4def3b1b82008bc17eb5d1c90.
> > > > > > > > > > > > v2:
> > > > > > > > > > > > add dma_fence_get/put() around timedout_job to avoid 
> > > > > > > > > > > > concurrent delete
> > > > > > > > > > > > during processing timedout_job
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Jingwen Chen 
> > > > > > > > > > > > ---
> > > > > > > > > > > >   drivers/gpu/drm/scheduler/sched_main.c | 23 
> > > > > > > > > > > > +--
> > > > > > > > > > > >   1 file changed, 5 insertions(+), 18 deletions(-)
> > > > > > > > > > > > 
> > > > > > > > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
> > > > > > > > > > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > > > > index a2a953693b45..f9b9b3aefc4a 100644
> > > > > > > > > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > > > > @@ -314,6 +314,7 @@ static void 
> > > > > > > > > > > > drm_sched_job_timedout(struct work_struct *work)
> > > > > > > > > > > >   {
> > > > > > > > > > > >  struct drm_gpu_scheduler *sched;
> > > > > > > > > > > >  struct drm_sched_job *job;
> > > > > > > > > > > > +   struct dma_fence *fence;
> > > > > > > > > > > >  enum drm_gpu_sched_stat status = 
> > > > > > > > > > > > DRM_GPU_SCHED_STAT_NOMINAL;
> > > > > > > > > > > > 
> > > > > > > > > > > >  sched = container_of(work, struct 
> > > > > > > > > > > > drm_gpu_scheduler, work_tdr.work);
> > > > > > > > > > > > @@ -325,11 +326,10 @@ static void 
> > > > > > > > > > > > drm_sched_job_timedout(struct work_struct *work)
> > > > > > > > > > > > 
> > > > > > > > > > > >  if (job) {
> > > > > > > > > > > >  /*
> > > > > > > > > > > > -* Remove the bad job so it cannot be 
> > > > > > > > > > > > freed by concurrent
> > > > > > > > > > > > -* drm_sched_cleanup_jobs. It will be 
> > > > > > > > > > > > reinserted back after sched->thread
> > > > > > > > > > > > -* is parked at which point it's safe.
> > > > > > > > > > > > +* Get job->s_fence->parent here to 
> > > > > > > > > > > > avoid concurrent delete during
> > > > > > > > > > > > +* processing timedout_job
> > > > > > > > > > > >   */
> > > > > > > > > > > > -   list_del_init(>list);
> > > > > > > > > > > > +   fence = 
> > > > > > > > > > > > dma_fence_get(job->s_fence->parent);
> > > > > > > > > > While this is true for amdgpu, it has no meaning for other 
> > > > > > > > > > drivers for whom
> > > > > > > > > > we haven't
> > > > > > > > > > done the refactoring of embedding HW fence (parent) into 
> > > > > > > > > > the job structure.
> > > > > > > > > > In fact thinking
> > > > > > > > > > about it, unless you do the HW fence embedding for all the 
> > > > > > > > > > drivers using the
> > > > > > > > > > scheduler you cannot
> > > > > > > > > > revert this patch or you will just break them.
> > > > > > > > > btw, why did you do that embedding? I do still have my 
> > > > > > > >

Re: [PATCH 2/7] drm/format-helper: Add drm_fb_xrgb8888_to_rgb332()

On Thu, Sep 02, 2021 at 04:08:14PM +0200, Noralf Trønnes wrote:
> 
> 
> Den 31.08.2021 14.23, skrev Daniel Vetter:
> > On Mon, Aug 30, 2021 at 02:08:14PM +0200, Noralf Trønnes wrote:
> >>
> >>
> >> Den 17.08.2021 15.56, skrev Daniel Vetter:
> >>> On Tue, Aug 17, 2021 at 02:29:12PM +0200, Noralf Trønnes wrote:
>  Add XRGB emulation support for devices that can only do RGB332.
> 
>  Cc: Thomas Zimmermann 
>  Signed-off-by: Noralf Trønnes 
>  ---
>   drivers/gpu/drm/drm_format_helper.c | 47 +
>   include/drm/drm_format_helper.h |  2 ++
>   2 files changed, 49 insertions(+)
> 
>  diff --git a/drivers/gpu/drm/drm_format_helper.c 
>  b/drivers/gpu/drm/drm_format_helper.c
>  index 5231104b1498..53b426da7467 100644
>  --- a/drivers/gpu/drm/drm_format_helper.c
>  +++ b/drivers/gpu/drm/drm_format_helper.c
>  @@ -135,6 +135,53 @@ void drm_fb_swab(void *dst, void *src, struct 
>  drm_framebuffer *fb,
>   }
>   EXPORT_SYMBOL(drm_fb_swab);
>   
>  +static void drm_fb_xrgb_to_rgb332_line(u8 *dbuf, u32 *sbuf, 
>  unsigned int pixels)
>  +{
>  +unsigned int x;
>  +
>  +for (x = 0; x < pixels; x++)
>  +dbuf[x] = ((sbuf[x] & 0x00e0) >> 16) |
> >>>
> >>> I think for 2/3 bits correct rounding would be useful, not just masking.
> >>> i.e. before you shift add 0x0010 here, and similar below.
> >>>
> >>
> >> Math isn't my strongest side and my brain failed to turn this into code.
> > 
> > Essentially add half of the lowest bit before you mask, so
> > 
> > ((sbuf[x] + 0x10) & 0xe0 )
> > 
> 
> But what if the value is 0xff, it overflows:

Your math is better than mine ...

> ((0xff + 0x10) & 0xe0 ) == 0x00
> 
> Should it be OR?
> 
> ((0xff | 0x10) & 0xe0 ) == 0xe0

Probably need a max on top to limit the overflow:

max(sbuf[x] + 0x10, 0xe)

But also maybe really not worth the head-scratching :-)
-Daniel

> 
> Noralf.
> 
> > I dropped the shift to make it clear what's going on. If you're mask is
> > 0xc0, then you simply add 0x20 before masking.
> > 
> >>> Also I just realized we've totally ignored endianess on these, which is
> >>> not great, because strictly speaking all the drm_fourcc codes should be
> >>> little endian. But I'm also not sure that's worth fixing ...
> >>>
> >>
> >> Is it as simple as using le32_to_cpu()?
> > 
> > I think so.
> > 
> > Plus on any format that has u16 output we'd need a cpu_to_le16 wrapped
> > around the entire thing.
> > -Daniel
> > 
> >> static void drm_fb_xrgb_to_rgb332_line(u8 *dbuf, __le32 *sbuf,
> >> unsigned int pixels)
> >> {
> >>unsigned int x;
> >>u32 pix;
> >>
> >>for (x = 0; x < pixels; x++) {
> >>pix = le32_to_cpu(sbuf[x]);
> >>dbuf[x] = ((pix & 0x00e0) >> 16) |
> >>  ((pix & 0xe000) >> 11) |
> >>  ((pix & 0x00c0) >> 6);
> >>}
> >> }
> >>
> >> Noralf.
> >>
> >>> Either way, lgtm:
> >>>
> >>> Reviewed-by: Daniel Vetter 
> >>>
>  +  ((sbuf[x] & 0xe000) >> 11) |
>  +  ((sbuf[x] & 0x00c0) >> 6);
>  +}
>  +
>  +/**
>  + * drm_fb_xrgb_to_rgb332 - Convert XRGB to RGB332 clip buffer
>  + * @dst: RGB332 destination buffer
>  + * @src: XRGB source buffer
>  + * @fb: DRM framebuffer
>  + * @clip: Clip rectangle area to copy
>  + *
>  + * Drivers can use this function for RGB332 devices that don't natively 
>  support XRGB.
>  + *
>  + * This function does not apply clipping on dst, i.e. the destination 
>  is a small buffer
>  + * containing the clip rect only.
>  + */
>  +void drm_fb_xrgb_to_rgb332(void *dst, void *src, struct 
>  drm_framebuffer *fb,
>  +   struct drm_rect *clip)
>  +{
>  +size_t width = drm_rect_width(clip);
>  +size_t src_len = width * sizeof(u32);
>  +unsigned int y;
>  +void *sbuf;
>  +
>  +/* Use a buffer to speed up access on buffers with uncached 
>  read mapping (i.e. WC) */
>  +sbuf = kmalloc(src_len, GFP_KERNEL);
>  +if (!sbuf)
>  +return;
>  +
>  +src += clip_offset(clip, fb->pitches[0], sizeof(u32));
>  +for (y = 0; y < drm_rect_height(clip); y++) {
>  +memcpy(sbuf, src, src_len);
>  +drm_fb_xrgb_to_rgb332_line(dst, sbuf, width);
>  +src += fb->pitches[0];
>  +dst += width;
>  +}
>  +
>  +kfree(sbuf);
>  +}
>  +EXPORT_SYMBOL(drm_fb_xrgb_to_rgb332);
>  +
>   static void drm_fb_xrgb_to_rgb565_line(u16 *dbuf, u32 *sbuf,
>  unsigned int pixels,
>

[PATCH 10/11] drm/i915: use xa_lock/unlock for fpriv->vm_xa lookups

We don't need the absolute speed of rcu for this. And
i915_address_space in general dont need rcu protection anywhere else,
after we've made gem contexts and engines a lot more immutable.

Note that this semantically reverts

commit aabbe344dc3ca5f7d8263a02608ba6179e8a4499
Author: Chris Wilson 
Date:   Fri Aug 30 19:03:25 2019 +0100

drm/i915: Use RCU for unlocked vm_idr lookup

except we have the conversion from idr to xarray in between.

v2: kref_get_unless_zero is no longer required (Maarten)

Reviewed-by: Maarten Lankhorst 
Signed-off-by: Daniel Vetter 
Cc: Jon Bloomfield 
Cc: Chris Wilson 
Cc: Maarten Lankhorst 
Cc: Joonas Lahtinen 
Cc: Daniel Vetter 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Cc: Lionel Landwerlin 
Cc: Dave Airlie 
Cc: Jason Ekstrand 
---
 drivers/gpu/drm/i915/i915_drv.h | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index be2392bbcecc..d89ff55d8fc8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1874,11 +1874,11 @@ i915_gem_vm_lookup(struct drm_i915_file_private 
*file_priv, u32 id)
 {
struct i915_address_space *vm;
 
-   rcu_read_lock();
+   xa_lock(_priv->vm_xa);
vm = xa_load(_priv->vm_xa, id);
-   if (vm && !kref_get_unless_zero(>ref))
-   vm = NULL;
-   rcu_read_unlock();
+   if (vm)
+   kref_get(>ref);
+   xa_unlock(_priv->vm_xa);
 
return vm;
 }
-- 
2.33.0

[PATCH 11/11] drm/i915: Stop rcu support for i915_address_space

The full audit is quite a bit of work:

- i915_dpt has very simple lifetime (somehow we create a display pagetable vm
  per object, so its _very_ simple, there's only ever a single vma in there),
  and uses i915_vm_close(), which internally does a i915_vm_put(). No rcu.

  Aside: wtf is i915_dpt doing in the intel_display.c garbage collector as a new
  feature, instead of added as a separate file with some clean-ish interface.

  Also, i915_dpt unfortunately re-introduces some coding patterns from
  pre-dma_resv_lock conversion times.

- i915_gem_proto_ctx is fully refcounted and no rcu, all protected by
  fpriv->proto_context_lock.

- i915_gem_context is itself rcu protected, and that might leak to anything it
  points at. Before

commit cf977e18610e66e48c31619e7e0cfa871be9eada
Author: Chris Wilson 
Date:   Wed Dec 2 11:21:40 2020 +

drm/i915/gem: Spring clean debugfs

  and

commit db80a1294c231b6ac725085f046bb2931e00c9db
Author: Chris Wilson 
Date:   Mon Jan 18 11:08:54 2021 +

drm/i915/gem: Remove per-client stats from debugfs/i915_gem_objects

  we had a bunch of debugfs files that relied on rcu protecting everything, but
  those are gone now. The main one was removed even earlier with

  There doesn't seem to be anything left that's actually protecting
  stuff now that the ctx->vm itself is invariant. See

commit ccbc1b97948ab671335e950271e39766729736c3
Author: Jason Ekstrand 
Date:   Thu Jul 8 10:48:30 2021 -0500

drm/i915/gem: Don't allow changing the VM on running contexts (v4)

  Note that we drop the vm refcount before the final release of the gem context
  refcount, so this is all very dangerous even without rcu. Note that aside from
  later on creating new engines (a defunct feature) and debug output we're never
  looked at gem_ctx->vm for anything functional, hence why this is ok.
  Fingers crossed.

  Preceeding patches removed all vestiges of rcu use from gem_ctx->vm
  derferencing to make it clear it's really not used.

  The gem_ctx->rcu protection was introduced in

commit a4e7ccdac38ec8335d9e4e2656c1a041c77feae1
Author: Chris Wilson 
Date:   Fri Oct 4 14:40:09 2019 +0100

drm/i915: Move context management under GEM

  The commit message is somewhat entertaining because it fails to
  mention this fact completely, and compensates that by an in-commit
  changelog entry that claims that ctx->vm is protected by ctx->mutex.
  Which was the case _before_ this commit, but no longer after it.

- intel_context holds a full reference. Unfortunately intel_context is also rcu
  protected and the reference to the ->vm is dropped before the
  rcu barrier - only the kfree is delayed. So again we need to check
  whether that leaks anywhere on the intel_context->vm. RCU is only
  used to protect intel_context sitting on the breadcrumb lists, which
  don't look at the vm anywhere, so we are fine.

  Nothing else relies on rcu protection of intel_context and hence is
  fully protected by the kref refcount alone, which protects
  intel_context->vm in turn.

  The breadcrumbs rcu usage was added in

commit c744d50363b714783bbc88d986cc16def13710f7
Author: Chris Wilson 
Date:   Thu Nov 26 14:04:06 2020 +

drm/i915/gt: Split the breadcrumb spinlock between global and 
contexts

  its parent commit added the intel_context rcu protection:

commit 14d1eaf08845c534963c83f754afe0cb14cb2512
Author: Chris Wilson 
Date:   Thu Nov 26 14:04:05 2020 +

drm/i915/gt: Protect context lifetime with RCU

  given some credence to my claim that I've actually caught them all.

- drm_i915_gem_object's shares_resv_from pointer has a full refcount to the
  dma_resv, which is a sub-refcount that's released after the final
  i915_vm_put() has been called. Safe.

  Aside: Maybe we should have a struct dma_resv_shared which is just dma_resv +
  kref as a stand-alone thing. It's a pretty useful pattern which other drivers
  might want to copy.

  For a bit more context see

commit 4d8151ae5329cf50781a02fd2298a909589a5bab
Author: Thomas Hellström 
Date:   Tue Jun 1 09:46:41 2021 +0200

drm/i915: Don't free shared locks while shared

- the fpriv->vm_xa was relying on rcu_read_lock for lookup, but that
  was updated in a prep patch too to just be a spinlock-protected
  lookup.

- intel_gt->vm is set at driver load in intel_gt_init() and released
  in intel_gt_driver_release(). There seems to be some issue that
  in some error paths this is called twice, but otherwise no rcu to be
  found anywhere. This was added in the below commit, which
  unfortunately doesn't explain why this complication exists.

commit e6ba76480299a0d77c51d846f7467b1673aad25b
Author: Chris Wilson 
Date:   Sat Dec 21 16:03:24 2019 +

drm/i915: Remove

[PATCH 09/11] drm/i915: Drop __rcu from gem_context->vm

It's been invariant since

commit ccbc1b97948ab671335e950271e39766729736c3
Author: Jason Ekstrand 
Date:   Thu Jul 8 10:48:30 2021 -0500

drm/i915/gem: Don't allow changing the VM on running contexts (v4)

this just completes the deed. I've tried to split out prep work for
more careful review as much as possible, this is what's left:

- get_ppgtt gets simplified since we don't need to grab a temporary
  reference - we can rely on the temporary reference for the gem_ctx
  while we inspect the vm. The new vm_id still needs a full
  i915_vm_open ofc. This also removes the final caller of context_get_vm_rcu

- A pile of selftests can now just look at ctx->vm instead of
  rcu_dereference_protected( , true) or similar things.

- All callers of i915_gem_context_vm also disappear.

- I've changed the hugepage selftest to set scrub_64K without any
  locking, because when we inspect that setting we're also not taking
  any locks either. It works because it's a selftests that's careful
  (single threaded gives you nice ordering) and not a live driver
  where races can happen from anywhere.

These can only be split up further if we have some intermediate state
with a bunch more rcu_dereference_protected(ctx->vm, true), just to
shut up lockdep and sparse.

The conversion to __rcu happened in

commit a4e7ccdac38ec8335d9e4e2656c1a041c77feae1
Author: Chris Wilson 
Date:   Fri Oct 4 14:40:09 2019 +0100

drm/i915: Move context management under GEM

Note that we're not breaking the actual bugfix in there: The real
bugfix is pushing the i915_vm_relase onto a separate worker, to avoid
locking inversion issues. The rcu conversion was just thrown in for
entertainment value on top (no vm lookup isn't even close to anything
that's a hotpath where removing the single spinlock can be measured).

v2: Rebase over the change to move the i915_vm_put() into
i915_gem_context_release().

Reviewed-by: Maarten Lankhorst 
Signed-off-by: Daniel Vetter 
Cc: Jon Bloomfield 
Cc: Chris Wilson 
Cc: Maarten Lankhorst 
Cc: Joonas Lahtinen 
Cc: Daniel Vetter 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Cc: Lionel Landwerlin 
Cc: Dave Airlie 
Cc: Jason Ekstrand 
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c   | 55 ++-
 drivers/gpu/drm/i915/gem/i915_gem_context.h   | 14 +
 .../gpu/drm/i915/gem/i915_gem_context_types.h |  2 +-
 .../gpu/drm/i915/gem/selftests/huge_pages.c   |  4 +-
 .../drm/i915/gem/selftests/i915_gem_context.c | 24 +++-
 drivers/gpu/drm/i915/i915_trace.h |  2 +-
 drivers/gpu/drm/i915/selftests/i915_vma.c |  2 +-
 7 files changed, 22 insertions(+), 81 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 18e23d9220ae..fe8cd5456438 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -742,44 +742,6 @@ static int set_proto_ctx_param(struct 
drm_i915_file_private *fpriv,
return ret;
 }
 
-static struct i915_address_space *
-context_get_vm_rcu(struct i915_gem_context *ctx)
-{
-   GEM_BUG_ON(!rcu_access_pointer(ctx->vm));
-
-   do {
-   struct i915_address_space *vm;
-
-   /*
-* We do not allow downgrading from full-ppgtt [to a shared
-* global gtt], so ctx->vm cannot become NULL.
-*/
-   vm = rcu_dereference(ctx->vm);
-   if (!kref_get_unless_zero(>ref))
-   continue;
-
-   /*
-* This ppgtt may have be reallocated between
-* the read and the kref, and reassigned to a third
-* context. In order to avoid inadvertent sharing
-* of this ppgtt with that third context (and not
-* src), we have to confirm that we have the same
-* ppgtt after passing through the strong memory
-* barrier implied by a successful
-* kref_get_unless_zero().
-*
-* Once we have acquired the current ppgtt of ctx,
-* we no longer care if it is released from ctx, as
-* it cannot be reallocated elsewhere.
-*/
-
-   if (vm == rcu_access_pointer(ctx->vm))
-   return rcu_pointer_handoff(vm);
-
-   i915_vm_put(vm);
-   } while (1);
-}
-
 static int intel_context_set_gem(struct intel_context *ce,
 struct i915_gem_context *ctx,
 struct intel_sseu sseu)
@@ -990,7 +952,7 @@ static void i915_gem_context_release_work(struct 
work_struct *work)
if (ctx->syncobj)
drm_syncobj_put(ctx->syncobj);
 
-   vm = i915_gem_context_vm(ctx);
+   vm = ctx->vm;
if (vm)
i915_vm_put(vm);
 
@@ -1216,7 +1178,7 @@ static void context_close(struct i915_gem_context *ctx)
 
set_closed_name(ctx);
 
-

[PATCH 06/11] drm/i915: Use i915_gem_context_get_eb_vm in ctx_getparam

Consolidates the "which is the vm my execbuf runs in" code a bit. We
do some get/put which isn't really required, but all the other users
want the refcounting, and I figured doing a function just for this
getparam to avoid 2 atomis is a bit much.

Reviewed-by: Maarten Lankhorst 
Signed-off-by: Daniel Vetter 
Cc: Jon Bloomfield 
Cc: Chris Wilson 
Cc: Maarten Lankhorst 
Cc: Joonas Lahtinen 
Cc: Daniel Vetter 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Cc: Lionel Landwerlin 
Cc: Dave Airlie 
Cc: Jason Ekstrand 
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 12e2de1db1a2..7a566fb7cca4 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -2108,6 +2108,7 @@ int i915_gem_context_getparam_ioctl(struct drm_device 
*dev, void *data,
struct drm_i915_file_private *file_priv = file->driver_priv;
struct drm_i915_gem_context_param *args = data;
struct i915_gem_context *ctx;
+   struct i915_address_space *vm;
int ret = 0;
 
ctx = i915_gem_context_lookup(file_priv, args->ctx_id);
@@ -2117,12 +2118,10 @@ int i915_gem_context_getparam_ioctl(struct drm_device 
*dev, void *data,
switch (args->param) {
case I915_CONTEXT_PARAM_GTT_SIZE:
args->size = 0;
-   rcu_read_lock();
-   if (rcu_access_pointer(ctx->vm))
-   args->value = rcu_dereference(ctx->vm)->total;
-   else
-   args->value = to_i915(dev)->ggtt.vm.total;
-   rcu_read_unlock();
+   vm = i915_gem_context_get_eb_vm(ctx);
+   args->value = vm->total;
+   i915_vm_put(vm);
+
break;
 
case I915_CONTEXT_PARAM_NO_ERROR_CAPTURE:
-- 
2.33.0

[PATCH 08/11] drm/i915: Use i915_gem_context_get_eb_vm in intel_context_set_gem

Since

commit ccbc1b97948ab671335e950271e39766729736c3
Author: Jason Ekstrand 
Date:   Thu Jul 8 10:48:30 2021 -0500

drm/i915/gem: Don't allow changing the VM on running contexts (v4)

the gem_ctx->vm can't change anymore. Plus we always set the
intel_context->vm, so might as well use the helper we have for that.

This makes it very clear that we always overwrite intel_context->vm
for userspace contexts, since the default is gt->vm, which is
explicitly reserved for kernel context use. It would be good to split
things up a bit further and avoid any possibility for an accident
where we run kernel stuff in userspace vm or the other way round.

Reviewed-by: Maarten Lankhorst 
Signed-off-by: Daniel Vetter 
Cc: Jon Bloomfield 
Cc: Chris Wilson 
Cc: Maarten Lankhorst 
Cc: Joonas Lahtinen 
Cc: Daniel Vetter 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Cc: Lionel Landwerlin 
Cc: Dave Airlie 
Cc: Jason Ekstrand 
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 1eec85944c1f..18e23d9220ae 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -791,16 +791,8 @@ static int intel_context_set_gem(struct intel_context *ce,
 
ce->ring_size = SZ_16K;
 
-   if (rcu_access_pointer(ctx->vm)) {
-   struct i915_address_space *vm;
-
-   rcu_read_lock();
-   vm = context_get_vm_rcu(ctx); /* hmm */
-   rcu_read_unlock();
-
-   i915_vm_put(ce->vm);
-   ce->vm = vm;
-   }
+   i915_vm_put(ce->vm);
+   ce->vm = i915_gem_context_get_eb_vm(ctx);
 
if (ctx->sched.priority >= I915_PRIORITY_NORMAL &&
intel_engine_has_timeslices(ce->engine) &&
-- 
2.33.0

[PATCH 07/11] drm/i915: Add i915_gem_context_is_full_ppgtt

And use it anywhere we have open-coded checks for ctx->vm that really
only check for full ppgtt.

Plus for paranoia add a GEM_BUG_ON that checks it's really only set
when we have full ppgtt, just in case. gem_context->vm is different
since it's NULL in ggtt mode, unlike intel_context->vm or gt->vm,
which is always set.

v2: 0day found a testcase that I missed.

Reviewed-by: Maarten Lankhorst 
Signed-off-by: Daniel Vetter 
Cc: Jon Bloomfield 
Cc: Chris Wilson 
Cc: Maarten Lankhorst 
Cc: Joonas Lahtinen 
Cc: Daniel Vetter 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Cc: Lionel Landwerlin 
Cc: Dave Airlie 
Cc: Jason Ekstrand 
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c   | 2 +-
 drivers/gpu/drm/i915/gem/i915_gem_context.h   | 7 +++
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +-
 drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 6 +++---
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 7a566fb7cca4..1eec85944c1f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1566,7 +1566,7 @@ static int get_ppgtt(struct drm_i915_file_private 
*file_priv,
int err;
u32 id;
 
-   if (!rcu_access_pointer(ctx->vm))
+   if (!i915_gem_context_is_full_ppgtt(ctx))
return -ENODEV;
 
rcu_read_lock();
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h 
b/drivers/gpu/drm/i915/gem/i915_gem_context.h
index da6e8b506d96..37536a260e6e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
@@ -154,6 +154,13 @@ i915_gem_context_vm(struct i915_gem_context *ctx)
return rcu_dereference_protected(ctx->vm, lockdep_is_held(>mutex));
 }
 
+static inline bool i915_gem_context_is_full_ppgtt(struct i915_gem_context *ctx)
+{
+   GEM_BUG_ON(!!rcu_access_pointer(ctx->vm) != HAS_FULL_PPGTT(ctx->i915));
+
+   return !!rcu_access_pointer(ctx->vm);
+}
+
 static inline struct i915_address_space *
 i915_gem_context_get_eb_vm(struct i915_gem_context *ctx)
 {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c 
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 905b1cbd22d5..40f08948f0b2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -749,7 +749,7 @@ static int eb_select_context(struct i915_execbuffer *eb)
return PTR_ERR(ctx);
 
eb->gem_context = ctx;
-   if (rcu_access_pointer(ctx->vm))
+   if (i915_gem_context_is_full_ppgtt(ctx))
eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
 
return 0;
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
index fc7fb33a3a52..947154e445a7 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
@@ -704,7 +704,7 @@ static int igt_ctx_exec(void *arg)
pr_err("Failed to fill dword %lu [%lu/%lu] with 
gpu (%s) [full-ppgtt? %s], err=%d\n",
   ndwords, dw, max_dwords(obj),
   engine->name,
-  yesno(!!rcu_access_pointer(ctx->vm)),
+  
yesno(i915_gem_context_is_full_ppgtt(ctx)),
   err);
intel_context_put(ce);
kernel_context_close(ctx);
@@ -838,7 +838,7 @@ static int igt_shared_ctx_exec(void *arg)
pr_err("Failed to fill dword %lu [%lu/%lu] with 
gpu (%s) [full-ppgtt? %s], err=%d\n",
   ndwords, dw, max_dwords(obj),
   engine->name,
-  yesno(!!rcu_access_pointer(ctx->vm)),
+  
yesno(i915_gem_context_is_full_ppgtt(ctx)),
   err);
intel_context_put(ce);
kernel_context_close(ctx);
@@ -1417,7 +1417,7 @@ static int igt_ctx_readonly(void *arg)
pr_err("Failed to fill dword %lu [%lu/%lu] with 
gpu (%s) [full-ppgtt? %s], err=%d\n",
   ndwords, dw, max_dwords(obj),
   ce->engine->name,
-  yesno(!!ctx_vm(ctx)),
+  
yesno(i915_gem_context_is_full_ppgtt(ctx)),
   err);
i915_gem_context_unlock_engines(ctx);
goto out_file;
-- 
2.33.0

[PATCH 04/11] drm/i915: Drop code to handle set-vm races from execbuf

Changing the vm from a finalized gem ctx is no longer possible, which
means we don't have to check for that anymore.

I was pondering whether to keep the check as a WARN_ON, but things go
boom real bad real fast if the vm of a vma is wrong. Plus we'd need to
also get the ggtt vm for !full-ppgtt platforms. Ditching it all seemed
like a better idea.

Reviewed-by: Maarten Lankhorst 
References: ccbc1b97948a ("drm/i915/gem: Don't allow changing the VM on running 
contexts (v4)")
Signed-off-by: Daniel Vetter 
Cc: Jon Bloomfield 
Cc: Chris Wilson 
Cc: Maarten Lankhorst 
Cc: Joonas Lahtinen 
Cc: Daniel Vetter 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Cc: Lionel Landwerlin 
Cc: Dave Airlie 
Cc: Jason Ekstrand 
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c 
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index e809aca00f72..905b1cbd22d5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -775,11 +775,7 @@ static int __eb_add_lut(struct i915_execbuffer *eb,
/* Check that the context hasn't been closed in the meantime */
err = -EINTR;
if (!mutex_lock_interruptible(>lut_mutex)) {
-   struct i915_address_space *vm = rcu_access_pointer(ctx->vm);
-
-   if (unlikely(vm && vma->vm != vm))
-   err = -EAGAIN; /* user racing with ctx set-vm */
-   else if (likely(!i915_gem_context_is_closed(ctx)))
+   if (likely(!i915_gem_context_is_closed(ctx)))
err = radix_tree_insert(>handles_vma, handle, vma);
else
err = -ENOENT;
-- 
2.33.0

[PATCH 05/11] drm/i915: Rename i915_gem_context_get_vm_rcu to i915_gem_context_get_eb_vm

The important part isn't so much that this does an rcu lookup - that's
more an implementation detail, which will also be removed.

The thing that makes this different from other functions is that it's
gettting you the vm that batchbuffers will run in for that gem
context, which is either a full ppgtt stored in gem->ctx, or the ggtt.

We'll make more use of this function later on.

Reviewed-by: Maarten Lankhorst 
Signed-off-by: Daniel Vetter 
Cc: Jon Bloomfield 
Cc: Chris Wilson 
Cc: Maarten Lankhorst 
Cc: Joonas Lahtinen 
Cc: Daniel Vetter 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Cc: Lionel Landwerlin 
Cc: Dave Airlie 
Cc: Jason Ekstrand 
---
 drivers/gpu/drm/i915/gem/i915_gem_context.h   | 2 +-
 drivers/gpu/drm/i915/gem/selftests/huge_pages.c   | 4 ++--
 drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 4 ++--
 drivers/gpu/drm/i915/gt/selftest_execlists.c  | 2 +-
 drivers/gpu/drm/i915/gt/selftest_hangcheck.c  | 2 +-
 drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 4 ++--
 drivers/gpu/drm/i915/selftests/i915_vma.c | 2 +-
 7 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h 
b/drivers/gpu/drm/i915/gem/i915_gem_context.h
index 18060536b0c2..da6e8b506d96 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
@@ -155,7 +155,7 @@ i915_gem_context_vm(struct i915_gem_context *ctx)
 }
 
 static inline struct i915_address_space *
-i915_gem_context_get_vm_rcu(struct i915_gem_context *ctx)
+i915_gem_context_get_eb_vm(struct i915_gem_context *ctx)
 {
struct i915_address_space *vm;
 
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c 
b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
index a094f3ce1a90..6c68fe26bb32 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
@@ -1456,7 +1456,7 @@ static int igt_tmpfs_fallback(void *arg)
struct i915_gem_context *ctx = arg;
struct drm_i915_private *i915 = ctx->i915;
struct vfsmount *gemfs = i915->mm.gemfs;
-   struct i915_address_space *vm = i915_gem_context_get_vm_rcu(ctx);
+   struct i915_address_space *vm = i915_gem_context_get_eb_vm(ctx);
struct drm_i915_gem_object *obj;
struct i915_vma *vma;
u32 *vaddr;
@@ -1512,7 +1512,7 @@ static int igt_shrink_thp(void *arg)
 {
struct i915_gem_context *ctx = arg;
struct drm_i915_private *i915 = ctx->i915;
-   struct i915_address_space *vm = i915_gem_context_get_vm_rcu(ctx);
+   struct i915_address_space *vm = i915_gem_context_get_eb_vm(ctx);
struct drm_i915_gem_object *obj;
struct i915_gem_engines_iter it;
struct intel_context *ce;
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
index 4d2758718d21..fc7fb33a3a52 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
@@ -1528,7 +1528,7 @@ static int write_to_scratch(struct i915_gem_context *ctx,
 
intel_gt_chipset_flush(engine->gt);
 
-   vm = i915_gem_context_get_vm_rcu(ctx);
+   vm = i915_gem_context_get_eb_vm(ctx);
vma = i915_vma_instance(obj, vm, NULL);
if (IS_ERR(vma)) {
err = PTR_ERR(vma);
@@ -1607,7 +1607,7 @@ static int read_from_scratch(struct i915_gem_context *ctx,
if (GRAPHICS_VER(i915) >= 8) {
const u32 GPR0 = engine->mmio_base + 0x600;
 
-   vm = i915_gem_context_get_vm_rcu(ctx);
+   vm = i915_gem_context_get_eb_vm(ctx);
vma = i915_vma_instance(obj, vm, NULL);
if (IS_ERR(vma)) {
err = PTR_ERR(vma);
diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c 
b/drivers/gpu/drm/i915/gt/selftest_execlists.c
index f12ffe797639..b3863abc51f5 100644
--- a/drivers/gpu/drm/i915/gt/selftest_execlists.c
+++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c
@@ -3493,7 +3493,7 @@ static int smoke_submit(struct preempt_smoke *smoke,
if (batch) {
struct i915_address_space *vm;
 
-   vm = i915_gem_context_get_vm_rcu(ctx);
+   vm = i915_gem_context_get_eb_vm(ctx);
vma = i915_vma_instance(batch, vm, NULL);
i915_vm_put(vm);
if (IS_ERR(vma))
diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c 
b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
index 2c1ed32ca5ac..8be23e0f9306 100644
--- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
+++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
@@ -117,7 +117,7 @@ static struct i915_request *
 hang_create_request(struct hang *h, struct intel_engine_cs *engine)
 {
struct intel_gt *gt = h->gt;
-   struct i915_address_space *vm = i915_gem_context_get_vm_rcu(h->ctx);
+   struct i915_address_space *vm =

[PATCH 03/11] drm/i915: Keep gem ctx->vm alive until the final put

The comment added in

commit b81dde719439c8f09bb61e742ed95bfc4b33946b
Author: Chris Wilson 
Date:   Tue May 21 22:11:29 2019 +0100

drm/i915: Allow userspace to clone contexts on creation

and moved in

commit 27dbae8f36c1c25008b7885fc07c57054b7dfba3
Author: Chris Wilson 
Date:   Wed Nov 6 09:13:12 2019 +

drm/i915/gem: Safely acquire the ctx->vm when copying

suggested that i915_address_space were at least intended to be managed
through SLAB_TYPESAFE_BY_RCU:

* This ppgtt may have be reallocated between
* the read and the kref, and reassigned to a third
* context. In order to avoid inadvertent sharing
* of this ppgtt with that third context (and not
* src), we have to confirm that we have the same
* ppgtt after passing through the strong memory
* barrier implied by a successful
* kref_get_unless_zero().

But extensive git history search has not brough any such reuse to
light.

What has come to light though is that ever since

commit 2850748ef8763ab46958e43a4d1c445f29eeb37d
Author: Chris Wilson 
Date:   Fri Oct 4 14:39:58 2019 +0100

drm/i915: Pull i915_vma_pin under the vm->mutex

(yes this commit is earlier) the final i915_vma_put call has been
moved from i915_gem_context_free (now called _release) to
context_close, which means it's not actually safe anymore to access
the ctx->vm pointer without lock helds, because it might disappear at
any moment. Note that superficially things all still work, because the
i915_address_space is RCU protected since

commit b32fa811156328aea5a3c2ff05cc096490382456
Author: Chris Wilson 
Date:   Thu Jun 20 19:37:05 2019 +0100

drm/i915/gtt: Defer address space cleanup to an RCU worker

except the very clever macro above (which is designed to protected
against object reuse due to SLAB_TYPESAFE_BY_RCU or similar tricks)
results in an endless loop if the refcount of the ctx->vm ever
permanently drops to 0. Which it totally now can.

Fix that by moving the final i915_vm_put to where it should be.

Note that i915_gem_context is rcu protected, but _only_ the final
kfree. This means anyone who chases a pointer to a gem ctx solely
under the protection can pretty only call kref_get_unless_zero(). This
seems to be pretty much the case, aside from a bunch of cases that
consult the scheduling information without any further protection.

Reviewed-by: Maarten Lankhorst 
Cc: Jason Ekstrand 
Cc: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
Cc: Matthew Brost 
Cc: Matthew Auld 
Cc: Maarten Lankhorst 
Cc: "Thomas Hellström" 
Cc: Lionel Landwerlin 
Cc: Dave Airlie 
Fixes: 2850748ef876 ("drm/i915: Pull i915_vma_pin under the vm->mutex")
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 5a053cf14948..12e2de1db1a2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -990,6 +990,7 @@ static void i915_gem_context_release_work(struct 
work_struct *work)
 {
struct i915_gem_context *ctx = container_of(work, typeof(*ctx),
release_work);
+   struct i915_address_space *vm;
 
trace_i915_context_free(ctx);
GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
@@ -997,6 +998,10 @@ static void i915_gem_context_release_work(struct 
work_struct *work)
if (ctx->syncobj)
drm_syncobj_put(ctx->syncobj);
 
+   vm = i915_gem_context_vm(ctx);
+   if (vm)
+   i915_vm_put(vm);
+
mutex_destroy(>engines_mutex);
mutex_destroy(>lut_mutex);
 
@@ -1220,8 +1225,15 @@ static void context_close(struct i915_gem_context *ctx)
set_closed_name(ctx);
 
vm = i915_gem_context_vm(ctx);
-   if (vm)
+   if (vm) {
+   /* i915_vm_close drops the final reference, which is a bit too
+* early and could result in surprises with concurrent
+* operations racing with thist ctx close. Keep a full reference
+* until the end.
+*/
+   i915_vm_get(vm);
i915_vm_close(vm);
+   }
 
ctx->file_priv = ERR_PTR(-EBADF);
 
-- 
2.33.0

[PATCH 02/11] drm/i915: Release ctx->syncobj on final put, not on ctx close

gem context refcounting is another exercise in least locking design it
seems, where most things get destroyed upon context closure (which can
race with anything really). Only the actual memory allocation and the
locks survive while holding a reference.

This tripped up Jason when reimplementing the single timeline feature
in

commit 00dae4d3d35d4f526929633b76e00b0ab4d3970d
Author: Jason Ekstrand 
Date:   Thu Jul 8 10:48:12 2021 -0500

drm/i915: Implement SINGLE_TIMELINE with a syncobj (v4)

We could fix the bug by holding ctx->mutex in execbuf and clear the
pointer (again while holding the mutex) context_close, but it's
cleaner to just make the context object actually invariant over its
_entire_ lifetime. This way any other ioctl that's potentially racing,
but holding a full reference, can still rely on ctx->syncobj being
an immutable pointer. Which without this change, is not the case.

Reviewed-by: Maarten Lankhorst 
Signed-off-by: Daniel Vetter 
Fixes: 00dae4d3d35d ("drm/i915: Implement SINGLE_TIMELINE with a syncobj (v4)")
Cc: Jason Ekstrand 
Cc: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
Cc: Matthew Brost 
Cc: Matthew Auld 
Cc: Maarten Lankhorst 
Cc: "Thomas Hellström" 
Cc: Lionel Landwerlin 
Cc: Dave Airlie 
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 051bc357ff65..5a053cf14948 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -994,6 +994,9 @@ static void i915_gem_context_release_work(struct 
work_struct *work)
trace_i915_context_free(ctx);
GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
 
+   if (ctx->syncobj)
+   drm_syncobj_put(ctx->syncobj);
+
mutex_destroy(>engines_mutex);
mutex_destroy(>lut_mutex);
 
@@ -1220,9 +1223,6 @@ static void context_close(struct i915_gem_context *ctx)
if (vm)
i915_vm_close(vm);
 
-   if (ctx->syncobj)
-   drm_syncobj_put(ctx->syncobj);
-
ctx->file_priv = ERR_PTR(-EBADF);
 
/*
-- 
2.33.0

[PATCH 01/11] drm/i915: Release i915_gem_context from a worker

The only reason for this really is the i915_gem_engines->fence
callback engines_notify(), which exists purely as a fairly funky
reference counting scheme for that. Otherwise all other callers are
from process context, and generally fairly benign locking context.

Unfortunately untangling that requires some major surgery, and we have
a few i915_gem_context reference counting bugs that need fixing, and
they blow in the current hardirq calling context, so we need a
stop-gap measure.

Put a FIXME comment in when this should be removable again.

v2: Fix mock_context(), noticed by intel-gfx-ci.

Acked-by: Acked-by: Maarten Lankhorst 
Signed-off-by: Daniel Vetter 
Cc: Jon Bloomfield 
Cc: Chris Wilson 
Cc: Maarten Lankhorst 
Cc: Joonas Lahtinen 
Cc: Daniel Vetter 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Cc: Lionel Landwerlin 
Cc: Dave Airlie 
Cc: Jason Ekstrand 
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c   | 13 +++--
 drivers/gpu/drm/i915/gem/i915_gem_context_types.h | 12 
 drivers/gpu/drm/i915/gem/selftests/mock_context.c |  1 +
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index fd169cf2f75a..051bc357ff65 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -986,9 +986,10 @@ static struct i915_gem_engines *user_engines(struct 
i915_gem_context *ctx,
return err;
 }
 
-void i915_gem_context_release(struct kref *ref)
+static void i915_gem_context_release_work(struct work_struct *work)
 {
-   struct i915_gem_context *ctx = container_of(ref, typeof(*ctx), ref);
+   struct i915_gem_context *ctx = container_of(work, typeof(*ctx),
+   release_work);
 
trace_i915_context_free(ctx);
GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
@@ -1002,6 +1003,13 @@ void i915_gem_context_release(struct kref *ref)
kfree_rcu(ctx, rcu);
 }
 
+void i915_gem_context_release(struct kref *ref)
+{
+   struct i915_gem_context *ctx = container_of(ref, typeof(*ctx), ref);
+
+   queue_work(ctx->i915->wq, >release_work);
+}
+
 static inline struct i915_gem_engines *
 __context_engines_static(const struct i915_gem_context *ctx)
 {
@@ -1303,6 +1311,7 @@ i915_gem_create_context(struct drm_i915_private *i915,
ctx->sched = pc->sched;
mutex_init(>mutex);
INIT_LIST_HEAD(>link);
+   INIT_WORK(>release_work, i915_gem_context_release_work);
 
spin_lock_init(>stale.lock);
INIT_LIST_HEAD(>stale.engines);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h 
b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
index 94c03a97cb77..0c38789bd4a8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
@@ -288,6 +288,18 @@ struct i915_gem_context {
 */
struct kref ref;
 
+   /**
+* @release_work:
+*
+* Work item for deferred cleanup, since i915_gem_context_put() tends to
+* be called from hardirq context.
+*
+* FIXME: The only real reason for this is _gem_engines.fence, all
+* other callers are from process context and need at most some mild
+* shuffling to pull the i915_gem_context_put() call out of a spinlock.
+*/
+   struct work_struct release_work;
+
/**
 * @rcu: rcu_head for deferred freeing.
 */
diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c 
b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
index fee070df1c97..067d68a6fe4c 100644
--- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
@@ -23,6 +23,7 @@ mock_context(struct drm_i915_private *i915,
kref_init(>ref);
INIT_LIST_HEAD(>link);
ctx->i915 = i915;
+   INIT_WORK(>release_work, i915_gem_context_release_work);
 
mutex_init(>mutex);
 
-- 
2.33.0

[Bug 211277] sometimes crash at s2ram-wake (Ryzen 3500U): amdgpu, drm, commit_tail, amdgpu_dm_atomic_commit_tail

https://bugzilla.kernel.org/show_bug.cgi?id=211277

--- Comment #43 from kolAflash (kolafl...@kolahilft.de) ---
(In reply to James Zhu from comment #42)
> Hi Jerome and kolAflash,
> 
> Thanks for confirmation. I have a workaround for this issue. But I wish I
> can find the root cause or better workaround.

Thanks too for your help James!

For me personally the situation is quite fine with pci=noats.
I'm sometimes using Qemu/KVM and VirtualBox. But no need for absolute bleeding
edge VM performance. So I'll probably be fine with pci=noats.

However, I'd love to contribute to a fix for all users without kernel parameter
stuff.
(including a fix in longterm Linux-5.10 for Debian)
So just tell me if I can help by doing more tests, sending logs, ... :-)

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

Re: [PATCH 2/7] drm/format-helper: Add drm_fb_xrgb8888_to_rgb332()

2021-09-02 Thread Noralf Trønnes




Den 31.08.2021 14.23, skrev Daniel Vetter:
> On Mon, Aug 30, 2021 at 02:08:14PM +0200, Noralf Trønnes wrote:
>>
>>
>> Den 17.08.2021 15.56, skrev Daniel Vetter:
>>> On Tue, Aug 17, 2021 at 02:29:12PM +0200, Noralf Trønnes wrote:
 Add XRGB emulation support for devices that can only do RGB332.

 Cc: Thomas Zimmermann 
 Signed-off-by: Noralf Trønnes 
 ---
  drivers/gpu/drm/drm_format_helper.c | 47 +
  include/drm/drm_format_helper.h |  2 ++
  2 files changed, 49 insertions(+)

 diff --git a/drivers/gpu/drm/drm_format_helper.c 
 b/drivers/gpu/drm/drm_format_helper.c
 index 5231104b1498..53b426da7467 100644
 --- a/drivers/gpu/drm/drm_format_helper.c
 +++ b/drivers/gpu/drm/drm_format_helper.c
 @@ -135,6 +135,53 @@ void drm_fb_swab(void *dst, void *src, struct 
 drm_framebuffer *fb,
  }
  EXPORT_SYMBOL(drm_fb_swab);
  
 +static void drm_fb_xrgb_to_rgb332_line(u8 *dbuf, u32 *sbuf, unsigned 
 int pixels)
 +{
 +  unsigned int x;
 +
 +  for (x = 0; x < pixels; x++)
 +  dbuf[x] = ((sbuf[x] & 0x00e0) >> 16) |
>>>
>>> I think for 2/3 bits correct rounding would be useful, not just masking.
>>> i.e. before you shift add 0x0010 here, and similar below.
>>>
>>
>> Math isn't my strongest side and my brain failed to turn this into code.
> 
> Essentially add half of the lowest bit before you mask, so
> 
> ((sbuf[x] + 0x10) & 0xe0 )
> 

But what if the value is 0xff, it overflows:

((0xff + 0x10) & 0xe0 ) == 0x00

Should it be OR?

((0xff | 0x10) & 0xe0 ) == 0xe0

Noralf.

> I dropped the shift to make it clear what's going on. If you're mask is
> 0xc0, then you simply add 0x20 before masking.
> 
>>> Also I just realized we've totally ignored endianess on these, which is
>>> not great, because strictly speaking all the drm_fourcc codes should be
>>> little endian. But I'm also not sure that's worth fixing ...
>>>
>>
>> Is it as simple as using le32_to_cpu()?
> 
> I think so.
> 
> Plus on any format that has u16 output we'd need a cpu_to_le16 wrapped
> around the entire thing.
> -Daniel
> 
>> static void drm_fb_xrgb_to_rgb332_line(u8 *dbuf, __le32 *sbuf,
>> unsigned int pixels)
>> {
>>  unsigned int x;
>>  u32 pix;
>>
>>  for (x = 0; x < pixels; x++) {
>>  pix = le32_to_cpu(sbuf[x]);
>>  dbuf[x] = ((pix & 0x00e0) >> 16) |
>>((pix & 0xe000) >> 11) |
>>((pix & 0x00c0) >> 6);
>>  }
>> }
>>
>> Noralf.
>>
>>> Either way, lgtm:
>>>
>>> Reviewed-by: Daniel Vetter 
>>>
 +((sbuf[x] & 0xe000) >> 11) |
 +((sbuf[x] & 0x00c0) >> 6);
 +}
 +
 +/**
 + * drm_fb_xrgb_to_rgb332 - Convert XRGB to RGB332 clip buffer
 + * @dst: RGB332 destination buffer
 + * @src: XRGB source buffer
 + * @fb: DRM framebuffer
 + * @clip: Clip rectangle area to copy
 + *
 + * Drivers can use this function for RGB332 devices that don't natively 
 support XRGB.
 + *
 + * This function does not apply clipping on dst, i.e. the destination is 
 a small buffer
 + * containing the clip rect only.
 + */
 +void drm_fb_xrgb_to_rgb332(void *dst, void *src, struct 
 drm_framebuffer *fb,
 + struct drm_rect *clip)
 +{
 +  size_t width = drm_rect_width(clip);
 +  size_t src_len = width * sizeof(u32);
 +  unsigned int y;
 +  void *sbuf;
 +
 +  /* Use a buffer to speed up access on buffers with uncached read 
 mapping (i.e. WC) */
 +  sbuf = kmalloc(src_len, GFP_KERNEL);
 +  if (!sbuf)
 +  return;
 +
 +  src += clip_offset(clip, fb->pitches[0], sizeof(u32));
 +  for (y = 0; y < drm_rect_height(clip); y++) {
 +  memcpy(sbuf, src, src_len);
 +  drm_fb_xrgb_to_rgb332_line(dst, sbuf, width);
 +  src += fb->pitches[0];
 +  dst += width;
 +  }
 +
 +  kfree(sbuf);
 +}
 +EXPORT_SYMBOL(drm_fb_xrgb_to_rgb332);
 +
  static void drm_fb_xrgb_to_rgb565_line(u16 *dbuf, u32 *sbuf,
   unsigned int pixels,
   bool swab)
 diff --git a/include/drm/drm_format_helper.h 
 b/include/drm/drm_format_helper.h
 index 4e0258a61311..d0809aff5cf8 100644
 --- a/include/drm/drm_format_helper.h
 +++ b/include/drm/drm_format_helper.h
 @@ -16,6 +16,8 @@ void drm_fb_memcpy_dstclip(void __iomem *dst, unsigned 
 int dst_pitch, void *vadd
   struct drm_rect *clip);
  void drm_fb_swab(void *dst, void *src, struct drm_framebuffer *fb,
 struct drm_rect *clip, bool cached);
 +void drm_fb_xrgb_to_rgb332(void *dst, void *vaddr, struct 
 drm_framebuffer *fb,
 +

[PATCH] drm/panfrost: Calculate lock region size correctly

2021-09-02 Thread Steven Price

It turns out that when locking a region, the region must be a naturally
aligned power of 2. The upshot of this is that if the desired region
crosses a 'large boundary' the region size must be increased
significantly to ensure that the locked region completely covers the
desired region. Previous calculations (including in kbase for the
proprietary driver) failed to take this into account.

Since it's known that the lock region must be naturally aligned we can
compute the required size by looking at the highest bit position which
changes between the start/end of the lock region (subtracting 1 from the
end because the end address is exclusive). The start address is then
aligned based on the size (this is technically unnecessary as the
hardware will ignore these bits, but the spec advises to do this "to
avoid confusion").

Signed-off-by: Steven Price 
---
See previous discussion[1] for more details. This bug also existed in
the 'kbase' driver, so it's unlikely to actually hit very often.

This patch is based on drm-misc-next-fixes as it builds on top of
Alyssa's changes to lock_region.

[1] 
https://lore.kernel.org/dri-devel/6fe675c4-d22b-22da-ba3c-f6d33419b...@arm.com/

 drivers/gpu/drm/panfrost/panfrost_mmu.c | 33 +++--
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c 
b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index dfe5f1d29763..afec15bb3db5 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -58,17 +58,36 @@ static int write_cmd(struct panfrost_device *pfdev, u32 
as_nr, u32 cmd)
 }
 
 static void lock_region(struct panfrost_device *pfdev, u32 as_nr,
-   u64 iova, u64 size)
+   u64 region_start, u64 size)
 {
u8 region_width;
-   u64 region = iova & PAGE_MASK;
+   u64 region;
+   u64 region_size;
+   u64 region_end = region_start + size;
 
-   /* The size is encoded as ceil(log2) minus(1), which may be calculated
-* with fls. The size must be clamped to hardware bounds.
+   if (!size)
+   return;
+
+   /*
+* The locked region is a naturally aligned power of 2 block encoded as
+* log2 minus(1).
+* Calculate the desired start/end and look for the highest bit which
+* differs. The smallest naturally aligned block must include this bit
+* change the desired region starts with this bit (and subsequent bits)
+* zeroed and ends with the bit (and subsequent bits) set to one.
+*
 */
-   size = max_t(u64, size, AS_LOCK_REGION_MIN_SIZE);
-   region_width = fls64(size - 1) - 1;
-   region |= region_width;
+   region_size = region_start ^ (region_end - 1);
+   region_width = max(fls64(region_size),
+  const_ilog2(AS_LOCK_REGION_MIN_SIZE)) - 1;
+
+   /*
+* Mask off the low bits of region_start (which would be ignored by
+* the hardware anyway)
+*/
+   region_start &= GENMASK_ULL(63, region_width);
+
+   region = region_width | region_start;
 
/* Lock the region that needs to be updated */
mmu_write(pfdev, AS_LOCKADDR_LO(as_nr), region & 0xUL);
-- 
2.30.2

Re: [diagnostic TDR mode patches] unify our solution opinions/suggestions in one thread

2021-09-02 Thread Alex Deucher

On Thu, Sep 2, 2021 at 1:52 AM Liu, Monk  wrote:
>
> [AMD Official Use Only]
>
> >>>
> I'm not sure I can add much to help this along, I'm sure Alex has some 
> internal training,
> Once your driver is upstream, it belongs to upstream, you can maintain it, 
> but you no longer control it 100%, it's a tradeoff, it's not one companies 
> always understand.
> Usually people are fine developing away internally, but once interaction with 
> other parts of the kernel/subsystem is required they have the realisation 
> that they needed to work upstream 6 months earlier.
> The best time to interact with upstream was 6 months ago, the second best 
> time is now.
> <<<
>
> Daniel/AlexD
>
> I didn't mean your changes on AMD driver need my personal approval or review 
> ... and  I'm totally already get used that our driver is not 100% under 
> control by AMDers,
> but supposedly any one from community (including you) who tend to change 
> AMD's driver need at least to get approvement from someone in AMD, e.g.: 
> AlexD or Christian, doesn't that reasonable?
> just like we need your approve if we try to modify DRM-sched, or need 
> panfrost's approval if we need to change panfrost code ...
>
> by only CC AMD's engineers looks not quite properly, how do you know if your 
> changes (on AMD code part) are conflicting with AMD's on-going internal 
> features/refactoring or not ?
>

We keep as up to date as possible with upstream.  I don't have the
bandwidth to verify every patch, but in most cases I try and look at
them.  In your first example, the patch basically just adds a new
parameter to some common functions.  Drivers that don't need that
parameter don't use it.  It shouldn't really affect the functionality.
There are lots of changes that touch our driver that we are largely
not aware of.  E.g., APIs that we may use may change the function
signatures with no intended functional changes.  If problems are found
they are reported and resolved.  It is a collective effort.  If there
are changes that would conflict with stuff we are doing in our tree we
should bring them up when the relevant patches are being discussed.
We can also make changes to core functionality like scheduler, ttm,
etc. that would affect other drivers.  When we send out the patches we
cc the relevant maintainers, but ultimately the ones who participate
in the discussion set the direction.  That's why participation is
important.

Alex


> Thanks
>
> --
> Monk Liu | Cloud-GPU Core team
> --
>
> -Original Message-
> From: Dave Airlie 
> Sent: Thursday, September 2, 2021 2:51 AM
> To: Alex Deucher 
> Cc: Liu, Monk ; Daniel Vetter ; Koenig, 
> Christian ; Grodzovsky, Andrey 
> ; Chen, JingWen ; DRI 
> Development ; amd-...@lists.freedesktop.org
> Subject: Re: [diagnostic TDR mode patches] unify our solution 
> opinions/suggestions in one thread
>
> On Thu, 2 Sept 2021 at 01:20, Alex Deucher  wrote:
> >
> > On Wed, Sep 1, 2021 at 6:19 AM Liu, Monk  wrote:
> > >
> > > [AMD Official Use Only]
> > >
> > > Daniel
> > >
> > > From the link you share it looks you(or someone else) have quite a bunch 
> > > patches that changes DRM_SCHED or even amdgpu, by that case before they 
> > > are merged to kernel tree I'm wondering if any AMD develop reviewed them ?
> > >
> > > They looks to me somehow conflicting with what we changed in our repo
> > >
> > > It is really a chaos for AMDer if someone else out side of AMD changes 
> > > our kernel driver (or/and scheduler) without reviewed by AMDer, just like 
> > > we are requiring your review if we tend to change scheduler's logic here 
> > > 
> > >
> > > This one changes AMD's code:
> > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
> > > re.kernel.org%2Fdri-devel%2F20210625133327.2598825-2-boris.brezillon
> > > %40collabora.com%2Fdata=04%7C01%7CMonk.Liu%40amd.com%7C6c507d18
> > > d65341ef53bb08d96d7976e6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%
> > > 7C637661190727875969%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > > QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=BWJSkKN
> > > y2%2BwjxbQrfxGPzuJ5PBpBwB4aV0ZH6QoJGEg%3Dreserved=0
> > > And I didn't see any reviewed-by from AMDers ...
> > >
> > > This one also touches AMD's code:
> > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
> > > re.kernel.org%2Fdri-devel%2F20200604081224.863494-12-daniel.vetter%4
> > > 0ffwll.ch%2Fdata=04%7C01%7CMonk.Liu%40amd.com%7C6c507d18d65341e
> > > f53bb08d96d7976e6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63766
> > > 1190727885929%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> > > luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=%2F8vIVXCWjHkM
> > > 56pcYI9EvuzhbsZhV9WczkKaBJE67KQ%3Dreserved=0
> > > Which is conflicting with one patch we submitted (in our repo
> > > rightnow), and neither see AMDder gave a review-by on this one (let
> > > me know if I missed it)
> > >
>

Re: [PATCH 2/5] drm/ttm: add busy and idle placement flags

2021-09-02 Thread Huang Rui

On Tue, Aug 31, 2021 at 07:21:07PM +0800, Christian König wrote:
> More flexible than the busy placements.
> 
> Signed-off-by: Christian König 

Patch 2 -> 5 are Acked-by: Huang Rui 

> ---
>  drivers/gpu/drm/ttm/ttm_bo.c| 8 +++-
>  include/drm/ttm/ttm_placement.h | 6 ++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 0a3127436f61..c7034040c67f 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -834,6 +834,9 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>   const struct ttm_place *place = >placement[i];
>   struct ttm_resource_manager *man;
>  
> + if (place->flags & TTM_PL_FLAG_BUSY)
> + continue;
> +
>   man = ttm_manager_type(bdev, place->mem_type);
>   if (!man || !ttm_resource_manager_used(man))
>   continue;
> @@ -860,6 +863,9 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>   const struct ttm_place *place = >busy_placement[i];
>   struct ttm_resource_manager *man;
>  
> + if (place->flags & TTM_PL_FLAG_IDLE)
> + continue;
> +
>   man = ttm_manager_type(bdev, place->mem_type);
>   if (!man || !ttm_resource_manager_used(man))
>   continue;
> @@ -869,7 +875,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>   if (likely(!ret))
>   return 0;
>  
> - if (ret && ret != -EBUSY)
> + if (ret != -EBUSY)
>   goto error;
>   }
>  
> diff --git a/include/drm/ttm/ttm_placement.h b/include/drm/ttm/ttm_placement.h
> index 8995c9e4ec1b..63f7217354c0 100644
> --- a/include/drm/ttm/ttm_placement.h
> +++ b/include/drm/ttm/ttm_placement.h
> @@ -53,6 +53,12 @@
>  /* For multihop handling */
>  #define TTM_PL_FLAG_TEMPORARY   (1 << 2)
>  
> +/* Placement is only used when we are evicting */
> +#define TTM_PL_FLAG_BUSY (1 << 3)
> +
> +/* Placement is only used when we are not evicting */
> +#define TTM_PL_FLAG_IDLE (1 << 4)
> +
>  /**
>   * struct ttm_place
>   *
> -- 
> 2.25.1
>

Re: [PATCH 4.19] fbmem: add margin check to fb_check_caps()

Hi Dongliang,

On Thu, Sep 2, 2021 at 8:04 AM Dongliang Mu  wrote:
> [ Upstream commit a49145acfb975d921464b84fe00279f99827d816 ]

Oops, looks like I missed when that one was submitted for review...

> A fb_ioctl() FBIOPUT_VSCREENINFO call with invalid xres setting
> or yres setting in struct fb_var_screeninfo will result in a
> KASAN: vmalloc-out-of-bounds failure in bitfill_aligned() as
> the margins are being cleared. The margins are cleared in
> chunks and if the xres setting or yres setting is a value of
> zero upto the chunk size, the failure will occur.
>
> Add a margin check to validate xres and yres settings.

Shouldn't (the caller of) bitfill_aligned() be fixed instead?
Can this be triggered by e.g. using the mini_4x6 font?

> Note that, this patch needs special handling to backport it to linux
> kernel 4.19, 4.14, 4.9, 4.4.
>
> Signed-off-by: George Kennedy 
> Reported-by: syzbot+e5fd3e65515b48c02...@syzkaller.appspotmail.com
> Reviewed-by: Dan Carpenter 
> Cc: Dhaval Giani 
> Signed-off-by: Bartlomiej Zolnierkiewicz 
> Link: 
> https://patchwork.freedesktop.org/patch/msgid/1594149963-13801-1-git-send-email-george.kenn...@oracle.com
> Signed-off-by: Sasha Levin 
> ---
>  drivers/video/fbdev/core/fbmem.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c 
> b/drivers/video/fbdev/core/fbmem.c
> index 84845275dbef..de04c097d67c 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -991,6 +991,10 @@ fb_set_var(struct fb_info *info, struct 
> fb_var_screeninfo *var)
> goto done;
> }
>
> +   /* bitfill_aligned() assumes that it's at least 8x8 */
> +   if (var->xres < 8 || var->yres < 8)
> +   return -EINVAL;

Are you sure there don't exist such small displays (e.g. OLED)?

> +
> ret = info->fbops->fb_check_var(var, info);
>
> if (ret)

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v2 2/2] clk: qcom: gcc-sdm660: Remove transient global "xo" clock

2021-09-02 Thread Marijn Suijten

On 2021-09-01 20:46:34, Stephen Boyd wrote:
> Quoting Marijn Suijten (2021-09-01 01:57:15)
> > On 2021-08-31 22:35:56, Stephen Boyd wrote:
> > > Quoting Marijn Suijten (2021-08-30 11:24:45)
> > > > The DSI PHY/PLL was relying on a global "xo" clock to be found, but the
> > > > real clock is named "xo_board" in the DT.  The standard nowadays is to
> > > > never use global clock names anymore but require the firmware (DT) to
> > > > provide every clock binding explicitly with .fw_name.  The DSI PLLs have
> > > > since been converted to this mechanism (specifically 14nm for SDM660)
> > > > and this transient clock can now be removed.
> > > > 
> > > > This issue was originally discovered in:
> > > > https://lore.kernel.org/linux-arm-msm/386db1a6-a1cd-3c7d-a88e-dc83f8a1b...@somainline.org/
> > > > and prevented the removal of "xo" at that time.
> > > > 
> > > > Signed-off-by: Marijn Suijten 
> > > > ---
> > > 
> > > Presumably this wants to go with the first one.
> > 
> > What are you referring to with "the first one"?  This patch can only go
> > in after patch 1/2 of this series, unless you are suggesting to squash
> > it with Bjorns cleanup and making sure that lands after the fix in the
> > DSI?
> 
> The first patch in this series.

Are you suggesting to squash this patch into the first patch in this
series?  Note that the first patch touches drm/msm/dsi, the second
(this) patch touches clk/qcom.

[Bug 211277] sometimes crash at s2ram-wake (Ryzen 3500U): amdgpu, drm, commit_tail, amdgpu_dm_atomic_commit_tail

https://bugzilla.kernel.org/show_bug.cgi?id=211277

--- Comment #42 from James Zhu (jam...@amd.com) ---
Hi Jerome and kolAflash,

Thanks for confirmation. I have a workaround for this issue. But I wish I can
find the root cause or better workaround.

James

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

[PATCH 0/3] dma-buf: Add missing dependencies on DMA_SHARED_BUFFER

Hi Sumit, Christian,

This patch series adds missing dependencies on DMA_SHARED_BUFFER to
various options of DMA-BUF, and drops a bogus select.

As drivers/dma-buf/Kconfig contains interleaved options that select or
depend on DMA_SHARED_BUFFER, they can't be wrapped inside a big
"if DMA_SHARED_BUFFER / endif" block.

Thanks!

Geert Uytterhoeven (3):
  dma-buf: DMABUF_MOVE_NOTIFY should depend on DMA_SHARED_BUFFER
  dma-buf: DMABUF_DEBUG should depend on DMA_SHARED_BUFFER
  dma-buf: DMABUF_SYSFS_STATS should depend on DMA_SHARED_BUFFER

 drivers/dma-buf/Kconfig | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

-- 
2.25.1

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[PATCH 2/3] dma-buf: DMABUF_DEBUG should depend on DMA_SHARED_BUFFER

DMA-BUF debug checks are an option of DMA-BUF.  Enabling DMABUF_DEBUG
without DMA_SHARED_BUFFER does not have any impact, as drivers/dma-buf/
is not entered during the build when DMA_SHARED_BUFFER is disabled.

Fixes: 84335675f2223cbd ("dma-buf: Add debug option")
Signed-off-by: Geert Uytterhoeven 
---
 drivers/dma-buf/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index de5a17c40a33d336..d53782f4f51b54f7 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -53,6 +53,7 @@ config DMABUF_MOVE_NOTIFY
 
 config DMABUF_DEBUG
bool "DMA-BUF debug checks"
+   depends on DMA_SHARED_BUFFER
default y if DMA_API_DEBUG
help
  This option enables additional checks for DMA-BUF importers and
-- 
2.25.1

[PATCH 1/3] dma-buf: DMABUF_MOVE_NOTIFY should depend on DMA_SHARED_BUFFER

Move notify between drivers is an option of DMA-BUF.  Enabling
DMABUF_MOVE_NOTIFY without DMA_SHARED_BUFFER does not have any impact,
as drivers/dma-buf/ is not entered during the build when
DMA_SHARED_BUFFER is disabled.

Fixes: bb42df4662a44765 ("dma-buf: add dynamic DMA-buf handling v15")
Signed-off-by: Geert Uytterhoeven 
---
 drivers/dma-buf/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index 9561e3d2d4285d55..de5a17c40a33d336 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -42,6 +42,7 @@ config UDMABUF
 config DMABUF_MOVE_NOTIFY
bool "Move notify between drivers (EXPERIMENTAL)"
default n
+   depends on DMA_SHARED_BUFFER
help
  Don't pin buffers if the dynamic DMA-buf interface is available on
  both the exporter as well as the importer. This fixes a security
-- 
2.25.1

[PATCH 3/3] dma-buf: DMABUF_SYSFS_STATS should depend on DMA_SHARED_BUFFER

DMA-BUF sysfs statistics are an option of DMA-BUF.  It does not make
much sense to bother the user with a question about DMA-BUF sysfs
statistics if DMA-BUF itself is not enabled.  Worse, enabling the
statistics enables the feature.

Fixes: bdb8d06dfefd666d ("dmabuf: Add the capability to expose DMA-BUF stats in 
sysfs")
Signed-off-by: Geert Uytterhoeven 
---
 drivers/dma-buf/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index d53782f4f51b54f7..541efe01abc7c432 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -76,7 +76,7 @@ menuconfig DMABUF_HEAPS
 
 menuconfig DMABUF_SYSFS_STATS
bool "DMA-BUF sysfs statistics"
-   select DMA_SHARED_BUFFER
+   depends on DMA_SHARED_BUFFER
help
   Choose this option to enable DMA-BUF sysfs statistics
   in location /sys/kernel/dmabuf/buffers.
-- 
2.25.1

Re: [PATCH 1/5] drm/ttm: cleanup ttm_resource_compat

2021-09-02 Thread Huang Rui

On Tue, Aug 31, 2021 at 07:21:06PM +0800, Christian König wrote:
> Move that function into the resource handling and remove an unused parameter.
> 
> Signed-off-by: Christian König 

Looks this patch is separate from others in this series.

new_flags won't be used anymore.

Reviewed-by: Huang Rui 

> ---
>  drivers/gpu/drm/ttm/ttm_bo.c   | 48 +
>  drivers/gpu/drm/ttm/ttm_resource.c | 49 ++
>  drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 15 -
>  include/drm/ttm/ttm_bo_api.h   | 12 
>  include/drm/ttm/ttm_resource.h |  3 ++
>  5 files changed, 59 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 3573f9e393be..0a3127436f61 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -924,57 +924,11 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object 
> *bo,
>   return ret;
>  }
>  
> -static bool ttm_bo_places_compat(const struct ttm_place *places,
> -  unsigned num_placement,
> -  struct ttm_resource *mem,
> -  uint32_t *new_flags)
> -{
> - unsigned i;
> -
> - if (mem->placement & TTM_PL_FLAG_TEMPORARY)
> - return false;
> -
> - for (i = 0; i < num_placement; i++) {
> - const struct ttm_place *heap = [i];
> -
> - if ((mem->start < heap->fpfn ||
> -  (heap->lpfn != 0 && (mem->start + mem->num_pages) > 
> heap->lpfn)))
> - continue;
> -
> - *new_flags = heap->flags;
> - if ((mem->mem_type == heap->mem_type) &&
> - (!(*new_flags & TTM_PL_FLAG_CONTIGUOUS) ||
> -  (mem->placement & TTM_PL_FLAG_CONTIGUOUS)))
> - return true;
> - }
> - return false;
> -}
> -
> -bool ttm_bo_mem_compat(struct ttm_placement *placement,
> -struct ttm_resource *mem,
> -uint32_t *new_flags)
> -{
> - if (ttm_bo_places_compat(placement->placement, placement->num_placement,
> -  mem, new_flags))
> - return true;
> -
> - if ((placement->busy_placement != placement->placement ||
> -  placement->num_busy_placement > placement->num_placement) &&
> - ttm_bo_places_compat(placement->busy_placement,
> -  placement->num_busy_placement,
> -  mem, new_flags))
> - return true;
> -
> - return false;
> -}
> -EXPORT_SYMBOL(ttm_bo_mem_compat);
> -
>  int ttm_bo_validate(struct ttm_buffer_object *bo,
>   struct ttm_placement *placement,
>   struct ttm_operation_ctx *ctx)
>  {
>   int ret;
> - uint32_t new_flags;
>  
>   dma_resv_assert_held(bo->base.resv);
>  
> @@ -987,7 +941,7 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
>   /*
>* Check whether we need to move buffer.
>*/
> - if (!ttm_bo_mem_compat(placement, bo->resource, _flags)) {
> + if (!ttm_resource_compat(bo->resource, placement)) {
>   ret = ttm_bo_move_buffer(bo, placement, ctx);
>   if (ret)
>   return ret;
> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c 
> b/drivers/gpu/drm/ttm/ttm_resource.c
> index 2431717376e7..035d71332d18 100644
> --- a/drivers/gpu/drm/ttm/ttm_resource.c
> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> @@ -67,6 +67,55 @@ void ttm_resource_free(struct ttm_buffer_object *bo, 
> struct ttm_resource **res)
>  }
>  EXPORT_SYMBOL(ttm_resource_free);
>  
> +static bool ttm_resource_places_compat(struct ttm_resource *res,
> +const struct ttm_place *places,
> +unsigned num_placement)
> +{
> + unsigned i;
> +
> + if (res->placement & TTM_PL_FLAG_TEMPORARY)
> + return false;
> +
> + for (i = 0; i < num_placement; i++) {
> + const struct ttm_place *heap = [i];
> +
> + if (res->start < heap->fpfn || (heap->lpfn &&
> + (res->start + res->num_pages) > heap->lpfn))
> + continue;
> +
> + if ((res->mem_type == heap->mem_type) &&
> + (!(heap->flags & TTM_PL_FLAG_CONTIGUOUS) ||
> +  (res->placement & TTM_PL_FLAG_CONTIGUOUS)))
> + return true;
> + }
> + return false;
> +}
> +
> +/**
> + * ttm_resource_compat - check if resource is compatible with placement
> + *
> + * @res: the resource to check
> + * @placement: the placement to check against
> + *
> + * Returns true if the placement is compatible.
> + */
> +bool ttm_resource_compat(struct ttm_resource *res,
> +  struct ttm_placement *placement)
> +{
> + if (ttm_resource_places_compat(res, placement->placement,
> +placement->num_placement))
> +

Re: [Intel-gfx] [PATCH 01/11] drm/i915: Release i915_gem_context from a worker




On 13/08/2021 21:30, Daniel Vetter wrote:

The only reason for this really is the i915_gem_engines->fence
callback engines_notify(), which exists purely as a fairly funky
reference counting scheme for that. Otherwise all other callers are
from process context, and generally fairly benign locking context.


There is reset which definitely isn't process context.

Otherwise I did not really get from the commit message is this patch 
fixing an existing problem or preparing something for the future. If the 
former then as I wrote above - I am pretty sure there are call sites 
from the tasklet already.


Regards,

Tvrtko


Unfortunately untangling that requires some major surgery, and we have
a few i915_gem_context reference counting bugs that need fixing, and
they blow in the current hardirq calling context, so we need a
stop-gap measure.

Put a FIXME comment in when this should be removable again.

Signed-off-by: Daniel Vetter 
Cc: Jon Bloomfield 
Cc: Chris Wilson 
Cc: Maarten Lankhorst 
Cc: Joonas Lahtinen 
Cc: Daniel Vetter 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Cc: Lionel Landwerlin 
Cc: Dave Airlie 
Cc: Jason Ekstrand 
---
  drivers/gpu/drm/i915/gem/i915_gem_context.c   | 13 +++--
  drivers/gpu/drm/i915/gem/i915_gem_context_types.h | 12 
  2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index fd169cf2f75a..051bc357ff65 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -986,9 +986,10 @@ static struct i915_gem_engines *user_engines(struct 
i915_gem_context *ctx,
return err;
  }
  
-void i915_gem_context_release(struct kref *ref)

+static void i915_gem_context_release_work(struct work_struct *work)
  {
-   struct i915_gem_context *ctx = container_of(ref, typeof(*ctx), ref);
+   struct i915_gem_context *ctx = container_of(work, typeof(*ctx),
+   release_work);
  
  	trace_i915_context_free(ctx);

GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
@@ -1002,6 +1003,13 @@ void i915_gem_context_release(struct kref *ref)
kfree_rcu(ctx, rcu);
  }
  
+void i915_gem_context_release(struct kref *ref)

+{
+   struct i915_gem_context *ctx = container_of(ref, typeof(*ctx), ref);
+
+   queue_work(ctx->i915->wq, >release_work);
+}
+
  static inline struct i915_gem_engines *
  __context_engines_static(const struct i915_gem_context *ctx)
  {
@@ -1303,6 +1311,7 @@ i915_gem_create_context(struct drm_i915_private *i915,
ctx->sched = pc->sched;
mutex_init(>mutex);
INIT_LIST_HEAD(>link);
+   INIT_WORK(>release_work, i915_gem_context_release_work);
  
  	spin_lock_init(>stale.lock);

INIT_LIST_HEAD(>stale.engines);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h 
b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
index 94c03a97cb77..0c38789bd4a8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
@@ -288,6 +288,18 @@ struct i915_gem_context {
 */
struct kref ref;
  
+	/**

+* @release_work:
+*
+* Work item for deferred cleanup, since i915_gem_context_put() tends to
+* be called from hardirq context.
+*
+* FIXME: The only real reason for this is _gem_engines.fence, all
+* other callers are from process context and need at most some mild
+* shuffling to pull the i915_gem_context_put() call out of a spinlock.
+*/
+   struct work_struct release_work;
+
/**
 * @rcu: rcu_head for deferred freeing.
 */

Re: [PATCH v6, 13/15] dt-bindings: media: mtk-vcodec: Adds decoder dt-bindings for mt8192

2021-09-02 Thread Rob Herring

On Wed, 01 Sep 2021 16:32:13 +0800, Yunfei Dong wrote:
> Adds decoder dt-bindings for mt8192.
> 
> Signed-off-by: Yunfei Dong 
> ---
> v6: add decoder block diagram for Laurent's comments.
> 
> This patch depends on "Mediatek MT8192 clock support"[1].
> 
> The definition of decoder clocks are in mt8192-clk.h, and this patch already 
> in clk tree[1].
> 
> [1]https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/?h=clk-next=f35f1a23e0e12e3173e9e9dedbc150d139027189
> ---
>  .../media/mediatek,vcodec-comp-decoder.yaml   | 192 ++
>  1 file changed, 192 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml:
 properties:compatible: [{'enum': ['mediatek,mt8192-vcodec-dec', 
'mediatek,mtk-vcodec-lat', 'mediatek,mtk-vcodec-core']}] is not of type 
'object', 'boolean'
from schema $id: http://json-schema.org/draft-07/schema#
./Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml: 
$id: relative path/filename doesn't match actual path or filename
expected: 
http://devicetree.org/schemas/media/mediatek,vcodec-comp-decoder.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml:
 ignoring, error in schema: properties: compatible
warning: no schema found in file: 
./Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml
Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.example.dts:22:18:
 fatal error: dt-bindings/clock/mt8192-clk.h: No such file or directory
   22 | #include 
  |  ^~~~
compilation terminated.
make[1]: *** [scripts/Makefile.lib:380: 
Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.example.dt.yaml]
 Error 1
make[1]: *** Waiting for unfinished jobs
make: *** [Makefile:1419: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1522931

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: VKMS: New plane formats

2021-09-02 Thread Igor Matheus Andrade Torrente


On 9/1/21 5:24 PM, Simon Ser wrote:

Ideally the final composition format would have enough precision for
all of the planes. I think it'd make sense to use ARGB16161616 if the
primary plane uses ARGB and an overlay plane uses ARGB16161616.

To simplify the code, maybe it's fine to always use ARGB16161616 for
the output, and add getters which fetch an ARGB16161616 row for each
supported plane format.



This makes sense to me. I will try to implement this way.

Thanks!

[PATCH 6/6] drm/i915: Reduce the number of objects subject to memcpy recover

We really only need memcpy restore for objects that affect the
operability of the migrate context. That is, primarily the page-table
objects of the migrate VM.

Add an object flag, I915_BO_ALLOC_PM_EARLY for objects that need early
restores using memcpy and a way to assign LMEM page-table object flags
to be used by the vms.

Restore objects without this flag with the gpu blitter and only objects
carrying the flag using TTM memcpy.

Initially mark the migrate, gt, gtt and vgpu vms to use this flag, and
defer for a later audit which vms actually need it. Most importantly, user-
allocated vms with pinned page-table objects can be restored using the
blitter.

Performance-wise memcpy restore is probably as fast as gpu restore if not
faster, but using gpu restore will help tackling future restrictions in
mappable LMEM size.

Signed-off-by: Thomas Hellström 
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c  |  4 ++--
 drivers/gpu/drm/i915/gem/i915_gem_object_types.h |  9 ++---
 drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c   |  3 ++-
 drivers/gpu/drm/i915/gem/selftests/huge_pages.c  |  2 +-
 drivers/gpu/drm/i915/gt/gen6_ppgtt.c |  2 +-
 drivers/gpu/drm/i915/gt/gen8_ppgtt.c |  5 +++--
 drivers/gpu/drm/i915/gt/gen8_ppgtt.h |  4 +++-
 drivers/gpu/drm/i915/gt/intel_ggtt.c |  2 +-
 drivers/gpu/drm/i915/gt/intel_gt.c   |  2 +-
 drivers/gpu/drm/i915/gt/intel_gtt.c  |  3 ++-
 drivers/gpu/drm/i915/gt/intel_gtt.h  |  9 +++--
 drivers/gpu/drm/i915/gt/intel_migrate.c  |  2 +-
 drivers/gpu/drm/i915/gt/intel_ppgtt.c| 13 -
 drivers/gpu/drm/i915/gt/selftest_hangcheck.c |  2 +-
 drivers/gpu/drm/i915/gvt/scheduler.c |  2 +-
 drivers/gpu/drm/i915/selftests/i915_gem_gtt.c|  4 ++--
 16 files changed, 42 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index fd169cf2f75a..3dbebced0950 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1312,7 +1312,7 @@ i915_gem_create_context(struct drm_i915_private *i915,
} else if (HAS_FULL_PPGTT(i915)) {
struct i915_ppgtt *ppgtt;
 
-   ppgtt = i915_ppgtt_create(>gt);
+   ppgtt = i915_ppgtt_create(>gt, 0);
if (IS_ERR(ppgtt)) {
drm_dbg(>drm, "PPGTT setup failed (%ld)\n",
PTR_ERR(ppgtt));
@@ -1490,7 +1490,7 @@ int i915_gem_vm_create_ioctl(struct drm_device *dev, void 
*data,
if (args->flags)
return -EINVAL;
 
-   ppgtt = i915_ppgtt_create(>gt);
+   ppgtt = i915_ppgtt_create(>gt, 0);
if (IS_ERR(ppgtt))
return PTR_ERR(ppgtt);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h 
b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index 66123ba46247..477b98b656b4 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -294,13 +294,16 @@ struct drm_i915_gem_object {
 #define I915_BO_ALLOC_USERBIT(3)
 /* Object may lose its contents on suspend / resume */
 #define I915_BO_ALLOC_PM_VOLATILE BIT(4)
+/* Object needs to be restored early using memcpy during resume */
+#define I915_BO_ALLOC_PM_EARLYBIT(5)
 #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \
 I915_BO_ALLOC_VOLATILE | \
 I915_BO_ALLOC_CPU_CLEAR | \
 I915_BO_ALLOC_USER | \
-I915_BO_ALLOC_PM_VOLATILE)
-#define I915_BO_READONLY  BIT(5)
-#define I915_TILING_QUIRK_BIT 6 /* unknown swizzling; do not release! */
+I915_BO_ALLOC_PM_VOLATILE | \
+I915_BO_ALLOC_PM_EARLY)
+#define I915_BO_READONLY  BIT(6)
+#define I915_TILING_QUIRK_BIT 7 /* unknown swizzling; do not release! */
 
/**
 * @mem_flags - Mutable placement-related flags
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c 
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c
index ff35c0535fa4..60c610bb2c88 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c
@@ -149,7 +149,8 @@ static int i915_ttm_restore(struct i915_gem_apply_to_region 
*apply,
if (!obj->ttm.backup)
return 0;
 
-   if (pm_apply->early_restore && (obj->flags & I915_BO_ALLOC_USER))
+   if (pm_apply->early_restore && ((obj->flags & I915_BO_ALLOC_USER) ||
+   !(obj->flags & I915_BO_ALLOC_PM_EARLY)))
return 0;
 
err = i915_gem_object_lock(backup, apply->ww);
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c 
b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
index a094f3ce1a90..149f62221a83 100644
---

[PATCH 5/6] drm/i915: Don't back up pinned LMEM context images and rings during suspend

Pinned context images are now reset during resume. Don't back them up,
and assuming that rings can be assumed empty at suspend, don't back them
up either.

Introduce a new object flag, I915_BO_ALLOC_PM_VOLATILE meaning that an
object is allowed to lose its content on suspend.

Signed-off-by: Thomas Hellström 
---
 .../gpu/drm/i915/gem/i915_gem_object_types.h| 17 ++---
 drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c  |  3 +++
 drivers/gpu/drm/i915/gt/intel_lrc.c |  3 ++-
 drivers/gpu/drm/i915/gt/intel_ring.c|  3 ++-
 4 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h 
b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index 734cc8e16481..66123ba46247 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -288,16 +288,19 @@ struct drm_i915_gem_object {
I915_SELFTEST_DECLARE(struct list_head st_link);
 
unsigned long flags;
-#define I915_BO_ALLOC_CONTIGUOUS BIT(0)
-#define I915_BO_ALLOC_VOLATILE   BIT(1)
-#define I915_BO_ALLOC_CPU_CLEAR  BIT(2)
-#define I915_BO_ALLOC_USER   BIT(3)
+#define I915_BO_ALLOC_CONTIGUOUS  BIT(0)
+#define I915_BO_ALLOC_VOLATILEBIT(1)
+#define I915_BO_ALLOC_CPU_CLEAR   BIT(2)
+#define I915_BO_ALLOC_USERBIT(3)
+/* Object may lose its contents on suspend / resume */
+#define I915_BO_ALLOC_PM_VOLATILE BIT(4)
 #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \
 I915_BO_ALLOC_VOLATILE | \
 I915_BO_ALLOC_CPU_CLEAR | \
-I915_BO_ALLOC_USER)
-#define I915_BO_READONLY BIT(4)
-#define I915_TILING_QUIRK_BIT5 /* unknown swizzling; do not release! */
+I915_BO_ALLOC_USER | \
+I915_BO_ALLOC_PM_VOLATILE)
+#define I915_BO_READONLY  BIT(5)
+#define I915_TILING_QUIRK_BIT 6 /* unknown swizzling; do not release! */
 
/**
 * @mem_flags - Mutable placement-related flags
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c 
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c
index 333aa753a723..ff35c0535fa4 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c
@@ -43,6 +43,9 @@ static int i915_ttm_backup(struct i915_gem_apply_to_region 
*apply,
if (i915_gem_object_evictable(obj))
return ttm_bo_validate(bo, i915_ttm_sys_placement(), );
 
+   if (obj->flags & I915_BO_ALLOC_PM_VOLATILE)
+   return 0;
+
sys_region = i915->mm.regions[INTEL_REGION_SMEM];
backup = i915_gem_object_create_region(sys_region,
   obj->base.size,
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c 
b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 6ba8daea2f56..3ef9eaf8c50e 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -942,7 +942,8 @@ __lrc_alloc_state(struct intel_context *ce, struct 
intel_engine_cs *engine)
context_size += PAGE_SIZE;
}
 
-   obj = i915_gem_object_create_lmem(engine->i915, context_size, 0);
+   obj = i915_gem_object_create_lmem(engine->i915, context_size,
+ I915_BO_ALLOC_PM_VOLATILE);
if (IS_ERR(obj))
obj = i915_gem_object_create_shmem(engine->i915, context_size);
if (IS_ERR(obj))
diff --git a/drivers/gpu/drm/i915/gt/intel_ring.c 
b/drivers/gpu/drm/i915/gt/intel_ring.c
index 7c4d5158e03b..2fdd52b62092 100644
--- a/drivers/gpu/drm/i915/gt/intel_ring.c
+++ b/drivers/gpu/drm/i915/gt/intel_ring.c
@@ -112,7 +112,8 @@ static struct i915_vma *create_ring_vma(struct i915_ggtt 
*ggtt, int size)
struct drm_i915_gem_object *obj;
struct i915_vma *vma;
 
-   obj = i915_gem_object_create_lmem(i915, size, I915_BO_ALLOC_VOLATILE);
+   obj = i915_gem_object_create_lmem(i915, size, I915_BO_ALLOC_VOLATILE |
+ I915_BO_ALLOC_PM_VOLATILE);
if (IS_ERR(obj) && i915_ggtt_has_aperture(ggtt))
obj = i915_gem_object_create_stolen(i915, size);
if (IS_ERR(obj))
-- 
2.31.1

[PATCH 4/6] drm/i915/gt: Register the migrate contexts with their engines

Pinned contexts, like the migrate contexts need reset after resume
since their context image may have been lost. Also the GuC needs to
register pinned contexts.

Add a list to struct intel_engine_cs where we add all pinned contexts on
creation, and traverse that list at resume time to reset the pinned
contexts.

This fixes the kms_pipe_crc_basic@suspend-read-crc-pipe-a selftest for now,
but proper LMEM backup / restore is needed for full suspend functionality.
However, note that even with full LMEM backup / restore it may be
desirable to keep the reset since backing up the migrate context images
must happen using memcpy() after the migrate context has become inactive,
and for performance- and other reasons we want to avoid memcpy() from
LMEM.

Also traverse the list at guc_init_lrc_mapping() calling
guc_kernel_context_pin() for the pinned contexts, like is already done
for the kernel context.

v2:
- Don't reset the contexts on each __engine_unpark() but rather at
  resume time (Chris Wilson).

Cc: Tvrtko Ursulin 
Cc: Matthew Auld 
Cc: Maarten Lankhorst 
Cc: Brost Matthew 
Cc: Chris Wilson 
Signed-off-by: Thomas Hellström 
---
 drivers/gpu/drm/i915/gt/intel_context_types.h |  8 +++
 drivers/gpu/drm/i915/gt/intel_engine_cs.c |  4 
 drivers/gpu/drm/i915/gt/intel_engine_pm.c | 23 +++
 drivers/gpu/drm/i915/gt/intel_engine_pm.h |  2 ++
 drivers/gpu/drm/i915/gt/intel_engine_types.h  |  7 ++
 drivers/gpu/drm/i915/gt/intel_gt_pm.c |  3 +++
 drivers/gpu/drm/i915/gt/mock_engine.c |  1 +
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 10 +---
 8 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h 
b/drivers/gpu/drm/i915/gt/intel_context_types.h
index e54351a170e2..a63631ea0ec4 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -152,6 +152,14 @@ struct intel_context {
/** sseu: Control eu/slice partitioning */
struct intel_sseu sseu;
 
+   /**
+* pinned_contexts_link: List link for the engine's pinned contexts.
+* This is only used if this is a perma-pinned kernel context and
+* the list is assumed to only be manipulated during driver load
+* or unload time so no mutex protection currently.
+*/
+   struct list_head pinned_contexts_link;
+
u8 wa_bb_page; /* if set, page num reserved for context workarounds */
 
struct {
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 332efea696a5..c606a4714904 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -320,6 +320,7 @@ static int intel_engine_setup(struct intel_gt *gt, enum 
intel_engine_id id)
 
BUILD_BUG_ON(BITS_PER_TYPE(engine->mask) < I915_NUM_ENGINES);
 
+   INIT_LIST_HEAD(>pinned_contexts_list);
engine->id = id;
engine->legacy_idx = INVALID_ENGINE;
engine->mask = BIT(id);
@@ -875,6 +876,8 @@ intel_engine_create_pinned_context(struct intel_engine_cs 
*engine,
return ERR_PTR(err);
}
 
+   list_add_tail(>pinned_contexts_link, >pinned_contexts_list);
+
/*
 * Give our perma-pinned kernel timelines a separate lockdep class,
 * so that we can use them from within the normal user timelines
@@ -897,6 +900,7 @@ void intel_engine_destroy_pinned_context(struct 
intel_context *ce)
list_del(>timeline->engine_link);
mutex_unlock(>vm->mutex);
 
+   list_del(>pinned_contexts_link);
intel_context_unpin(ce);
intel_context_put(ce);
 }
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c 
b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
index 1f07ac4e0672..dacd62773735 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
@@ -298,6 +298,29 @@ void intel_engine_init__pm(struct intel_engine_cs *engine)
intel_engine_init_heartbeat(engine);
 }
 
+/**
+ * intel_engine_reset_pinned_contexts - Reset the pinned contexts of
+ * an engine.
+ * @engine: The engine whose pinned contexts we want to reset.
+ *
+ * Typically the pinned context LMEM images lose or get their content
+ * corrupted on suspend. This function resets their images.
+ */
+void intel_engine_reset_pinned_contexts(struct intel_engine_cs *engine)
+{
+   struct intel_context *ce;
+
+   list_for_each_entry(ce, >pinned_contexts_list,
+   pinned_contexts_link) {
+   /* kernel context gets reset at __engine_unpark() */
+   if (ce == engine->kernel_context)
+   continue;
+
+   dbg_poison_ce(ce);
+   ce->ops->reset(ce);
+   }
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftest_engine_pm.c"
 #endif
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.h

[PATCH 3/6] drm/i915 Implement LMEM backup and restore for suspend / resume

Just evict unpinned objects to system. For pinned LMEM objects,
make a backup system object and blit the contents to that.

For now, for pinned objects, backup using gpu blits and restore using
memcpy except for occasional user-space objects which are restored
using gpu blits once the migration context is functional.

Signed-off-by: Thomas Hellström 
---
 drivers/gpu/drm/i915/Makefile |   1 +
 .../gpu/drm/i915/gem/i915_gem_object_types.h  |   1 +
 drivers/gpu/drm/i915/gem/i915_gem_pm.c|  46 +
 drivers/gpu/drm/i915/gem/i915_gem_pm.h|   1 +
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c   |  18 +-
 drivers/gpu/drm/i915/gem/i915_gem_ttm.h   |   4 +
 drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c| 195 ++
 drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.h|  23 +++
 drivers/gpu/drm/i915/i915_drv.c   |   4 +-
 9 files changed, 289 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c
 create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index c36c8a4f0716..3379a0a6c91e 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -155,6 +155,7 @@ gem-y += \
gem/i915_gem_throttle.o \
gem/i915_gem_tiling.o \
gem/i915_gem_ttm.o \
+   gem/i915_gem_ttm_pm.o \
gem/i915_gem_userptr.o \
gem/i915_gem_wait.o \
gem/i915_gemfs.o
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h 
b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index 2471f36aaff3..734cc8e16481 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -534,6 +534,7 @@ struct drm_i915_gem_object {
struct {
struct sg_table *cached_io_st;
struct i915_gem_object_page_iter get_io_page;
+   struct drm_i915_gem_object *backup;
bool created:1;
} ttm;
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c 
b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
index 8b9d7d14c4bd..e1be995d782b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
@@ -5,6 +5,7 @@
  */
 
 #include "gem/i915_gem_pm.h"
+#include "gem/i915_gem_ttm_pm.h"
 #include "gt/intel_gt.h"
 #include "gt/intel_gt_pm.h"
 #include "gt/intel_gt_requests.h"
@@ -39,6 +40,32 @@ void i915_gem_suspend(struct drm_i915_private *i915)
i915_gem_drain_freed_objects(i915);
 }
 
+int i915_gem_backup_suspend(struct drm_i915_private *i915)
+{
+   struct intel_memory_region *mr;
+   int ret = 0, id;
+
+   for_each_memory_region(mr, i915, id) {
+   if (mr->type == INTEL_MEMORY_LOCAL) {
+   ret = i915_ttm_backup_region(mr);
+   if (ret)
+   break;
+   }
+   }
+
+   if (!ret) {
+   i915_gem_suspend(i915);
+   return 0;
+   }
+
+   for_each_memory_region(mr, i915, id) {
+   if (mr->type == INTEL_MEMORY_LOCAL)
+   i915_ttm_recover_region(mr);
+   }
+
+   return ret;
+}
+
 void i915_gem_suspend_late(struct drm_i915_private *i915)
 {
struct drm_i915_gem_object *obj;
@@ -128,12 +155,31 @@ int i915_gem_freeze_late(struct drm_i915_private *i915)
 
 void i915_gem_resume(struct drm_i915_private *i915)
 {
+   int ret, id;
+   struct intel_memory_region *mr;
+
GEM_TRACE("%s\n", dev_name(i915->drm.dev));
 
+   for_each_memory_region(mr, i915, id) {
+   if (mr->type == INTEL_MEMORY_LOCAL) {
+   ret = i915_ttm_restore_region(mr, true);
+   if (ret)
+   WARN_ON(ret);
+   }
+   }
+
/*
 * As we didn't flush the kernel context before suspend, we cannot
 * guarantee that the context image is complete. So let's just reset
 * it and start again.
 */
intel_gt_resume(>gt);
+
+   for_each_memory_region(mr, i915, id) {
+   if (mr->type == INTEL_MEMORY_LOCAL) {
+   ret = i915_ttm_restore_region(mr, false);
+   if (ret)
+   WARN_ON(ret);
+   }
+   }
 }
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.h 
b/drivers/gpu/drm/i915/gem/i915_gem_pm.h
index c9a66630e92e..bedf1e95941a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pm.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.h
@@ -18,6 +18,7 @@ void i915_gem_idle_work_handler(struct work_struct *work);
 
 void i915_gem_suspend(struct drm_i915_private *i915);
 void i915_gem_suspend_late(struct drm_i915_private *i915);
+int i915_gem_backup_suspend(struct drm_i915_private *i915);
 
 int i915_gem_freeze(struct drm_i915_private *i915);
 int i915_gem_freeze_late(struct drm_i915_private *i915);
diff --git

[PATCH 2/6] drm/i915/gem: Implement a function to process all gem objects of a region

An upcoming common pattern is to traverse the region object list and
perform certain actions on all objects in a region. It's a little tricky
to get the list locking right, in particular since a gem object may
change region unless it's pinned or the object lock is held.

Define a function that does this for us and that takes an argument that
defines the action to be performed on each object.

Signed-off-by: Thomas Hellström 
---
 drivers/gpu/drm/i915/gem/i915_gem_region.c | 70 ++
 drivers/gpu/drm/i915/gem/i915_gem_region.h | 33 ++
 2 files changed, 103 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c 
b/drivers/gpu/drm/i915/gem/i915_gem_region.c
index 1f557b2178ed..a016ccec36f3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
@@ -80,3 +80,73 @@ i915_gem_object_create_region(struct intel_memory_region 
*mem,
i915_gem_object_free(obj);
return ERR_PTR(err);
 }
+
+/**
+ * i915_gem_process_region - Iterate over all objects of a region using ops
+ * to process and optionally skip objects
+ * @mr: The memory region
+ * @apply: ops and private data
+ *
+ * This function can be used to iterate over the regions object list,
+ * checking whether to skip objects, and, if not, lock the objects and
+ * process them using the supplied ops. Note that this function temporarily
+ * removes objects from the region list while iterating, so that if run
+ * concurrently with itself may not iterate over all objects.
+ *
+ * Return: 0 if successful, negative error code on failure.
+ */
+int i915_gem_process_region(struct intel_memory_region *mr,
+   struct i915_gem_apply_to_region *apply)
+{
+   const struct i915_gem_apply_to_region_ops *ops = apply->ops;
+   struct drm_i915_gem_object *obj;
+   struct list_head still_in_list;
+   int ret = 0;
+
+   /*
+* In the future, a non-NULL apply->ww could mean the caller is
+* already in a locking transaction and provides its own context.
+*/
+   GEM_WARN_ON(apply->ww);
+
+   INIT_LIST_HEAD(_in_list);
+   mutex_lock(>objects.lock);
+   for (;;) {
+   struct i915_gem_ww_ctx ww;
+
+   obj = list_first_entry_or_null(>objects.list, typeof(*obj),
+  mm.region_link);
+   if (!obj)
+   break;
+
+   list_move_tail(>mm.region_link, _in_list);
+   if (!kref_get_unless_zero(>base.refcount))
+   continue;
+
+   /*
+* Note: Someone else might be migrating the object at this
+* point. The object's region is not stable until we lock
+* the object.
+*/
+   mutex_unlock(>objects.lock);
+   apply->ww = 
+   for_i915_gem_ww(, ret, apply->interruptible) {
+   ret = i915_gem_object_lock(obj, apply->ww);
+   if (ret)
+   continue;
+
+   if (obj->mm.region == mr)
+   ret = ops->process_obj(apply, obj);
+   /* Implicit object unlock */
+   }
+
+   i915_gem_object_put(obj);
+   mutex_lock(>objects.lock);
+   if (ret)
+   break;
+   }
+   list_splice_tail(_in_list, >objects.list);
+   mutex_unlock(>objects.lock);
+
+   return ret;
+}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.h 
b/drivers/gpu/drm/i915/gem/i915_gem_region.h
index 1008e580a89a..f62195847056 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_region.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_region.h
@@ -12,6 +12,37 @@ struct intel_memory_region;
 struct drm_i915_gem_object;
 struct sg_table;
 
+struct i915_gem_apply_to_region;
+
+/**
+ * struct i915_gem_apply_to_region_ops - ops to use when iterating over all
+ * region objects.
+ */
+struct i915_gem_apply_to_region_ops {
+   /**
+* process_obj - Process the current object
+* @apply: Embed this for provate data
+* @obj: The current object.
+*/
+   int (*process_obj)(struct i915_gem_apply_to_region *apply,
+  struct drm_i915_gem_object *obj);
+};
+
+/**
+ * struct i915_gem_apply_to_region - Argument to the struct
+ * i915_gem_apply_to_region_ops functions.
+ * @ops: The ops for the operation.
+ * @ww: Locking context used for the transaction.
+ * @interruptible: Whether to perform object locking interruptible.
+ *
+ * This structure is intended to be embedded in a private struct if needed
+ */
+struct i915_gem_apply_to_region {
+   const struct i915_gem_apply_to_region_ops *ops;
+   struct i915_gem_ww_ctx *ww;
+   u32 interruptible:1;
+};
+
 void i915_gem_object_init_memory_region(struct drm_i915_gem_object *obj,

[PATCH 1/6] drm/i915/ttm: Implement a function to copy the contents of two TTM-base objects

When backing up or restoring contents of pinned objects at suspend /
resume time we need to allocate a new object as the backup. Add a function
to facilitate copies between the two. Some data needs to be copied before
the migration context is ready for operation, so make sure we can
disable accelerated copies.

Signed-off-by: Thomas Hellström 
---
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 69 +
 drivers/gpu/drm/i915/gem/i915_gem_ttm.h |  4 ++
 2 files changed, 64 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c 
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 59ca53a3ef6a..df2dcbad1eb9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -432,6 +432,7 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj,
 static int i915_ttm_accel_move(struct ttm_buffer_object *bo,
   bool clear,
   struct ttm_resource *dst_mem,
+  struct ttm_tt *dst_ttm,
   struct sg_table *dst_st)
 {
struct drm_i915_private *i915 = container_of(bo->bdev, typeof(*i915),
@@ -441,14 +442,14 @@ static int i915_ttm_accel_move(struct ttm_buffer_object 
*bo,
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
struct sg_table *src_st;
struct i915_request *rq;
-   struct ttm_tt *ttm = bo->ttm;
+   struct ttm_tt *src_ttm = bo->ttm;
enum i915_cache_level src_level, dst_level;
int ret;
 
if (!i915->gt.migrate.context)
return -EINVAL;
 
-   dst_level = i915_ttm_cache_level(i915, dst_mem, ttm);
+   dst_level = i915_ttm_cache_level(i915, dst_mem, dst_ttm);
if (clear) {
if (bo->type == ttm_bo_type_kernel)
return -EINVAL;
@@ -465,10 +466,10 @@ static int i915_ttm_accel_move(struct ttm_buffer_object 
*bo,
}
intel_engine_pm_put(i915->gt.migrate.context->engine);
} else {
-   src_st = src_man->use_tt ? i915_ttm_tt_get_st(ttm) :
+   src_st = src_man->use_tt ? i915_ttm_tt_get_st(src_ttm) :
obj->ttm.cached_io_st;
 
-   src_level = i915_ttm_cache_level(i915, bo->resource, ttm);
+   src_level = i915_ttm_cache_level(i915, bo->resource, src_ttm);
intel_engine_pm_get(i915->gt.migrate.context->engine);
ret = intel_context_migrate_copy(i915->gt.migrate.context,
 NULL, src_st->sgl, src_level,
@@ -488,11 +489,14 @@ static int i915_ttm_accel_move(struct ttm_buffer_object 
*bo,
 
 static void __i915_ttm_move(struct ttm_buffer_object *bo, bool clear,
struct ttm_resource *dst_mem,
-   struct sg_table *dst_st)
+   struct ttm_tt *dst_ttm,
+   struct sg_table *dst_st,
+   bool allow_accel)
 {
-   int ret;
+   int ret = -EINVAL;
 
-   ret = i915_ttm_accel_move(bo, clear, dst_mem, dst_st);
+   if (allow_accel)
+   ret = i915_ttm_accel_move(bo, clear, dst_mem, dst_ttm, dst_st);
if (ret) {
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
struct intel_memory_region *dst_reg, *src_reg;
@@ -507,7 +511,7 @@ static void __i915_ttm_move(struct ttm_buffer_object *bo, 
bool clear,
GEM_BUG_ON(!dst_reg || !src_reg);
 
dst_iter = !cpu_maps_iomem(dst_mem) ?
-   ttm_kmap_iter_tt_init(&_dst_iter.tt, bo->ttm) :
+   ttm_kmap_iter_tt_init(&_dst_iter.tt, dst_ttm) :
ttm_kmap_iter_iomap_init(&_dst_iter.io, _reg->iomap,
 dst_st, dst_reg->region.start);
 
@@ -562,7 +566,7 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool 
evict,
 
clear = !cpu_maps_iomem(bo->resource) && (!ttm || 
!ttm_tt_is_populated(ttm));
if (!(clear && ttm && !(ttm->page_flags & TTM_PAGE_FLAG_ZERO_ALLOC)))
-   __i915_ttm_move(bo, clear, dst_mem, dst_st);
+   __i915_ttm_move(bo, clear, dst_mem, bo->ttm, dst_st, true);
 
ttm_bo_move_sync_cleanup(bo, dst_mem);
i915_ttm_adjust_domains_after_move(obj);
@@ -973,3 +977,50 @@ i915_gem_ttm_system_setup(struct drm_i915_private *i915,
intel_memory_region_set_name(mr, "system-ttm");
return mr;
 }
+
+/**
+ * i915_gem_obj_copy_ttm - Copy the contents of one ttm-based gem object to
+ * another
+ * @dst: The destination object
+ * @src: The source object
+ * @allow_accel: Allow using the blitter. Otherwise TTM memcpy is used.
+ * @intr: Whether to perform waits interruptible:
+ *
+ * Note: The caller is responsible for assuring that the underlying
+ * TTM objects are populated if needed and locked.
+ *
+ * Return: Zero on

[PATCH 0/6] drm/i915: Suspend / resume backup- and restore of LMEM.

Implement backup and restore of LMEM during suspend / resume.
What complicates things a bit is handling of pinned LMEM memory during
suspend and the fact that we might be dealing with unmappable LMEM in
the future, which makes us want to restrict the number of pinned objects that
need memcpy resume.

The first two patches are prereq patches implementing object content copy
and a generic means of iterating through all objects in a region.
The third patch adds the backup / recover / restore functions and the
two last patches deal with restricting the number of objects we need to
use memcpy for.

Thomas Hellström (6):
  drm/i915/ttm: Implement a function to copy the contents of two
TTM-base objects
  drm/i915/gem: Implement a function to process all gem objects of a
region
  drm/i915 Implement LMEM backup and restore for suspend / resume
  drm/i915/gt: Register the migrate contexts with their engines
  drm/i915: Don't back up pinned LMEM context images and rings during
suspend
  drm/i915: Reduce the number of objects subject to memcpy recover

 drivers/gpu/drm/i915/Makefile |   1 +
 drivers/gpu/drm/i915/gem/i915_gem_context.c   |   4 +-
 .../gpu/drm/i915/gem/i915_gem_object_types.h  |  21 +-
 drivers/gpu/drm/i915/gem/i915_gem_pm.c|  46 
 drivers/gpu/drm/i915/gem/i915_gem_pm.h|   1 +
 drivers/gpu/drm/i915/gem/i915_gem_region.c|  70 ++
 drivers/gpu/drm/i915/gem/i915_gem_region.h|  33 +++
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c   |  87 +++-
 drivers/gpu/drm/i915/gem/i915_gem_ttm.h   |   8 +
 drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c| 199 ++
 drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.h|  23 ++
 .../gpu/drm/i915/gem/selftests/huge_pages.c   |   2 +-
 drivers/gpu/drm/i915/gt/gen6_ppgtt.c  |   2 +-
 drivers/gpu/drm/i915/gt/gen8_ppgtt.c  |   5 +-
 drivers/gpu/drm/i915/gt/gen8_ppgtt.h  |   4 +-
 drivers/gpu/drm/i915/gt/intel_context_types.h |   8 +
 drivers/gpu/drm/i915/gt/intel_engine_cs.c |   4 +
 drivers/gpu/drm/i915/gt/intel_engine_pm.c |  23 ++
 drivers/gpu/drm/i915/gt/intel_engine_pm.h |   2 +
 drivers/gpu/drm/i915/gt/intel_engine_types.h  |   7 +
 drivers/gpu/drm/i915/gt/intel_ggtt.c  |   2 +-
 drivers/gpu/drm/i915/gt/intel_gt.c|   2 +-
 drivers/gpu/drm/i915/gt/intel_gt_pm.c |   3 +
 drivers/gpu/drm/i915/gt/intel_gtt.c   |   3 +-
 drivers/gpu/drm/i915/gt/intel_gtt.h   |   9 +-
 drivers/gpu/drm/i915/gt/intel_lrc.c   |   3 +-
 drivers/gpu/drm/i915/gt/intel_migrate.c   |   2 +-
 drivers/gpu/drm/i915/gt/intel_ppgtt.c |  13 +-
 drivers/gpu/drm/i915/gt/intel_ring.c  |   3 +-
 drivers/gpu/drm/i915/gt/mock_engine.c |   1 +
 drivers/gpu/drm/i915/gt/selftest_hangcheck.c  |   2 +-
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  10 +-
 drivers/gpu/drm/i915/gvt/scheduler.c  |   2 +-
 drivers/gpu/drm/i915/i915_drv.c   |   4 +-
 drivers/gpu/drm/i915/selftests/i915_gem_gtt.c |   4 +-
 35 files changed, 566 insertions(+), 47 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c
 create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.h

-- 
2.31.1

Re: [PATCH] dma-buf: cleanup kerneldoc of removed component

2021-09-02 Thread Christian König


Am 01.09.21 um 16:53 schrieb Sumit Semwal:

Hi Christian,

On Wed, 1 Sept 2021 at 13:30, Christian König
 wrote:

The seqno-fence was removed, cleanup the kerneldoc include as well.

Signed-off-by: Christian König 
Fixes: 992c238188a8 ("dma-buf: nuke seqno-fence")

Thanks for fixing; LGTM, please feel free to add my
Acked-by: Sumit Semwal 

Or I could push it as well, if you'd like.


Already taken care of.

Thanks,
Christian.




---
  Documentation/driver-api/dma-buf.rst | 6 --
  1 file changed, 6 deletions(-)

diff --git a/Documentation/driver-api/dma-buf.rst 
b/Documentation/driver-api/dma-buf.rst
index f5ac4c90b237..2cd7db82d9fe 100644
--- a/Documentation/driver-api/dma-buf.rst
+++ b/Documentation/driver-api/dma-buf.rst
@@ -176,12 +176,6 @@ DMA Fences Functions Reference
  .. kernel-doc:: include/linux/dma-fence.h
 :internal:

-Seqno Hardware Fences
-~
-
-.. kernel-doc:: include/linux/seqno-fence.h
-   :internal:
-
  DMA Fence Array
  ~~~

--
2.25.1


Best,
Sumit.

Re: [diagnostic TDR mode patches] unify our solution opinions/suggestions in one thread

2021-09-02 Thread Christian König


Hi Monk,

Am 02.09.21 um 07:52 schrieb Liu, Monk:

[AMD Official Use Only]

I'm not sure I can add much to help this along, I'm sure Alex has some internal 
training,
Once your driver is upstream, it belongs to upstream, you can maintain it, but 
you no longer control it 100%, it's a tradeoff, it's not one companies always 
understand.
Usually people are fine developing away internally, but once interaction with 
other parts of the kernel/subsystem is required they have the realisation that 
they needed to work upstream 6 months earlier.
The best time to interact with upstream was 6 months ago, the second best time 
is now.
<<<

Daniel/AlexD

I didn't mean your changes on AMD driver need my personal approval or review 
... and  I'm totally already get used that our driver is not 100% under control 
by AMDers,
but supposedly any one from community (including you) who tend to change AMD's 
driver need at least to get approvement from someone in AMD, e.g.: AlexD or 
Christian, doesn't that reasonable?


I'm fearing that just repeating what Alex said, but to make it clear 
once more: That is *not* necessary!


The shared repository is owned by upstream maintainers and they are 
usually free to do restructuring work without getting acknowledge from 
every single driver maintainer.


Anybody can of course technically object to upstream design decisions, 
but that means that you need to pay attention to the mailing lists in 
the first place.



just like we need your approve if we try to modify DRM-sched, or need 
panfrost's approval if we need to change panfrost code ...

by only CC AMD's engineers looks not quite properly, how do you know if your 
changes (on AMD code part) are conflicting with AMD's on-going internal 
features/refactoring or not ?


Well because AMD is supposed to work in public as much as possible and 
ask upstream before doing changes to the code base.


Additional to that design decisions are supposed to be discussed on the 
mailing list and *not* internally.


Regards,
Christian.



Thanks

--
Monk Liu | Cloud-GPU Core team
--

-Original Message-
From: Dave Airlie 
Sent: Thursday, September 2, 2021 2:51 AM
To: Alex Deucher 
Cc: Liu, Monk ; Daniel Vetter ; Koenig, Christian 
; Grodzovsky, Andrey ; Chen, JingWen 
; DRI Development ; 
amd-...@lists.freedesktop.org
Subject: Re: [diagnostic TDR mode patches] unify our solution 
opinions/suggestions in one thread

On Thu, 2 Sept 2021 at 01:20, Alex Deucher  wrote:

On Wed, Sep 1, 2021 at 6:19 AM Liu, Monk  wrote:

[AMD Official Use Only]

Daniel

 From the link you share it looks you(or someone else) have quite a bunch 
patches that changes DRM_SCHED or even amdgpu, by that case before they are 
merged to kernel tree I'm wondering if any AMD develop reviewed them ?

They looks to me somehow conflicting with what we changed in our repo

It is really a chaos for AMDer if someone else out side of AMD changes our 
kernel driver (or/and scheduler) without reviewed by AMDer, just like we are 
requiring your review if we tend to change scheduler's logic here 

This one changes AMD's code:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
re.kernel.org%2Fdri-devel%2F20210625133327.2598825-2-boris.brezillon
%40collabora.com%2Fdata=04%7C01%7CMonk.Liu%40amd.com%7C6c507d18
d65341ef53bb08d96d7976e6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%
7C637661190727875969%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=BWJSkKN
y2%2BwjxbQrfxGPzuJ5PBpBwB4aV0ZH6QoJGEg%3Dreserved=0
And I didn't see any reviewed-by from AMDers ...

This one also touches AMD's code:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
re.kernel.org%2Fdri-devel%2F20200604081224.863494-12-daniel.vetter%4
0ffwll.ch%2Fdata=04%7C01%7CMonk.Liu%40amd.com%7C6c507d18d65341e
f53bb08d96d7976e6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63766
1190727885929%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=%2F8vIVXCWjHkM
56pcYI9EvuzhbsZhV9WczkKaBJE67KQ%3Dreserved=0
Which is conflicting with one patch we submitted (in our repo
rightnow), and neither see AMDder gave a review-by on this one (let
me know if I missed it)


Monk, this is not how upstream works.  You need to participate.
That's how communities work.  There's a reason all these discussions
happen on public mailing lists.  The patch author can't be expected to
know every person on every vendor team to CC with a patch.  If you
have concerns, you need to raise them when the patches are being
discussed.


I'm not sure I can add much to help this along, I'm sure Alex has some internal 
training,

Once your driver is upstream, it belongs to upstream, you can maintain it, but 
you no longer control it 100%, it's a tradeoff, it's not one companies always 
understand.

Usually people are fine developing away

Re: [PATCH] drm/i915: Release i915_gem_context from a worker

2021-09-02 Thread Maarten Lankhorst

Op 31-08-2021 om 17:14 schreef Daniel Vetter:
> On Tue, Aug 31, 2021 at 02:16:56PM +0200, Daniel Vetter wrote:
>> On Tue, Aug 31, 2021 at 11:38:27AM +0200, Maarten Lankhorst wrote:
>>> Op 14-08-2021 om 12:43 schreef Daniel Vetter:
 The only reason for this really is the i915_gem_engines->fence
 callback engines_notify(), which exists purely as a fairly funky
 reference counting scheme for that. Otherwise all other callers are
 from process context, and generally fairly benign locking context.

 Unfortunately untangling that requires some major surgery, and we have
 a few i915_gem_context reference counting bugs that need fixing, and
 they blow in the current hardirq calling context, so we need a
 stop-gap measure.

 Put a FIXME comment in when this should be removable again.

 v2: Fix mock_context(), noticed by intel-gfx-ci.

 Signed-off-by: Daniel Vetter 
 Cc: Jon Bloomfield 
 Cc: Chris Wilson 
 Cc: Maarten Lankhorst 
 Cc: Joonas Lahtinen 
 Cc: Daniel Vetter 
 Cc: "Thomas Hellström" 
 Cc: Matthew Auld 
 Cc: Lionel Landwerlin 
 Cc: Dave Airlie 
 Cc: Jason Ekstrand 
 ---
  drivers/gpu/drm/i915/gem/i915_gem_context.c   | 13 +++--
  drivers/gpu/drm/i915/gem/i915_gem_context_types.h | 12 
  drivers/gpu/drm/i915/gem/selftests/mock_context.c |  1 +
  3 files changed, 24 insertions(+), 2 deletions(-)

 diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
 b/drivers/gpu/drm/i915/gem/i915_gem_context.c
 index fd169cf2f75a..051bc357ff65 100644
 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
 +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
 @@ -986,9 +986,10 @@ static struct i915_gem_engines *user_engines(struct 
 i915_gem_context *ctx,
return err;
  }
  
 -void i915_gem_context_release(struct kref *ref)
 +static void i915_gem_context_release_work(struct work_struct *work)
  {
 -  struct i915_gem_context *ctx = container_of(ref, typeof(*ctx), ref);
 +  struct i915_gem_context *ctx = container_of(work, typeof(*ctx),
 +  release_work);
  
trace_i915_context_free(ctx);
GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
 @@ -1002,6 +1003,13 @@ void i915_gem_context_release(struct kref *ref)
kfree_rcu(ctx, rcu);
  }
  
 +void i915_gem_context_release(struct kref *ref)
 +{
 +  struct i915_gem_context *ctx = container_of(ref, typeof(*ctx), ref);
 +
 +  queue_work(ctx->i915->wq, >release_work);
 +}
 +
  static inline struct i915_gem_engines *
  __context_engines_static(const struct i915_gem_context *ctx)
  {
 @@ -1303,6 +1311,7 @@ i915_gem_create_context(struct drm_i915_private 
 *i915,
ctx->sched = pc->sched;
mutex_init(>mutex);
INIT_LIST_HEAD(>link);
 +  INIT_WORK(>release_work, i915_gem_context_release_work);
  
spin_lock_init(>stale.lock);
INIT_LIST_HEAD(>stale.engines);
 diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h 
 b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
 index 94c03a97cb77..0c38789bd4a8 100644
 --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
 +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
 @@ -288,6 +288,18 @@ struct i915_gem_context {
 */
struct kref ref;
  
 +  /**
 +   * @release_work:
 +   *
 +   * Work item for deferred cleanup, since i915_gem_context_put() tends to
 +   * be called from hardirq context.
 +   *
 +   * FIXME: The only real reason for this is _gem_engines.fence, all
 +   * other callers are from process context and need at most some mild
 +   * shuffling to pull the i915_gem_context_put() call out of a spinlock.
 +   */
 +  struct work_struct release_work;
 +
/**
 * @rcu: rcu_head for deferred freeing.
 */
 diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c 
 b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
 index fee070df1c97..067d68a6fe4c 100644
 --- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
 +++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
 @@ -23,6 +23,7 @@ mock_context(struct drm_i915_private *i915,
kref_init(>ref);
INIT_LIST_HEAD(>link);
ctx->i915 = i915;
 +  INIT_WORK(>release_work, i915_gem_context_release_work);
  
mutex_init(>mutex);
  
>>> 
>>> Is the workqueue really needed? I'm not sure you could still race in
>>> drm_syncobj_free when refcount is zero, so in that case removing locking
>>> from _release would work as well as a workqueue.
>>>
>>> Something like below would keep the drm_sync_obj_put hardirq safe.
>>>
>>> I assume when freeing, the  cb list is supposed to be empty, so I added a 
>>> WARN_ON just to be sure,

Re: [PATCH] drm/ttm: provide default page protection for UML

2021-09-02 Thread Anton Ivanov


On 02/09/2021 08:43, Johannes Berg wrote:

On Thu, 2021-09-02 at 07:19 +0100, Anton Ivanov wrote:


I have a question though - why all of DRM is not !UML in config. Not
like we can use them.


I have no idea about that.
Hopefully one of the (other) UML maintainers can answer you.


Touche.

We will discuss that and possibly push a patch to !UML that part of the
tree. IMHO it is not applicable.


As I just said on the other patch, all of this is fallout from my commit
68f5d3f3b654 ("um: add PCI over virtio emulation driver") which is the
first time that you could have PCI on UML.

Without having checked, in this particular case it's probably something
like

depends on PCI && X86_64

as we've seen in other drivers (idxd, ioat).

The biggest problem is probably that UML internally uses X86_64
(arch/x86/um/Kconfig), which is ... unexpected ... since CONFIG_X86_64
is typically considered the ARCH, and now the ARCH is actually um.

I think we can just fix that and get rid of this entire class of
problems? Something like

https://p.sipsolutions.net/fbac19d86637e286.txt


Good idea. I suspect that DRM will not be the only thing to pop up out 
of this so we might as well fix it "at the top".


A



johannes



___
linux-um mailing list
linux...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um




--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/

Re: [PATCH 2/5] drm/meson: Use common drm_fixed_16_16 helper

2021-09-02 Thread Neil Armstrong

On 01/09/2021 19:54, Alyssa Rosenzweig wrote:
> Replace our open-coded FRAC_16_16 with the common drm_fixed_16_16
> helper to reduce code duplication between drivers.
> 
> Signed-off-by: Alyssa Rosenzweig 
> Cc: linux-amlo...@lists.infradead.org
> ---
>  drivers/gpu/drm/meson/meson_overlay.c | 7 +++
>  drivers/gpu/drm/meson/meson_plane.c   | 5 ++---
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_overlay.c 
> b/drivers/gpu/drm/meson/meson_overlay.c
> index dfef8afcc245..d8fc6bbb332f 100644
> --- a/drivers/gpu/drm/meson/meson_overlay.c
> +++ b/drivers/gpu/drm/meson/meson_overlay.c
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "meson_overlay.h"
>  #include "meson_registers.h"
> @@ -162,8 +163,6 @@ struct meson_overlay {
>  };
>  #define to_meson_overlay(x) container_of(x, struct meson_overlay, base)
>  
> -#define FRAC_16_16(mult, div)(((mult) << 16) / (div))
> -
>  static int meson_overlay_atomic_check(struct drm_plane *plane,
> struct drm_atomic_state *state)
>  {
> @@ -181,8 +180,8 @@ static int meson_overlay_atomic_check(struct drm_plane 
> *plane,
>  
>   return drm_atomic_helper_check_plane_state(new_plane_state,
>  crtc_state,
> -FRAC_16_16(1, 5),
> -FRAC_16_16(5, 1),
> +drm_fixed_16_16(1, 5),
> +drm_fixed_16_16(5, 1),
>  true, true);
>  }
>  
> diff --git a/drivers/gpu/drm/meson/meson_plane.c 
> b/drivers/gpu/drm/meson/meson_plane.c
> index 8640a8a8a469..4fae9ebbf178 100644
> --- a/drivers/gpu/drm/meson/meson_plane.c
> +++ b/drivers/gpu/drm/meson/meson_plane.c
> @@ -19,6 +19,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "meson_plane.h"
>  #include "meson_registers.h"
> @@ -68,8 +69,6 @@ struct meson_plane {
>  };
>  #define to_meson_plane(x) container_of(x, struct meson_plane, base)
>  
> -#define FRAC_16_16(mult, div)(((mult) << 16) / (div))
> -
>  static int meson_plane_atomic_check(struct drm_plane *plane,
>   struct drm_atomic_state *state)
>  {
> @@ -92,7 +91,7 @@ static int meson_plane_atomic_check(struct drm_plane *plane,
>*/
>   return drm_atomic_helper_check_plane_state(new_plane_state,
>  crtc_state,
> -FRAC_16_16(1, 5),
> +drm_fixed_16_16(1, 5),
>  DRM_PLANE_HELPER_NO_SCALING,
>  false, true);
>  }
> 

Reviewed-by: Neil Armstrong

[PATCH] drm/lease: allow empty leases

2021-09-02 Thread Simon Ser

This can be used to create a separate DRM file description, thus
creating a new GEM handle namespace. See [1].

Example usage in wlroots is available at [2].

[1]: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/110
[2]: https://github.com/swaywm/wlroots/pull/3158

Signed-off-by: Simon Ser 
Cc: Daniel Vetter 
Cc: Daniel Stone 
Cc: Pekka Paalanen 
Cc: Michel Dänzer 
Cc: Emil Velikov 
Cc: Keith Packard 
Cc: Boris Brezillon 
Cc: Dave Airlie 
---
 drivers/gpu/drm/drm_lease.c | 39 +
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index dee4f24a1808..d72c2fac0ff1 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -489,12 +489,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
if (!drm_core_check_feature(dev, DRIVER_MODESET))
return -EOPNOTSUPP;
 
-   /* need some objects */
-   if (cl->object_count == 0) {
-   DRM_DEBUG_LEASE("no objects in lease\n");
-   return -EINVAL;
-   }
-
if (cl->flags && (cl->flags & ~(O_CLOEXEC | O_NONBLOCK))) {
DRM_DEBUG_LEASE("invalid flags\n");
return -EINVAL;
@@ -510,23 +504,26 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 
object_count = cl->object_count;
 
-   object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
-   array_size(object_count, sizeof(__u32)));
-   if (IS_ERR(object_ids)) {
-   ret = PTR_ERR(object_ids);
-   goto out_lessor;
-   }
-
+   /* Handle leased objects, if any */
idr_init();
+   if (object_count != 0) {
+   object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
+array_size(object_count, 
sizeof(__u32)));
+   if (IS_ERR(object_ids)) {
+   ret = PTR_ERR(object_ids);
+   idr_destroy();
+   goto out_lessor;
+   }
 
-   /* fill and validate the object idr */
-   ret = fill_object_idr(dev, lessor_priv, ,
- object_count, object_ids);
-   kfree(object_ids);
-   if (ret) {
-   DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret);
-   idr_destroy();
-   goto out_lessor;
+   /* fill and validate the object idr */
+   ret = fill_object_idr(dev, lessor_priv, ,
+ object_count, object_ids);
+   kfree(object_ids);
+   if (ret) {
+   DRM_DEBUG_LEASE("lease object lookup failed: %i\n", 
ret);
+   idr_destroy();
+   goto out_lessor;
+   }
}
 
/* Allocate a file descriptor for the lease */
-- 
2.33.0

Re: [PATCH] drm/panfrost: Make use of the helper function devm_platform_ioremap_resource()

2021-09-02 Thread Steven Price

On 31/08/2021 08:53, Cai Huoqing wrote:
> Use the devm_platform_ioremap_resource() helper instead of
> calling platform_get_resource() and devm_ioremap_resource()
> separately
> 
> Signed-off-by: Cai Huoqing 

Reviewed-by: Steven Price 

I'll push this to drm-misc-next.

Thanks,

Steve

> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c 
> b/drivers/gpu/drm/panfrost/panfrost_device.c
> index bd9b7be63b0f..1c692428b0d4 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -198,7 +198,6 @@ static int panfrost_pm_domain_init(struct panfrost_device 
> *pfdev)
>  int panfrost_device_init(struct panfrost_device *pfdev)
>  {
>   int err;
> - struct resource *res;
>  
>   mutex_init(>sched_lock);
>   INIT_LIST_HEAD(>scheduled_jobs);
> @@ -236,8 +235,7 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>   if (err)
>   goto out_reset;
>  
> - res = platform_get_resource(pfdev->pdev, IORESOURCE_MEM, 0);
> - pfdev->iomem = devm_ioremap_resource(pfdev->dev, res);
> + pfdev->iomem = devm_platform_ioremap_resource(pfdev->pdev, 0);
>   if (IS_ERR(pfdev->iomem)) {
>   err = PTR_ERR(pfdev->iomem);
>   goto out_pm_domain;
>

Regression with mainline kernel on rpi4

2021-09-02 Thread Sudip Mukherjee

Hi All,

Our last night's test on rpi4 had a nasty trace. The test was with
7c636d4d20f8 ("Merge tag 'dt-5.15' of
git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc"). Previous test
was on 9e9fb7655ed5 ("Merge tag 'net-next-5.15' of
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next") and it
did not have this trace.

[   40.975161] Unable to handle kernel access to user memory outside
uaccess routines at virtual address 0348
[   40.986187] Mem abort info:
[   40.989062]   ESR = 0x9604
[   40.992233]   EC = 0x25: DABT (current EL), IL = 32 bits
[   40.997699]   SET = 0, FnV = 0
[   41.001205]   EA = 0, S1PTW = 0
[   41.004428]   FSC = 0x04: level 0 translation fault
[   41.009468] Data abort info:
[   41.012410]   ISV = 0, ISS = 0x0004
[   41.016325]   CM = 0, WnR = 0
[   41.019358] user pgtable: 4k pages, 48-bit VAs, pgdp=42ae1000
[   41.025926] [0348] pgd=, p4d=
[   41.032845] Internal error: Oops: 9604 [#1] PREEMPT SMP
[   41.038495] Modules linked in: overlay algif_hash algif_skcipher
af_alg bnep sch_fq_codel ppdev lp parport ip_tables x_tables autofs4
btrfs blake2b_generic zstd_compress raid10 raid456 async_raid6_recov
async_memcpy async_pq async_xor async_tx xor xor_neon raid6_pq
libcrc32c raid1 raid0 multipath linear uas usb_storage
snd_soc_hdmi_codec btsdio brcmfmac brcmutil hci_uart btqca btrtl
bcm2835_v4l2(C) btbcm crct10dif_ce bcm2835_mmal_vchiq(C) btintel
raspberrypi_hwmon videobuf2_vmalloc videobuf2_memops bluetooth
videobuf2_v4l2 videobuf2_common cfg80211 ecdh_generic ecc vc4
drm_kms_helper videodev dwc2 cec snd_bcm2835(C) i2c_brcmstb udc_core
roles drm xhci_pci mc pwm_bcm2835 xhci_pci_renesas snd_soc_core
ac97_bus snd_pcm_dmaengine snd_pcm phy_generic snd_timer
uio_pdrv_genirq snd fb_sys_fops syscopyarea sysfillrect sysimgblt uio
aes_neon_bs aes_neon_blk crypto_simd cryptd
[   41.116584] CPU: 0 PID: 1569 Comm: pulseaudio Tainted: G C
  5.14.0-7c636d4d20f8 #1
[   41.125494] Hardware name: Raspberry Pi 4 Model B (DT)
[   41.130699] pstate: 6045 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   41.137756] pc : vc4_hdmi_audio_prepare+0x3c0/0xba4 [vc4]
[   41.143256] lr : vc4_hdmi_audio_prepare+0x308/0xba4 [vc4]
[   41.148747] sp : 800012f73a50
[   41.152099] x29: 800012f73a50 x28: 562ecc00 x27: 
[   41.159338] x26:  x25: ac44 x24: 21002003
[   41.166574] x23: 800012f73b40 x22: 0003 x21: 59400080
[   41.173811] x20: 594004c8 x19: 000583380600 x18: 
[   41.181047] x17:  x16:  x15: 
[   41.188283] x14:  x13:  x12: 0991
[   41.195520] x11: 0001 x10: 0001d4c0 x9 : 89047838
[   41.202757] x8 : 0031 x7 : 0001d4c0 x6 : 0030
[   41.209993] x5 : 800012f73a98 x4 : 8905bb60 x3 : 10624dd3
[   41.217230] x2 : 03e8 x1 :  x0 : 00562200
[   41.224466] Call trace:
[   41.226939]  vc4_hdmi_audio_prepare+0x3c0/0xba4 [vc4]
[   41.232080]  hdmi_codec_prepare+0xe8/0x1b0 [snd_soc_hdmi_codec]
[   41.238083]  snd_soc_pcm_dai_prepare+0x5c/0x10c [snd_soc_core]
[   41.244038]  soc_pcm_prepare+0x5c/0x130 [snd_soc_core]
[   41.249276]  snd_pcm_prepare+0x150/0x1f0 [snd_pcm]
[   41.254149]  snd_pcm_common_ioctl+0x1644/0x1d14 [snd_pcm]
[   41.259635]  snd_pcm_ioctl+0x3c/0x5c [snd_pcm]
[   41.264152]  __arm64_sys_ioctl+0xb4/0x100
[   41.268216]  invoke_syscall+0x50/0x120
[   41.272014]  el0_svc_common+0x18c/0x1a4
[   41.275899]  do_el0_svc+0x34/0x9c
[   41.279254]  el0_svc+0x2c/0xc0
[   41.282348]  el0t_64_sync_handler+0xa4/0x12c
[   41.286673]  el0t_64_sync+0x1a4/0x1a8
[   41.290385] Code: 52807d02 72a20c43 f9400421 9ba37c13 (f941a423)
[   41.296563] ---[ end trace dcfe08f10aaf6873 ]---

You can see the complete dmesg at
https://openqa.qa.codethink.co.uk/tests/76#step/dmesg/8

-- 
Regards
Sudip

Re: [PATCH] drm/ttm: provide default page protection for UML

2021-09-02 Thread Christian König

Am 02.09.21 um 09:43 schrieb Johannes Berg:

On Thu, 2021-09-02 at 07:19 +0100, Anton Ivanov wrote:

I have a question though - why all of DRM is not !UML in config. Not
like we can use them.

I have no idea about that.
Hopefully one of the (other) UML maintainers can answer you.

Touche.

We will discuss that and possibly push a patch to !UML that part of the
tree. IMHO it is not applicable.

As I just said on the other patch, all of this is fallout from my commit
68f5d3f3b654 ("um: add PCI over virtio emulation driver") which is the
first time that you could have PCI on UML.

Without having checked, in this particular case it's probably something
like

depends on PCI && X86_64

as we've seen in other drivers (idxd, ioat).

The biggest problem is probably that UML internally uses X86_64
(arch/x86/um/Kconfig), which is ... unexpected ... since CONFIG_X86_64
is typically considered the ARCH, and now the ARCH is actually um.

Yeah, as TTM maintainer I was about to NAK that approach here.

Basically you are claiming to be X86_64, but then you don't use the
X86_64 architecture and are surprised that it things break somewhere else.

This is not something you can blame on subsystems or even drivers, but
rather just a broken architectural design and so needs to be fixed there.

Regards,
Christian.

I think we can just fix that and get rid of this entire class of
problems? Something like

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fp.sipsolutions.net%2Ffbac19d86637e286.txtdata=04%7C01%7Cchristian.koenig%40amd.com%7Cd773b1e8b66643874d1308d96de56a86%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637661654674393046%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=xBT%2Fj%2FbEgltQfvE%2B7%2FGRV7IctGn3sDvy8ycmBvTTSXU%3Dreserved=0

johannes

Re: [PATCH] drm/ttm: provide default page protection for UML

2021-09-02 Thread Johannes Berg

On Thu, 2021-09-02 at 09:10 +0100, Anton Ivanov wrote:
> On 02/09/2021 08:43, Johannes Berg wrote:
> > On Thu, 2021-09-02 at 07:19 +0100, Anton Ivanov wrote:
> > > > > 
> > > > > I have a question though - why all of DRM is not !UML in config. Not
> > > > > like we can use them.
> > > > 
> > > > I have no idea about that.
> > > > Hopefully one of the (other) UML maintainers can answer you.
> > > 
> > > Touche.
> > > 
> > > We will discuss that and possibly push a patch to !UML that part of the
> > > tree. IMHO it is not applicable.
> > 
> > As I just said on the other patch, all of this is fallout from my commit
> > 68f5d3f3b654 ("um: add PCI over virtio emulation driver") which is the
> > first time that you could have PCI on UML.
> > 
> > Without having checked, in this particular case it's probably something
> > like
> > 
> > depends on PCI && X86_64
> > 
> > as we've seen in other drivers (idxd, ioat).
> > 
> > The biggest problem is probably that UML internally uses X86_64
> > (arch/x86/um/Kconfig), which is ... unexpected ... since CONFIG_X86_64
> > is typically considered the ARCH, and now the ARCH is actually um.
> > 
> > I think we can just fix that and get rid of this entire class of
> > problems? Something like
> > 
> > https://p.sipsolutions.net/fbac19d86637e286.txt
> 
> Good idea. I suspect that DRM will not be the only thing to pop up out 
> of this so we might as well fix it "at the top".

Yeah I sent like a handful of similar patches ...

I'll send that patch out in a minute, just writing a reasonable commit
log. Seems to build & work fine for me.

johannes

[Bug 212655] AMDGPU crashes when resuming from suspend when amd_iommu=on