linux-kernel - Re: [PATCHv5,RESEND 2/8] gpu: host1x: Add syncpoint wait and interrupts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130206203854.GA1012@avionic-0098.mockup.avionic-design.de>
Date:	Wed, 6 Feb 2013 21:38:54 +0100
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Terje Bergström <tbergstrom@...dia.com>
Cc:	Arto Merilainen <amerilainen@...dia.com>,
	"airlied@...ux.ie" <airlied@...ux.ie>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv5,RESEND 2/8] gpu: host1x: Add syncpoint wait and
 interrupts

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

Content of type "application/pgp-signature" skipped