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

[Eric Engestrom <eric.engestrom@intel.com>]

On Thursday, 2018-09-20 11:12:49 -0700, Lucas De Marchi wrote:
> 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@intel.com>
> > > > Cc: Mark Janes <mark.a.janes@intel.com>
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108006
> > > > Signed-off-by: Eric Engestrom <eric.engestrom@intel.com>
> > > 
> > > Reviewed-by: Lucas De Marchi <lucas.demarchi@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 :)
> 
> I double checked everything. With your patches applied (including the omap one and the
> radeon_cs_space_check) and the additional drm_public in fd_bo_from_fbdev, then
> it passes the symbol check I did with before vs after.

OK, I pushed everything (including the freedreno one, which was technically
not reviewed, but I'm going home and I'd rather not leave it like that).

> 
> thanks for fixing my breakage.

Sure, and thanks for helping me :)

> 
> Lucas De Marchi
> 
> > 
> > > 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 :)
> > 
> > >  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
> > > > 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel