[Intel-gfx] [RFC PATCH 00/97] Basic GuC submission support in the i915

[Matthew Brost]

On Thu, Jun 03, 2021 at 09:51:19AM +0100, Tvrtko Ursulin wrote:
> 
> On 03/06/2021 05:10, Matthew Brost wrote:
> > On Wed, Jun 02, 2021 at 04:27:18PM +0100, Tvrtko Ursulin wrote:
> > > 
> > > On 25/05/2021 17:45, Matthew Brost wrote:
> 
> [snip]
> 
> > > > >    * Kludgy way of interfacing with rest of the driver instead of refactoring
> > > > > to fit (idling, breadcrumbs, scheduler, tasklets, ...).
> > > > > 
> > > > 
> > > > Idling and breadcrumbs seem clean to me. Scheduler + tasklet are going
> > > > away once the DRM scheduler lands. No need rework those as we are just
> > > > going to rework this again.
> > > 
> > > Well today I read the breadcrumbs patch and there is no way that's clean. It
> > > goes and creates one object per engine, then deletes them, replacing with
> > > GuC special one. All in the same engine setup. The same pattern of bolting
> > > on the GuC repeats too much for my taste.
> > > 
> > 
> > I don't think creating a default object /w a ref count then decrementing
> > the ref count + replacing it with a new object is that hard to
> > understand. IMO that is way better than how things worked previously
> 
> It's not about it being hard to understand, although it certainly is far
> from the usual patterns, but about it being lazy design which in normal
> times would never be allowed. Because reduced and flattened to highlight the
> principal complaint it looks like this:
> 
> engine_setup_for_each_engine:
>    engine->breadcrumbs = create_breadcrumbs();
>    if (guc) {
>       if (!first_class_engine) {
>         kfree(engine->breadcrumbs);
>         engine->breadcrumbs = first_class_engine->breadcrumbs;
>       } else {
>         first_class_engine->breadcrumbs->vfuncs = guc_vfuncs;
>       }
>    }
> 

I think are diving way to deep into individual patches on the cover
letter.

Agree this could be refactored bit more. Let me try a rework on this
patch in particular before this patch gets posted again.

Matt 

> What I suggested is that patch should not break and hack the object oriented
> design and instead could do along the lines:
> 
> gt_guc_setup:
>    for_each_class:
>       gt->uc.breadcrumbs[class] = create_guc_breadcrumbs();
> 
> engine_setup_for_each_engine:
>    if (guc)
>       engine->breadcrumbs = get(gt->uc.breadcrumbs[class]);
>    else
>       engine->breadcrumbs = create_breadcrumbs();
> 
> > where we just made implicit assumptions all over the driver of the
> > execlists backend behavior. If this was done properly in the current
> > i915 code base this really wouldn't be an issue.
> 
> Don't really follow you here but it probably goes back to how upstream code
> was there available to be refactored all this time.
> 
> Regards,
> 
> Tvrtko