Bug 106283 - Shader replacements works only for limited use cases
Summary: Shader replacements works only for limited use cases
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: unspecified
Hardware: All All
: medium enhancement
Assignee: Tapani Pälli
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-04-27 13:29 UTC by i.kalvachev
Modified: 2018-08-15 08:49 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
fix wip (3.97 KB, patch)
2018-06-26 10:38 UTC, Tapani Pälli
Details | Splinter Review

Description i.kalvachev 2018-04-27 13:29:21 UTC
In the mesa documentation here: https://www.mesa3d.org/shading.html#envvars
you can find a section about "Shader Replacement" that tells you how you can dump the source of GLSL shaders and even replace some shaders with your own.

Unfortunately, I couldn't make this work for the app that is having a problem.

I dig a bit in the Mesa code and came to the conclusion that the dump&replace works only for the  glProgramStringARB(),  while the app seems to provide the shader through  glShaderSourceARB()  that seems to be completely separate from the code that supports dump&replace.

It would be nice if this feature could work with all code paths.

In the past, I've been able to narrow down at least two R600 assembly bugs, by tweaking the shaders that generate them (using d3d9 library that can do that).
It's very disappointing that Mesa has the functionality, but it works only for a limited code paths.

At very least, document properly that quirk.
Comment 1 Tapani Pälli 2018-04-28 06:15:17 UTC
If you are referring to MESA_SHADER_DUMP_PATH and MESA_SHADER_READ_PATH, your conclusion seems wrong.

Shader replacement works when application calls glShaderSource(). You need to set the environment variables pointing to existing paths for it to work. Shader replacements don't work for ancient glProgramString() API.
Comment 2 Tapani Pälli 2018-04-28 06:28:26 UTC
One source of confusion here might be that there exists 2 shader dumping paths in Mesa, created for different purposes.

Shader replacement is done with MESA_SHADER_DUMP_PATH and MESA_SHADER_READ_PATH, this can be used to analyze performance, debug shaders and so on. Then there is MESA_SHADER_CAPTURE_PATH which is used to capture shaders in a special form that can be used with 'shader-db' tool. This supports ARB programs but cannot be used together with MESA_SHADER_DUMP_PATH and MESA_SHADER_READ_PATH, these are separate things and used for different purposes.

Hopefully this helps!
Comment 3 i.kalvachev 2018-04-28 09:18:26 UTC
No it does not help.
Yes, I swapped the working and not working functions.
Sorry, my mistake. But the point remains.

As you have said yourself, Shader replacements don't work for ancient glProgramString().

I need that thing to work for glProgramString() too. This is what I request here.

