On Sat, Aug 8, 2015 at 4:04 AM, Francisco Jerez <curroje...@riseup.net> wrote:
> Jason Ekstrand <ja...@jlekstrand.net> writes:
>
>> On Wed, May 13, 2015 at 9:43 AM, Francisco Jerez <curroje...@riseup.net>
>> wrote:
>>> v2: Add SKL support.
>>> ---
>>> src/mesa/drivers/dri/i965/brw_context.h | 2 +
>>> src/mesa/drivers/dri/i965/brw_surface_formats.c | 109
>>> +++++++++++++++++++++++
>>> src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 77 ++++++++++++++++
>>> 3 files changed, 188 insertions(+)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
>>> b/src/mesa/drivers/dri/i965/brw_context.h
>>> index 2dcc23c..c55dcec 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_context.h
>>> +++ b/src/mesa/drivers/dri/i965/brw_context.h
>>> @@ -1741,6 +1741,8 @@ void brw_upload_abo_surfaces(struct brw_context *brw,
>>> bool brw_render_target_supported(struct brw_context *brw,
>>> struct gl_renderbuffer *rb);
>>> uint32_t brw_depth_format(struct brw_context *brw, mesa_format format);
>>> +mesa_format brw_lower_mesa_image_format(const struct brw_device_info
>>> *devinfo,
>>> + mesa_format format);
>>>
>>> /* brw_performance_monitor.c */
>>> void brw_init_performance_monitors(struct brw_context *brw);
>>> diff --git a/src/mesa/drivers/dri/i965/brw_surface_formats.c
>>> b/src/mesa/drivers/dri/i965/brw_surface_formats.c
>>> index 016f87a..e0e7fb6 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_surface_formats.c
>>> +++ b/src/mesa/drivers/dri/i965/brw_surface_formats.c
>>> @@ -805,3 +805,112 @@ brw_depth_format(struct brw_context *brw, mesa_format
>>> format)
>>> unreachable("Unexpected depth format.");
>>> }
>>> }
>>> +
>>> +mesa_format
>>> +brw_lower_mesa_image_format(const struct brw_device_info *devinfo,
>>> + mesa_format format)
>>> +{
>>> + switch (format) {
>>> + /* These are never lowered. Up to BDW we'll have to fall back to
>>> untyped
>>> + * surface access for 128bpp formats.
>>> + */
>>> + case MESA_FORMAT_RGBA_UINT32:
>>> + case MESA_FORMAT_RGBA_SINT32:
>>> + case MESA_FORMAT_RGBA_FLOAT32:
>>> + case MESA_FORMAT_R_UINT32:
>>> + case MESA_FORMAT_R_SINT32:
>>> + case MESA_FORMAT_R_FLOAT32:
>>> + return format;
>>
>> If it's an unsupported format, why not use MESA_FORMAT_NONE? That
>> would make it easier for functions that call this function to know
>> that it's just not supported and they shouldn't bother trying.
>>
> Because they are all supported. As the comment says these formats are
> always accessed unlowered, the catch is that the shader may have to use
> a different (much more painful) mechanism to access them depending on
> the hardware generation -- But this function simply implements the
> mapping between API image formats and what the 3D pipeline will see,
> which is R(GBA)_*32 in this case regardless of whether untyped or typed
> messages can be used.
Ok, I failed to read. Sorry for the noise.
>>> +
>>> + /* From HSW to BDW the only 64bpp format supported for typed access is
>>> + * RGBA_UINT16. IVB falls back to untyped.
>>> + */
>>> + case MESA_FORMAT_RGBA_UINT16:
>>> + case MESA_FORMAT_RGBA_SINT16:
>>> + case MESA_FORMAT_RGBA_FLOAT16:
>>> + case MESA_FORMAT_RG_UINT32:
>>> + case MESA_FORMAT_RG_SINT32:
>>> + case MESA_FORMAT_RG_FLOAT32:
>>> + return (devinfo->gen >= 9 ? format :
>>> + devinfo->gen >= 8 || devinfo->is_haswell ?
>>> + MESA_FORMAT_RGBA_UINT16 : MESA_FORMAT_RG_UINT32);
>>> +
>>> + /* Up to BDW no SINT or FLOAT formats of less than 32 bits per component
>>> + * are supported. IVB doesn't support formats with more than one
>>> component
>>> + * for typed access. For 8 and 16 bpp formats IVB relies on the
>>> + * undocumented behavior that typed reads from R_UINT8 and R_UINT16
>>> + * surfaces actually do a 32-bit misaligned read. The alternative
>>> would be
>>> + * to use two surface state entries with different formats for each
>>> image,
>>> + * one for reading (using R_UINT32) and another one for writing (using
>>> + * R_UINT8 or R_UINT16), but that would complicate the shaders we
>>> generate
>>> + * even more.
>>> + */
>>
>> Let's make sure I'm understanding this correctly. On BDW and HSW, we
>> can just use the UINT format for SINT and FLOAT. On IVB, we set the
>> surface state to UINT32 and unpack the components in the shader? What
>> happens with writes on IVB to 16-bpp images?
>>
> Heh, not exactly. On IVB we disobey the hardware spec and bind 16-bpp
> formats as R16_UINT and 8-bpp formats as R8_UINT, which aren't
> documented to work for typed reads. Writes work just fine and update
> the right 8- or 16-bit word on the image. Reads *almost* do what one
> would expect: The LSBs of the values returned from the data port will
> contain the texel we asked for, but the MSBs will be filled with garbage
> which we simply mask out
> (c.f. image_format_info::has_undefined_high_bits).
>
>>> + case MESA_FORMAT_RGBA_UINT8:
>>> + case MESA_FORMAT_RGBA_SINT8:
>>> + return (devinfo->gen >= 9 ? format :
>>> + devinfo->gen >= 8 || devinfo->is_haswell ?
>>> + MESA_FORMAT_RGBA_UINT8 : MESA_FORMAT_R_UINT32);
>>> +
>>> + case MESA_FORMAT_RG_UINT16:
>>> + case MESA_FORMAT_RG_SINT16:
>>> + case MESA_FORMAT_RG_FLOAT16:
>>> + return (devinfo->gen >= 9 ? format :
>>> + devinfo->gen >= 8 || devinfo->is_haswell ?
>>> + MESA_FORMAT_RG_UINT16 : MESA_FORMAT_R_UINT32);
>>> +
>>> + case MESA_FORMAT_RG_UINT8:
>>> + case MESA_FORMAT_RG_SINT8:
>>> + return (devinfo->gen >= 9 ? format :
>>> + devinfo->gen >= 8 || devinfo->is_haswell ?
>>> + MESA_FORMAT_RG_UINT8 : MESA_FORMAT_R_UINT16);
>>> +
>>> + case MESA_FORMAT_R_UINT16:
>>> + case MESA_FORMAT_R_FLOAT16:
>>> + case MESA_FORMAT_R_SINT16:
>>> + return (devinfo->gen >= 9 ? format : MESA_FORMAT_R_UINT16);
>>> +
>>> + case MESA_FORMAT_R_UINT8:
>>> + case MESA_FORMAT_R_SINT8:
>>> + return (devinfo->gen >= 9 ? format : MESA_FORMAT_R_UINT8);
>>> +
>>> + /* Neither the 2/10/10/10 nor the 11/11/10 packed formats are supported
>>> + * by the hardware.
>>> + */
>>> + case MESA_FORMAT_R10G10B10A2_UINT:
>>> + case MESA_FORMAT_R10G10B10A2_UNORM:
>>> + case MESA_FORMAT_R11G11B10_FLOAT:
>>> + return MESA_FORMAT_R_UINT32;
>>> +
>>> + /* No normalized fixed-point formats are supported by the hardware. */
>>> + case MESA_FORMAT_RGBA_UNORM16:
>>> + case MESA_FORMAT_RGBA_SNORM16:
>>> + return (devinfo->gen >= 8 || devinfo->is_haswell ?
>>> + MESA_FORMAT_RGBA_UINT16 : MESA_FORMAT_RG_UINT32);
>>> +
>>> + case MESA_FORMAT_R8G8B8A8_UNORM:
>>> + case MESA_FORMAT_R8G8B8A8_SNORM:
>>> + return (devinfo->gen >= 8 || devinfo->is_haswell ?
>>> + MESA_FORMAT_RGBA_UINT8 : MESA_FORMAT_R_UINT32);
>>> +
>>> + case MESA_FORMAT_R16G16_UNORM:
>>> + case MESA_FORMAT_R16G16_SNORM:
>>> + return (devinfo->gen >= 8 || devinfo->is_haswell ?
>>> + MESA_FORMAT_RG_UINT16 : MESA_FORMAT_R_UINT32);
>>> +
>>> + case MESA_FORMAT_R8G8_UNORM:
>>> + case MESA_FORMAT_R8G8_SNORM:
>>> + return (devinfo->gen >= 8 || devinfo->is_haswell ?
>>> + MESA_FORMAT_RG_UINT8 : MESA_FORMAT_R_UINT16);
>>> +
>>> + case MESA_FORMAT_R_UNORM16:
>>> + case MESA_FORMAT_R_SNORM16:
>>> + return MESA_FORMAT_R_UINT16;
>>> +
>>> + case MESA_FORMAT_R_UNORM8:
>>> + case MESA_FORMAT_R_SNORM8:
>>> + return MESA_FORMAT_R_UINT8;
>>> +
>>> + default:
>>> + unreachable("Not reached");
>>
>> Please be more descriptive.
>>
> Changed to "Unknown image format" locally.
>
>>> + }
>>> +}
>>> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>>> b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>>> index 160dd2f..f8cb143 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>>> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>>> @@ -1022,6 +1022,83 @@ const struct brw_tracked_state brw_cs_abo_surfaces =
>>> {
>>> .emit = brw_upload_cs_abo_surfaces,
>>> };
>>>
>>> +static uint32_t
>>> +get_image_format(struct brw_context *brw, mesa_format format, GLenum
>>> access)
>>> +{
>>> + if (access == GL_WRITE_ONLY) {
>>> + return brw_format_for_mesa_format(format);
>>> + } else {
>>> + /* Typed surface reads support a very limited subset of the shader
>>> + * image formats. Translate it into the closest format the
>>> + * hardware supports.
>>> + */
>>> + if ((_mesa_get_format_bytes(format) >= 16 && brw->gen <= 8) ||
>>> + (_mesa_get_format_bytes(format) >= 8 &&
>>> + (brw->gen == 7 && !brw->is_haswell)))
>>
>> It's tricky to verify, but this list of conditions doesn't look much
>> like the switch above. Maybe I'm missing something.
>>
>
> This check is equivalent to the one in
> image_format_info::has_matching_typed_format (I considered re-using the
> same function but then noticed that this file is C so I'd have to change
> a number of things of the image_format_info helpers in order to be able
> to include them here, what I didn't quite feel like doing and didn't
> seem worth the effort in order to save 2 LOC). The check is orthogonal
> to format lowering -- In cases where the lowered format can be accessed
> using typed messages we just go ahead and bind it, otherwise I set the
> format in the surface state structure to RAW in the expectation that the
> shader will want access them using untyped messages, because it's a
> requirement for them to work.
>
>>> + return BRW_SURFACEFORMAT_RAW;
>>> + else
>>> + return brw_format_for_mesa_format(
>>> + brw_lower_mesa_image_format(brw->intelScreen->devinfo,
>>> format));
>>> + }
>>> +}
>>
>> Why are get_image_format and lower_mesa_image_format separate
>> functions? Again, it seems like we have decisions being made two
>> different places. brw_lower_mesa_image_format lowers it to the format
>> as if it were write_only and then we do more switching here based on
>> gen. Maybe this is truly easier, but it seems odd.
>>
> brw_lower_mesa_image_format() implements the mapping between API formats
> and the format the shader will see. This function translates the
> lowered shader-format into the format that will be set in the surface
> state structure. None of these conditions directly apply to the format
> translation that will be done in the compiler, which is why it's kept
> separate from brw_lower_mesa_image_format() (which is also called from
> the compiler back-end). One condition relates to setting the surface
> state format to RAW (which forgets useful format information so we don't
> want to return that to the compiler back-end), and the other is about
> the additional optimization done in cases where we know that the image
> is only ever written to by the shader -- In those cases we just bind the
> original unlowered format and expect the shader to skip format
> conversion completely. You could argue that this could be handled in
> brw_lower_mesa_image_format() by adding a "GLenum access" parameter,
> however almost none of the callers of brw_lower_mesa_image_format() have
> the image access mode readily available, and passing it down from
> brw_fs_nir.cpp would uglify a number of interfaces, so it seemed easier
> to keep the access mode-dependent translation orthogonal from the actual
> format lowering.
Ok, this is all making a lot more sense. I guess I probably should
have read/understood this code before/during trying to read/understand
the lowering in surface_builder. I think I would like to see both
this code and the has_undefined_high_bits-type helpers put into a
single file with all of these interactions documented. However, that
can be done as a follow-on and doesn't need to block the extension.
Reviewed-by: Jason Ekstrand <jason.ekstr...@intel.com>
You can also consider most of my reservations about the
emit_typed_read() patch rescinded pending the above cleanup.
--Jason
>>> +
>>> +static void
>>> +update_image_surface(struct brw_context *brw,
>>> + struct gl_image_unit *u,
>>> + GLenum access,
>>> + unsigned surface_idx,
>>> + uint32_t *surf_offset,
>>> + struct brw_image_param *param)
>>> +{
>>> + if (u->_Valid) {
>>> + struct gl_texture_object *obj = u->TexObj;
>>> + const unsigned format = get_image_format(brw, u->_ActualFormat,
>>> access);
>>> +
>>> + if (obj->Target == GL_TEXTURE_BUFFER) {
>>> + struct intel_buffer_object *intel_obj =
>>> + intel_buffer_object(obj->BufferObject);
>>> + const unsigned texel_size = (format == BRW_SURFACEFORMAT_RAW ? 1 :
>>> +
>>> _mesa_get_format_bytes(u->_ActualFormat));
>>> +
>>> + brw->vtbl.emit_buffer_surface_state(
>>> + brw, surf_offset, intel_obj->buffer, obj->BufferOffset,
>>> + format, intel_obj->Base.Size / texel_size, texel_size,
>>> + access != GL_READ_ONLY);
>>> +
>>> + } else {
>>> + struct intel_texture_object *intel_obj =
>>> intel_texture_object(obj);
>>> + struct intel_mipmap_tree *mt = intel_obj->mt;
>>> +
>>> + if (format == BRW_SURFACEFORMAT_RAW) {
>>> + brw->vtbl.emit_buffer_surface_state(
>>> + brw, surf_offset, mt->bo, mt->offset,
>>> + format, mt->bo->size - mt->offset, 1 /* pitch */,
>>> + access != GL_READ_ONLY);
>>> +
>>> + } else {
>>> + const unsigned min_layer = obj->MinLayer + u->Layer;
>>> + const unsigned min_level = obj->MinLevel + u->Level;
>>> + const unsigned num_layers = (!u->Layered ? 1 :
>>> + obj->Target ==
>>> GL_TEXTURE_CUBE_MAP ? 6 :
>>> + mt->logical_depth0);
>>> + const GLenum target = (obj->Target == GL_TEXTURE_CUBE_MAP ||
>>> + obj->Target ==
>>> GL_TEXTURE_CUBE_MAP_ARRAY ?
>>> + GL_TEXTURE_2D_ARRAY : obj->Target);
>>> +
>>> + brw->vtbl.emit_texture_surface_state(
>>> + brw, mt, target,
>>> + min_layer, min_layer + num_layers,
>>> + min_level, min_level + 1,
>>> + format, SWIZZLE_XYZW,
>>> + surf_offset, access != GL_READ_ONLY, false);
>>> + }
>>> + }
>>> +
>>> + } else {
>>> + brw->vtbl.emit_null_surface_state(brw, 1, 1, 1, surf_offset);
>>> + }
>>> +}
>>> +
>>> void
>>> gen4_init_vtable_surface_functions(struct brw_context *brw)
>>> {
>>> --
>>> 2.3.5
>>>
>>> _______________________________________________
>>> 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
