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

Previews show small and tiled in Davinci Resolve on A770 #736

Closed
myownfriend opened this issue May 25, 2024 · 24 comments
Closed

Previews show small and tiled in Davinci Resolve on A770 #736

myownfriend opened this issue May 25, 2024 · 24 comments
Labels
merged change was merged

Comments

@myownfriend
Copy link

In Davinci Resolve 18 and 19 with an A770 on Fedora, footage shows properly in the timeline and media pool but the preview in the monitor is small and tiled. This only appears to happen on discrete GPUs because people with integrated graphics don't appear to be having the same issue. I don't know for sure if it's a compute runtime issue or if it's on the Mesa end.

Screenshot from 2024-05-25 14-56-56

@smunaut
Copy link
Contributor

smunaut commented May 27, 2024

As a bit of context for the issue, Resolve uses CL/GL sharing and I'm fairly sure the issue comes down to a disagreement between mesa and compute-runtime as to what tiling format the buffer is in (so compute side writes it in one format and the GL part displays it assuming it's another format). The details about the format are given as the "modifier" when exporting the dmabuf from mesa but ATM the only two supported case are "is zero" -> Force linear and "not zero" -> We assume that magically mesa and compute-runtime will happen to pick the same format for the same image dimensions / pixel format ...

@myownfriend
Copy link
Author

So would it be more likely to be on the Mesa end? I'm assuming the CL side doesn't differentiate between discrete and integrated cards but the Mesa driver would. Or is it a little of both?

@smunaut
Copy link
Contributor

smunaut commented May 28, 2024

Both have special case. It's not so much a function of dedicated vs igpu, just the generation of the core, the Arch (and newer iGPU too) have different tiling modes.

In CL/GL sharing the GL buffer is created first and at that point Mesa doesn't even know if that buffer is for sharing or not ...
So ideally the compute part should just adapt its access layout to whatever mesa picked.

@myownfriend
Copy link
Author

Just for completeness, the same behavior also effects Blackmagic's BRAW Player.

@Tiefseetauchner
Copy link

@myownfriend what MESA version are you on? Can you reproduce the issue with 24.1 or 24.2 (git)? I was debugging around and somewhere along the line I couldn't reproduce the issue with 21.1 anymore and before we start messing with GmmLib, I'd like a confirmation that it isn't just a Mesa issue

@myownfriend
Copy link
Author

@Tiefseetauchner I'm on 24.0.9: the latest version of the package for Fedora 40. I don't know when they're gonna roll out 24.1 either.

I was just told about meson devenv yesterday so I'm gonna try compiling mesa today.

@Tiefseetauchner
Copy link

Yeah that's what I've been doing on Manjaro

Thanks for trying, while there's a extremely slim change this'll fix it, I need the sanity check lol

@myownfriend
Copy link
Author

myownfriend commented Jun 11, 2024

@Tiefseetauchner just tried it out with 24.2(git) and it's waaay better but it's not fixed. Now the tiles are larger and longer lol

Screenshot from 2024-06-11 10-35-31

@Tiefseetauchner
Copy link

Ok that's good

Well no, on multiple levels, like, I seem tk have a broken GmmLib on mt computer

But it means my theory was right and I can implement a fix... Somehow

@smunaut
Copy link
Contributor

smunaut commented Jun 11, 2024

For the record it changed because this bug got fixed : https://gitlab.freedesktop.org/mesa/mesa/-/issues/9994
I thought the fix was in 24.0.x because I reported it a while ago but apparently it was never backported to the 24.0.x series.

Previously the exported buffer would be reported with a modifier = 0 meaning linear even though it's not. Mesa is most likely to pick Tile4 for all images on Arc. But intel-compute-runtime picks Tile64 most likely.

@myownfriend
Copy link
Author

For some reason I had thought this was merged already but I guess it isn't.

https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29244

I'm gonna see if I can make it work with that patch

@smunaut
Copy link
Contributor

smunaut commented Jun 11, 2024

I'm not sure it would help because Tile64 needs an aux buffer and I think iris doesn't currently support dmabuf export of surface with aux buffers so they get converted at export time into supported formats.

@myownfriend
Copy link
Author

Screenshot from 2024-06-11 10-51-17
That got it working!

@smunaut
Copy link
Contributor

smunaut commented Jun 11, 2024

I don't understand how ...
https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/gallium/drivers/iris/iris_resource.c?ref_type=heads#L1732

