[Mesa-dev] [PATCH 4/6] [v2] i965: Handle Y-tiled modifier
Jason Ekstrand
jason at jlekstrand.net
Tue Mar 21 15:07:22 UTC 2017
On Tue, Mar 21, 2017 at 8:04 AM, Jason Ekstrand <jason at jlekstrand.net>
wrote:
> On Mon, Mar 20, 2017 at 8:35 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 kmscbe:
>> 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>
>> ---
>> src/gbm/backends/dri/gbm_dri.c | 18 ++++++++++++++++
>> src/mesa/drivers/dri/i965/intel_screen.c | 36
>> ++++++++++++++++++++++++++++----
>> 2 files changed, 50 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/gbm/backends/dri/gbm_dri.c
>> b/src/gbm/backends/dri/gbm_dri.c
>> index a7ac149365..a78ea89fca 100644
>> --- a/src/gbm/backends/dri/gbm_dri.c
>> +++ b/src/gbm/backends/dri/gbm_dri.c
>> @@ -1143,6 +1143,15 @@ gbm_dri_bo_create(struct gbm_device *gbm,
>> goto failed;
>> }
>>
>> + if (modifiers) {
>> + for (int i = 0; i < count; i++)
>> + if (modifiers[i] == DRM_FORMAT_MOD_INVALID) {
>> + fprintf(stderr, "Invalid modifier passed in
>> DRM_FORMAT_MOD_INVALID");
>> + errno = EINVAL;
>> + goto failed;
>> + }
>> + }
>> +
>> bo->image =
>> dri->image->createImageWithModifiers(dri->screen,
>> width, height,
>> @@ -1240,6 +1249,15 @@ gbm_dri_surface_create(struct gbm_device *gbm,
>> return NULL;
>> }
>>
>> + if (modifiers) {
>> + for (int i = 0; i < count; i++)
>> + if (modifiers[i] == DRM_FORMAT_MOD_INVALID) {
>> + fprintf(stderr, "Invalid modifier passed in
>> DRM_FORMAT_MOD_INVALID");
>> + errno = EINVAL;
>> + return NULL;
>> + }
>> + }
>>
>
> This could be its own patch but I don't care too much.
>
Also, you don't need this anymore now that MOD_INVALID just falls through
and doesn't assert below. Feel free to drop it, pull it into its own
patch, or leave it here. (I have a mild preference for dropping it and
just ignoring MOD_INVALID but don't care too much). With that, series is
Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
> +
>> surf = calloc(1, sizeof *surf);
>> if (surf == NULL) {
>> errno = ENOMEM;
>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
>> b/src/mesa/drivers/dri/i965/intel_screen.c
>> index 14e60ef1a1..e4f858ed33 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:
>> default:
>> break;
>> @@ -626,8 +634,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");
>> + default:
>> + /* Modifiers from other vendors would land here. */
>> + break;
>> + }
>> + }
>> +
>> + return intel_create_image_common(dri_screen, width, height, format,
>> 0,
>> + local_mods, local_count,
>> loaderPrivate);
>> }
>>
>> static GLboolean
>> @@ -1997,7 +2023,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
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170321/c4ad10d4/attachment.html>
More information about the mesa-dev
mailing list