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

Initial RGBA16F capabilities for vc4 #115

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

nkreeger
Copy link

Hi @anholt - here is a few things that I think are needed for #114

I am not a domain expert in drivers so I'm mostly piecing together bits that I see the v3d driver does already. Happy to iterate on this if you are open to some collaboration in helping me bridge some domain knowledge gap here.

@anholt
Copy link
Owner

anholt commented Oct 29, 2018

Looks like a reasonable start. Note that src/gallium/driivers/vc4/kernel/is just copies of the kernel code used for the software simulator, and you'll need to modify your actual kernel drivers/gpu/drm/vc4/ code :) (I'll make sure that vc4/kernel is synced up when merging)

@nkreeger
Copy link
Author

Oh ok - so the actual change needs to go in here? https://github.com/torvalds/linux/tree/master/drivers/gpu/drm/vc4

@nkreeger
Copy link
Author

Or if I'm just targeting Raspberry Pi - here https://github.com/raspberrypi/linux/tree/rpi-4.14.y/drivers/gpu/drm/vc4 ?

@anholt
Copy link
Owner

anholt commented Oct 30, 2018

Yeah, feel free to work against downstream rpi's kernel. The validation code hasn't changed in so long that we should be able to apply any patches that you send (to the dri-devel mailing list, since the kernel has an archaic contribution process) to master successfully.

@nkreeger
Copy link
Author

nkreeger commented Nov 1, 2018

Great thanks for the info - I just sent the vc4_validate.c change to dri-devel mailing list. I assume it is a good idea to keep these patches small - so I'll try tackling the HDR changes later in another patch.

@nkreeger
Copy link
Author

nkreeger commented Nov 5, 2018

@anholt PTAL when you have a chance.

I'm not sure we need the "is float texture" flag - can we just rely on the enum pipe_format format field of vc4_key? Do you just want to avoid a future if condition with a bunch of format == .... checks?

Also - it looks like we can rely on the nir_op-* commands for unpack and packing?

@anholt
Copy link
Owner

anholt commented Nov 5, 2018

Yeah, I'd forgotten that we had the format in the key already.

Ideally we would have our texturing unpacks be in NIR, but I haven't done that yet. If we did, there would be a chance for NIR to do some useful optimizations ("oh, hey, you unpacked and then repacked the value, we can just skip that") You'll see at the end of ntq_emit_tex() that we do qir_UNPACK_8_F() for unorm8-to-float conversion, so that's the thing that would need a variant for unpacking 16f instead.

@nkreeger
Copy link
Author

nkreeger commented Nov 5, 2018

OK thanks that is helpful - to clarify: Texture packs are handled from NIR but texture unpacks are not?

@anholt
Copy link
Owner

anholt commented Nov 5, 2018

You mean packing for TLB writes? Yeah, I moved that to NIR, and some sort of similar logic might work for texturing. (It's kind of weird because we don't get to have NIR have a different number of components returned from the texture instructions, so we'd end up having returned components that are just unused)

@anholt
Copy link
Owner

anholt commented Dec 20, 2018

WIP moving the texture unpacking to NIR for v3d, should be straightforward to use in vc4 too:

https://gitlab.freedesktop.org/anholt/mesa/commits/v3d-lower-tex-results

@anholt
Copy link
Owner

anholt commented Jan 5, 2019

The NIR unpacking support is now merged:
https://gitlab.freedesktop.org/mesa/mesa/merge_requests/45

@nkreeger
Copy link
Author

nkreeger commented Jan 6, 2019 via email

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.

None yet

2 participants