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;
}
}
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
