date:20211222

Re: [PATCH] drm/ttm: Don't inherit GEM object VMAs in child process

2021-12-22 Thread Christian König


Am 22.12.21 um 21:53 schrieb Daniel Vetter:

On Mon, Dec 20, 2021 at 01:12:51PM -0500, Bhardwaj, Rajneesh wrote:

[SNIP]
Still sounds funky. I think minimally we should have an ack from CRIU
developers that this is officially the right way to solve this problem. I
really don't want to have random one-off hacks that don't work across the
board, for a problem where we (drm subsystem) really shouldn't be the only
one with this problem. Where "this problem" means that the mmap space is
per file description, and not per underlying inode or real device or
whatever. That part sounds like a CRIU problem, and I expect CRIU folks
want a consistent solution across the board for this. Hence please grab an
ack from them.


Unfortunately it's a KFD design problem. AMD used a single device node, 
then mmaped different objects from the same offset to different 
processes and expected it to work the rest of the fs subsystem without 
churn.


So yes, this is indeed because the mmap space is per file descriptor for 
the use case here.


And thanks for pointing this out, this indeed makes the whole change 
extremely questionable.


Regards,
Christian.



Cheers, Daniel

Re: mmotm 2021-12-22-19-02 uploaded (drivers/gpu/drm/i915/display/intel_backlight.o)

2021-12-22 Thread Randy Dunlap



On 12/22/21 19:02, a...@linux-foundation.org wrote:
> The mm-of-the-moment snapshot 2021-12-22-19-02 has been uploaded to
> 
>https://www.ozlabs.org/~akpm/mmotm/
> 
> mmotm-readme.txt says
> 
> README for mm-of-the-moment:
> 
> https://www.ozlabs.org/~akpm/mmotm/
> 
> This is a snapshot of my -mm patch queue.  Uploaded at random hopefully
> more than once a week.
> 


on x86_64:

