Re: [Mesa-dev] [PATCH 1/2] i965: Rework the modifier info map

[Jason Ekstrand]

On Mon, Jul 17, 2017 at 8:24 AM, Daniel Stone  wrote:

> Hi,
>
> On 17 July 2017 at 15:39, Jason Ekstrand  wrote:
> > On Mon, Jul 17, 2017 at 2:24 AM, Daniel Stone 
> wrote:
> >> On 17 July 2017 at 01:48, Jason Ekstrand  wrote:
> >> > This commit splits the mapping in half.  The modifier_infos table now
> >> > only contains the modifier and the since_gen field.  The tiling bits
> >> > have been moved into a table in modifier_to_tiling as that's the only
> >> > place it was ever used.  The modifier_is_supported function now takes
> a
> >> > devinfo and does the since_gen check.
> >>
> >> Any reason to not just drop it the modifier <-> tiling map completely
> >> and use isl_drm_modifier_get_info() + isl_tiling_to_i915_tiling()? I'm
> >> all for more of this code being deleted! :)
> >
> > Because the commit message is wrong.  It's the tiling_to_modifier
> function
> > into which it gets moved. :-)
>
> Sure. What I meant is that it seems like relatively low-hanging fruit
> to do this all in ISL rather than open-coded.
>

Sure.  Pulling tiling_to_modifier into ISL may not be a bad idea long-term.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] i965: Rework the modifier info map

[Daniel Stone]

Hi,

On 17 July 2017 at 15:39, Jason Ekstrand  wrote:
> On Mon, Jul 17, 2017 at 2:24 AM, Daniel Stone  wrote:
>> On 17 July 2017 at 01:48, Jason Ekstrand  wrote:
>> > This commit splits the mapping in half.  The modifier_infos table now
>> > only contains the modifier and the since_gen field.  The tiling bits
>> > have been moved into a table in modifier_to_tiling as that's the only
>> > place it was ever used.  The modifier_is_supported function now takes a
>> > devinfo and does the since_gen check.
>>
>> Any reason to not just drop it the modifier <-> tiling map completely
>> and use isl_drm_modifier_get_info() + isl_tiling_to_i915_tiling()? I'm
>> all for more of this code being deleted! :)
>
> Because the commit message is wrong.  It's the tiling_to_modifier function
> into which it gets moved. :-)

Sure. What I meant is that it seems like relatively low-hanging fruit
to do this all in ISL rather than open-coded.

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

Re: [Mesa-dev] [PATCH 1/2] i965: Rework the modifier info map

[Jason Ekstrand]

On Mon, Jul 17, 2017 at 2:24 AM, Daniel Stone  wrote:

> Hi,
>
> On 17 July 2017 at 01:48, Jason Ekstrand  wrote:
> > This commit splits the mapping in half.  The modifier_infos table now
> > only contains the modifier and the since_gen field.  The tiling bits
> > have been moved into a table in modifier_to_tiling as that's the only
> > place it was ever used.  The modifier_is_supported function now takes a
> > devinfo and does the since_gen check.
>
> Any reason to not just drop it the modifier <-> tiling map completely
> and use isl_drm_modifier_get_info() + isl_tiling_to_i915_tiling()? I'm
> all for more of this code being deleted! :)
>

Because the commit message is wrong.  It's the tiling_to_modifier function
into which it gets moved. :-)


> >  static const struct {
> > -   uint32_t tiling;
> > uint64_t modifier;
> > unsigned since_gen;
> > -} tiling_modifier_map[] = {
> > -   { .tiling = I915_TILING_NONE, .modifier = DRM_FORMAT_MOD_LINEAR,
> > - .since_gen = 1 },
> > -   { .tiling = I915_TILING_X, .modifier = I915_FORMAT_MOD_X_TILED,
> > - .since_gen = 1 },
> > -   { .tiling = I915_TILING_Y, .modifier = I915_FORMAT_MOD_Y_TILED,
> > - .since_gen = 6 },
> > +} supported_modifiers[] = {
> > +   { DRM_FORMAT_MOD_LINEAR   , 1 },
> > +   { I915_FORMAT_MOD_X_TILED , 1 },
> > +   { I915_FORMAT_MOD_Y_TILED , 6 },
> >  };
>
> Losing named initialisation makes me very sad. Is there any
> buildsys/compiler reason to do this?
>

Just because it made it one line per modifier.  That said, I think I could
probably put them back in.


> The rest LGTM, so with or without ISL:
> Reviewed-by: Daniel Stone 
>

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

Re: [Mesa-dev] [PATCH 1/2] i965: Rework the modifier info map

[Daniel Stone]

Hi,

On 17 July 2017 at 01:48, Jason Ekstrand  wrote:
> This commit splits the mapping in half.  The modifier_infos table now
> only contains the modifier and the since_gen field.  The tiling bits
> have been moved into a table in modifier_to_tiling as that's the only
> place it was ever used.  The modifier_is_supported function now takes a
> devinfo and does the since_gen check.

Any reason to not just drop it the modifier <-> tiling map completely
and use isl_drm_modifier_get_info() + isl_tiling_to_i915_tiling()? I'm
all for more of this code being deleted! :)

>  static const struct {
> -   uint32_t tiling;
> uint64_t modifier;
> unsigned since_gen;
> -} tiling_modifier_map[] = {
> -   { .tiling = I915_TILING_NONE, .modifier = DRM_FORMAT_MOD_LINEAR,
> - .since_gen = 1 },
> -   { .tiling = I915_TILING_X, .modifier = I915_FORMAT_MOD_X_TILED,
> - .since_gen = 1 },
> -   { .tiling = I915_TILING_Y, .modifier = I915_FORMAT_MOD_Y_TILED,
> - .since_gen = 6 },
> +} supported_modifiers[] = {
> +   { DRM_FORMAT_MOD_LINEAR   , 1 },
> +   { I915_FORMAT_MOD_X_TILED , 1 },
> +   { I915_FORMAT_MOD_Y_TILED , 6 },
>  };

Losing named initialisation makes me very sad. Is there any
buildsys/compiler reason to do this?

The rest LGTM, so with or without ISL:
Reviewed-by: Daniel Stone 

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