There isn't even a modifier for tile64 assigned ...
Only thing I can think of is that it ends up converting it to linear before export but this would murder access performance to the image data.

@smunaut
Copy link
Contributor

smunaut commented Jun 11, 2024

@Tiefseetauchner If you try that patch, could you tell me what modifier you end up getting from mesa ?

@Tiefseetauchner
Copy link

I'm gonna be home in about two hours, I'll see then

@myownfriend
Copy link
Author

I don't understand how ... https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/gallium/drivers/iris/iris_resource.c?ref_type=heads#L1732

There isn't even a modifier for tile64 assigned ... Only thing I can think of is that it ends up converting it to linear before export but this would murder access performance to the image data.

Maybe that's why playback performance falls just short of real-time for me but it runs at real-time on Windows.

@smunaut
Copy link
Contributor

smunaut commented Jun 11, 2024

@myownfriend So the leading theory ATM talking to devs on IRC is that in the code I posted, it ends up on the unsupported line, but if you're not running a debug build, assert are disabled so it would just return false, never set modifier and so what gets returned is some random garbage.

And on the intel-compute-runtime side, since all it check is if modifier is 0 or not, it would hit the "not" case and use tiling format which is Tile64, so it ends up matching by random luck (and also only if the garbage returned happens to not be 0 ...)

But this would need to be confirmed with gdb.

@Tiefseetauchner
Copy link

I get the following

p texOut.modifier 
$1 = 140682631639616

(Sorry forgot how to actually debug with gdb one day I'll remember)

@Tiefseetauchner
Copy link

It is indeed a different modifier than I got last time, but I can't make heads or tails of it. 0x7FF33A48F240 shouldn't be a valid DRM_FORMAT...

@Tiefseetauchner
Copy link

I can confirm that that patch is the fix. I tried without and I get the error, and I don't with. So this was the issue. Sadly I will not get a contributor badge for this repo today /hj

@smunaut
Copy link
Contributor

smunaut commented Jun 11, 2024

Yeah 0x7FF33A48F240 is just random garbage ... because that MR introduces a mesa bug in export that just happens to combine most of the time to make it work ...

@smunaut
Copy link
Contributor

smunaut commented Jun 21, 2024

@JablonskiMateusz So looking at how to fix this properly, I need to have the ability to specify a specific tiling format when calling createGraphicsAllocationFromSharedHandle.

And that info has to get down to Gmm::setupImageResourceParams so I could set the resourceParams.Flags.Info.* flags appropriately. And the only thing that goes from point A to point B is imgInfo.

So do you think it's be acceptable to add a new field to the imgInfo struct, something like :

enum {
  TileAuto = 0
  TileX,
  TileY,
  TileYf,
  TileYs,
  Tile4,
  Tile64,
} forceTiling;

Or do you see a better option ?

EDIT: smunaut@3c13f91 This is what I'm proposing.

smunaut added a commit to smunaut/compute-runtime that referenced this issue Jun 21, 2024
Previously we just assumed that whatever tiling mode was picked by mesa
will match the one picked by GMMLIB but that's not always the case
and in particular on Arc and Xe it doesn't work ... Mesa picks Tile4
and GMMLIB picks Tile64 ...

Fixes: intel#736

Signed-off-by: Sylvain Munaut <[email protected]>
smunaut added a commit to smunaut/compute-runtime that referenced this issue Sep 2, 2024
Previously we just assumed that whatever tiling mode was picked by mesa
will match the one picked by GMMLIB but that's not always the case
and in particular on Arc and Xe it doesn't work ... Mesa picks Tile4
and GMMLIB picks Tile64 ...

Fixes: intel#736

Signed-off-by: Sylvain Munaut <[email protected]>
@smunaut
Copy link
Contributor

smunaut commented Sep 2, 2024

Finally opened a PR to fix this since all reports I got were positive, so I forward ported it to master, checked it works for me and that the ocl tests were not broken by changes ... hopefully that can get reviewed quickly 😅

smunaut added a commit to smunaut/compute-runtime that referenced this issue Sep 3, 2024
Previously we just assumed that whatever tiling mode was picked by mesa
will match the one picked by GMMLIB but that's not always the case
and in particular on Arc and Xe it doesn't work ... Mesa picks Tile4
and GMMLIB picks Tile64 ...

Fixes: intel#736

Signed-off-by: Sylvain Munaut <[email protected]>
@JablonskiMateusz JablonskiMateusz added the merged change was merged label Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged change was merged
Projects
None yet
Development

No branches or pull requests

4 participants