Re: [Mesa-dev] [PATCH 10/12] i965/cs: Get max_cs_threads from brw_compiler devinfo

[Kristian Høgsberg]

On Wed, Oct 7, 2015 at 10:09 PM, Jason Ekstrand  wrote:
>
> On Oct 7, 2015 3:36 PM, "Kristian Høgsberg"  wrote:
>>
>> On Wed, Oct 7, 2015 at 3:11 PM, Matt Turner  wrote:
>> > On Wed, Oct 7, 2015 at 7:11 AM, Kristian Høgsberg Kristensen
>> >  wrote:
>> >> Signed-off-by: Kristian Høgsberg Kristensen 
>> >> ---
>> >>  src/mesa/drivers/dri/i965/brw_fs.cpp | 5 +++--
>> >>  1 file changed, 3 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> index 65c3628..b79b4a4 100644
>> >> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> @@ -5276,6 +5276,7 @@ brw_cs_emit(struct brw_context *brw,
>> >> prog_data->local_size[2] = cp->LocalSize[2];
>> >> unsigned local_workgroup_size =
>> >>cp->LocalSize[0] * cp->LocalSize[1] * cp->LocalSize[2];
>> >> +   unsigned max_cs_threads =
>> >> brw->intelScreen->compiler->devinfo->max_cs_threads;
>> >
>> > I'm not following how this helps? Seems like you want to just pass in
>> > devinfo or something?
>>
>> We want to pass in just brw_compiler eventually. At that point
>> brw_context won't be available. Jason will do that next, and I guess
>> you could argue that this patch belongs in that series.
>
> I would tend to agree with Matt. This patch doesn't really do anything on
> its own and I plan on doing it "for real" soon enough that "other callers"
> won't have to wait long.  Let's put this off until we can do it "the right
> way".

I don't feel too strongly about this one and it certainly isn't
directly related to or required for what this patch series tries to
do. However, whatever we do in future patch series, this would have to
be a patch on its own so as to isolate potential side effects from
moving from brw->max_cs_threads to the devinfo field. We can do it now
or later.

Kristian

> --Jason
>
>> However, this one occurrence is the only place the compiler accesses
>> devinfo values through brw_context and getting rid of it means callers
>> without a real brw_context have to fake a little less.
>>
>> Kristian
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 10/12] i965/cs: Get max_cs_threads from brw_compiler devinfo

[Jason Ekstrand]

On Thu, Oct 8, 2015 at 11:35 AM, Kristian Høgsberg  wrote:
> On Wed, Oct 7, 2015 at 10:09 PM, Jason Ekstrand  wrote:
>>
>> On Oct 7, 2015 3:36 PM, "Kristian Høgsberg"  wrote:
>>>
>>> On Wed, Oct 7, 2015 at 3:11 PM, Matt Turner  wrote:
>>> > On Wed, Oct 7, 2015 at 7:11 AM, Kristian Høgsberg Kristensen
>>> >  wrote:
>>> >> Signed-off-by: Kristian Høgsberg Kristensen 
>>> >> ---
>>> >>  src/mesa/drivers/dri/i965/brw_fs.cpp | 5 +++--
>>> >>  1 file changed, 3 insertions(+), 2 deletions(-)
>>> >>
>>> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> >> b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> >> index 65c3628..b79b4a4 100644
>>> >> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> >> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> >> @@ -5276,6 +5276,7 @@ brw_cs_emit(struct brw_context *brw,
>>> >> prog_data->local_size[2] = cp->LocalSize[2];
>>> >> unsigned local_workgroup_size =
>>> >>cp->LocalSize[0] * cp->LocalSize[1] * cp->LocalSize[2];
>>> >> +   unsigned max_cs_threads =
>>> >> brw->intelScreen->compiler->devinfo->max_cs_threads;
>>> >
>>> > I'm not following how this helps? Seems like you want to just pass in
>>> > devinfo or something?
>>>
>>> We want to pass in just brw_compiler eventually. At that point
>>> brw_context won't be available. Jason will do that next, and I guess
>>> you could argue that this patch belongs in that series.
>>
>> I would tend to agree with Matt. This patch doesn't really do anything on
>> its own and I plan on doing it "for real" soon enough that "other callers"
>> won't have to wait long.  Let's put this off until we can do it "the right
>> way".
>
> I don't feel too strongly about this one and it certainly isn't
> directly related to or required for what this patch series tries to
> do. However, whatever we do in future patch series, this would have to
> be a patch on its own so as to isolate potential side effects from
> moving from brw->max_cs_threads to the devinfo field. We can do it now
> or later.

That seems like a reasonable argument.  I can include it in mine later
or, if you'd prefer,

Reviewed-by: Jason Ekstrand 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 10/12] i965/cs: Get max_cs_threads from brw_compiler devinfo

