Re: [Mesa-dev] [PATCH v3 06/10] swr: Windows-related changes

2016-11-18 Thread Emil Velikov 
On 17 November 2016 at 20:51, Kyriazis, George
<george.kyria...@intel.com> wrote:
>
>
>> -Original Message-
>> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On
>> Behalf Of Emil Velikov
>> Sent: Thursday, November 17, 2016 11:12 AM
>> To: Kyriazis, George <george.kyria...@intel.com>
>> Cc: ML mesa-dev <mesa-dev@lists.freedesktop.org>
>> Subject: Re: [Mesa-dev] [PATCH v3 06/10] swr: Windows-related changes
>>
>> Hi George,
>>
>> Seems I was unclear as a few suggestions got missed.
>>
>> On 16 November 2016 at 02:26, George Kyriazis <george.kyria...@intel.com>
>> wrote:
>> > - Handle dynamic library loading for windows
>> > - Implement swap for gdi
>> > - fix prototypes
>> > - update include paths on configure-based build for swr_loader.cpp
>> > ---
>> >  src/gallium/drivers/swr/Makefile.am|  7 +++
>> >  src/gallium/drivers/swr/swr_loader.cpp | 28
>> +---
>> >  src/gallium/drivers/swr/swr_public.h   | 11 +++
>> >  3 files changed, 39 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/src/gallium/drivers/swr/Makefile.am
>> > b/src/gallium/drivers/swr/Makefile.am
>> > index dd1c2e6..305154f 100644
>> > --- a/src/gallium/drivers/swr/Makefile.am
>> > +++ b/src/gallium/drivers/swr/Makefile.am
>> > @@ -217,6 +217,12 @@ libswrAVX2_la_CXXFLAGS = \
>> libswrAVX2_la_SOURCES
>> > = \
>> > $(COMMON_SOURCES)
>> >
>> > +# XXX: $(SWR_AVX_CXXFLAGS) should not be included, but we end up
>> > +including # simdintrin.h, which throws a warning if AVX is not
>> > +enabled libmesaswr_la_CXXFLAGS = \
>> > +   $(COMMON_CXXFLAGS) \
>> > +   $(SWR_AVX_CXXFLAGS)
>> > +
>> Drop this.
>>
> This is needed for linux configure-based build.
>
As you can see per your v4 this don't fold true. I would kindly ask
you to compile test (2 minute job) - saves a lot of confusing moments
on each end.


>> > --- a/src/gallium/drivers/swr/swr_loader.cpp
>> > +++ b/src/gallium/drivers/swr/swr_loader.cpp
>>
>> > +#include "swr_screen.h"
>> > +#include "swr_resource.h"
>> > +
>> You only need p_screen.h here. Adding the swr ones is wrong (afaict).
>>
> Ok, replaced.
>
Same goes here - these wrong includes were the reason for extra includes above.

Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 06/10] swr: Windows-related changes

2016-11-17 Thread Kyriazis, George 


> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On
> Behalf Of Emil Velikov
> Sent: Thursday, November 17, 2016 11:12 AM
> To: Kyriazis, George <george.kyria...@intel.com>
> Cc: ML mesa-dev <mesa-dev@lists.freedesktop.org>
> Subject: Re: [Mesa-dev] [PATCH v3 06/10] swr: Windows-related changes
> 
> Hi George,
> 
> Seems I was unclear as a few suggestions got missed.
> 
> On 16 November 2016 at 02:26, George Kyriazis <george.kyria...@intel.com>
> wrote:
> > - Handle dynamic library loading for windows
> > - Implement swap for gdi
> > - fix prototypes
> > - update include paths on configure-based build for swr_loader.cpp
> > ---
> >  src/gallium/drivers/swr/Makefile.am|  7 +++
> >  src/gallium/drivers/swr/swr_loader.cpp | 28
> +---
> >  src/gallium/drivers/swr/swr_public.h   | 11 +++
> >  3 files changed, 39 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/gallium/drivers/swr/Makefile.am
> > b/src/gallium/drivers/swr/Makefile.am
> > index dd1c2e6..305154f 100644
> > --- a/src/gallium/drivers/swr/Makefile.am
> > +++ b/src/gallium/drivers/swr/Makefile.am
> > @@ -217,6 +217,12 @@ libswrAVX2_la_CXXFLAGS = \
> libswrAVX2_la_SOURCES
> > = \
> > $(COMMON_SOURCES)
> >
> > +# XXX: $(SWR_AVX_CXXFLAGS) should not be included, but we end up
> > +including # simdintrin.h, which throws a warning if AVX is not
> > +enabled libmesaswr_la_CXXFLAGS = \
> > +   $(COMMON_CXXFLAGS) \
> > +   $(SWR_AVX_CXXFLAGS)
> > +
> Drop this.
> 
This is needed for linux configure-based build.

> >  # XXX: Don't ship these generated sources for now, since they are
> > specific  # to the LLVM version they are generated from. Thus a
> > release tarball  # containing the said files, generated against eg.
> > LLVM 3.8 will fail to build @@ -235,6 +241,7 @@ libswrAVX2_la_LDFLAGS
> > = \  include $(top_srcdir)/install-gallium-links.mk
> >
> >  EXTRA_DIST = \
> > +   SConscript \
> This should be part of the patch which brings the file - namely 08/10.
> 
Ok, will do.

> > rasterizer/archrast/events.proto \
> > rasterizer/jitter/scripts/gen_llvm_ir_macros.py \
> > rasterizer/jitter/scripts/gen_llvm_types.py \ diff --git
> > a/src/gallium/drivers/swr/swr_loader.cpp
> > b/src/gallium/drivers/swr/swr_loader.cpp
> > index 2113c37..4f3329e 100644
> > --- a/src/gallium/drivers/swr/swr_loader.cpp
> > +++ b/src/gallium/drivers/swr/swr_loader.cpp
> 
> > +#include "swr_screen.h"
> > +#include "swr_resource.h"
> > +
> You only need p_screen.h here. Adding the swr ones is wrong (afaict).
> 
Ok, replaced.

> 
> > -struct pipe_screen *swr_create_screen(struct sw_winsys *winsys);
> > +PUBLIC struct pipe_screen *swr_create_screen(struct sw_winsys
> > +*winsys);
> >
> Do you really need this ? As-it this will make the swr loader
> swr_create_screen public which is bad.
> 
> Either way - [re]name the loader and backend entrypoints differently and
> use separate declarations. You can squash it here, but a separate patch
> (5.5/10) would be better.
> 
Ok, renaming backend entry points.

I'll resend another batch of changes for review, since there is order 
dependency and I can't send 6/10 ad 5.5/10 after the later ones.

Thanks,

george

> Thanks
> Emil
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 06/10] swr: Windows-related changes

2016-11-17 Thread Kyriazis, George 


> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On
> Behalf Of Emil Velikov
> Sent: Thursday, November 17, 2016 11:12 AM
> To: Kyriazis, George <george.kyria...@intel.com>
> Cc: ML mesa-dev <mesa-dev@lists.freedesktop.org>
> Subject: Re: [Mesa-dev] [PATCH v3 06/10] swr: Windows-related changes
> 
> Hi George,
> 
> Seems I was unclear as a few suggestions got missed.
> 
> On 16 November 2016 at 02:26, George Kyriazis <george.kyria...@intel.com>
> wrote:
> > - Handle dynamic library loading for windows
> > - Implement swap for gdi
> > - fix prototypes
> > - update include paths on configure-based build for swr_loader.cpp
> > ---
> >  src/gallium/drivers/swr/Makefile.am|  7 +++
> >  src/gallium/drivers/swr/swr_loader.cpp | 28
> +---
> >  src/gallium/drivers/swr/swr_public.h   | 11 +++
> >  3 files changed, 39 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/gallium/drivers/swr/Makefile.am
> > b/src/gallium/drivers/swr/Makefile.am
> > index dd1c2e6..305154f 100644
> > --- a/src/gallium/drivers/swr/Makefile.am
> > +++ b/src/gallium/drivers/swr/Makefile.am
> > @@ -217,6 +217,12 @@ libswrAVX2_la_CXXFLAGS = \
> libswrAVX2_la_SOURCES
> > = \
> > $(COMMON_SOURCES)
> >
> > +# XXX: $(SWR_AVX_CXXFLAGS) should not be included, but we end up
> > +including # simdintrin.h, which throws a warning if AVX is not
> > +enabled libmesaswr_la_CXXFLAGS = \
> > +   $(COMMON_CXXFLAGS) \
> > +   $(SWR_AVX_CXXFLAGS)
> > +
> Drop this.
> 
These are needed, otherwise the linux configure build breaks.

> >  # XXX: Don't ship these generated sources for now, since they are
> > specific  # to the LLVM version they are generated from. Thus a
> > release tarball  # containing the said files, generated against eg.
> > LLVM 3.8 will fail to build @@ -235,6 +241,7 @@ libswrAVX2_la_LDFLAGS
> > = \  include $(top_srcdir)/install-gallium-links.mk
> >
> >  EXTRA_DIST = \
> > +   SConscript \
> This should be part of the patch which brings the file - namely 08/10.
> 
OK.

