freedreno header uses not installed xf86atomic.h

[Rob Clark]

On Tue, Feb 19, 2019 at 9:07 AM Eric Engestrom <eric.engestrom at intel.com> wrote:
>
> 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"

Thanks

> > > 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?

mesa 19.0 and later uses drm code imported into mesa (but should not
install system headers).  The libdrm code (which does install the
headers) is mostly for the benefit of building older mesa.

(Plus I have a handful of devel tools which use libdrm_freedreno)

BR,
-R