The application is definitely using the ancient API that cannot dump&replace.
Comment 4 Tapani Pälli 2018-04-28 12:36:23 UTC
(In reply to iive from comment #3)
> No it does not help.
> Yes, I swapped the working and not working functions.
> Sorry, my mistake. But the point remains.
> 
> As you have said yourself, Shader replacements don't work for ancient
> glProgramString().
> 
> I need that thing to work for glProgramString() too. This is what I request
> here.
> 
> The application is definitely using the ancient API that cannot dump&replace.

OK I see .. it looks like it should be pretty straightforward to implement this support by sharing the dumping code. Will take a look at this later.
Comment 5 ben@besd.de 2018-04-28 14:41:11 UTC
Hi,

if you are looking at the shader capture code, would you consider looking at this patch too?

https://lists.freedesktop.org/archives/mesa-dev/2018-April/193430.html

I just send it to the list. It changes the naming convention for captured shaders so that is easier to identify them.
Comment 6 Mark Janes 2018-04-29 16:39:54 UTC
FrameRetrace supports gl shader replacement.  I'll enhance it to support glProgramString.  It will be easier to debug shaders with FrameRetrace than using the environment variables.

https://fosdem.org/2018/schedule/event/apitrace/
Comment 7 i.kalvachev 2018-05-08 11:22:42 UTC
(In reply to Tapani Pälli from comment #4)
> (In reply to iive from comment #3)
> OK I see .. it looks like it should be pretty straightforward to implement
> this support by sharing the dumping code. Will take a look at this later.

Your words made this seems trivial and something that you could make straight away.
Yet it's been more than a week without any new development (afaik).

I hope you haven't forgotten about it and you could work on it in reasonable time frame.
Comment 8 Tapani Pälli 2018-05-08 11:56:23 UTC
(In reply to iive from comment #7)
> (In reply to Tapani Pälli from comment #4)
> > (In reply to iive from comment #3)
> > OK I see .. it looks like it should be pretty straightforward to implement
> > this support by sharing the dumping code. Will take a look at this later.
> 
> Your words made this seems trivial and something that you could make
> straight away.
> Yet it's been more than a week without any new development (afaik).
> 
> I hope you haven't forgotten about it and you could work on it in reasonable
> time frame.

I'm working on something different ATM and would not like to context switch but will look at this later. If changes from ben@besd.de make sense, it would be good to land those first. I'm not sure what implications his set has on using shader-db so I'm hoping some active user of shader-db to comment/review his series.
Comment 9 i.kalvachev 2018-05-09 18:59:32 UTC
(In reply to Tapani Pälli from comment #8)
> I'm working on something different ATM and would not like to context switch
> but will look at this later. If changes from ben@besd.de make sense, it
> would be good to land those first. I'm not sure what implications his set
> has on using shader-db so I'm hoping some active user of shader-db to
> comment/review his series.

Since the cause of that bug was discovered and there is working patch for it,
my urgency for this feature has decreased.

Just tell me after how many days/weeks I should remind you to check this again.
Comment 10 i.kalvachev 2018-05-21 21:49:45 UTC
(In reply to iive from comment #9)
> (In reply to Tapani Pälli from comment #8)
> > I'm working on something different ATM and would not like to context switch
> > but will look at this later. If changes from ben@besd.de make sense, it
> > would be good to land those first. I'm not sure what implications his set
> > has on using shader-db so I'm hoping some active user of shader-db to
> > comment/review his series.
> 
[...]
> 
> Just tell me after how many days/weeks I should remind you to check this
> again.

Just a reminder about the shader replacement feature request.

I hope that you haven't forgotten about it.
Comment 11 i.kalvachev 2018-06-21 14:44:24 UTC
(In reply to iive from comment #10)
> Just a reminder about the shader replacement feature request.
> 
> I hope that you haven't forgotten about it.

When I got my first reply I was so hopeful that this
simple request could be fulfilled in a few days.

Yet, here we are, one month after my last reminder
and I don't even get replies anymore.

Please, don't postpone it to infinity.
Comment 12 Mark Janes 2018-06-21 19:08:36 UTC
do you have an apitrace file of your workload, so I can test this functionality in FrameRetrace?
Comment 13 i.kalvachev 2018-06-22 11:08:47 UTC
(In reply to Mark Janes from comment #12)
> do you have an apitrace file of your workload, so I can test this
> functionality in FrameRetrace?

The needed functionality was to dig deeper into bug #91808.
You can find the link to my trace here:
https://bugs.freedesktop.org/show_bug.cgi?id=91808#c8
Comment 14 Tapani Pälli 2018-06-26 10:38:30 UTC
Created attachment 140342 [details] [review]
fix wip

I believe this should do the trick. If this works OK for you I can clean it up and send it forward.
Comment 15 Tapani Pälli 2018-07-25 09:28:52 UTC
FYI I've sent patch for review:
https://lists.freedesktop.org/archives/mesa-dev/2018-July/200885.html
Comment 16 Tapani Pälli 2018-08-15 08:49:37 UTC
commit 656ccf4ef890b91debbb72b38957723ca04411d0
Author: Tapani Pälli <tapani.palli@intel.com>
Date:   Tue Jul 24 10:41:46 2018 +0300

    mesa: shader dump/read support for ARB programs
    
    Signed-off-by: Tapani Pälli <tapani.palli@intel.com>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106283
    Reviewed-by: Alejandro Piñeiro <apinheiro@igalia.com>


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.