> > rasterizer/archrast/events.proto \
> > rasterizer/jitter/scripts/gen_llvm_ir_macros.py \
> > rasterizer/jitter/scripts/gen_llvm_types.py \ diff --git
> > a/src/gallium/drivers/swr/swr_loader.cpp
> > b/src/gallium/drivers/swr/swr_loader.cpp
> > index 2113c37..4f3329e 100644
> > --- a/src/gallium/drivers/swr/swr_loader.cpp
> > +++ b/src/gallium/drivers/swr/swr_loader.cpp
> 
> > +#include "swr_screen.h"
> > +#include "swr_resource.h"
> > +
> You only need p_screen.h here. Adding the swr ones is wrong (afaict).
> 
These are needed for the screen struct definition needed to call 
screen->flush_frontbuffer()

> 
> > -struct pipe_screen *swr_create_screen(struct sw_winsys *winsys);
> > +PUBLIC struct pipe_screen *swr_create_screen(struct sw_winsys
> > +*winsys);
> >
> Do you really need this ? As-it this will make the swr loader
> swr_create_screen public which is bad.
> 
Ok.  I will rename our internal back-end one.  BTW, the same entry points are 
used on linux.

> Either way - [re]name the loader and backend entrypoints differently and
> use separate declarations. You can squash it here, but a separate patch
> (5.5/10) would be better.
> 
> Thanks
> Emil
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 06/10] swr: Windows-related changes

2016-11-17 Thread Emil Velikov 
Hi George,

Seems I was unclear as a few suggestions got missed.

On 16 November 2016 at 02:26, George Kyriazis  wrote:
> - Handle dynamic library loading for windows
> - Implement swap for gdi
> - fix prototypes
> - update include paths on configure-based build for swr_loader.cpp
> ---
>  src/gallium/drivers/swr/Makefile.am|  7 +++
>  src/gallium/drivers/swr/swr_loader.cpp | 28 +---
>  src/gallium/drivers/swr/swr_public.h   | 11 +++
>  3 files changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/src/gallium/drivers/swr/Makefile.am 
> b/src/gallium/drivers/swr/Makefile.am
> index dd1c2e6..305154f 100644
> --- a/src/gallium/drivers/swr/Makefile.am
> +++ b/src/gallium/drivers/swr/Makefile.am
> @@ -217,6 +217,12 @@ libswrAVX2_la_CXXFLAGS = \
>  libswrAVX2_la_SOURCES = \
> $(COMMON_SOURCES)
>
> +# XXX: $(SWR_AVX_CXXFLAGS) should not be included, but we end up including
> +# simdintrin.h, which throws a warning if AVX is not enabled
> +libmesaswr_la_CXXFLAGS = \
> +   $(COMMON_CXXFLAGS) \
> +   $(SWR_AVX_CXXFLAGS)
> +
Drop this.

>  # XXX: Don't ship these generated sources for now, since they are specific
>  # to the LLVM version they are generated from. Thus a release tarball
>  # containing the said files, generated against eg. LLVM 3.8 will fail to 
> build
> @@ -235,6 +241,7 @@ libswrAVX2_la_LDFLAGS = \
>  include $(top_srcdir)/install-gallium-links.mk
>
>  EXTRA_DIST = \
> +   SConscript \
This should be part of the patch which brings the file - namely 08/10.

> rasterizer/archrast/events.proto \
> rasterizer/jitter/scripts/gen_llvm_ir_macros.py \
> rasterizer/jitter/scripts/gen_llvm_types.py \
> diff --git a/src/gallium/drivers/swr/swr_loader.cpp 
> b/src/gallium/drivers/swr/swr_loader.cpp
> index 2113c37..4f3329e 100644
> --- a/src/gallium/drivers/swr/swr_loader.cpp
> +++ b/src/gallium/drivers/swr/swr_loader.cpp

> +#include "swr_screen.h"
> +#include "swr_resource.h"
> +
You only need p_screen.h here. Adding the swr ones is wrong (afaict).


> -struct pipe_screen *swr_create_screen(struct sw_winsys *winsys);
> +PUBLIC struct pipe_screen *swr_create_screen(struct sw_winsys *winsys);
>
Do you really need this ? As-it this will make the swr loader
swr_create_screen public which is bad.

Either way - [re]name the loader and backend entrypoints differently
and use separate declarations. You can squash it here, but a separate
patch (5.5/10) would be better.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev