freedreno header uses not installed xf86atomic.h

[Eric Engestrom]

On Tuesday, 2019-02-19 11:56:21 +0000, Emil Velikov wrote:
> On Tue, 19 Feb 2019 at 10:08, Eric Engestrom <eric.engestrom at intel.com> wrote:
> >
> > On Friday, 2019-02-15 15:08:22 +0000, Emil Velikov via dri-devel wrote:
> > > On Fri, 15 Feb 2019 at 15:06, Rob Clark via dri-devel
> > > <dri-devel at lists.freedesktop.org> wrote:
> > > >
> > > > On Fri, Feb 15, 2019 at 8:42 AM Eric Engestrom <eric.engestrom at intel.com> wrote:
> > > > >
> > > > > On Friday, 2019-02-15 13:36:39 +0000, Eric Engestrom wrote:
> > > > > > On Friday, 2019-02-15 07:11:55 -0500, Rob Clark wrote:
> > > > > > > On Fri, Feb 15, 2019 at 3:55 AM Daniel Drake <drake at endlessm.com> wrote:
> > > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > Using libdrm-2.4.97, mesa fails to build on ARM with:
> > > > > > > >
> > > > > > > > [  456s] In file included from
> > > > > > > > ../../../../../src/gallium/drivers/freedreno/freedreno_util.h:33,
> > > > > > > > [  456s]                  from
> > > > > > > > ../../../../../src/gallium/drivers/freedreno/freedreno_batch.h:34,
> > > > > > > > [  456s]                  from
> > > > > > > > ../../../../../src/gallium/drivers/freedreno/freedreno_context.h:39,
> > > > > > > > [  456s]                  from
> > > > > > > > ../../../../../src/gallium/drivers/freedreno/freedreno_program.c:33:
> > > > > > > > [  456s] /usr/include/freedreno/freedreno_ringbuffer.h:32:10: fatal
> > > > > > > > error: xf86atomic.h: No such file or directory
> > > > > > > >
> > > > > > > > The freedreno headers were recently modified to use xf86atomic.h:
> > > > > > > > https://gitlab.freedesktop.org/mesa/drm/commit/b541d21a0a908bf98d44375720f4430297720743
> > > > > > > >
> > > > > > >
> > > > > > > oh, that union/ifdef hack was specifically to avoid this issue..
> > > > > > > probably the patch removing it should be reverted.
> > > > > >
> > > > > > Right, I messed up with that commit, I didn't realise freedreno_ringbuffer.h
> > > > > > was installed. We need to remove that include.
> > > > > >
> > > > > > That said, I'm confused as to how freedreno_ringbuffer.h users in Mesa
> > > > > > knows whether it's safe to use refcnt from that union?
> > > > > > It doesn't check for HAS_ATOMIC_OPS, so it can't know whether it
> > > > > > contains garbage padding or a refcount, can it?
> > > > >
> > > > > No, that wouldn't even compile?
> > > > >
> > > > > (with the code before my messed up commit:)
> > > > > Mesa includes freedreno_ringbuffer.h but doesn't define HAS_ATOMIC_OPS,
> > > > > so fd_ringbuffer::refcnt doesn't get compiled in (but the padding is
> > > > > still there), so code in mesa can't use ->refcnt because the compiler
> > > > > wouldn't know what that is.
> > > > >
> > > > > I must be missing something, how did this ever compile?
> > > >
> > > > So, these days, mesa has it's own copy of the libdrm code,
> > > > libdrm_freedreno really only exists so that you can still build old
> > > > mesa with new libdrm.  And for a handful of small standalone utilities
> > > > (fdperf, and some test code I use to poke the hw standalone)..
> > > >
> > > > But the way it works is that mesa never needs to access the refcnt, it
> > > > mostly only needs to access cur/end (and it wants to do that in a way
> > > > that can be inlined, not fxn call into a different dso, since that is
> > > > a hot path).  The only code that accesses the refcnt is in the libdrm
> > > > code itself.  Hence this ugly union hack, just to make the struct the
> > > > same size both for mesa and for libdrm.
> > > >
> > > Ouch, I did not see the header was installed either.
> > >
> > > Just skimmed through Mesa prior to the libdrm_freedreno merge - there
> > > are no references of fd_ringbuffer::refcnt.
> > > So a simple revert will do the job. To avoid repeating this mistake,
> > > we want to add an inline comment.
> >
> > Reverting the commits now, and I went to add a comment and saw that
> > there was already one that I blindly missed last time around:
> >
> >   /* This is a bit gross, but we can't use atomic_t in exported
> >    * headers.  OTOH, we don't need the refcnt to be publicly
> >    * visible.  The only reason that this struct is exported is
> >    * because fd_ringbuffer_emit needs to be something that can
> >    * be inlined for performance reasons.
> >    */
> >
> > I just pushed the revert:
> > e09f327765902f3b7d31 "freedreno: revert bad freedreno/atomic_ops commits"
> >
> > Emil, I'd like to release libdrm-2.4.98 today/tomorrow, I assume you're
> > ok with that (since you got your MODALIAS change in there as well, which
> > you wanted for your mesa series)?
> > The drmIsMaster() issue also got fixed, so I think we're good to have
> > a better release than the previous one :]
> >
> > Should I send an MR blacklisting libdrm-2.4.97 in mesa/freedreno?
> Sounds great, thanks!

Actually, as far as I can tell Mesa doesn't use libdrm_freedreno (anymore)?
The issue will only happen with old mesa + libdrm 2.4.97, so there's
nothing we can do? Can you confirm Rob?

> 
> I'll stuck with the original releasing procedure.

Do you mean you'll do a release? I've never done one so that's easier
for me :)