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