| Message ID | 20180920155834.8470-1-eric.engestrom@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [libdrm,1/3] intel: add missing drm_public exports | expand |
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 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 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 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 >
Quoting Eric Engestrom (2018-09-20 08:58:32) > 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> > --- > 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 > Reviewed-by: Dylan Baker <dylan@pnwbakers.com> Tested-by: Dylan Baker <dylan@pnwbakers.com>
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 :) > 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 > >
On Thursday, 2018-09-20 18:12:59 +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 :) Actually, I went ahead and did that patch anyway, sending it in a minute. > > > 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 > > >
On 20 September 2018 at 16:58, Eric Engestrom <eric.engestrom@intel.com> 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> > --- > intel/intel_bufmgr_fake.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > This indicated my earlier concern it's not an issue of drm_public vs drm_private - people simply forget to run make check. As said earlier - if you want this in Intel sure, but don't push it onto others. Thanks Emil
On Thursday, 2018-09-20 18:30:54 +0100, Emil Velikov wrote: > On 20 September 2018 at 16:58, Eric Engestrom <eric.engestrom@intel.com> 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> > > --- > > intel/intel_bufmgr_fake.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > This indicated my earlier concern it's not an issue of drm_public vs > drm_private - people simply forget to run make check. Actually, this is a limitation of the current symbol check system: it only checks that nothing is exported when it shouldn't be, but it doesn't check that what should be exported actually is. My symbols check series fixes that and would've caught these; I really need to find the time to finish it :/ > As said earlier - if you want this in Intel sure, but don't push it onto others. Intel or not isn't the issue here, but I understand your concern. If someone using one of those broken compilers raises a bug, we might have to reconsider and revert it, but for now let's just try to fix it :) > > Thanks > Emil
On Thu, Sep 20, 2018 at 06:30:54PM +0100, Emil Velikov wrote: > On 20 September 2018 at 16:58, Eric Engestrom <eric.engestrom@intel.com> 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> > > --- > > intel/intel_bufmgr_fake.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > This indicated my earlier concern it's not an issue of drm_public vs > drm_private - people simply forget to run make check. make check with all build systems was passing, this was not the problem > As said earlier - if you want this in Intel sure, but don't push it onto others. I will respond to this in the other thread. Lucas De Marchi > > Thanks > Emil
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 :) > > > 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 > > >
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. thanks for fixing my breakage. 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 > > >
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 > > > >
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,
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> --- intel/intel_bufmgr_fake.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)