ld: drivers/gpu/drm/i915/display/intel_backlight.o: in function 
`intel_backlight_device_register':
intel_backlight.c:(.text+0x27ba): undefined reference to 
`backlight_device_register'
ld: intel_backlight.c:(.text+0x2871): undefined reference to 
`backlight_device_register'
ld: drivers/gpu/drm/i915/display/intel_backlight.o: in function 
`intel_backlight_device_unregister':
intel_backlight.c:(.text+0x28c4): undefined reference to 
`backlight_device_unregister'



Full randconfig file is attached.

-- 
~Randy

config-intel-backlight.gz
Description: application/gzip

[PATCH V2] drm: nouveau: lsfw: cleanup coccinelle warning

2021-12-22 Thread Qing Wang

From: Wang Qing 

odd_ptr_err.cocci has complained about this warning for a long time:
lsfw.c:194:5-11: inconsistent IS_ERR and PTR_ERR on line 195.

Although there is no actual impact, it can improve scanning efficiency.

Signed-off-by: Wang Qing 
---
 drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c 
b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c
index 9b1cf67..0f70d14
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c
@@ -191,7 +191,8 @@ nvkm_acr_lsfw_load_bl_inst_data_sig(struct nvkm_subdev 
*subdev,
u32 *bldata;
int ret;
 
-   if (IS_ERR((lsfw = nvkm_acr_lsfw_add(func, acr, falcon, id
+   lsfw = nvkm_acr_lsfw_add(func, acr, falcon, id);
+   if (IS_ERR(lsfw))
return PTR_ERR(lsfw);
 
ret = nvkm_firmware_load_name(subdev, path, "bl", ver, );
-- 
2.7.4

Re: [PATCH] drm/ast: Support 1600x900 with 108MHz PCLK

2021-12-22 Thread Dave Airlie

On Wed, 22 Dec 2021 at 11:19, Kuo-Hsiang Chou
 wrote:
>
> Hi
>
> -Original Message-
> From: Dave Airlie [mailto:airl...@gmail.com]
> Sent: Wednesday, December 22, 2021 5:56 AM
> To: Thomas Zimmermann 
>
> Subject: Re: [PATCH] drm/ast: Support 1600x900 with 108MHz PCLK
>
> On Mon, 2 Nov 2020 at 17:57, Thomas Zimmermann  wrote:
> >
> > Hi
> >
> > Am 30.10.20 um 08:42 schrieb KuoHsiang Chou:
> > > [New] Create the setting for 1600x900 @60Hz refresh rate
> > >   by 108MHz pixel-clock.
> > >
> > > Signed-off-by: KuoHsiang Chou 
> >
> > Acked-by: Thomas Zimmermann 
> >
> > I'll add your patch to drm-misc-next.
> >
> > As Sam mentioned, you should use scripts/get_maintainers.pl to
> > retrieve the relevant people. These include those in MAINTAINERS, but
> > also developers that have previously worked on the code.
>
> We are seeing a possible report of a regression on an ast2600 server with 
> this patch.
>
> I haven't ascertained that reverting it fixes it for the customer yet, but 
> this is a heads up in case anyone else has seen issues.
>
> Hi Dave,
>
> Yes, you're right, The patch needs to be removed. The patch occurs incorrect 
> timing on CRT and ASTDP when 1600x900 are selected.
> So, do I need to commit a new patch to remove/revert it from drm/ast?

Yes, do a git revert 

Fixup the resulting message, to say why, and add a
Fixes: <12 chars of sha1> ("commitmsg")

and send to the list.
Dave.

Re: [PATCH] drm/ttm: Don't inherit GEM object VMAs in child process

2021-12-22 Thread Bhardwaj, Rajneesh

Sorry for the typo in my previous email. Please read Adrian Reber*

On 12/22/2021 8:49 PM, Bhardwaj, Rajneesh wrote:

Adding Adrian Rebel who is the CRIU maintainer and CRIU list

On 12/22/2021 3:53 PM, Daniel Vetter wrote:

On Mon, Dec 20, 2021 at 01:12:51PM -0500, Bhardwaj, Rajneesh wrote:

On 12/20/2021 4:29 AM, Daniel Vetter wrote:

On Fri, Dec 10, 2021 at 07:58:50AM +0100, Christian König wrote:

Am 09.12.21 um 19:28 schrieb Felix Kuehling:

Am 2021-12-09 um 10:30 a.m. schrieb Christian König:

That still won't work.

But I think we could do this change for the amdgpu mmap callback only.

If graphics user mode has problems with it, we could even make this
specific to KFD BOs in the amdgpu_gem_object_mmap callback.

I think it's fine for the whole amdgpu stack, my concern is more about
radeon, nouveau and the ARM stacks which are using this as well.

That blew up so nicely the last time we tried to change it and I know of at
least one case where radeon was/is used with BOs in a child process.

I'm way late and burried again, but I think it'd be good to be consistent

I had committed this change into our amd-staging-drm-next branch last
week after I got the ACK and RB from Felix and Christian.

here across drivers. Or at least across drm drivers. And we've had the vma
open/close refcounting to make fork work since forever.

I think if we do this we should really only do this for mmap() where this
applies, but reading through the thread here I'm honestly confused why
this is a problem. If CRIU can't handle forked mmaps it needs to be
thought that, not hacked around. Or at least I'm not understanding why
this shouldn't work ...
-Daniel

Hi Daniel

In the v2
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2Fa1a865f5-ad2c-29c8-cbe4-2635d53eceb6%40amd.com%2FT%2Fdata=04%7C01%7Crajneesh.bhardwaj%40amd.com%7Ce4634a16c37149da173408d9c58d1338%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637758031981907821%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=h0z4sO19bsJecMqeHGdz%2BHZElKuyzK%2BW%2FMbLWA79I10%3Dreserved=0
I pretty much limited the scope of the change to KFD BOs on mmap. Regarding
CRIU, I think its not a CRIU problem as CRIU on restore, only tries to
recreate all the child processes and then mmaps all the VMAs it sees (as per
checkpoint snapshot) in the new process address space after the VMA
placements are finalized in the position independent code phase. Since the
inherited VMAs don't have access rights the criu mmap fails.

Still sounds funky. I think minimally we should have an ack from CRIU
developers that this is officially the right way to solve this problem. I
really don't want to have random one-off hacks that don't work across the
board, for a problem where we (drm subsystem) really shouldn't be the only
one with this problem. Where "this problem" means that the mmap space is
per file description, and not per underlying inode or real device or
whatever. That part sounds like a CRIU problem, and I expect CRIU folks
want a consistent solution across the board for this. Hence please grab an
ack from them.

Cheers, Daniel

Maybe Adrian can share his views on this.

Hi Adrian - For the context, on CRIU restore we see mmap failures ( in
PIE restore phase) due to permission issues on the (render node) VMAs
that were inherited since the application that check pointed had
forked. The VMAs ideally should not be in the child process but the
smaps file shows these VMAs in the child address space. We didn't want
to use madvise to avoid this copy and rather change in the kernel mode
to limit the impact to our user space library thunk. Based on my
understanding, during PIE restore phase, after the VMA placements are
finalized, CRIU does a sys_mmap on all the VMA it sees in the VmaEntry
list and I think its not an issue as per CRIU design but do you think
we could handle this corner case better inside CRIU?

Regards,

Rajneesh

Regards,
Christian.

Regards,
Felix

Regards,
Christian.

Am 09.12.21 um 16:29 schrieb Bhardwaj, Rajneesh:

Sounds good. I will send a v2 with only ttm_bo_mmap_obj change. Thank
you!

On 12/9/2021 10:27 AM, Christian König wrote:

Hi Rajneesh,

yes, separating this from the drm_gem_mmap_obj() change is certainly
a good idea.

The child cannot access the BOs mapped by the parent anyway with
access restrictions applied

exactly that is not correct. That behavior is actively used by some
userspace stacks as far as I know.

Regards,
Christian.

Am 09.12.21 um 16:23 schrieb Bhardwaj, Rajneesh:

Thanks Christian. Would it make it less intrusive if I just use the
flag for ttm bo mmap and remove the drm_gem_mmap_obj change from
this patch? For our use case, just the ttm_bo_mmap_obj change
should suffice and we don't want to put any more work arounds in
the user space (thunk, in our case).

The child cannot access the BOs mapped by the parent anyway with
access

Re: [PATCH] drm/ttm: Don't inherit GEM object VMAs in child process

2021-12-22 Thread Bhardwaj, Rajneesh

Adding Adrian Rebel who is the CRIU maintainer and CRIU list

On 12/22/2021 3:53 PM, Daniel Vetter wrote:

On Mon, Dec 20, 2021 at 01:12:51PM -0500, Bhardwaj, Rajneesh wrote:

On 12/20/2021 4:29 AM, Daniel Vetter wrote:

On Fri, Dec 10, 2021 at 07:58:50AM +0100, Christian König wrote:

Am 09.12.21 um 19:28 schrieb Felix Kuehling:

Am 2021-12-09 um 10:30 a.m. schrieb Christian König:

That still won't work.

But I think we could do this change for the amdgpu mmap callback only.

If graphics user mode has problems with it, we could even make this
specific to KFD BOs in the amdgpu_gem_object_mmap callback.

I think it's fine for the whole amdgpu stack, my concern is more about
radeon, nouveau and the ARM stacks which are using this as well.

That blew up so nicely the last time we tried to change it and I know of at
least one case where radeon was/is used with BOs in a child process.

I'm way late and burried again, but I think it'd be good to be consistent

I had committed this change into our amd-staging-drm-next branch last
week after I got the ACK and RB from Felix and Christian.

here across drivers. Or at least across drm drivers. And we've had the vma
open/close refcounting to make fork work since forever.

Hi Daniel

Cheers, Daniel

Maybe Adrian can share his views on this.

Hi Adrian - For the context, on CRIU restore we see mmap failures ( in
PIE restore phase) due to permission issues on the (render node) VMAs
that were inherited since the application that check pointed had
forked. The VMAs ideally should not be in the child process but the
smaps file shows these VMAs in the child address space. We didn't want
to use madvise to avoid this copy and rather change in the kernel mode
to limit the impact to our user space library thunk. Based on my
understanding, during PIE restore phase, after the VMA placements are
finalized, CRIU does a sys_mmap on all the VMA it sees in the VmaEntry
list and I think its not an issue as per CRIU design but do you think we
could handle this corner case better inside CRIU?

Regards,

Rajneesh

Regards,
Christian.

Regards,
Felix

Regards,
Christian.

Am 09.12.21 um 16:29 schrieb Bhardwaj, Rajneesh:

Sounds good. I will send a v2 with only ttm_bo_mmap_obj change. Thank
you!

On 12/9/2021 10:27 AM, Christian König wrote:

Hi Rajneesh,

yes, separating this from the drm_gem_mmap_obj() change is certainly
a good idea.

The child cannot access the BOs mapped by the parent anyway with
access restrictions applied

exactly that is not correct. That behavior is actively used by some
userspace stacks as far as I know.

Regards,
Christian.

Am 09.12.21 um 16:23 schrieb Bhardwaj, Rajneesh:

The child cannot access the BOs mapped by the parent anyway with
access restrictions applied so I wonder why even inherit the vma?

On 12/9/2021 2:54 AM, Christian König wrote:

Am 08.12.21 um

[PATCH] drm/i915/guc: Report error on invalid reset notification

2021-12-22 Thread John . C . Harrison

From: John Harrison 

Don't silently drop reset notifications from the GuC. It might not be
safe to do an error capture but we still want some kind of report that
the reset happened.

Signed-off-by: John Harrison 
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index e7517206af82..0fbf24b8d5e1 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -3979,6 +3979,11 @@ static void guc_handle_context_reset(struct intel_guc 
*guc,
   !context_blocked(ce))) {
capture_error_state(guc, ce);
guc_context_replay(ce);
+   } else {
+   drm_err(_to_gt(guc)->i915->drm,
+   "Invalid GuC engine reset notificaion for 0x%04X on %s: 
banned = %d, blocked = %d",
+   ce->guc_id.id, ce->engine->name, 
intel_context_is_banned(ce),
+   context_blocked(ce));
}
 }
 
-- 
2.25.1

Re: [PATCH] drm/i915/guc: Use lockless list for destroyed contexts

2021-12-22 Thread Matthew Brost

On Wed, Dec 22, 2021 at 04:48:36PM -0800, John Harrison wrote:
> On 12/22/2021 15:29, Matthew Brost wrote:
> > Use a lockless list structure for destroyed contexts to avoid hammering
> > on global submission spin lock.
> I thought the guidance was that lockless anything without an explanation
> longer than War And Peace comes with an automatic termination penalty?
> 

I was thinking that was for custom lockless algorithms not using core
uAPIs. If this is really concern I could protect the llist_del_all by a
lock but the doc explicitly says how I'm using this uAPI is safe without
a lock. 

> Also, I thought the simple suggestion was to just move the entire list
> sideways under the existing lock and then loop through the local list safely
> without requiring locks because it is now local only.
> 

That's basically what this uAPI does in a few simple calls rather than
our own algorithm to move to a new list.

Matt

> John.
> 
> 
> > 
> > Suggested-by: Tvrtko Ursulin 
> > Signed-off-by: Matthew Brost 
> > ---
> >   drivers/gpu/drm/i915/gt/intel_context.c   |  2 -
> >   drivers/gpu/drm/i915/gt/intel_context_types.h |  3 +-
> >   drivers/gpu/drm/i915/gt/uc/intel_guc.h|  3 +-
> >   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 44 +--
> >   4 files changed, 16 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c 
> > b/drivers/gpu/drm/i915/gt/intel_context.c
> > index 5d0ec7c49b6a..4aacb4b0418d 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> > @@ -403,8 +403,6 @@ intel_context_init(struct intel_context *ce, struct 
> > intel_engine_cs *engine)
> > ce->guc_id.id = GUC_INVALID_LRC_ID;
> > INIT_LIST_HEAD(>guc_id.link);
> > -   INIT_LIST_HEAD(>destroyed_link);
> > -
> > INIT_LIST_HEAD(>parallel.child_list);
> > /*
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h 
> > b/drivers/gpu/drm/i915/gt/intel_context_types.h
> > index 30cd81ad8911..4532d43ec9c0 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> > @@ -9,6 +9,7 @@
> >   #include 
> >   #include 
> >   #include 
> > +#include 
> >   #include 
> >   #include 
> > @@ -224,7 +225,7 @@ struct intel_context {
> >  * list when context is pending to be destroyed (deregistered with the
> >  * GuC), protected by guc->submission_state.lock
> >  */
> > -   struct list_head destroyed_link;
> > +   struct llist_node destroyed_link;
> > /** @parallel: sub-structure for parallel submission members */
> > struct {
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > index f9240d4baa69..705085058411 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > @@ -8,6 +8,7 @@
> >   #include 
> >   #include 
> > +#include 
> >   #include "intel_uncore.h"
> >   #include "intel_guc_fw.h"
> > @@ -112,7 +113,7 @@ struct intel_guc {
> >  * @destroyed_contexts: list of contexts waiting to be destroyed
> >  * (deregistered with the GuC)
> >  */
> > -   struct list_head destroyed_contexts;
> > +   struct llist_head destroyed_contexts;
> > /**
> >  * @destroyed_worker: worker to deregister contexts, need as we
> >  * need to take a GT PM reference and can't from destroy
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > index 0a03a30e4c6d..6f7643edc139 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -1771,7 +1771,7 @@ int intel_guc_submission_init(struct intel_guc *guc)
> > spin_lock_init(>submission_state.lock);
> > INIT_LIST_HEAD(>submission_state.guc_id_list);
> > ida_init(>submission_state.guc_ids);
> > -   INIT_LIST_HEAD(>submission_state.destroyed_contexts);
> > +   init_llist_head(>submission_state.destroyed_contexts);
> > INIT_WORK(>submission_state.destroyed_worker,
> >   destroyed_worker_func);
> > @@ -2696,26 +2696,18 @@ static void __guc_context_destroy(struct 
> > intel_context *ce)
> > }
> >   }
> > +#define take_destroyed_contexts(guc) \
> > +   llist_del_all(>submission_state.destroyed_contexts)
> > +
> >   static void guc_flush_destroyed_contexts(struct intel_guc *guc)
> >   {
> > -   struct intel_context *ce;
> > -   unsigned long flags;
> > +   struct intel_context *ce, *cn;
> > GEM_BUG_ON(!submission_disabled(guc) &&
> >guc_submission_initialized(guc));
> > -   while (!list_empty(>submission_state.destroyed_contexts)) {
> > -   spin_lock_irqsave(>submission_state.lock, flags);
> > -   ce = 
> > list_first_entry_or_null(>submission_state.destroyed_contexts,
> > - struct

Re: [PATCH] drm/i915/execlists: Weak parallel submission support for execlists

2021-12-22 Thread John Harrison


On 12/22/2021 14:35, Matthew Brost wrote:

A weak implementation of parallel submission (multi-bb execbuf IOCTL) for
execlists. Doing as little as possible to support this interface for
execlists - basically just passing submit fences between each request
generated and virtual engines are not allowed. This is on par with what
is there for the existing (hopefully soon deprecated) bonding interface.

We perma-pin these execlists contexts to align with GuC implementation.

v2:
  (John Harrison)
   - Drop siblings array as num_siblings must be 1
v3:
  (John Harrison)
   - Drop single submission
v4:
  (John Harrison)
   - Actually drop single submission
   - Use IS_ERR check on return value from intel_context_create
   - Set last request to NULL on unpin

Signed-off-by: Matthew Brost 
---
  drivers/gpu/drm/i915/gem/i915_gem_context.c   | 11 --
  drivers/gpu/drm/i915/gt/intel_context.c   |  4 +-
  .../drm/i915/gt/intel_execlists_submission.c  | 38 +++
  drivers/gpu/drm/i915/gt/intel_lrc.c   |  4 ++
  .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  2 -
  5 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index cad3f0b2be9e..b0d2d81fc3b3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -570,10 +570,6 @@ set_proto_ctx_engines_parallel_submit(struct 
i915_user_extension __user *base,
struct intel_engine_cs **siblings = NULL;
intel_engine_mask_t prev_mask;
  
-	/* FIXME: This is NIY for execlists */

-   if (!(intel_uc_uses_guc_submission(_gt(i915)->uc)))
-   return -ENODEV;
-
if (get_user(slot, >engine_index))
return -EFAULT;
  
@@ -583,6 +579,13 @@ set_proto_ctx_engines_parallel_submit(struct i915_user_extension __user *base,

if (get_user(num_siblings, >num_siblings))
return -EFAULT;
  
+	if (!intel_uc_uses_guc_submission(_gt(i915)->uc) &&

+   num_siblings != 1) {
+   drm_dbg(>drm, "Only 1 sibling (%d) supported in non-GuC 
mode\n",
+   num_siblings);
+   return -EINVAL;
+   }
+
if (slot >= set->num_engines) {
drm_dbg(>drm, "Invalid placement value, %d >= %d\n",
slot, set->num_engines);
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c 
b/drivers/gpu/drm/i915/gt/intel_context.c
index ba083d800a08..5d0ec7c49b6a 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -79,7 +79,8 @@ static int intel_context_active_acquire(struct intel_context 
*ce)
  
  	__i915_active_acquire(>active);
  
-	if (intel_context_is_barrier(ce) || intel_engine_uses_guc(ce->engine))

+   if (intel_context_is_barrier(ce) || intel_engine_uses_guc(ce->engine) ||
+   intel_context_is_parallel(ce))
return 0;
  
  	/* Preallocate tracking nodes */

@@ -563,7 +564,6 @@ void intel_context_bind_parent_child(struct intel_context 
*parent,
 * Callers responsibility to validate that this function is used
 * correctly but we use GEM_BUG_ON here ensure that they do.
 */
-   GEM_BUG_ON(!intel_engine_uses_guc(parent->engine));
GEM_BUG_ON(intel_context_is_pinned(parent));
GEM_BUG_ON(intel_context_is_child(parent));
GEM_BUG_ON(intel_context_is_pinned(child));
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c 
b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index a69df5e9e77a..be56d0b41892 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -2599,6 +2599,43 @@ static void execlists_context_cancel_request(struct 
intel_context *ce,
  current->comm);
  }
  
+static struct intel_context *

+execlists_create_parallel(struct intel_engine_cs **engines,
+ unsigned int num_siblings,
+ unsigned int width)
+{
+   struct intel_context *parent = NULL, *ce, *err;
+   int i;
+
+   GEM_BUG_ON(num_siblings != 1);
+
+   for (i = 0; i < width; ++i) {
+   ce = intel_context_create(engines[i]);
+   if (IS_ERR(ce)) {
+   err = ce;

Could get rid of 'err' and just say 'return ce;' at the end of 'unwind:'.

Either way:
Reviewed-by: John Harrison 



+   goto unwind;
+   }
+
+   if (i == 0)
+   parent = ce;
+   else
+   intel_context_bind_parent_child(parent, ce);
+   }
+
+   parent->parallel.fence_context = dma_fence_context_alloc(1);
+
+   intel_context_set_nopreempt(parent);
+   for_each_child(parent, ce)
+   intel_context_set_nopreempt(ce);
+
+   return parent;
+
+unwind:
+   if (parent)
+

[PATCH v9 2/2] drm/msm/dp: do not initialize phy until plugin interrupt received

2021-12-22 Thread Kuogee Hsieh

Current DP drivers have regulators, clocks, irq and phy are grouped
together within a function and executed not in a symmetric manner.
This increase difficulty of code maintenance and limited code scalability.
This patch divides the driver life cycle of operation into four states,
resume (including booting up), dongle plugin, dongle unplugged and suspend.
Regulators, core clocks and irq are grouped together and enabled at resume
(or booting up) so that the DP controller is armed and ready to receive HPD
plugin interrupts. HPD plugin interrupt is generated when a dongle plugs
into DUT (device under test). Once HPD plugin interrupt is received, DP
controller will initialize phy so that dpcd read/write will function and
following link training can be proceeded successfully. DP phy will be
disabled after main link is teared down at end of unplugged HPD interrupt
handle triggered by dongle unplugged out of DUT. Finally regulators, code
clocks and irq are disabled at corresponding suspension.

Changes in V2:
-- removed unnecessary dp_ctrl NULL check
-- removed unnecessary phy init_count and power_count DRM_DEBUG_DP logs
-- remove flip parameter out of dp_ctrl_irq_enable()
-- add fixes tag

Changes in V3:
-- call dp_display_host_phy_init() instead of dp_ctrl_phy_init() at
dp_display_host_init() for eDP

Changes in V4:
-- rewording commit text to match this commit changes

Changes in V5:
-- rebase on top of msm-next branch

Changes in V6:
-- delete flip variable

Changes in V7:
-- dp_ctrl_irq_enable/disabe() merged into dp_ctrl_reset_irq_ctrl()

Changes in V8:
-- add more detail comment regrading dp phy at dp_display_host_init()

Changes in V9:
-- remove set phy_initialized to false when -ECONNRESET detected

Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon 
Chipsets")
Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c| 80 +
 drivers/gpu/drm/msm/dp/dp_ctrl.h|  8 ++--
 drivers/gpu/drm/msm/dp/dp_display.c | 89 -
 3 files changed, 94 insertions(+), 83 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index c724cb0..9c80b49 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1365,60 +1365,44 @@ static int dp_ctrl_enable_stream_clocks(struct 
dp_ctrl_private *ctrl)
return ret;
 }
 
-int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
+void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable)
+{
+   struct dp_ctrl_private *ctrl;
+
+   ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+   dp_catalog_ctrl_reset(ctrl->catalog);
+
+   if (enable)
+   dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
+}
+
+void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
 {
struct dp_ctrl_private *ctrl;
struct dp_io *dp_io;
struct phy *phy;
 
-   if (!dp_ctrl) {
-   DRM_ERROR("Invalid input data\n");
-   return -EINVAL;
-   }
-
ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
dp_io = >parser->io;
phy = dp_io->phy;
 
-   ctrl->dp_ctrl.orientation = flip;
-
-   if (reset)
-   dp_catalog_ctrl_reset(ctrl->catalog);
-
-   DRM_DEBUG_DP("flip=%d\n", flip);
dp_catalog_ctrl_phy_reset(ctrl->catalog);
phy_init(phy);
-   dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
-
-   return 0;
 }
 
-/**
- * dp_ctrl_host_deinit() - Uninitialize DP controller
- * @dp_ctrl: Display Port Driver data
- *
- * Perform required steps to uninitialize DP controller
- * and its resources.
- */
-void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
+void dp_ctrl_phy_exit(struct dp_ctrl *dp_ctrl)
 {
struct dp_ctrl_private *ctrl;
struct dp_io *dp_io;
struct phy *phy;
 
-   if (!dp_ctrl) {
-   DRM_ERROR("Invalid input data\n");
-   return;
-   }
-
ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
dp_io = >parser->io;
phy = dp_io->phy;
 
-   dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
+   dp_catalog_ctrl_phy_reset(ctrl->catalog);
phy_exit(phy);
-
-   DRM_DEBUG_DP("Host deinitialized successfully\n");
 }
 
 static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *ctrl)
@@ -1488,7 +1472,10 @@ static int dp_ctrl_deinitialize_mainlink(struct 
dp_ctrl_private *ctrl)
}
 
phy_power_off(phy);
+
+   /* aux channel down, reinit phy */
phy_exit(phy);
+   phy_init(phy);
 
return 0;
 }
@@ -1893,8 +1880,14 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
return ret;
}
 
+   DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
+   (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
+
phy_power_off(phy);
 
+   DRM_DEBUG_DP("After, phy=%x init_count=%d

[PATCH v9 1/2] drm/msm/dp: dp_link_parse_sink_count() return immediately if aux read failed

2021-12-22 Thread Kuogee Hsieh

Add checking aux read/write status at both dp_link_parse_sink_count()
and dp_link_parse_sink_status_filed() to avoid long timeout delay if
dp aux read/write failed at timeout due to cable unplugged. Also make
sure dp controller had been initialized before start dpcd read and write.

Changes in V4:
-- split this patch as stand alone patch

Changes in v5:
-- rebase on msm-next branch

Changes in v6:
-- add more details commit text

Signed-off-by: Kuogee Hsieh 
Reviewed-by: Stephen Boyd 
Tested-by: Stephen Boyd 
---
 drivers/gpu/drm/msm/dp/dp_display.c | 12 +---
 drivers/gpu/drm/msm/dp/dp_link.c| 19 ++-
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 3d61459..0766752 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -692,9 +692,15 @@ static int dp_irq_hpd_handle(struct dp_display_private 
*dp, u32 data)
return 0;
}
 
-   ret = dp_display_usbpd_attention_cb(>pdev->dev);
-   if (ret == -ECONNRESET) { /* cable unplugged */
-   dp->core_initialized = false;
+   /*
+* dp core (ahb/aux clks) must be initialized before
+* irq_hpd be handled
+*/
+   if (dp->core_initialized) {
+   ret = dp_display_usbpd_attention_cb(>pdev->dev);
+   if (ret == -ECONNRESET) { /* cable unplugged */
+   dp->core_initialized = false;
+   }
}
DRM_DEBUG_DP("hpd_state=%d\n", state);
 
diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
index a5bdfc5..d4d31e5 100644
--- a/drivers/gpu/drm/msm/dp/dp_link.c
+++ b/drivers/gpu/drm/msm/dp/dp_link.c
@@ -737,18 +737,25 @@ static int dp_link_parse_sink_count(struct dp_link 
*dp_link)
return 0;
 }
 
-static void dp_link_parse_sink_status_field(struct dp_link_private *link)
+static int dp_link_parse_sink_status_field(struct dp_link_private *link)
 {
int len = 0;
 
link->prev_sink_count = link->dp_link.sink_count;
-   dp_link_parse_sink_count(>dp_link);
+   len = dp_link_parse_sink_count(>dp_link);
+   if (len < 0) {
+   DRM_ERROR("DP parse sink count failed\n");
+   return len;
+   }
 
len = drm_dp_dpcd_read_link_status(link->aux,
link->link_status);
-   if (len < DP_LINK_STATUS_SIZE)
+   if (len < DP_LINK_STATUS_SIZE) {
DRM_ERROR("DP link status read failed\n");
-   dp_link_parse_request(link);
+   return len;
+   }
+
+   return dp_link_parse_request(link);
 }
 
 /**
@@ -1023,7 +1030,9 @@ int dp_link_process_request(struct dp_link *dp_link)
 
dp_link_reset_data(link);
 
-   dp_link_parse_sink_status_field(link);
+   ret = dp_link_parse_sink_status_field(link);
+   if (ret)
+   return ret;
 
if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] drm/i915/guc: Use lockless list for destroyed contexts

2021-12-22 Thread John Harrison


On 12/22/2021 15:29, Matthew Brost wrote:

Use a lockless list structure for destroyed contexts to avoid hammering
on global submission spin lock.
I thought the guidance was that lockless anything without an explanation 
longer than War And Peace comes with an automatic termination penalty?


Also, I thought the simple suggestion was to just move the entire list 
sideways under the existing lock and then loop through the local list 
safely without requiring locks because it is now local only.


John.




Suggested-by: Tvrtko Ursulin 
Signed-off-by: Matthew Brost 
---
  drivers/gpu/drm/i915/gt/intel_context.c   |  2 -
  drivers/gpu/drm/i915/gt/intel_context_types.h |  3 +-
  drivers/gpu/drm/i915/gt/uc/intel_guc.h|  3 +-
  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 44 +--
  4 files changed, 16 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c 
b/drivers/gpu/drm/i915/gt/intel_context.c
index 5d0ec7c49b6a..4aacb4b0418d 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -403,8 +403,6 @@ intel_context_init(struct intel_context *ce, struct 
intel_engine_cs *engine)
ce->guc_id.id = GUC_INVALID_LRC_ID;
INIT_LIST_HEAD(>guc_id.link);
  
-	INIT_LIST_HEAD(>destroyed_link);

-
INIT_LIST_HEAD(>parallel.child_list);
  
  	/*

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h 
b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 30cd81ad8911..4532d43ec9c0 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -9,6 +9,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  
@@ -224,7 +225,7 @@ struct intel_context {

 * list when context is pending to be destroyed (deregistered with the
 * GuC), protected by guc->submission_state.lock
 */
-   struct list_head destroyed_link;
+   struct llist_node destroyed_link;
  
  	/** @parallel: sub-structure for parallel submission members */

struct {
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index f9240d4baa69..705085058411 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -8,6 +8,7 @@
  
  #include 

  #include 
+#include 
  
  #include "intel_uncore.h"

  #include "intel_guc_fw.h"
@@ -112,7 +113,7 @@ struct intel_guc {
 * @destroyed_contexts: list of contexts waiting to be destroyed
 * (deregistered with the GuC)
 */
-   struct list_head destroyed_contexts;
+   struct llist_head destroyed_contexts;
/**
 * @destroyed_worker: worker to deregister contexts, need as we
 * need to take a GT PM reference and can't from destroy
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 0a03a30e4c6d..6f7643edc139 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -1771,7 +1771,7 @@ int intel_guc_submission_init(struct intel_guc *guc)
spin_lock_init(>submission_state.lock);
INIT_LIST_HEAD(>submission_state.guc_id_list);
ida_init(>submission_state.guc_ids);
-   INIT_LIST_HEAD(>submission_state.destroyed_contexts);
+   init_llist_head(>submission_state.destroyed_contexts);
INIT_WORK(>submission_state.destroyed_worker,
  destroyed_worker_func);
  
@@ -2696,26 +2696,18 @@ static void __guc_context_destroy(struct intel_context *ce)

}
  }
  
+#define take_destroyed_contexts(guc) \

+   llist_del_all(>submission_state.destroyed_contexts)
+
  static void guc_flush_destroyed_contexts(struct intel_guc *guc)
  {
-   struct intel_context *ce;
-   unsigned long flags;
+   struct intel_context *ce, *cn;
  
  	GEM_BUG_ON(!submission_disabled(guc) &&

   guc_submission_initialized(guc));
  
-	while (!list_empty(>submission_state.destroyed_contexts)) {

-   spin_lock_irqsave(>submission_state.lock, flags);
-   ce = 
list_first_entry_or_null(>submission_state.destroyed_contexts,
- struct intel_context,
- destroyed_link);
-   if (ce)
-   list_del_init(>destroyed_link);
-   spin_unlock_irqrestore(>submission_state.lock, flags);
-
-   if (!ce)
-   break;
-
+   llist_for_each_entry_safe(ce, cn, take_destroyed_contexts(guc),
+destroyed_link) {
release_guc_id(guc, ce);
__guc_context_destroy(ce);
}
@@ -2723,23 +2715,11 @@ static void guc_flush_destroyed_contexts(struct 
intel_guc *guc)
  
  static void deregister_destroyed_contexts(struct

1 2 >

1 - 100 of 148 matches

Mail list logo