Jason Ekstrand <ja...@jlekstrand.net> writes:
> 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.
>
Thanks!
> --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
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
