Re: [Mesa-dev] [PATCH 1/2] i965: Rework the modifier info map
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
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
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
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