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
