On Wed, Feb 01, 2023 at 10:37:06AM -0800, John Harrison wrote:
> On 2/1/2023 07:31, Rodrigo Vivi wrote:
> > On Wed, Feb 01, 2023 at 03:11:31PM +1100, Stephen Rothwell wrote:
> > > Hi all,
> > >
> > > On Tue, 31 Jan 2023 10:27:29 -0800 John Harrison <john.c.harrison@xxxxxxxxx> wrote:
> > > > On 1/31/2023 04:44, Andy Shevchenko wrote:
> > > > > On Tue, Jan 31, 2023 at 01:03:05PM +1100, Stephen Rothwell wrote:
> > > > > > Today's linux-next merge of the usb tree got a conflict in:
> > > > > >
> > > > > > drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > > > >
> > > > > > between commit:
> > > > > >
> > > > > > 5bc4b43d5c6c ("drm/i915: Fix up locking around dumping requests lists")
> > > > > >
> > > > > > from the drm-intel-fixes tree and commit:
> > > > > >
> > > > > > 4d70c74659d9 ("i915: Move list_count() to list.h as list_count_nodes() for broader use")
> > > > > >
> > > > > > from the usb tree.
> > > > > >
> > > > > > I fixed it up (the former removed the code changed by the latter)
> > > > > Hmm... Currently I see that 20230127002842.3169194-4-John.C.Harrison@xxxxxxxxx
> > > > > moves the code to the drivers/gpu/drm/i915/gt/intel_execlists_submission.c.
> > > > >
> > > > > Is there any new series beside the above mentioned that touches that file and
> > > > > actually _removes_ that code?
> > > > As long as the removal is limited to list_count/list_count_nodes,
> > > > that's fine. I only moved it from one file to another because the one
> > > > and only function that was using it was being moved to the other
> > > > file. If someone else has found a use for the same and wants to move
> > > > it to a more common place then great. I assume there was no conflict
> > > > happening in the i915 specific code.
> > > I have added this fix up patch to linux-next today (more or less - this
> > > is a hand hacked version, but you get the idea):
> > >
> > > From: Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx>
> > > Date: Wed, 1 Feb 2023 13:13:01 +1100
> > > Subject: [PATCH] i915: fix up for "drm/i915: Fix up locking around dumping requests lists"
> > >
> > > interacting with "i915: Move list_count() to list.h as list_count_nodes() for broader use"
> > >
> > > Signed-off-by: Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx>
> > > ---
> > > .../gpu/drm/i915/gt/intel_execlists_submission.c | 15 +------------
> > > 1 file changed, 2 insertion(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > > index 3c573d41d404..e919d41a48d9 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > > @@ -4150,17 +4150,6 @@ void intel_execlists_show_requests(struct intel_engine_cs *engine,
> > > spin_unlock_irqrestore(&sched_engine->lock, flags);
> > > }
> > > -static unsigned long list_count(struct list_head *list)
> > > -{
> > > - struct list_head *pos;
> > > - unsigned long count = 0;
> > > -
> > > - list_for_each(pos, list)
> > > - count++;
> > > -
> > > - return count;
> > > -}
> > > -
> > > void intel_execlists_dump_active_requests(struct intel_engine_cs *engine,
> > > struct i915_request *hung_rq,
> > > struct drm_printer *m)
> > > @@ -4172,7 +4161,7 @@ void intel_execlists_dump_active_requests(struct intel_engine_cs *engine,
> > > intel_engine_dump_active_requests(&engine->sched_engine->requests, hung_rq, m);
> > > - drm_printf(m, "\tOn hold?: %lu\n",
> > > - list_count(&engine->sched_engine->hold));
> > > + drm_printf(m, "\tOn hold?: %zu\n",
> > > + list_count_nodes(&engine->sched_engine->hold));
> > something awkward here.
> > The resolution on linux-next should align with the resolution on drm-tip
> > where we have the list_count still there as we preferred the version
> > on drm-intel-gt-next as the resolution of the conflict instead of the
> > fixes one.
> Not following why you want to keep list_count as a local function in the
> i915 driver? Surely the correct fix is to move it to the common header and
> share the code? In which case, the correct name is list_count_nodes() as
> that is what got merged to the common header.
right. please ignore my previous email and accept my apologies for the
unnecessary noise. I had just read the commit '4d70c74659d9 ("i915: Move
list_count() to list.h as list_count_nodes() for broader use")'
and it now the final resolution makes total sense. And that patch had
been reviewed and acked by us, so everything is good.
I just confused with other conflict that we have with our on gt-next
and -fixes tree but with an easier resolution.
Sorry,
Rodrigo.
>
> John.
>
> >
> > > spin_unlock_irqrestore(&engine->sched_engine->lock, flags);
> > > }
> > > --
> > > 2.35.1
> > >
> > > --
> > > Cheers,
> > > Stephen Rothwell
> >
>
