Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for tiled textures #61

Open
wants to merge 4 commits into
base: lima-18.1
Choose a base branch
from
Open

Add support for tiled textures #61

wants to merge 4 commits into from

Conversation

anarsoul
Copy link
Contributor

@anarsoul anarsoul commented Jun 4, 2018

This is initial support for tiled textures - tiling/untiling routines aren't optimized yet.

lima-18.1 branch can't compile fragment shader of "kmscube -M rgba" due to ppir regalloc failure, I'm using this patch as a workaround for testing: anarsoul/mesa-lima@14e0665

Tested with kmscube -M rgba from my repo (it uses 500x500 texture) and gbm-surface-fbo from your gfx repo with 260x260 fbo.

shadeslayer and others added 4 commits June 3, 2018 19:56
Fixes gbm_surface_create_with_modifiers() failure in kmscube

Signed-off-by: Vasily Khoruzhick <[email protected]>
Copy link
Owner

@yuq yuq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch.

wb[0].pitch = res->stride / 8;
if (res->tiled) {
wb[0].pixel_layout = 0x2;
wb[0].pitch = align(ctx->framebuffer.width, 16) / 16;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as ctx->framebuffer.tiled_w

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will use that

res->base = *templat;
res->base.screen = pscreen;
pipe_reference_init(&res->base.reference, 1);

/* TODO: mipmap */
pres = &res->base;
res->stride = util_format_get_stride(pres->format, width);
res->tiled = should_tile;
res->stride = util_format_get_stride(pres->format, should_tile ? align(width, 16) : width);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like add this:

if (should_tile) {
   width = align(width, 16);
   height = align(height, 16);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that is not correct. Width and height don't have to be multiply of 16. But stride has to.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

width & height passed in is already not the logical one but the real one used to alloc buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

ptrans->box.y = box_y1;
ptrans->box.width = box_x2 - box_x1;
ptrans->box.height = box_y2 - box_y1;
trans->map = malloc(ptrans->stride * ptrans->box.height * ptrans->box.depth);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can trans->map only contain the box size data? Currently the stride is the whole texture stride. Besides I'd like to use "staging" instead of "map" for trans. Maybe u_transfer_helper_transfer_map is a reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, but we can limit height. I'll change map to staging, it sounds reasonable.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? Seems u_transfer_helper_transfer_map does like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll look into u_transfer_helper_transfer_map implementation.

struct pipe_resource *pres;

if (trans->map) {
pres = &res->base;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use ptrans->resource?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we certainly can, will fix.

/*
* Copyright (c) 2011-2013 Luc Verhaegen <[email protected]>
* Copyright (c) 2018 Alyssa Rosenzweig <[email protected]>
* Copyright (c) 2018 Lima Project
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may sign your name as the feedback in drm-lima RFC "Lima Project" is not a real org.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

struct pipe_resource tmpl = *templat;

/*
* We currently assume that all buffers allocated through this interface
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we assume this? Can't we just return NULL for not support or add a modifier for "tiled"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just mimicking etnaviv behavior. If we don't do it here kmscube doesn't work.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, we may improve it latter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants