| Message ID | 1468492786-32226-1-git-send-email-marius.c.vlad@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Marius, Just double-checking - this is an i-g-t patch, isn't it ? On 14 July 2016 at 11:39, Marius Vlad <marius.c.vlad@intel.com> wrote: > Required by commit 2603b98ca (aubdump: Support softpin bos). > > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > CC: Kristian Høgsberg Kristensen <kristian.h.kristensen@intel.com> > --- > configure.ac | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/configure.ac b/configure.ac > index f05bcb0..ade9756 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then > fi > AC_SUBST(ASSEMBLER_WARN_CFLAGS) > > -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) > +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) Yes please. As you do that one can nuke most/all the "define LOCAL_" and "struct local_" (in lib/ioctl_wrappers.h) and replace with with the official symbols. A very nice plus imho ;-) Side note: this will conflict with Robert Foss's work to make libdrm_intel an optional dependency. Have you/others had the chance to look at the series ? What can we do to get that moving/accepted ? Thanks Emil
On Thu, Jul 14, 2016 at 05:29:55PM +0100, Emil Velikov wrote: > Hi Marius, > > Just double-checking - this is an i-g-t patch, isn't it ? > > On 14 July 2016 at 11:39, Marius Vlad <marius.c.vlad@intel.com> wrote: > > Required by commit 2603b98ca (aubdump: Support softpin bos). > > > > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > > CC: Kristian Høgsberg Kristensen <kristian.h.kristensen@intel.com> > > --- > > configure.ac | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/configure.ac b/configure.ac > > index f05bcb0..ade9756 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then > > fi > > AC_SUBST(ASSEMBLER_WARN_CFLAGS) > > > > -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) > > +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) > Yes please. As you do that one can nuke most/all the "define LOCAL_" > and "struct local_" (in lib/ioctl_wrappers.h) > and replace with with the official symbols. A very nice plus imho ;-) Please don't. It makes running on older installations even more cumbersome. -Chris
On 14 July 2016 at 17:49, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Thu, Jul 14, 2016 at 05:29:55PM +0100, Emil Velikov wrote: >> Hi Marius, >> >> Just double-checking - this is an i-g-t patch, isn't it ? >> >> On 14 July 2016 at 11:39, Marius Vlad <marius.c.vlad@intel.com> wrote: >> > Required by commit 2603b98ca (aubdump: Support softpin bos). >> > >> > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> >> > CC: Kristian Høgsberg Kristensen <kristian.h.kristensen@intel.com> >> > --- >> > configure.ac | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/configure.ac b/configure.ac >> > index f05bcb0..ade9756 100644 >> > --- a/configure.ac >> > +++ b/configure.ac >> > @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then >> > fi >> > AC_SUBST(ASSEMBLER_WARN_CFLAGS) >> > >> > -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) >> > +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) >> Yes please. As you do that one can nuke most/all the "define LOCAL_" >> and "struct local_" (in lib/ioctl_wrappers.h) >> and replace with with the official symbols. A very nice plus imho ;-) > > Please don't. It makes running on older installations even more > cumbersome. Slightly confused here: are you against the libdrm_intel bump, or the removal of the local symbols ? Admittedly sometimes distros don't bother/refuse to update libdrm which could be an issue in the former case. Although if the package (with all the definitions) is compulsory, how would that cause an issue ? Genuine question here, not trying to be smart/cheeky/etc. Thanks Emil
On Thu, Jul 14, 2016 at 07:54:59PM +0100, Emil Velikov wrote: > On 14 July 2016 at 17:49, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Thu, Jul 14, 2016 at 05:29:55PM +0100, Emil Velikov wrote: > >> Hi Marius, > >> > >> Just double-checking - this is an i-g-t patch, isn't it ? > >> > >> On 14 July 2016 at 11:39, Marius Vlad <marius.c.vlad@intel.com> wrote: > >> > Required by commit 2603b98ca (aubdump: Support softpin bos). > >> > > >> > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > >> > CC: Kristian Høgsberg Kristensen <kristian.h.kristensen@intel.com> > >> > --- > >> > configure.ac | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/configure.ac b/configure.ac > >> > index f05bcb0..ade9756 100644 > >> > --- a/configure.ac > >> > +++ b/configure.ac > >> > @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then > >> > fi > >> > AC_SUBST(ASSEMBLER_WARN_CFLAGS) > >> > > >> > -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) > >> > +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) > >> Yes please. As you do that one can nuke most/all the "define LOCAL_" > >> and "struct local_" (in lib/ioctl_wrappers.h) > >> and replace with with the official symbols. A very nice plus imho ;-) > > > > Please don't. It makes running on older installations even more > > cumbersome. > Slightly confused here: are you against the libdrm_intel bump, or the > removal of the local symbols ? Local symbols. They save a lot of time if you can just get the test you need compiling and not worry about dependencies. One of the basic tenents is that we drop a new kernel into an old userspace and expect to have not broken anything. Being lazy, for smoke testing I just build in situ. > Admittedly sometimes distros don't bother/refuse to update libdrm > which could be an issue in the former case. Although if the package > (with all the definitions) is compulsory, how would that cause an > issue ? The package may not even exist when testing on v.old distro images. It is mostly a major of convenience, but since the work is already done to be independent, removing causes more work. -Chris
On 14 July 2016 at 21:03, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Thu, Jul 14, 2016 at 07:54:59PM +0100, Emil Velikov wrote: >> On 14 July 2016 at 17:49, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > On Thu, Jul 14, 2016 at 05:29:55PM +0100, Emil Velikov wrote: >> >> Hi Marius, >> >> >> >> Just double-checking - this is an i-g-t patch, isn't it ? >> >> >> >> On 14 July 2016 at 11:39, Marius Vlad <marius.c.vlad@intel.com> wrote: >> >> > Required by commit 2603b98ca (aubdump: Support softpin bos). >> >> > >> >> > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> >> >> > CC: Kristian Høgsberg Kristensen <kristian.h.kristensen@intel.com> >> >> > --- >> >> > configure.ac | 2 +- >> >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> >> > >> >> > diff --git a/configure.ac b/configure.ac >> >> > index f05bcb0..ade9756 100644 >> >> > --- a/configure.ac >> >> > +++ b/configure.ac >> >> > @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then >> >> > fi >> >> > AC_SUBST(ASSEMBLER_WARN_CFLAGS) >> >> > >> >> > -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) >> >> > +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) >> >> Yes please. As you do that one can nuke most/all the "define LOCAL_" >> >> and "struct local_" (in lib/ioctl_wrappers.h) >> >> and replace with with the official symbols. A very nice plus imho ;-) >> > >> > Please don't. It makes running on older installations even more >> > cumbersome. >> Slightly confused here: are you against the libdrm_intel bump, or the >> removal of the local symbols ? > > Local symbols. They save a lot of time if you can just get the test you > need compiling and not worry about dependencies. One of the basic > tenents is that we drop a new kernel into an old userspace and expect to > have not broken anything. Being lazy, for smoke testing I just build > in situ. > Fully agree. >> Admittedly sometimes distros don't bother/refuse to update libdrm >> which could be an issue in the former case. Although if the package >> (with all the definitions) is compulsory, how would that cause an >> issue ? > > The package may not even exist when testing on v.old distro images. > It is mostly a major of convenience, but since the work is already done > to be independent, removing causes more work. Seems that things are not completely independent, as illustrated by the version bump. Thus was wondering what's the benefit of (or why object against nuking) the local copies. Since even if they were needed before, they won't be after the bump.That said, it seems that it's a picky/sore topic so I won't dig into it. If anything I'll just send patches to be acked/nacked. -Emil
On Thu, Jul 14, 2016 at 09:52:24PM +0100, Emil Velikov wrote: > On 14 July 2016 at 21:03, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Thu, Jul 14, 2016 at 07:54:59PM +0100, Emil Velikov wrote: > >> On 14 July 2016 at 17:49, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> > On Thu, Jul 14, 2016 at 05:29:55PM +0100, Emil Velikov wrote: > >> >> Hi Marius, > >> >> > >> >> Just double-checking - this is an i-g-t patch, isn't it ? > >> >> > >> >> On 14 July 2016 at 11:39, Marius Vlad <marius.c.vlad@intel.com> wrote: > >> >> > Required by commit 2603b98ca (aubdump: Support softpin bos). > >> >> > > >> >> > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > >> >> > CC: Kristian Høgsberg Kristensen <kristian.h.kristensen@intel.com> > >> >> > --- > >> >> > configure.ac | 2 +- > >> >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> >> > > >> >> > diff --git a/configure.ac b/configure.ac > >> >> > index f05bcb0..ade9756 100644 > >> >> > --- a/configure.ac > >> >> > +++ b/configure.ac > >> >> > @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then > >> >> > fi > >> >> > AC_SUBST(ASSEMBLER_WARN_CFLAGS) > >> >> > > >> >> > -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) > >> >> > +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) > >> >> Yes please. As you do that one can nuke most/all the "define LOCAL_" > >> >> and "struct local_" (in lib/ioctl_wrappers.h) > >> >> and replace with with the official symbols. A very nice plus imho ;-) > >> > > >> > Please don't. It makes running on older installations even more > >> > cumbersome. > >> Slightly confused here: are you against the libdrm_intel bump, or the > >> removal of the local symbols ? > > > > Local symbols. They save a lot of time if you can just get the test you > > need compiling and not worry about dependencies. One of the basic > > tenents is that we drop a new kernel into an old userspace and expect to > > have not broken anything. Being lazy, for smoke testing I just build > > in situ. > > > Fully agree. > > >> Admittedly sometimes distros don't bother/refuse to update libdrm > >> which could be an issue in the former case. Although if the package > >> (with all the definitions) is compulsory, how would that cause an > >> issue ? > > > > The package may not even exist when testing on v.old distro images. > > It is mostly a major of convenience, but since the work is already done > > to be independent, removing causes more work. > Seems that things are not completely independent, as illustrated by > the version bump. > > Thus was wondering what's the benefit of (or why object against > nuking) the local copies. Since even if they were needed before, they > won't be after the bump.That said, it seems that it's a picky/sore > topic so I won't dig into it. I don't have uptodate or even recent libdrm everywhere. Getting rid of the locals means another dependency chain to build. Just being completely lazy and saying "if ain't broke, don't try to fix it". -Chris
So, the issue is that i-g-t will not build on our builder system, with the current version of librm-intel. It just happens to work because I took the liberity of upgrading the entire system, a while ago. No build, no newer i-g-t, no tests, no CI. There are a few of _PINNED macros defined by Chris all over the gem tests. Thought of doing just that, figured it would be better to bump the library instead. On Thu, Jul 14, 2016 at 05:29:55PM +0100, Emil Velikov wrote: > Hi Marius, > > Just double-checking - this is an i-g-t patch, isn't it ? Sorry, my bad! > > On 14 July 2016 at 11:39, Marius Vlad <marius.c.vlad@intel.com> wrote: > > Required by commit 2603b98ca (aubdump: Support softpin bos). > > > > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > > CC: Kristian Høgsberg Kristensen <kristian.h.kristensen@intel.com> > > --- > > configure.ac | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/configure.ac b/configure.ac > > index f05bcb0..ade9756 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then > > fi > > AC_SUBST(ASSEMBLER_WARN_CFLAGS) > > > > -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) > > +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) > Yes please. As you do that one can nuke most/all the "define LOCAL_" > and "struct local_" (in lib/ioctl_wrappers.h) > and replace with with the official symbols. A very nice plus imho ;-) > > Side note: this will conflict with Robert Foss's work to make > libdrm_intel an optional dependency. Have you/others had the chance to > look at the series ? What can we do to get that moving/accepted ? Yes I've seen them. Someone to Ack'em :-). > > Thanks > Emil
On 15 July 2016 at 16:42, Marius Vlad <marius.c.vlad@intel.com> wrote: >> Side note: this will conflict with Robert Foss's work to make >> libdrm_intel an optional dependency. Have you/others had the chance to >> look at the series ? What can we do to get that moving/accepted ? > > Yes I've seen them. Someone to Ack'em :-). > Can you please elaborate who can/should Ack them - is that someone from the IGT team ? From functionality POV they're pretty good, as I've been through them a few times already (and have my r-b, fwiw). Thanks Emil
On Fri, Jul 22, 2016 at 03:21:29PM +0100, Emil Velikov wrote: > On 15 July 2016 at 16:42, Marius Vlad <marius.c.vlad@intel.com> wrote: > > >> Side note: this will conflict with Robert Foss's work to make > >> libdrm_intel an optional dependency. Have you/others had the chance to > >> look at the series ? What can we do to get that moving/accepted ? > > > > Yes I've seen them. Someone to Ack'em :-). > > > Can you please elaborate who can/should Ack them - is that someone > from the IGT team ? From functionality POV they're pretty good, as > I've been through them a few times already (and have my r-b, fwiw). Not sure where the work from Robert Foss is right now tbh ... Do you want commit rights to igt to move it forward? Preemptive ack from me, just ask Daniel Stone to add you to the right groups on fd.o. -Daniel
On 27 July 2016 at 11:23, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Jul 22, 2016 at 03:21:29PM +0100, Emil Velikov wrote: >> On 15 July 2016 at 16:42, Marius Vlad <marius.c.vlad@intel.com> wrote: >> >> >> Side note: this will conflict with Robert Foss's work to make >> >> libdrm_intel an optional dependency. Have you/others had the chance to >> >> look at the series ? What can we do to get that moving/accepted ? >> > >> > Yes I've seen them. Someone to Ack'em :-). >> > >> Can you please elaborate who can/should Ack them - is that someone >> from the IGT team ? From functionality POV they're pretty good, as >> I've been through them a few times already (and have my r-b, fwiw). > > Not sure where the work from Robert Foss is right now tbh ... Do you want > commit rights to igt to move it forward? Preemptive ack from me, just ask > Daniel Stone to add you to the right groups on fd.o. Ack will check with Daniel for next round. Getting access for Tomeu/Rob or myself about pushing generic fixes to IGT will be great. Meanwhile thanks for pushing v5 ;-) -Emil
diff --git a/configure.ac b/configure.ac index f05bcb0..ade9756 100644 --- a/configure.ac +++ b/configure.ac @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then fi AC_SUBST(ASSEMBLER_WARN_CFLAGS) -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) PKG_CHECK_MODULES(PCIACCESS, [pciaccess >= 0.10]) case "$target_cpu" in
Required by commit 2603b98ca (aubdump: Support softpin bos). Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> CC: Kristian Høgsberg Kristensen <kristian.h.kristensen@intel.com> --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)