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

[Tvrtko Ursulin]

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