Re: [PATCHv5,RESEND 2/8] gpu: host1x: Add syncpoint wait andinterrupts

[Thierry Reding]

On Wed, Feb 06, 2013 at 12:29:26PM -0800, Terje BergstrÃm wrote:
> On 05.02.2013 00:42, Thierry Reding wrote:
[...]
> > Or if that doesn't work it would still be preferable to allocate memory
> > in host1x_syncpt_wait() directly instead of going through the wrapper.
> 
> This was done purely, because I'm hiding the struct size from the
> caller. If the caller needs to allocate, I need to expose the struct in
> a header, not just a forward declaration.

I don't think we need to hide the struct from the caller. This is all
host1x internal. Even if a host1x client uses the struct it makes little
sense to hide it. They are all part of the same code base so there's not
much to be gained by hiding the structure definition.

> >>>> +void host1x_intr_stop(struct host1x_intr *intr)
> >>>> +{
> >>>> +     unsigned int id;
> >>>> +     struct host1x *host1x = intr_to_host1x(intr);
> >>>> +     struct host1x_intr_syncpt *syncpt;
> >>>> +     u32 nb_pts = host1x_syncpt_nb_pts(intr_to_host1x(intr));
> >>>> +
> >>>> +     mutex_lock(&intr->mutex);
> >>>> +
> >>>> +     host1x->intr_op.disable_all_syncpt_intrs(intr);
> >>>
> >>> I haven't commented on this everywhere, but I think this could benefit
> >>> from a wrapper that forwards this to the intr_op. The same goes for the
> >>> sync_op.
> >>
> >> You mean something like "host1x_disable_all_syncpt_intrs"?
> > 
> > Yes. I think that'd be useful for each of the op functions. Perhaps you
> > could even pass in a struct host1x * to make calls more uniform.
> 
> Ok, I'll add the wrapper, and I'll check if passing struct host1x *
> would make sense. In effect that'd render struct host1x_intr mostly
> unused, so how about if we just merge the contents of host1x_intr to host1x?

We can probably do that. It might make some sense to keep it in order to
scope the related fields but struct host1x isn't very large yet, so I
think omitting host1x_intr should be fine.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature