[Mesa-dev] [PATCH 6/8] i965/dri: Handle Y-tiled modifier

[Ben Widawsky]

On 17-03-21 13:52:12, Jason Ekstrand wrote:
>On Tue, Mar 21, 2017 at 1:45 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
>
>> This patch begins introducing how we'll actually handle the potentially
>> many modifiers coming in from the API, how we'll store them, and the
>> structure in the code to support it.
>>
>> Prior to this patch, the Y-tiled modifier would be entirely ignored. It
>> shouldn't actually be used until this point because we've not bumped the
>> DRIimage extension version (which is a requirement to use modifiers).
>>
>> Measuring later in the series with kmscube:
>> Linear:
>> Read bandwidth: 1048.44 MiB/s
>> Write bandwidth: 1483.17 MiB/s
>>
>> Y-tiled:
>> Read bandwidth: 471.13 MiB/s
>> Write bandwidth: 589.10 MiB/s
>>
>> Similar functionality was introduced and then reverted here:
>>
>> commit 6a0d036483caf87d43ebe2edd1905873446c9589
>> Author: Ben Widawsky <ben at bwidawsk.net>
>> Date:   Thu Apr 21 20:14:58 2016 -0700
>>
>>     i965: Always use Y-tiled buffers on SKL+
>>
>> v2: Use last set bit instead of first set bit in modifiers to address
>> bug found by Daniel Stone.
>>
>> v3: Use the new priority modifier selection thing. This nullifies the
>> bug fixed by v2 also.
>>
>> Signed-off-by: Ben Widawsky <benjamin.widawsky at intel.com>
>> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
>> Acked-by: Daniel Stone <daniels at collabora.com>
>> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
>> ---
>>  src/mesa/drivers/dri/i965/intel_screen.c | 36
>> ++++++++++++++++++++++++++++----
>>  1 file changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
>> b/src/mesa/drivers/dri/i965/intel_screen.c
>> index 8ba90f0a0c..8bb3aca946 100644
>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>> @@ -23,6 +23,7 @@
>>   * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>>   */
>>
>> +#include <drm_fourcc.h>
>>  #include <errno.h>
>>  #include <time.h>
>>  #include <unistd.h>
>> @@ -518,11 +519,13 @@ intel_destroy_image(__DRIimage *image)
>>  enum modifier_priority {
>>     MODIFIER_PRIORITY_INVALID = 0,
>>     MODIFIER_PRIORITY_LINEAR,
>> +   MODIFIER_PRIORITY_Y,
>>  };
>>
>>  const uint64_t priority_to_modifier[] = {
>>     [MODIFIER_PRIORITY_INVALID] = DRM_FORMAT_MOD_INVALID,
>>     [MODIFIER_PRIORITY_LINEAR] = DRM_FORMAT_MOD_LINEAR,
>> +   [MODIFIER_PRIORITY_Y] = I915_FORMAT_MOD_Y_TILED,
>>  };
>>
>>  static uint64_t
>> @@ -530,11 +533,13 @@ select_best_modifier(struct gen_device_info *devinfo,
>>                       const uint64_t *modifiers,
>>                       const unsigned count)
>>  {
>> -
>>     enum modifier_priority prio = MODIFIER_PRIORITY_INVALID;
>>
>>     for (int i = 0; i < count; i++) {
>>        switch (modifiers[i]) {
>> +      case I915_FORMAT_MOD_Y_TILED:
>> +         prio = MAX2(prio, MODIFIER_PRIORITY_Y);
>> +         break;
>>        case DRM_FORMAT_MOD_LINEAR:
>>           prio = MAX2(prio, MODIFIER_PRIORITY_LINEAR);
>>           break;
>> @@ -575,6 +580,9 @@ intel_create_image_common(__DRIscreen *dri_screen,
>>     case DRM_FORMAT_MOD_LINEAR:
>>        tiling = I915_TILING_NONE;
>>        break;
>> +   case I915_FORMAT_MOD_Y_TILED:
>> +      tiling = I915_TILING_Y;
>> +      break;
>>     case DRM_FORMAT_MOD_INVALID:
>>        if (modifiers)
>>           return NULL;
>> @@ -628,8 +636,26 @@ intel_create_image_with_modifiers(__DRIscreen
>> *dri_screen,
>>                                    const unsigned count,
>>                                    void *loaderPrivate)
>>  {
>> -   return intel_create_image_common(dri_screen, width, height, format,
>> 0, NULL,
>> -                                    0, loaderPrivate);
>> +   uint64_t local_mods[count];
>> +   int local_count = 0;
>> +
>> +   /* This compacts the actual modifiers to the ones we know how to
>> handle */
>> +   for (int i = 0; i < count; i++) {
>> +      switch (modifiers[i]) {
>> +      case I915_FORMAT_MOD_Y_TILED:
>> +      case DRM_FORMAT_MOD_LINEAR:
>> +         local_mods[local_count++] = modifiers[i];
>> +         break;
>> +      case DRM_FORMAT_MOD_INVALID:
>> +         unreachable("Invalid modifiers specified\n");
>>
>
>I don't think you want unreachable here.  The higher layers only check
>modifier zero.  They don't walk the entire list.
>
>

Yeah, same problem as before.

>> +      default:
>> +         /* Modifiers from other vendors would land here. */
>> +         break;
>> +      }
>> +   }
>>
>
>Also, what's the benifit of the local modifiers thing?  The
>select_best_modifier can handle an arbitrarily long list so I don't see
>what this is gaining us.
>
>

It's a relic from the original patches which actually stored a list of potential
modifiers (so it was calloc'd here). That required getting a count.  It can be
removed now because intel_create_image_common() doesn't do anything on
unrecognized modifies (again, it used to).

>> +
>> +   return intel_create_image_common(dri_screen, width, height, format, 0,
>> +                                    local_mods, local_count,
>> loaderPrivate);
>>  }
>>
>>  static GLboolean
>> @@ -1999,7 +2025,9 @@ intelAllocateBuffer(__DRIscreen *dri_screen,
>>     if (intelBuffer == NULL)
>>        return NULL;
>>
>> -   /* The front and back buffers are color buffers, which are X tiled. */
>> +   /* The front and back buffers are color buffers, which are X tiled.
>> GEN9+
>> +    * supports Y tiled and compressed buffers, but there is no way to
>> plumb that
>> +    * through to here. */
>>     uint32_t tiling = I915_TILING_X;
>>     unsigned long pitch;
>>     int cpp = format / 8;
>> --
>> 2.12.0
>>
>>