[PATCH libdrm 1/3] intel: add missing drm_public exports

Thu Sep 20 17:58:40 UTC 2018

On Thu, Sep 20, 2018 at 06:12:59PM +0100, Eric Engestrom wrote:
> On Thursday, 2018-09-20 09:46:48 -0700, Lucas De Marchi wrote:
> > On Thu, Sep 20, 2018 at 04:58:32PM +0100, Eric Engestrom wrote:
> > > Fixes: 36bb0ea47b71d220b31e "intel: annotate public functions"
> > > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > > Cc: Mark Janes <mark.a.janes at intel.com>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108006
> > > Signed-off-by: Eric Engestrom <eric.engestrom at intel.com>
> > 
> > Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
> > 
> > But for the series, I went to check what went wrong. It seems my script
> > missed the cases in which we had more than one symbol with the same
> > name (yes, we do have some static functions that have the same name
> > of an exported symbol) and cases in which for some reason
> > cscope didn't return the location of the symbold definition.
> > 
> > So... I decided to double check if we are now exporting all symbols
> > that we were previously. My check is with:
> > 
> > $ git checkout 473e2d2
> > $ flags="-D amdgpu=true -D cairo-tests=true -D etnaviv=true -D exynos=true
> >          -D freedreno=true -D freedreno-kgsl=true -D intel=true -D libkms=true
> >          -D man-pages=true -D nouveau=true -D omap=true -D radeon=true
> >          -D tegra=true -D udev=true -D valgrind=true -D vc4=true -D vmwgfx=true"
> > $ meson _buildold
> > $ ninja -C _buildold
> > $ git checkout master
> > $ ls /tmp/patches/
> > 0001-intel-add-missing-drm-public-exports.patch    0003-radeon-add-missing-drm-public-exports.patch
> > 0002-nouveau-add-missing-drm-public-exports.patch
> > $ git am /tmp/patches/*
> > $ meson _buildnew
> > $ ninja -C _buildnew
> > $ find _buildnew/ -name '*.so' | while read f; do
> >     fold=${f/_buildnew/_buildold}
> >     nm --dynamic --defined-only $f | grep -i " T " | cut -d' ' -f3 | sort -u > /tmp/new.txt
> >     nm --dynamic --defined-only $fold | grep -i " T " | cut -d' ' -f3 | sort -u > /tmp/old.txt
> >     git diff /tmp/old.txt /tmp/new.txt
> > done
> > 
> > So.. we still have the following symbols missing.
> > 
> > diff --git a/tmp/old.txt b/tmp/new.txt
> > index 799b63d8..dd0c30c2 100644
> > --- a/tmp/old.txt
> > +++ b/tmp/new.txt
> > @@ -29,7 +29,6 @@ radeon_cs_need_flush
> >  radeon_cs_print
> >  radeon_cs_set_limit
> >  radeon_cs_space_add_persistent_bo
> > -radeon_cs_space_check
> 
> Ha, I missed that one because my grep found the one with the `_with_bo`
> suffix which was properly exported... :eyeroll:
> I'll send a v2 in a bit with that added.
> 
> >  radeon_cs_space_check_with_bo
> >  radeon_cs_space_reset_bos
> >  radeon_cs_space_set_flush
> > diff --git a/tmp/old.txt b/tmp/new.txt
> > index d846faf0..e08eac77 100644
> > --- a/tmp/old.txt
> > +++ b/tmp/new.txt
> > @@ -4,13 +4,9 @@ omap_bo_cpu_fini
> >  omap_bo_cpu_prep
> >  omap_bo_del
> >  omap_bo_dmabuf
> > -omap_bo_from_dmabuf
> > -omap_bo_from_name
> >  omap_bo_get_name
> >  omap_bo_handle
> >  omap_bo_map
> > -omap_bo_new
> > -omap_bo_new_tiled
> >  omap_bo_ref
> >  omap_bo_size
> >  omap_device_del
> 
> These ones actually do have the drm_public annotation, although it was
> not written the "normal way", which might be why it's not working?
> 
> For example:
> 
>   struct omap_bo *
>   drm_public omap_bo_from_name(struct omap_device *dev, uint32_t name)
>   {
>   ...
> 
> I guess from your script above that they are actually not exported?
> Could you double-check, and send a patch for these omap ones? Thanks :)
> 
> > diff --git a/tmp/old.txt b/tmp/new.txt
> > index fe0dbf39..40a459ad 100644
> > --- a/tmp/old.txt
> > +++ b/tmp/new.txt
> > @@ -3,7 +3,6 @@ fd_bo_cpu_prep
> >  fd_bo_del
> >  fd_bo_dmabuf
> >  fd_bo_from_dmabuf
> > -fd_bo_from_fbdev
> 
> This one is actually all good:
> 
>   drm_public struct fd_bo * fd_bo_from_fbdev(struct fd_pipe *pipe, int fbfd, uint32_t size)
> 
> It is hidden behind `!HAVE_FREEDRENO_KGSL` which is itself controlled by
> `-D freedreno-kgsl=false` on meson; make sure you're using the same
> config for your before/after comparison :)

that's why I used a flags variable... to pass the same configuration flags
before and after.

it's drm_public in the !HAVE_FREEDRENO_KGSL  case, but is should also be drm_public
when this configure flag is true. In this case the function is defined in
kgsl_bo.c rather than freedreno_bo.c.

Lucas De Marchi

> 
> >  fd_bo_from_handle
> >  fd_bo_from_name
> >  fd_bo_get_iova
> > 
> > I hope I'm covering everything now.
> > 
> > Thanks
> > Lucas De Marchi
> > 
> > > ---
> > >  intel/intel_bufmgr_fake.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/intel/intel_bufmgr_fake.c b/intel/intel_bufmgr_fake.c
> > > index 57cbc5365b9f0779dd5a..0cec51f5b836d26e57e0 100644
> > > --- a/intel/intel_bufmgr_fake.c
> > > +++ b/intel/intel_bufmgr_fake.c
> > > @@ -241,7 +241,7 @@ FENCE_LTE(unsigned a, unsigned b)
> > >  	return 0;
> > >  }
> > >  
> > > -void
> > > +drm_public void
> > >  drm_intel_bufmgr_fake_set_fence_callback(drm_intel_bufmgr *bufmgr,
> > >  					 unsigned int (*emit) (void *priv),
> > >  					 void (*wait) (unsigned int fence,
> > > @@ -955,7 +955,7 @@ drm_intel_fake_bo_unreference(drm_intel_bo *bo)
> > >   * Set the buffer as not requiring backing store, and instead get the callback
> > >   * invoked whenever it would be set dirty.
> > >   */
> > > -void
> > > +drm_public void
> > >  drm_intel_bo_fake_disable_backing_store(drm_intel_bo *bo,
> > >  					void (*invalidate_cb) (drm_intel_bo *bo,
> > >  							       void *ptr),
> > > @@ -1409,7 +1409,7 @@ drm_intel_bo_fake_post_submit(drm_intel_bo *bo)
> > >  	bo_fake->write_domain = 0;
> > >  }
> > >  
> > > -void
> > > +drm_public void
> > >  drm_intel_bufmgr_fake_set_exec_callback(drm_intel_bufmgr *bufmgr,
> > >  					     int (*exec) (drm_intel_bo *bo,
> > >  							  unsigned int used,
> > > -- 
> > > Cheers,
> > >   Eric
> > > 