On pe, 2017-02-10 at 12:03 -0800, Daniele Ceraolo Spurio wrote:
> <snip>
> >
> > >
> > > +
> > > /*
> > > * Tell the GuC to allocate or deallocate a specific doorbell
> > > */
> > >
> > > -static int guc_allocate_doorbell(struct intel_guc *guc,
> > > - struct i915_guc_client *client)
> > > +static int __create_doorbell_hw(struct i915_guc_client *client)
> >
> > I would rather prefer to only change signature of this function into
> >
> > static int guc_allocate_doorbell(struct intel_guc *guc, u32 index)
> >
> > as a clean wrap around GUC_ACTION_ALLOCATE_DOORBELL. This way we also preserve
> > consistency between function name and the guc action name used inside.
> >
> > Based on the above we can still add
> >
> > static int __create_doorbell_hw(struct i915_guc_client *client)
> > {
> > return guc_allocate_doorbell(client->guc, client->ctx_index);
> > }
> >
> > Note that location of the ctx_index member may change in the future, and this
> > approach will minimize impact of these future changes.
> >
>
> +1.
> The client is a SW abstraction that we use to track our registrations
> with GuC, but it only works with our current mode of operation. If we
> plumb it too deep into the low-level functions it'll be more difficult
> to do any reworks in the future.
Indeed, the ctx_index should move. I'd like to keep it __ prefixed
function, because the client needs the desc updated for the function to
be meaningful.
So I'll make it __guc_{de}allocate_doorbell for now.
Might be good to have i915_guc_doorbell, i915_guc_context,
i915_guc_client in the future to reduce code entangelment.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