[Matt Turner]

On Wed, Oct 7, 2015 at 7:11 AM, Kristian Høgsberg Kristensen
 wrote:
> Signed-off-by: Kristian Høgsberg Kristensen 
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp 
> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 65c3628..b79b4a4 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -5276,6 +5276,7 @@ brw_cs_emit(struct brw_context *brw,
> prog_data->local_size[2] = cp->LocalSize[2];
> unsigned local_workgroup_size =
>cp->LocalSize[0] * cp->LocalSize[1] * cp->LocalSize[2];
> +   unsigned max_cs_threads = 
> brw->intelScreen->compiler->devinfo->max_cs_threads;

I'm not following how this helps? Seems like you want to just pass in
devinfo or something?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 10/12] i965/cs: Get max_cs_threads from brw_compiler devinfo

[Kristian Høgsberg]

On Wed, Oct 7, 2015 at 3:11 PM, Matt Turner  wrote:
> On Wed, Oct 7, 2015 at 7:11 AM, Kristian Høgsberg Kristensen
>  wrote:
>> Signed-off-by: Kristian Høgsberg Kristensen 
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp 
>> b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 65c3628..b79b4a4 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -5276,6 +5276,7 @@ brw_cs_emit(struct brw_context *brw,
>> prog_data->local_size[2] = cp->LocalSize[2];
>> unsigned local_workgroup_size =
>>cp->LocalSize[0] * cp->LocalSize[1] * cp->LocalSize[2];
>> +   unsigned max_cs_threads = 
>> brw->intelScreen->compiler->devinfo->max_cs_threads;
>
> I'm not following how this helps? Seems like you want to just pass in
> devinfo or something?

We want to pass in just brw_compiler eventually. At that point
brw_context won't be available. Jason will do that next, and I guess
you could argue that this patch belongs in that series.

However, this one occurrence is the only place the compiler accesses
devinfo values through brw_context and getting rid of it means callers
without a real brw_context have to fake a little less.

Kristian
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 10/12] i965/cs: Get max_cs_threads from brw_compiler devinfo

[Jason Ekstrand]

On Oct 7, 2015 3:36 PM, "Kristian Høgsberg"  wrote:
>
> On Wed, Oct 7, 2015 at 3:11 PM, Matt Turner  wrote:
> > On Wed, Oct 7, 2015 at 7:11 AM, Kristian Høgsberg Kristensen
> >  wrote:
> >> Signed-off-by: Kristian Høgsberg Kristensen 
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_fs.cpp | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> index 65c3628..b79b4a4 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> @@ -5276,6 +5276,7 @@ brw_cs_emit(struct brw_context *brw,
> >> prog_data->local_size[2] = cp->LocalSize[2];
> >> unsigned local_workgroup_size =
> >>cp->LocalSize[0] * cp->LocalSize[1] * cp->LocalSize[2];
> >> +   unsigned max_cs_threads =
brw->intelScreen->compiler->devinfo->max_cs_threads;
> >
> > I'm not following how this helps? Seems like you want to just pass in
> > devinfo or something?
>
> We want to pass in just brw_compiler eventually. At that point
> brw_context won't be available. Jason will do that next, and I guess
> you could argue that this patch belongs in that series.

I would tend to agree with Matt. This patch doesn't really do anything on
its own and I plan on doing it "for real" soon enough that "other callers"
won't have to wait long.  Let's put this off until we can do it "the right
way".
--Jason

> However, this one occurrence is the only place the compiler accesses
> devinfo values through brw_context and getting rid of it means callers
> without a real brw_context have to fake a little less.
>
> Kristian
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev