Re: [Mesa-dev] Perfetto CPU/GPU tracing

[Mark Janes]

Rob Clark  writes:

> On Fri, Feb 12, 2021 at 5:01 AM Tamminen, Eero T
>  wrote:
>>
>> Hi,
>>
>> On Thu, 2021-02-11 at 17:39 -0800, John Bates wrote:
>> > I recently opened issue 4262
>> >  to begin the
>> > discussion on integrating perfetto into mesa.
>> >
>> > *Background*
>> >
>> > System-wide tracing is an invaluable tool for developers to find and
>> > fix
>> > performance problems. The perfetto project enables a combined view of
>> > trace
>> > data from kernel ftrace, GPU driver and various manually-instrumented
>> > tracepoints throughout the application and system.
>>
>> Unlike some other Linux tracing solutions, Perfetto appears to be for
>> Android / Chrome(OS?), and not available from in common Linux distro
>> repos.
>
> I don't think there is anything about perfetto that would not be
> usable in a generic linux distro.. and mesa support for perfetto would
> perhaps be a compelling reason for distro's to add support
>
>> So, why Perfetto instead of one of the other solutions, e.g. from ones
>> mentioned here:
>> https://tracingsummit.org/ts/2018/
>> ?
>>
>> And, if tracing API is added to Mesa, shouldn't it support also
>> tracepoints for other tracing solutions?
>
> perfetto does have systrace collectors
>
> And a general comment on perfetto vs other things.. we end up needing
> to support perfetto regardless (for android and CrOS).. we don't
> *need* to enable it on generic linux, but I think we should (but maybe
> using the mode that does not require a system server.. at least
> initially.. that may limit it's ability to collect systrace and traces
> from other parts of the system, but that wouldn't depend on distro's
> enabling perfetto system server).

Perfetto seems like an awful lot of infrastructure to capture trace
events.  Why not follow the example of GPUVis, and write generic
trace_markers to ftrace?  It limits impact to Mesa, while allowing any
trace visualizer to use the trace points.

>> I mean, code added to drivers themselves preferably should not have
>> anything perfetto/percetto specific.  Tracing system specific code
>> should be only in one place (even if it's just macros in common header).
>>
>>
>> > This helps developers
>> > quickly answer questions like:
>> >
>> >- How long are frames taking?
>>
>> That doesn't require any changes to Mesa.  Just set uprobe for suitable
>> buffer swap function [1], and parse kernel ftrace events.  This way
>> starting tracing doesn't require even restarting the tracked processes.
>>
>
> But this doesn't tell you how long the GPU is spending doing what.  My
> rough idea is to hook up an optional callback to u_tracepoint so we
> can get generate perfetto traces on the GPU timeline (ie. with
> timestamps captured from GPU), fwiw

I implemented a feature called INTEL_MEASURE based off of a tool that
Ken wrote.  It captures render/batch/frame timestamps in a BO, providing
durations on the GPU timeline.  It works for Iris and Anv.

The approach provides accurate gpu timing, with minimal stalling.  This
data could be presented in Perfetto or GPUVis.

> BR,
> -R
>
>> [1] glXSwapBuffers, eglSwapBuffers, eglSwapBuffersWithDamageEXT,
>> anv_QueuePresentKHR[2]..
>>
>> [2] Many apps resolve "vkQueuePresentKHR" Vulkan API loader wrapper
>> function and call the backend function like "anv_QueuePresentKHR"
>> directly, so it's  better to track latter instead.
>>
>>
>> >- What caused a particular frame drop?
>> >- Is it CPU bound or GPU bound?
>>
>> That doesn't require adding tracepoints to Mesa, just checking CPU & GPU
>> utilization (which is lower level thing).
>>
>>
>> >- Did a CPU core frequency drop cause something to go slower than
>> > usual?
>>
>> Note that nowadays actual CPU frequencies are often controlled by HW /
>> firmware, so you don't necessarily get any ftrace event from freq
>> change, you would need to poll MSR registers instead (which is
>> privileged operation, and polling can easily miss changes).
>>
>>
>> >- Is something else running that is stealing CPU or GPU time? Could
>> > I
>> >fix that with better thread/context priorities?
>> >- Are all CPU cores being used effectively? Do I need
>> > sched_setaffinity
>> >to keep my thread on a big or little core?
>>
>> I don't think these to require adding tracepoints to Mesa either...
>>
>>
>> >- What’s the latency between CPU frame submit and GPU start?
>>
>> I think this would require tracepoints in kernel GPU code more than in
>> Mesa?
>>
>>
>> - Eero
>>
>>
>> > *What Does Mesa + Perfetto Provide?*
>> >
>> > Mesa is in a unique position to produce GPU trace data for several GPU
>> > vendors without requiring the developer to build and install
>> > additional
>> > tools like gfx-pps .
>> >
>> > The key is making it easy for developers to use. Ideally, perfetto is
>> > eventually available by default in mesa so that if your 

Re: [Mesa-dev] Perfetto CPU/GPU tracing

[Mark Janes]

I've recently been using GPUVis to look at trace events.  On Intel
platforms, GPUVis incorporates ftrace events from the i915 driver,
performance metrics from igt-gpu-tools, and userspace ftrace markers
that I locally hack up in Mesa.

It is very easy to compile the GPUVis UI.  Userspace instrumentation
requires a single C/C++ header.  You don't have to access an external
web service to analyze trace data (a big no-no for devs working on
preproduction hardware).

Is it possible to build and run the Perfetto UI locally?  Can it display
arbitrary trace events that are written to
/sys/kernel/tracing/trace_marker ?  Can it be extended to show i915 and
i915-perf-recorder events?

John Bates  writes:

> I recently opened issue 4262
>  to begin the
> discussion on integrating perfetto into mesa.
>
> *Background*
>
> System-wide tracing is an invaluable tool for developers to find and fix
> performance problems. The perfetto project enables a combined view of trace
> data from kernel ftrace, GPU driver and various manually-instrumented
> tracepoints throughout the application and system. This helps developers
> quickly answer questions like:
>
>- How long are frames taking?
>- What caused a particular frame drop?
>- Is it CPU bound or GPU bound?
>- Did a CPU core frequency drop cause something to go slower than usual?
>- Is something else running that is stealing CPU or GPU time? Could I
>fix that with better thread/context priorities?
>- Are all CPU cores being used effectively? Do I need sched_setaffinity
>to keep my thread on a big or little core?
>- What’s the latency between CPU frame submit and GPU start?
>
> *What Does Mesa + Perfetto Provide?*
>
> Mesa is in a unique position to produce GPU trace data for several GPU
> vendors without requiring the developer to build and install additional
> tools like gfx-pps .
>
> The key is making it easy for developers to use. Ideally, perfetto is
> eventually available by default in mesa so that if your system has perfetto
> traced running, you just need to run perfetto (perhaps along with setting
> an environment variable) with the mesa categories to see:
>
>- GPU processing timeline events.
>- GPU counters.
>- CPU events for potentially slow functions in mesa like shader compiles.
>
> Example of what this data might look like (with fake GPU events):
> [image: percetto-gpu-example.png]
>
> *Runtime Characteristics*
>
>- ~500KB additional binary size. Even with using only the basic features
>of perfetto, it will increase the binary size of mesa by about 500KB.
>- Background thread. Perfetto uses a background thread for communication
>with the system tracing daemon (traced) to advertise trace data and get
>notification of trace start/stop.
>- Runtime overhead when disabled is designed to be optimal with one
>predicted branch, typically a few CPU cycles
> per
>event. While enabled, the overhead can be around 1 us per event.
>
> *Integration Challenges*
>
>- The perfetto SDK is C++ and designed around macros, lambdas, inline
>templates, etc. There are ongoing discussions on providing an official
>perfetto C API, but it is not yet clear when this will land on the perfetto
>roadmap.
>- The perfetto SDK is an amalgamated .h and .cc that adds up to 100K
>lines of code.
>- Anything that includes perfetto.h takes a long time to compile.
>- The current Perfetto SDK design is incompatible with being a shared
>library behind a C API.
>
> *Percetto*
>
> The percetto library  was recently
> implemented to provide an interim C API for perfetto. It provides efficient
> support for scoped trace events, multiple categories, counters, custom
> timestamps, and debug data annotations. Percetto also provides some
> features that are important to mesa, but not available yet with perfetto
> SDK:
>
>- Trace events from multiple perfetto instances in separate shared
>libraries (like mesa and virglrenderer) show correctly in a single process
>and thread view.
>- Counter tracks and macro API.
>
> Percetto is missing API for perfetto's GPU DataSource and counter support,
> but that feature could be implemented next if it is important for mesa.
> With the existing percetto API mesa could present GPU trace data as named
> 'slice' events and int64_t counters with custom timestamps as shown in the
> image above (based on this sample
> ).
>
> *Mesa Integration Alternatives*
>
> Note: we have some pressing needs for performance analysis in Chrome OS, so
> I'm intentionally leaving out the alternative of waiting for an official
> perfetto C API. Of course, once that C API is available it would 

Re: [Mesa-dev] Mesa main repor branch clean-up

[Mark Janes]

Thank you for doing this.  The dead branches in the mesa repo always
annoyed me.

There are still many tags that look like they were inadvertently pushed
to the origin by developers.  Does it make sense to delete them as well?

-Mark

Jason Ekstrand  writes:

> All,
>
> At suggestion from several people on IRC, I've done a bit of house
> cleaning of the main Mesa repo.  I created a new mesa/mesa-archive
> repo and moved all of the stale feature branches to that repo and
> removed them from mesa/mesa.  Many of those branches haven't seen a
> commit in 10-20 years so all they're doing for most mesa devs is
> adding clutter.
>
> I left all the release branches since 7.8.  Dylan suggested I get rid
> of those too but it seems like a good idea to leave them for archival
> purposes.  If others think they should be removed, I'm happy to do
> that.
>
> If someone objects to moving stale branches to an archive repo, feel
> free to say so and I can always put them back.
>
> --Jason
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [ANNOUNCE] mesa 20.0.5

[Mark Janes]

Michel Dänzer  writes:

> On 2020-04-23 8:37 p.m., Mark Janes wrote:
>>  - search for issues mentioning a test name (unless it is in the title)
>
> https://gitlab.freedesktop.org/mesa/mesa/-/issues?scope=all=%E2%9C%93=opened=test
>
> lists issues without "test" in the title, so this doesn't seem quite as
> bad as you're making it.

The search finds text in the title and body of the issue.  Comments are
not searchable.  Most of the content of an issue is in the comments.

>>  - search for issues mentioning a commit
>
> https://gitlab.freedesktop.org/mesa/mesa/-/issues?scope=all=%E2%9C%93=opened=76dbcb1f5e48e10467b15a0e19232eccc3a57ae3
>
> seems to work?

In this example, the commit blob is matched because it is in the body
text.  Commits mentioned in comments are not matched.  Searching for
commits will not match a short sha.

Surprisingly, even when a commit *closes* an issue, you cannot find the
issue from the commit:

 https://gitlab.freedesktop.org/mesa/mesa/-/issues/2822
 
https://gitlab.freedesktop.org/mesa/mesa/-/issues?scope=all=%E2%9C%93=closed=665250e8300e2b0f3eae27628a9a6f2666e650dd

>>  - subscribe to issue comments in a way that would let you search
>>offline
>
> On https://gitlab.freedesktop.org/mesa/mesa (or its parent page, for the
> whole Mesa group of projects) click the drop-down next to the
> notification bell, select "Custom" and check the "New note" box, then
> you get an e-mail for every comment on every issue & MR.

The goal is to monitor bug investigations for our component and identify
problems that affect releases.  This mechanism will generate a lot of
mail, 95% of which is review comments on merge requests.  Filtering out
MR comments, the majority of remaining issue comments will still be for
other components.

A second email address is required to receive these messages, because
all other gitlab notifications (eg, for my own issues/MRs) would be lost
in the deluge.  A second account could filter out merge request
notifications, solving part of the problem.

Perhaps some creative scripting can be combined with your suggestion to
subscribe to all notes, tagging threads that have a message indicating
labels were added.

I need to subscribe to "all events for issues with labels: ANV i965 i915
iris".  This use case has not been implemented: label subscription only
tells you when a label is added/removed.

It's a lot of work to cobble together a workflow that should be
supported by Gitlab.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [ANNOUNCE] mesa 20.0.5

[Mark Janes]

Michel Dänzer  writes:

> On 2020-04-23 6:19 p.m., Mark Janes wrote:
>> Michel Dänzer  writes:
>>> On 2020-04-23 5:14 p.m., Mark Janes wrote:
>>>>
>>>> Does anyone have recommendations for how to use Gitlab to verify that
>>>> there are no identified-but-unfixed bugs in a pending release?
>>>
>>> I'd say GitLab milestones could be used to address the issues you raised
>>> above: Create a milestone for each release, and only cut the release
>>> once all issues and MRs assigned to it have been dealt with.
>> 
>> Milestones have been used to track progress toward recent releases.  It
>> is non-trivial to audit all open bugs to determine which ones must be
>> assigned to a milestone.  Bugs are opened long before milestones are
>> created.
>
> Of course there are more complicated cases, but the particular case
> which spawned this thread should have been pretty straightforward to
> handle with a 20.0.5 milestone.

I do not agree that release milestones are helpful for this category of
bugs.  The majority of stable point releases will have zero issues in a
release milestone.  Opening/closing empty milestones all the time is a
lot of busy work.

Milestones are helpful for *initial* releases of stable branches, not
point releases.  Even for initial releases important use cases for
milestones are not supported by gitlab.  As an example, we may be able
to verify that a specific test is regressed with an A/B test of the
previous release -- and perhaps identify the commit that regressed it.
How can we find if an issue exists for this test?  You cannot:

 - search for issues mentioning a test name (unless it is in the title)
 - search for issues mentioning a commit
 - subscribe to issue comments in a way that would let you search
   offline

How can we audit new issues for items that have not been triaged since
the last release?  The only workflow that I can imagine is to sort all
issues by "last updated" and read through every open issue changed in
the past 3 months.  Of course, the list changes as you read through it.
I'm contrasting this with Bugzilla, where we could subscribe to bug
comments and read through them on a daily basis.  At release time, I
could have confidence that I had seen all bugs that might affect end
users.

The labels "bisected" and "regression" serve as a good indicator of bugs
that should block release, assuming someone has by chance applied the
labels correctly.  For point releases, adding a "stable" label may tell
us when an issue blocks point releases as well.  Any issue related to a
commit that CC's stable would get this label.  Personally I think this
label will not solve the problems either, because it requires careful
monitoring of issues to notice regressions which cc stable.

The information needed to *automate verification* of stable releases
exists, within gitlab and the git log.  However, a sane and robust
release process cannot be implemented on top of gitlab's pitiful search
interface.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [ANNOUNCE] mesa 20.0.5

[Mark Janes]

Michel Dänzer  writes:

> On 2020-04-23 5:14 p.m., Mark Janes wrote:
>> 
>> Does anyone have recommendations for how to use Gitlab to verify that
>> there are no identified-but-unfixed bugs in a pending release?
>
> I'd say GitLab milestones could be used to address the issues you raised
> above: Create a milestone for each release, and only cut the release
> once all issues and MRs assigned to it have been dealt with.

Milestones have been used to track progress toward recent releases.  It
is non-trivial to audit all open bugs to determine which ones must be
assigned to a milestone.  Bugs are opened long before milestones are
created.

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [ANNOUNCE] mesa 20.0.5

[Mark Janes]

Danylo Piliaiev  writes:

> I want to sum up what happened from my perspective, I think it could be 
> useful to improve the process:
>
> 1) There was a regression in 20.* 
> (https://gitlab.freedesktop.org/mesa/mesa/-/issues/2758)
> 2) I "fixed" the regression and broke non-iris drivers 
> (https://gitlab.freedesktop.org/mesa/mesa/-/commit/d684fb37bfbc47d098158cb03c0672119a4469fe)
> 3) I "fixed" the new regression of fix 2) 
> (https://gitlab.freedesktop.org/mesa/mesa/-/commit/829013d0cad0fa2513b32ae07cf8d745f6e5c62d)
> 4) After that, it appeared that due to a bug in piglit, Intel CI didn't 
> run piglit tests which gave me a false sense of commit's correctness
>    (https://gitlab.freedesktop.org/mesa/mesa/-/issues/2815)
> 5) I aimed to have a fix before the next minor release on 2020-04-29 by 
> consulting the release calendar.
> 6) I accidentally saw 20.0.5 being released with one of the two of my 
> commits.
>
> I see multiple failure points:
> 1) Me not carefully examining all code paths, because at least one that 
> failed should have been obvious to test.

You missed one important failure point:

  1a) untested piglit commits pushed to master which disabled the test
  suite.

I was impressed by how quickly regressions were added to master in the
few days that piglit was disabled in CI.  At least 4 regressions in as
many days.

> 2) CI not communicating that piglit tests were not executed. Again, I 
> could have seen this, examined CI results, but did not.

This was my fault.  Over 5 years ago, I change the CI harness to ignore
errors thrown by piglit when it is run on an empty test set.  There are
valid CI use cases (when bisecting) where the CI will attempt to run a
small test set on older platforms that do not support any of the tests.

A more defensive implementation would check that an empty test set is
allowed only in the specific/expected use case.

> 3) After restoration of CI capabilities test what added to "expected 
> failure" and this may have contributed to regression
>     being missed when testing the release. I'm not sure about this part 
> so correct me if I'm wrong.

You are correct.  CI cannot discern the context of a text failure
automatically.  Typically, test failures on the stable branch are not
release blockers.  Test failures which represent regressions are written
up as gitlab issues and tracked there.

Which brings up another failure point:

 3a) Bisected regressions tagged with Fixes or mesa-stable are
 automatically applied to Mesa's release branch.

This failure point has burned us many times, most recently with the 20.0
regression fixed by 2120f106e0e.

Mesa currently has no mechanism for blocking a release with a gitlab
issue.  This current example is tagged with "bisected" and "regression",
but the important distinction is that the bisected commit ALSO has tags
which apply it to stable releases.

Mesa's release process does not include a check of bugs that have been
written up in gitlab (https://www.mesa3d.org/releasing.html).  My own
opinion is that gitlab's issues are unusable for this purpose, due to
its lack of search functionality.  I have found no way to audit gitlab
issues leading up to a release.

Gitlab's issues may work well for developing on master, but they are not
as good as Bugzilla for managing releases.

> 4) I didn't know about this release and that this release was help up 
> for the fix of 2758.
> 5) There were now window between announcing the scope of the release and 
> release itself. Since I knew about regression
>     I could have notified about it. Also there is no milestone for minor 
> releases so it's problematic to link issue and release.
>
> It's a second release in a row with clear regression crept in. I believe 
> that we can use this to improve the process and
> safeguard against such regressions in the future.

Does anyone have recommendations for how to use Gitlab to verify that
there are no identified-but-unfixed bugs in a pending release?

> P.S. I'm preparing and will test a final fix which will be sent soon.
>
> Danylo
>
> On 23.04.20 07:40, Dylan Baker wrote:
>> Quoting Ilia Mirkin (2020-04-22 15:47:59)
>>> On Wed, Apr 22, 2020 at 6:39 PM Danylo Piliaiev
>>>  wrote:
 I'm sorry for this trouble. However looking at it I think: maybe something 
 could be
 changed about applying patches to stable to safeguard against such issues.
>>> We used to get pre-announcements a few days prior to a release which
>>> would allow developers to look things over, which would allow us to
>>> notice things like that. Not sure when this got dropped.
>>>
>>> Cheers,
>>>
>>>-ilia
>> That was dropped in favor of a live staging branch that is updated daily (at
>> least on week days).
>>
>> Dylan
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list

Re: [Mesa-dev] Requiring a full author name when contributing to mesa?

[Mark Janes]

Ian Romanick  writes:

> On 12/11/19 2:27 PM, Timothy Arceri wrote:
>> Hi,
>> 
>> So it seems lately we have been increasingly merging patches with made
>> up names, or single names etc [1]. The latest submitted patch has the
>> name Icecream95. This seems wrong to me from a point of keeping up the
>> integrity of the project. I'm not a legal expert but it doesn't seem
>> ideal to be amassing commits with these type of author tags from that
>> point of view either.
>> 
>> Is it just me or do others agree we should at least require a proper
>> name on the commits (as fake as that may be also)? Seems like a low bar
>> to me.
>
> First we don't allow single Unicode characters or emojis, and now you
> want to require realistic looking names.  Where does it end, Tim?!?
> WHERE DOES IT END???
>
> 藍
>
> But seriously... I definitely agree with the sentiment.  It seems really
> lame to have a bunch of commits from clearly nonsense names.  Who are
> these randos?

Perhaps they are graphics developers working at corporations where
management is not enthusiastic about contributions to mesa?

Or, agents seeking to damage mesa by submitting vulnerable or
IP-entangled code?

The kernel does not allow anonymous contributions, to ensure all
contributions are compatible with the GPL.

> Where's the accountability?
>
> As far as any possible legal aspects go, since we don't require (as far
> as I'm aware) submitters to sign any sort of certificate of origin, I
> don't know that Icecream95 is any better or worse than Ralphio Grant (a
> realistic looking name that I just made up) or Ian Romanck.
>
> It seems to me that this is a social problem, so it likely has a social
> solution.  If we don't want people to be anonymous cowards with clearly
> phony names, we should try to make the alternatives of being involved in
> the community and using a real name more attractive.  We should do our
> best to encourage people to "do the right thing."
>
> I don't think it's very realistic for us to try to compel people to do
> so, and I don't think there's much value in it... especially if it
> drives away people making technically competent contributions.
>
>> [1]
>> https://gitlab.freedesktop.org/mesa/mesa/merge_requests/3050#note_361924
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] How to merge Mesa changes which require corresponding piglit changes

[Mark Janes]

Michel Dänzer  writes:

> On 2019-12-02 3:15 p.m., Tapani Pälli wrote:
>> On 11/15/19 8:41 PM, Mark Janes wrote:
>>> Michel Dänzer  writes:
>>>
>>>> On 2019-11-15 4:02 p.m., Mark Janes wrote:
>>>>> Michel Dänzer  writes:
>>>>>
>>>>>> Now that the GitLab CI pipeline tests a snapshot of piglit with
>>>>>> llvmpipe
>>>>>> (https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2468), the
>>>>>> question has come up how to deal with inter-dependent Mesa/piglit
>>>>>> changes (where merging only one or the other causes some piglit
>>>>>> regressions).
>>>>>>
>>>>>>
>>>>>> First of all, let it be clear that just merging the Mesa changes as-is
>>>>>> and breaking the GitLab CI pipeline is not acceptable.
>>>>>>
>>>>>>
>>>>>>  From the Mesa POV, the easiest solution is:
>>>>>>
>>>>>> 1. Merge the piglit changes
>>>>>> 2. In the Mesa MR (merge request), add a commit which updates
>>>>>> piglit[0]
>>>>>> 3. If the CI pipeline is green, the MR can be merged
>>>>>>
>>>>>>
>>>>>> In case one wants to avoid alarms from external CI systems, another
>>>>>> possibility is:
>>>>>
>>>>> For the Intel CI, no alarm is generated if the piglit test is pushed
>>>>> first.  Normal development process includes writing a piglit test to
>>>>> illustrate the bug that is being fixed.
>>>>
>>>> Cool, but what if the piglit changes affect the results of existing
>>>> tests? That was the situation yesterday which prompted this thread.
>>>
>>> We attribute the status change to piglit in the CI config, within a few
>>> hours.  The test shows up as a failure in CI until it is triaged.
>> 
>> I think we have a problem with current gitlab CI process.
>> 
>> Right now if someone needs to update piglit commit used by CI, he also
>> ends up fixing and editing the .gitlab-ci/piglit/quick_gl.txt (and
>> glslparser+quick_shader.txt) as CI reports numerous failures because of
>> completely unrelated stuff as meanwhile people added other tests,
>> removed tests and modified them.
>
> This is at least somewhat intentional, as the results of any newly added
> tests should be carefully checked for plausibility.

If a piglit (or any other suite) commit causes a test failure, the
failure is not a Mesa regression, by definition.  CI is for identifying
regressions.  The simple fact that a failure is due to a non-Mesa commit
means it can be immediately masked in CI.

>> I think we should turn such warnings on only when we have more
>> sophisticated algorithm to detect actual regression (not just 'state
>> change', like additional test or removed test).
>
> It's unclear what exactly you're proposing. In order to catch
> regressions (e.g. pass -> warn, pass -> fail, pass -> skip, pass ->
> crash), we need a list of all tests on at least one side of each
> transition. We're currently keeping the list of all
> warning/failing/skipped/crashing tests, but not passing tests (to keep
> the lists as small as possible).

CI must track the development of the test suites to capture the the
required transitions for tests.

If CI does not track each test suite commit, then some developer (eg
Tapani) has to go and triage test results from other piglit committers
in order to deploy tests in CI.  This is a barrier to test-first
development, and it is also unfair to the developers that are diligent
with testing.

Piglit and Crucible are maintained by the Mesa community and it makes
sense that Mesa CI should track their development.

Tracking other test suites (dEQP, CTS, etc) means that the Mesa
community may be distracted by test failures that are bugs in the suite
instead of bugs in Mesa.  Mesa developers are not enabled to fix bugs in
dEQP.  However, tracking external suites also identifies new conformance
requirements that Mesa will eventually be required to pass.

In practice, some test suites are easy to track and have developers that
are eager to resolve issues that are identified by the Mesa community
(eg dEQP, VulkanCTS).  Other suites are in a constant state of build
churn and are hard to track (Skia).

Tracking test suites can be done without too much effort, but it
requires a centralized role similar to a release manager.

> One possibility might be to remove the summary at the end of the lists.
> That would allow new passing tests to be silently added, but it would
> mean we could no longer catch pass -> notrun regressions.
>
>
> -- 
> Earthling Michel Dänzer   |   https://redhat.com
> Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] How to merge Mesa changes which require corresponding piglit changes

[Mark Janes]

Michel Dänzer  writes:

> On 2019-11-15 4:02 p.m., Mark Janes wrote:
>> Michel Dänzer  writes:
>> 
>>> Now that the GitLab CI pipeline tests a snapshot of piglit with llvmpipe
>>> (https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2468), the
>>> question has come up how to deal with inter-dependent Mesa/piglit
>>> changes (where merging only one or the other causes some piglit
>>> regressions).
>>>
>>>
>>> First of all, let it be clear that just merging the Mesa changes as-is
>>> and breaking the GitLab CI pipeline is not acceptable.
>>>
>>>
>>> From the Mesa POV, the easiest solution is:
>>>
>>> 1. Merge the piglit changes
>>> 2. In the Mesa MR (merge request), add a commit which updates piglit[0]
>>> 3. If the CI pipeline is green, the MR can be merged
>>>
>>>
>>> In case one wants to avoid alarms from external CI systems, another
>>> possibility is:
>> 
>> For the Intel CI, no alarm is generated if the piglit test is pushed
>> first.  Normal development process includes writing a piglit test to
>> illustrate the bug that is being fixed.
>
> Cool, but what if the piglit changes affect the results of existing
> tests? That was the situation yesterday which prompted this thread.

We attribute the status change to piglit in the CI config, within a few
hours.  The test shows up as a failure in CI until it is triaged.

>>> 1. In the Mesa MR, add a commit which disables the piglit tests broken
>>> by the Mesa changes.
>>> 2. If the CI pipeline is green, the MR can be merged
>>> 3. Merge the piglit changes
>>> 4. Create another Mesa MR which updates piglit[0] and re-enables the
>>> tests disabled in step 1
>>>
>>> I hope that covers it, don't hesitate to ask questions if something's
>>> still unclear.
>> 
>> It might help developers if CI generated the patch to make their pipeline
>> pass.
>
> It does for the test result list, if that's what you mean.
>
> However, that patch shouldn't be applied mechanically, but only after
> confirming that all changes in test results are expected. Ideally,
> whenever there are any new tests, the corresponding CI jobs should be
> run several times to make sure the new results are stable, otherwise any
> flaky tests should be excluded.
>
>
> -- 
> Earthling Michel Dänzer   |   https://redhat.com
> Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] How to merge Mesa changes which require corresponding piglit changes

[Mark Janes]

Michel Dänzer  writes:

> Now that the GitLab CI pipeline tests a snapshot of piglit with llvmpipe
> (https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2468), the
> question has come up how to deal with inter-dependent Mesa/piglit
> changes (where merging only one or the other causes some piglit
> regressions).
>
>
> First of all, let it be clear that just merging the Mesa changes as-is
> and breaking the GitLab CI pipeline is not acceptable.
>
>
> From the Mesa POV, the easiest solution is:
>
> 1. Merge the piglit changes
> 2. In the Mesa MR (merge request), add a commit which updates piglit[0]
> 3. If the CI pipeline is green, the MR can be merged
>
>
> In case one wants to avoid alarms from external CI systems, another
> possibility is:

For the Intel CI, no alarm is generated if the piglit test is pushed
first.  Normal development process includes writing a piglit test to
illustrate the bug that is being fixed.

> 1. In the Mesa MR, add a commit which disables the piglit tests broken
> by the Mesa changes.
> 2. If the CI pipeline is green, the MR can be merged
> 3. Merge the piglit changes
> 4. Create another Mesa MR which updates piglit[0] and re-enables the
> tests disabled in step 1
>
> I hope that covers it, don't hesitate to ask questions if something's
> still unclear.

It might help developers if CI generated the patch to make their pipeline
pass.

> [0] How to update piglit in the CI pipeline:
>
> * Change the commit hash on the "git checkout" line in
> .gitlab-ci/debian-test-install.sh[1] to a later commit from the piglit
> master branch
> * Bump DEBIAN_TEST_TAG[1] in .gitlab-ci.yml to the current date
> * May also need to update .gitlab-ci/piglit/*.txt to match any expected
> changes in test results
>
> See https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2748 for an
> example.
>
>
> [1] Might soon be .gitlab-ci/container/x86_test.sh and DEBIAN_TAG in the
> x86_test job definition, respectively, once
> https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2722 is merged.
>
>
> -- 
> Earthling Michel Dänzer   |   https://redhat.com
> Libre software enthusiast | Mesa and X developer
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Release workflow with gitlab issues

[Mark Janes]

Adam Jackson  writes:

> On Wed, 2019-09-18 at 12:08 -0700, Mark Janes wrote:
>> Adding the release managers to the CC to make sure they see the thread...
>> 
>> Dylan Baker  writes:
>> 
>> > Hi everyone,
>> > 
>> > Since we're migrating to gitlab issues, it seems like a good time to 
>> > review how
>> > we track stable releases, particularly release blockers.
>
> For the avoidance of doubt and confusion, I don't plan to migrate any
> of the components with bugs on the 19.2 release tracker until 19.2 is
> actually released.
>
>> > I'd like to use a gitlab milestone as a replacement for the tracker issues 
>> > from
>> > gitlab. The process would work much the same way as it does now, but with a
>> > milestone.
>> 
>> Right now, anyone can create milestones.  Is there a way to limit the
>> capability to release managers?  Would that be desirable?
>> 
>> I see the same issue with labels...  Anyone could delete a label, and
>> I'm not sure how we would recover that information.
>
> Strictly, the "Reporter" access level and above can manage labels,
> milestones require "Developer" or above. Not super meaningful since the
> mesa group really only has Developer or above.
>
> I'm not super worried about it to be honest, people tend not to be
> malicious.

I agree that malicious users will be less of a problem with gitlab
issues.  I am somewhat worried about inadvertent mistakes.  "I thought I
was in the browser tab with my forked repo..."

That particular mistake would be hard to make, but it's hard to
anticipate accidents.

When you delete a label, gitlab warns that you can't get it back.
Labels will hold a lot of information for us.

>> > We'll also need to replace the Bugzilla: tag with a tag that gitlab 
>> > recognizes
>> > for closing issues. Since we already use "Fixes:" for something else, I'd 
>> > like
>> > to propose "Closes:"
>> > 
>> > so we'd replace:
>> > Buzilla: https://...
>> > with:
>> > Closes: !0
>> 
>> A more exact replacement would be:
>>   Gitlab: https://gitlab.freedesktop.org/mesa/mesa/issues/{issue}
>> 
>> Is there an advantage to the !{issue} format?  Perhaps gitlab parses it
>> and closes the issue?
>
> gitlab can parse both the !nnn format and full URLs, and yes, it does
> close issues when a commit with such a Closes: line is merged.
>
> - ajax
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Release workflow with gitlab issues

[Mark Janes]

Adding the release managers to the CC to make sure they see the thread...

Dylan Baker  writes:

> Hi everyone,
>
> Since we're migrating to gitlab issues, it seems like a good time to review 
> how
> we track stable releases, particularly release blockers.
>
> I'd like to use a gitlab milestone as a replacement for the tracker issues 
> from
> gitlab. The process would work much the same way as it does now, but with a
> milestone.

Right now, anyone can create milestones.  Is there a way to limit the
capability to release managers?  Would that be desirable?

I see the same issue with labels...  Anyone could delete a label, and
I'm not sure how we would recover that information.

> We'll also need to replace the Bugzilla: tag with a tag that gitlab recognizes
> for closing issues. Since we already use "Fixes:" for something else, I'd like
> to propose "Closes:"
>
> so we'd replace:
> Buzilla: https://...
> with:
> Closes: !0

A more exact replacement would be:
  Gitlab: https://gitlab.freedesktop.org/mesa/mesa/issues/{issue}

Is there an advantage to the !{issue} format?  Perhaps gitlab parses it
and closes the issue?

> If we like this, I'll follow up with a script that can fetch issues 
> information
> from gitlab and produce release information.
>
> Does this sound reasonable to everyone?
>
> Dylan
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Mesa 19.2.0 release plan

[Mark Janes]

Eric Engestrom  writes:

> On 2019-07-31 at 09:38, Emil Velikov  wrote:
>> Hi all,
>> 
>> Here is the tentative release plan for 19.2.0.
>> 
>> As many of you are well aware, it's time to the next branch point.
>> The calendar is already updated, so these are the tentative dates:
>> 
>>  Aug 06 2019 - Feature freeze/Release candidate 1
>>  Aug 13 2019 - Release candidate 2
>>  Aug 20 2019 - Release candidate 3
>>  Aug 27 2019 - Release candidate 4/final release
>> 
>> This gives us around 1 week until the branch point.
>> 
>> Note: In the spirit of keeping things clearer and more transparent, we
>> will be keeping track of any features planned for the release in
>> Bugzilla [1].
>> 
>> Do add a separate "Depends on" for each work you have planned.
>> Alternatively you can reply to this email and I'll add them for you.
>> 
>> [1] https://bugs.freedesktop.org/show_bug.cgi?id=111265
>
> Thanks!
>
> As per previous discussions (I don't remember where, sorry) as well as
> internal discussions, I think we should add all currently open
> regressions since 19.1 as blockers for this release.

My understanding is that the "feature tracker" blocks the creation of
the release branchpoint.  A separate "release tracker" blocks the
release of 19.2.0.  Unfixed regressions go on the "release tracker", not
the "feature tracker".  We backport bug fixes to release branches, but
we don't backport features.

> We should also add that to the procedure in our docs; I'll do that
> later today if nobody beats me to it.

> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/3] android: amd/addrlib: add gfx10 support

[Mark Janes]

Do you have some documentation on how you are building Android with
Mesa?  We set up a buildtest using an Android (Celadon) tree from Intel,
and it seems to be less up-to-date than your build.

Last week, you found a build error that was missed by our CI.  When you
pushed this series, we got:

 src/egl/main/egldevice.c:126:11: error: implicit declaration of function 
'drmDevicesEqual'

I'd like to leverage your work to improve our automation, so we don't
break your tree again.

-Mark

Mauro Rossi  writes:

> Fix the following building error:
>
> external/mesa/src/amd/addrlib/src/gfx10/gfx10addrlib.cpp:35:10:
> fatal error: 'gfx10_gb_reg.h' file not found
>  ^~~~
> 1 error generated.
>
> Fixes: 78cdf9a ("amd/addrlib: add gfx10 support")
> Signed-off-by: Mauro Rossi 
> ---
>  src/amd/Android.addrlib.mk | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/amd/Android.addrlib.mk b/src/amd/Android.addrlib.mk
> index 428fe19b20..eec78fc8bf 100644
> --- a/src/amd/Android.addrlib.mk
> +++ b/src/amd/Android.addrlib.mk
> @@ -37,6 +37,7 @@ LOCAL_C_INCLUDES := \
>   $(MESA_TOP)/src/amd/addrlib/src \
>   $(MESA_TOP)/src/amd/addrlib/src/core \
>   $(MESA_TOP)/src/amd/addrlib/src/chip/gfx9 \
> + $(MESA_TOP)/src/amd/addrlib/src/chip/gfx10 \
>   $(MESA_TOP)/src/amd/addrlib/src/chip/r800
>  
>  LOCAL_EXPORT_C_INCLUDE_DIRS := \
> -- 
> 2.20.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 00/11] i965 shader debug through KHR_debug

[Mark Janes]

I pushed the patches that got review (1-3).  I haven't prompted anyone
for more review, because FrameRetrace is the only app that will
immediately benefit from the patches.  Eventually, the existing
mechanism will break, and someone that needs to debug their shader
assemblies will have a reason to review.

I can make a MR in gitlab if you think it would help.

Jason Ekstrand  writes:

> Did this get dropped on the ground?
>
> On Fri, Dec 7, 2018 at 12:07 PM Mark Janes  wrote:
>
>> Ilia Mirkin  writes:
>>
>> > On Thu, Dec 6, 2018 at 7:36 PM Mark Janes 
>> wrote:
>> >>
>> >> This series provides Intel shader compilation debug information via
>> >> KHR_debug.  Previously, shader assembly and related compilation
>> >> artifacts were dumped to stderr.  Tools associating compilation
>> >> artifacts with programs (e.g. FrameRetrace*) parsed stderr, which was
>> >> error prone.  Changes to the shader debug formats and the addition of
>> >> shader cache assembly dumps further complicate the task of parsing
>> >> stderr.
>> >>
>> >> KHR_debug provides synchronous callbacks with stable identifiers,
>> >> simplifying the task of matching debug artifacts with the originating
>> >> GLSL.
>> >
>> > One observation is that while the identifiers may be stable within a
>> > single execution, they will not be stable across different
>> > applications / traces. id's are set on a first-come first-serve basis,
>> > so depending on the exact order, the id's will end up different.
>> >
>> > Is that OK for frameretrace? Another approach would be to create
>> > globally-hardcoded id's for these, and start the auto-allocation in a
>> > higher range.
>>
>> I did take a few steps down the path of globally hard-coded id's.  I
>> agree with Eric's assessment in debug_output.c that a giant enum list of
>> all debug message id's is unworkable.
>>
>> We could divide id's into regions based on the high bits, and allocate
>> the lower bits to components to declare and manage.  This would break up
>> the monolithic declaration of id's, but you still have the issue of id
>> stability as the driver changes over time.  Refactoring functionality to
>> a location where it can be shared would involve changing the id's of
>> emitted debug messages.  Also, any taxonomy used to split up the id's
>> will probably look shortsighted in 5 years time.
>>
>> I personally haven't seen a great solution for sharing a 32 debug id
>> within a large project.  The idea of exporting the id's to external
>> tools is even more problematic.
>>
>> Rather than try to solve that problem, I decided to associate unknown
>> id's with the semantic meaning by parsing the first few words in
>> frameretrace.  A stable id means subsequent messages can be handled
>> without parsing.  Unknown messages are turned of via KHR_debug.  This is
>> a much better parsing stderr, where it is difficult to determine message
>> boundaries.
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [ANNOUNCE] mesa 19.0.0-rc1

[Mark Janes]

Eero Tamminen  writes:

> Hi,
>
> On 31.1.2019 1.37, Dylan Baker wrote:
>> This email announces the mesa 19.0 release candidate 1. I'll keep this email
>> fairly brief since I'm already running a little late on getting this done :)
>> I've just had to resolve quite a few autotools issues to get the dist built.
>> 
>> Notable in the 19.0-rc1 branch is SWR is set to require LLVM 7 instead of 
>> LLVM
>> 6. It is impossible to bootstrap SWR with LLVM 6 and compile with  LLVM 7 
>> due to
>> LLVM API changes. Since RadeonSI and Radv both require LLVM 7 I've taken the
>> liberty of bumping SWR so that we could get a tarball built.
>> 
>> We've had an exciting release cycle, plenty of GL and Vulkan extensions, 
>> ~1600
>> commits since the 18.3 branchpoint with substantial work across all areas of
>> mesa.
>
> Are all the recent (i965) perf regressions included to it:
> * https://bugs.freedesktop.org/show_bug.cgi?id=109517 (spilling)
> * https://bugs.freedesktop.org/show_bug.cgi?id=109505 (Unigine)
> * https://bugs.freedesktop.org/show_bug.cgi?id=109216 (Vulkan)

These regressions all need to be added to the release tracker.  Thank
you for reporting them.

>   - Eero
>
> PS. There's also much older:
> * https://bugs.freedesktop.org/show_bug.cgi?id=107510
>
> Which was already fixed, but then regressed again, and regressing commit 
> wasn't anymore reverted.  I'm mentioning it because Timothy had a patch 
> series in October that fixed the tess/geom shader regressions (which 
> were largest), but for some reason it's not yet in upstream.
>
>> Expect rc2 about this time next week, see you then.
>> 
>> Dylan
>> 
>> git tag: mesa-19.0.0-rc1
>> 
>> https://mesa.freedesktop.org/archive/mesa-19.0.0-rc1.tar.gz
>> MD5:  b3a610b204d0cb3431823353a8cbe8e6  mesa-19.0.0-rc1.tar.gz
>> SHA1: d1f0d0bc49ec7e02d0cd7d141127fd2fefc72e35  mesa-19.0.0-rc1.tar.gz
>> SHA256: 0a14bb059f6cead4e50923df9c24d3c5025d9310803ca5189e019f07e539639e  
>> mesa-19.0.0-rc1.tar.gz
>> SHA512: 
>> 5bedc917afecef6a0dd11c56688a3e3fdbbaeaceca33062d6825b5525c6e78663e873bdecc96b98b0448d988ad81a7a8617c523e2d312384369c6a333b790b86
>>   mesa-19.0.0-rc1.tar.gz
>> PGP:  https://mesa.freedesktop.org/archive/mesa-19.0.0-rc1.tar.gz.sig
>> 
>> https://mesa.freedesktop.org/archive/mesa-19.0.0-rc1.tar.xz
>> MD5:  727abb6469e518ff1a2e1bde33543503  mesa-19.0.0-rc1.tar.xz
>> SHA1: 577642259cd269c883007df7c2772c8c636fabfb  mesa-19.0.0-rc1.tar.xz
>> SHA256: 8efb32956c428d23f78364f9eace5491bda9feaafd767128133672a5f79659e8  
>> mesa-19.0.0-rc1.tar.xz
>> SHA512: 
>> 23d21d6c4f03a1d9073ecb1f43dc251d581cdeb6b7cc24a19c299571070b4184ad4f22b0ca170ca42e58c62bb46eca0dadc334a952bbb7e0379961a30a6ca856
>>   mesa-19.0.0-rc1.tar.xz
>> PGP:  https://mesa.freedesktop.org/archive/mesa-19.0.0-rc1.tar.xz.sig
>> 
>> 
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> 
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v4] anv/device: fix maximum number of images supported

[Mark Janes]

Hi Iago,

It looks like you tested this patch in CI and got the same failures that
we are seeing on master:

http://mesa-ci-results.jf.intel.com/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845

Why was this patch pushed?

-Mark

Mark Janes  writes:

> This patch regresses thousands of tests on BDW and HSW:
>
> http://mesa-ci-results.jf.intel.com/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails
>
> I'll revert it as soon as my testing completes.
>
> Iago Toral Quiroga  writes:
>
>> We had defined MAX_IMAGES as 8, which we used to size the array for
>> image push constant data. The comment there stated that this was for
>> gen8, but anv_nir_apply_pipeline_layout runs for all gens and writes
>> that array, asserting that we don't exceed that number of images,
>> which imposes a limit of MAX_IMAGES on all gens.
>>
>> Furthermore, despite this, we are exposing up to 64 images per shader
>> stage on all gens, gen8 included.
>>
>> This patch lowers the number of images we expose in gen8 to 8 and
>> keeps 64 images for gen9+ while making sure that only pre-SKL gens
>> use push constant space to handle images.
>>
>> v2:
>>  - <= instead of < in the assert (Eric, Lionel)
>>  - Change the way the assertion is written (Eric)
>>
>> v3:
>>  - Revert the way the assertion is written to the form it had in v1,
>>the version in v2 was not equivalent and was incorrect. (Lionel)
>>
>> v4:
>>  - gen9+ doesn't need push constants for images at all (Jason)
>> ---
>>  src/intel/vulkan/anv_device.c |  7 --
>>  .../vulkan/anv_nir_apply_pipeline_layout.c|  4 +--
>>  src/intel/vulkan/anv_private.h|  5 ++--
>>  src/intel/vulkan/genX_cmd_buffer.c| 25 +--
>>  4 files changed, 28 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
>> index 523f1483e29..f85458b672e 100644
>> --- a/src/intel/vulkan/anv_device.c
>> +++ b/src/intel/vulkan/anv_device.c
>> @@ -987,9 +987,12 @@ void anv_GetPhysicalDeviceProperties(
>> const uint32_t max_samplers = (devinfo->gen >= 8 || devinfo->is_haswell) 
>> ?
>>   128 : 16;
>>  
>> +   const uint32_t max_images = devinfo->gen < 9 ? MAX_GEN8_IMAGES : 
>> MAX_IMAGES;
>> +
>> VkSampleCountFlags sample_counts =
>>isl_device_get_sample_counts(>isl_dev);
>>  
>> +
>> VkPhysicalDeviceLimits limits = {
>>.maxImageDimension1D  = (1 << 14),
>>.maxImageDimension2D  = (1 << 14),
>> @@ -1009,7 +1012,7 @@ void anv_GetPhysicalDeviceProperties(
>>.maxPerStageDescriptorUniformBuffers  = 64,
>>.maxPerStageDescriptorStorageBuffers  = 64,
>>.maxPerStageDescriptorSampledImages   = max_samplers,
>> -  .maxPerStageDescriptorStorageImages   = 64,
>> +  .maxPerStageDescriptorStorageImages   = max_images,
>>.maxPerStageDescriptorInputAttachments= 64,
>>.maxPerStageResources = 250,
>>.maxDescriptorSetSamplers = 6 * max_samplers, /* 
>> number of stages * maxPerStageDescriptorSamplers */
>> @@ -1018,7 +1021,7 @@ void anv_GetPhysicalDeviceProperties(
>>.maxDescriptorSetStorageBuffers   = 6 * 64,   /* 
>> number of stages * maxPerStageDescriptorStorageBuffers */
>>.maxDescriptorSetStorageBuffersDynamic= MAX_DYNAMIC_BUFFERS / 2,
>>.maxDescriptorSetSampledImages= 6 * max_samplers, /* 
>> number of stages * maxPerStageDescriptorSampledImages */
>> -  .maxDescriptorSetStorageImages= 6 * 64,   /* 
>> number of stages * maxPerStageDescriptorStorageImages */
>> +  .maxDescriptorSetStorageImages= 6 * max_images,   /* 
>> number of stages * maxPerStageDescriptorStorageImages */
>>.maxDescriptorSetInputAttachments = 256,
>>.maxVertexInputAttributes = MAX_VBS,
>>.maxVertexInputBindings   = MAX_VBS,
>> diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c 
>> b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
>> index b3daf702bc0..623984b0f8c 100644
>> --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
>> +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
>> @@ -528,8 +528,8 @@ anv_nir_apply_pipeline_layout(const struct 
>> anv_physical_device *pdevice,
>>}

Re: [Mesa-dev] [PATCH v4] anv/device: fix maximum number of images supported

[Mark Janes]

This patch regresses thousands of tests on BDW and HSW:

http://mesa-ci-results.jf.intel.com/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails

I'll revert it as soon as my testing completes.

Iago Toral Quiroga  writes:

> We had defined MAX_IMAGES as 8, which we used to size the array for
> image push constant data. The comment there stated that this was for
> gen8, but anv_nir_apply_pipeline_layout runs for all gens and writes
> that array, asserting that we don't exceed that number of images,
> which imposes a limit of MAX_IMAGES on all gens.
>
> Furthermore, despite this, we are exposing up to 64 images per shader
> stage on all gens, gen8 included.
>
> This patch lowers the number of images we expose in gen8 to 8 and
> keeps 64 images for gen9+ while making sure that only pre-SKL gens
> use push constant space to handle images.
>
> v2:
>  - <= instead of < in the assert (Eric, Lionel)
>  - Change the way the assertion is written (Eric)
>
> v3:
>  - Revert the way the assertion is written to the form it had in v1,
>the version in v2 was not equivalent and was incorrect. (Lionel)
>
> v4:
>  - gen9+ doesn't need push constants for images at all (Jason)
> ---
>  src/intel/vulkan/anv_device.c |  7 --
>  .../vulkan/anv_nir_apply_pipeline_layout.c|  4 +--
>  src/intel/vulkan/anv_private.h|  5 ++--
>  src/intel/vulkan/genX_cmd_buffer.c| 25 +--
>  4 files changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index 523f1483e29..f85458b672e 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -987,9 +987,12 @@ void anv_GetPhysicalDeviceProperties(
> const uint32_t max_samplers = (devinfo->gen >= 8 || devinfo->is_haswell) ?
>   128 : 16;
>  
> +   const uint32_t max_images = devinfo->gen < 9 ? MAX_GEN8_IMAGES : 
> MAX_IMAGES;
> +
> VkSampleCountFlags sample_counts =
>isl_device_get_sample_counts(>isl_dev);
>  
> +
> VkPhysicalDeviceLimits limits = {
>.maxImageDimension1D  = (1 << 14),
>.maxImageDimension2D  = (1 << 14),
> @@ -1009,7 +1012,7 @@ void anv_GetPhysicalDeviceProperties(
>.maxPerStageDescriptorUniformBuffers  = 64,
>.maxPerStageDescriptorStorageBuffers  = 64,
>.maxPerStageDescriptorSampledImages   = max_samplers,
> -  .maxPerStageDescriptorStorageImages   = 64,
> +  .maxPerStageDescriptorStorageImages   = max_images,
>.maxPerStageDescriptorInputAttachments= 64,
>.maxPerStageResources = 250,
>.maxDescriptorSetSamplers = 6 * max_samplers, /* 
> number of stages * maxPerStageDescriptorSamplers */
> @@ -1018,7 +1021,7 @@ void anv_GetPhysicalDeviceProperties(
>.maxDescriptorSetStorageBuffers   = 6 * 64,   /* 
> number of stages * maxPerStageDescriptorStorageBuffers */
>.maxDescriptorSetStorageBuffersDynamic= MAX_DYNAMIC_BUFFERS / 2,
>.maxDescriptorSetSampledImages= 6 * max_samplers, /* 
> number of stages * maxPerStageDescriptorSampledImages */
> -  .maxDescriptorSetStorageImages= 6 * 64,   /* 
> number of stages * maxPerStageDescriptorStorageImages */
> +  .maxDescriptorSetStorageImages= 6 * max_images,   /* 
> number of stages * maxPerStageDescriptorStorageImages */
>.maxDescriptorSetInputAttachments = 256,
>.maxVertexInputAttributes = MAX_VBS,
>.maxVertexInputBindings   = MAX_VBS,
> diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c 
> b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> index b3daf702bc0..623984b0f8c 100644
> --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> @@ -528,8 +528,8 @@ anv_nir_apply_pipeline_layout(const struct 
> anv_physical_device *pdevice,
>}
> }
>  
> -   if (map->image_count > 0) {
> -  assert(map->image_count <= MAX_IMAGES);
> +   if (map->image_count > 0 && pdevice->compiler->devinfo->gen < 9) {
> +  assert(map->image_count <= MAX_GEN8_IMAGES);
>assert(shader->num_uniforms == prog_data->nr_params * 4);
>state.first_image_uniform = shader->num_uniforms;
>uint32_t *param = brw_stage_prog_data_add_params(prog_data,
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
> index 770254e93ea..47878adb066 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -157,7 +157,8 @@ struct gen_l3_config;
>  #define MAX_SCISSORS16
>  #define MAX_PUSH_CONSTANTS_SIZE 128
>  #define MAX_DYNAMIC_BUFFERS 16
> -#define MAX_IMAGES 8
> +#define MAX_IMAGES 64
> +#define MAX_GEN8_IMAGES 8
>  #define MAX_PUSH_DESCRIPTORS 32 /* Minimum requirement 

Re: [Mesa-dev] [PATCH 00/11] i965 shader debug through KHR_debug

[Mark Janes]

Ilia Mirkin  writes:

> On Thu, Dec 6, 2018 at 7:36 PM Mark Janes  wrote:
>>
>> This series provides Intel shader compilation debug information via
>> KHR_debug.  Previously, shader assembly and related compilation
>> artifacts were dumped to stderr.  Tools associating compilation
>> artifacts with programs (e.g. FrameRetrace*) parsed stderr, which was
>> error prone.  Changes to the shader debug formats and the addition of
>> shader cache assembly dumps further complicate the task of parsing
>> stderr.
>>
>> KHR_debug provides synchronous callbacks with stable identifiers,
>> simplifying the task of matching debug artifacts with the originating
>> GLSL.
>
> One observation is that while the identifiers may be stable within a
> single execution, they will not be stable across different
> applications / traces. id's are set on a first-come first-serve basis,
> so depending on the exact order, the id's will end up different.
>
> Is that OK for frameretrace? Another approach would be to create
> globally-hardcoded id's for these, and start the auto-allocation in a
> higher range.

I did take a few steps down the path of globally hard-coded id's.  I
agree with Eric's assessment in debug_output.c that a giant enum list of
all debug message id's is unworkable.

We could divide id's into regions based on the high bits, and allocate
the lower bits to components to declare and manage.  This would break up
the monolithic declaration of id's, but you still have the issue of id
stability as the driver changes over time.  Refactoring functionality to
a location where it can be shared would involve changing the id's of
emitted debug messages.  Also, any taxonomy used to split up the id's
will probably look shortsighted in 5 years time.

I personally haven't seen a great solution for sharing a 32 debug id
within a large project.  The idea of exporting the id's to external
tools is even more problematic.

Rather than try to solve that problem, I decided to associate unknown
id's with the semantic meaning by parsing the first few words in
frameretrace.  A stable id means subsequent messages can be handled
without parsing.  Unknown messages are turned of via KHR_debug.  This is
a much better parsing stderr, where it is difficult to determine message
boundaries.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 04/11] i965/disasm: allow caller to specify output filehandle.

[Mark Janes]

---
 src/intel/compiler/brw_disasm_info.c  | 28 +++
 src/intel/compiler/brw_disasm_info.h  |  2 +-
 src/intel/compiler/brw_fs_generator.cpp   |  2 +-
 src/intel/compiler/brw_vec4_generator.cpp |  2 +-
 src/intel/compiler/test_eu_validate.cpp   |  2 +-
 5 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/src/intel/compiler/brw_disasm_info.c 
b/src/intel/compiler/brw_disasm_info.c
index 7a9a55d83ef..21a1170b0a2 100644
--- a/src/intel/compiler/brw_disasm_info.c
+++ b/src/intel/compiler/brw_disasm_info.c
@@ -31,7 +31,7 @@ __attribute__((weak)) void nir_print_instr(UNUSED const 
nir_instr *instr,
UNUSED FILE *fp) {}
 
 void
-dump_assembly(void *assembly, struct disasm_info *disasm)
+dump_assembly(FILE *fp, void *assembly, struct disasm_info *disasm)
 {
const struct gen_device_info *devinfo = disasm->devinfo;
const char *last_annotation_string = NULL;
@@ -49,47 +49,47 @@ dump_assembly(void *assembly, struct disasm_info *disasm)
   int end_offset = next->offset;
 
   if (group->block_start) {
- fprintf(stderr, "   START B%d", group->block_start->num);
+ fprintf(fp, "   START B%d", group->block_start->num);
  foreach_list_typed(struct bblock_link, predecessor_link, link,
 >block_start->parents) {
 struct bblock_t *predecessor_block = predecessor_link->block;
-fprintf(stderr, " <-B%d", predecessor_block->num);
+fprintf(fp, " <-B%d", predecessor_block->num);
  }
- fprintf(stderr, " (%u cycles)\n", group->block_start->cycle_count);
+ fprintf(fp, " (%u cycles)\n", group->block_start->cycle_count);
   }
 
   if (last_annotation_ir != group->ir) {
  last_annotation_ir = group->ir;
  if (last_annotation_ir) {
-fprintf(stderr, "   ");
-nir_print_instr(group->ir, stderr);
-fprintf(stderr, "\n");
+fprintf(fp, "   ");
+nir_print_instr(group->ir, fp);
+fprintf(fp, "\n");
  }
   }
 
   if (last_annotation_string != group->annotation) {
  last_annotation_string = group->annotation;
  if (last_annotation_string)
-fprintf(stderr, "   %s\n", last_annotation_string);
+fprintf(fp, "   %s\n", last_annotation_string);
   }
 
-  brw_disassemble(devinfo, assembly, start_offset, end_offset, stderr);
+  brw_disassemble(devinfo, assembly, start_offset, end_offset, fp);
 
   if (group->error) {
- fputs(group->error, stderr);
+ fputs(group->error, fp);
   }
 
   if (group->block_end) {
- fprintf(stderr, "   END B%d", group->block_end->num);
+ fprintf(fp, "   END B%d", group->block_end->num);
  foreach_list_typed(struct bblock_link, successor_link, link,
 >block_end->children) {
 struct bblock_t *successor_block = successor_link->block;
-fprintf(stderr, " ->B%d", successor_block->num);
+fprintf(fp, " ->B%d", successor_block->num);
  }
- fprintf(stderr, "\n");
+ fprintf(fp, "\n");
   }
}
-   fprintf(stderr, "\n");
+   fprintf(fp, "\n");
 }
 
 struct disasm_info *
diff --git a/src/intel/compiler/brw_disasm_info.h 
b/src/intel/compiler/brw_disasm_info.h
index b8826e68175..afdc6bc24f7 100644
--- a/src/intel/compiler/brw_disasm_info.h
+++ b/src/intel/compiler/brw_disasm_info.h
@@ -65,7 +65,7 @@ struct disasm_info {
 };
 
 void
-dump_assembly(void *assembly, struct disasm_info *disasm);
+dump_assembly(FILE *fp, void *assembly, struct disasm_info *disasm);
 
 struct disasm_info *
 disasm_initialize(const struct gen_device_info *devinfo,
diff --git a/src/intel/compiler/brw_fs_generator.cpp 
b/src/intel/compiler/brw_fs_generator.cpp
index 08dd83dded7..ebf63b07837 100644
--- a/src/intel/compiler/brw_fs_generator.cpp
+++ b/src/intel/compiler/brw_fs_generator.cpp
@@ -2471,7 +2471,7 @@ fs_generator::generate_code(const cfg_t *cfg, int 
dispatch_width)
   spill_count, fill_count, promoted_constants, before_size, 
after_size,
   100.0f * (before_size - after_size) / before_size);
 
-  dump_assembly(p->store, disasm_info);
+  dump_assembly(stderr, p->store, disasm_info);
}
ralloc_free(disasm_info);
assert(validated);
diff --git a/src/intel/compiler/brw_vec4_generator.cpp 
b/src/intel/compiler/brw_vec4_generator.cpp
index 888cb358fd1..ee232cbf96e 100644
--- a/src/intel/compiler/brw_vec4_generator.cpp
+++ b/src/intel/compiler/brw_vec4_generator.cpp
@@ -2206,7 +2206,7 @@ generate_code(struct brw_codegen *p,
   spill_count, fill_count, before_size, after_size,
   100.0f * (before_size - after_size) / before_size);
 
-  dump_assembly(p->store, disasm_info);
+  dump_assembly(stderr, p->store, disasm_info);
}
ralloc_free(disasm_info);
assert(validated);
diff 

[Mesa-dev] [PATCH 11/11] i965: provide shader cache assemblies to KHR_debug

[Mark Janes]

---
 src/mesa/drivers/dri/i965/brw_disk_cache.c | 23 ++
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c 
b/src/mesa/drivers/dri/i965/brw_disk_cache.c
index 65fcab24b7f..0acbef4f20e 100644
--- a/src/mesa/drivers/dri/i965/brw_disk_cache.c
+++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
@@ -199,16 +199,31 @@ read_and_upload(struct brw_context *brw, struct 
disk_cache *cache,
brw_alloc_stage_scratch(brw, stage_state, prog_data->total_scratch);
 
if (unlikely(debug_enabled_for_stage(stage))) {
-  fprintf(stderr, "NIR for %s program %d loaded from disk shader cache:\n",
+  char *buf;
+  size_t buf_size;
+  FILE * log_fp = open_memstream(, _size);
+  fprintf(log_fp, "NIR for %s program %d loaded from disk shader cache:\n",
   _mesa_shader_stage_to_abbrev(stage), brw_program(prog)->id);
   brw_program_deserialize_driver_blob(>ctx, prog, stage);
   nir_shader *nir = prog->nir;
-  nir_print_shader(nir, stderr);
-  fprintf(stderr, "Native code for %s %s shader %s from disk cache:\n",
+  nir_print_shader(nir, log_fp);
+  fclose(log_fp);
+  static GLuint nir_msg_id = 0;
+  shader_debug_log_mesa(brw, _msg_id, buf);
+  fputs(buf, stderr);
+  free(buf);
+
+  log_fp = open_memstream(, _size);
+  fprintf(log_fp, "Native code for %s %s shader %s from disk cache:\n",
   nir->info.label ? nir->info.label : "unnamed",
   _mesa_shader_stage_to_string(nir->info.stage), nir->info.name);
   brw_disassemble(>screen->devinfo, program, 0,
-  prog_data->program_size, stderr);
+  prog_data->program_size, log_fp);
+  fclose(log_fp);
+  static GLuint native_msg_id = 0;
+  shader_debug_log_mesa(brw, _msg_id, buf);
+  fputs(buf, stderr);
+  free(buf);
}
 
brw_upload_cache(>cache, cache_id, _key, brw_prog_key_size(stage),
-- 
2.19.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 00/11] i965 shader debug through KHR_debug

[Mark Janes]

This series provides Intel shader compilation debug information via
KHR_debug.  Previously, shader assembly and related compilation
artifacts were dumped to stderr.  Tools associating compilation
artifacts with programs (e.g. FrameRetrace*) parsed stderr, which was
error prone.  Changes to the shader debug formats and the addition of
shader cache assembly dumps further complicate the task of parsing
stderr.

KHR_debug provides synchronous callbacks with stable identifiers,
simplifying the task of matching debug artifacts with the originating
GLSL.

For backward compatibility, artifacts continue to be provided on
stderr.  Pre-existing KHR_debug messages used by shader-db are
unchanged.

 * wip branch to handle KHR_debug:
   https://github.com/janesma/apitrace/tree/khr_debug

Mark Janes (11):
  mesa: properly report the length of truncated log messages
  mesa: rename logging functions to reflect that they format strings
  mesa: add logging function for formatted string
  i965/disasm: allow caller to specify output filehandle.
  i965: provide stable message id's for shader_debug_log
  i965/disasm: provide shader assembly to KHR_debug.
  i965/compiler: provide formatted strings to the shader debug log
  intel/nir: provide SSA form to KHR_debug
  i965: make shader_debug_log_mesa callable
  i965: provide GLSL IR to KHR_debug
  i965: provide shader cache assemblies to KHR_debug

 src/intel/compiler/brw_compiler.h |  2 +-
 src/intel/compiler/brw_disasm_info.c  | 28 +-
 src/intel/compiler/brw_disasm_info.h  |  2 +-
 src/intel/compiler/brw_fs.cpp | 11 ++--
 src/intel/compiler/brw_fs_generator.cpp   | 39 +++---
 src/intel/compiler/brw_nir.c  | 26 +++--
 src/intel/compiler/brw_nir.h  |  1 +
 src/intel/compiler/brw_shader.cpp |  2 +-
 src/intel/compiler/brw_vec4.cpp   |  2 +-
 src/intel/compiler/brw_vec4_generator.cpp | 24 +
 src/intel/compiler/brw_vec4_gs_visitor.cpp|  2 +-
 src/intel/compiler/brw_vec4_tcs.cpp   |  2 +-
 src/intel/compiler/test_eu_validate.cpp   |  2 +-
 src/intel/vulkan/anv_device.c |  2 +-
 src/mesa/drivers/dri/i915/intel_context.h | 18 +++
 src/mesa/drivers/dri/i915/intel_fbo.c | 10 ++--
 src/mesa/drivers/dri/i965/brw_context.h   | 18 +++
 src/mesa/drivers/dri/i965/brw_disk_cache.c| 23 ++--
 src/mesa/drivers/dri/i965/brw_link.cpp| 14 +++--
 src/mesa/drivers/dri/i965/genX_state_upload.c | 14 ++---
 src/mesa/drivers/dri/i965/intel_fbo.c | 10 ++--
 src/mesa/drivers/dri/i965/intel_screen.c  | 30 +--
 src/mesa/drivers/dri/i965/intel_screen.h  |  3 ++
 src/mesa/main/bufferobj.c | 10 ++--
 src/mesa/main/errors.c| 54 ++-
 src/mesa/main/errors.h| 28 ++
 src/mesa/main/fbobject.c  | 10 ++--
 src/mesa/main/pipelineobj.c   | 16 +++---
 src/mesa/state_tracker/st_debug.c |  2 +-
 29 files changed, 247 insertions(+), 158 deletions(-)

-- 
2.19.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 01/11] mesa: properly report the length of truncated log messages

[Mark Janes]

_mesa_log_msg must provide the length of the string passed into the
KHR_debug api.  When the string formatted by _mesa_gl_vdebugf exceeds
MAX_DEBUG_MESSAGE_LENGTH, the length is incorrectly set to the number
of characters that would have been written if enough space had been
available.

Fixes: 30256805784450b8bb9d4dabfb56226271ca9d24
   ("mesa: Add support for GL_ARB_debug_output with dynamic ID allocation.")
---
 src/mesa/main/errors.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/mesa/main/errors.c b/src/mesa/main/errors.c
index a9687913627..30560ba047e 100644
--- a/src/mesa/main/errors.c
+++ b/src/mesa/main/errors.c
@@ -231,6 +231,9 @@ _mesa_gl_vdebug(struct gl_context *ctx,
_mesa_debug_get_id(id);
 
len = _mesa_vsnprintf(s, MAX_DEBUG_MESSAGE_LENGTH, fmtString, args);
+   if (len >= MAX_DEBUG_MESSAGE_LENGTH)
+  /* message was truncated */
+  len = MAX_DEBUG_MESSAGE_LENGTH - 1;
 
_mesa_log_msg(ctx, source, type, *id, severity, len, s);
 }
-- 
2.19.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 10/11] i965: provide GLSL IR to KHR_debug

[Mark Janes]

---
 src/mesa/drivers/dri/i965/brw_link.cpp | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp 
b/src/mesa/drivers/dri/i965/brw_link.cpp
index 2cbb1e0b879..24e0079fc5d 100644
--- a/src/mesa/drivers/dri/i965/brw_link.cpp
+++ b/src/mesa/drivers/dri/i965/brw_link.cpp
@@ -251,10 +251,18 @@ brw_link_shader(struct gl_context *ctx, struct 
gl_shader_program *shProg)
  (INTEL_DEBUG & intel_debug_flag_for_shader_stage(shader->Stage));
 
   if (debug_enabled && shader->ir) {
- fprintf(stderr, "GLSL IR for native %s shader %d:\n",
+ char *buf;
+ size_t buf_size;
+ FILE * log_fp = open_memstream(, _size);
+ fprintf(log_fp, "GLSL IR for native %s shader %d:\n",
  _mesa_shader_stage_to_string(shader->Stage), shProg->Name);
- _mesa_print_ir(stderr, shader->ir, NULL);
- fprintf(stderr, "\n\n");
+ _mesa_print_ir(log_fp, shader->ir, NULL);
+ fprintf(log_fp, "\n\n");
+ fclose(log_fp);
+ static GLuint msg_id = 0;
+ shader_debug_log_mesa(brw, _id, buf);
+ fputs(buf, stderr);
+ free(buf);
   }
 
   prog->nir = brw_create_nir(brw, shProg, prog, (gl_shader_stage) stage,
-- 
2.19.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 06/11] i965/disasm: provide shader assembly to KHR_debug.

[Mark Janes]

Shader assemblies can be more easily incorporated into debug tools if
they are presented through KHR_debug.
---
 src/intel/compiler/brw_fs_generator.cpp   | 38 ---
 src/intel/compiler/brw_vec4_generator.cpp | 22 +++--
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/src/intel/compiler/brw_fs_generator.cpp 
b/src/intel/compiler/brw_fs_generator.cpp
index a8618912cf4..6b80f8433b2 100644
--- a/src/intel/compiler/brw_fs_generator.cpp
+++ b/src/intel/compiler/brw_fs_generator.cpp
@@ -2463,30 +2463,32 @@ fs_generator::generate_code(const cfg_t *cfg, int 
dispatch_width)
brw_compact_instructions(p, start_offset, disasm_info);
int after_size = p->next_insn_offset - start_offset;
 
+   char *buf;
+   size_t buf_size;
+   FILE * log_fp = open_memstream(, _size);
if (unlikely(debug_flag)) {
-  fprintf(stderr, "Native code for %s\n"
-  "SIMD%d shader: %d instructions. %d loops. %u cycles. %d:%d 
spills:fills. Promoted %u constants. Compacted %d to %d"
-  " bytes (%.0f%%)\n",
-  shader_name, dispatch_width, before_size / 16, loop_count, 
cfg->cycle_count,
-  spill_count, fill_count, promoted_constants, before_size, 
after_size,
-  100.0f * (before_size - after_size) / before_size);
-
-  dump_assembly(stderr, p->store, disasm_info);
+  fprintf(log_fp, "Native code for %s\n", shader_name);
}
+   fprintf(log_fp, "%s SIMD%d shader: %d inst, %d loops, %u cycles, "
+   "%d:%d spills:fills, Promoted %u constants, "
+   "compacted %d to %d bytes.",
+   _mesa_shader_stage_to_abbrev(stage),
+   dispatch_width, before_size / 16,
+   loop_count, cfg->cycle_count, spill_count,
+   fill_count, promoted_constants, before_size,
+   after_size);
+   if (unlikely(debug_flag)) {
+  dump_assembly(log_fp, p->store, disasm_info);
+  fflush(log_fp);
+  fputs(buf, stderr);
+   }
+   fclose(log_fp);
ralloc_free(disasm_info);
assert(validated);
 
static GLuint msg_id = 0;
-   compiler->shader_debug_log(log_data, _id,
-  "%s SIMD%d shader: %d inst, %d loops, %u cycles, 
"
-  "%d:%d spills:fills, Promoted %u constants, "
-  "compacted %d to %d bytes.",
-  _mesa_shader_stage_to_abbrev(stage),
-  dispatch_width, before_size / 16,
-  loop_count, cfg->cycle_count, spill_count,
-  fill_count, promoted_constants, before_size,
-  after_size);
-
+   compiler->shader_debug_log(log_data, _id, "%s", buf);
+   free(buf);
return start_offset;
 }
 
diff --git a/src/intel/compiler/brw_vec4_generator.cpp 
b/src/intel/compiler/brw_vec4_generator.cpp
index 00c89b21daf..f56ec6aa9a1 100644
--- a/src/intel/compiler/brw_vec4_generator.cpp
+++ b/src/intel/compiler/brw_vec4_generator.cpp
@@ -2195,30 +2195,34 @@ generate_code(struct brw_codegen *p,
brw_compact_instructions(p, 0, disasm_info);
int after_size = p->next_insn_offset;
 
+   char *buf;
+   size_t buf_size;
+   FILE * log_fp = open_memstream(, _size);
if (unlikely(debug_flag)) {
-  fprintf(stderr, "Native code for %s %s shader %s:\n",
+  fprintf(log_fp, "Native code for %s %s shader %s:\n",
   nir->info.label ? nir->info.label : "unnamed",
   _mesa_shader_stage_to_string(nir->info.stage), nir->info.name);
+   }
 
-  fprintf(stderr, "%s vec4 shader: %d instructions. %d loops. %u cycles. 
%d:%d "
+   fprintf(log_fp, "%s vec4 shader: %d instructions. %d loops. %u cycles. 
%d:%d "
   "spills:fills. Compacted %d to %d bytes (%.0f%%)\n",
   stage_abbrev, before_size / 16, loop_count, cfg->cycle_count,
   spill_count, fill_count, before_size, after_size,
   100.0f * (before_size - after_size) / before_size);
 
-  dump_assembly(stderr, p->store, disasm_info);
+   if (unlikely(debug_flag)) {
+  dump_assembly(log_fp, p->store, disasm_info);
+  fflush(log_fp);
+  fputs(buf, stderr);
}
ralloc_free(disasm_info);
assert(validated);
 
+   fclose(log_fp);
static GLuint msg_id = 0;
compiler->shader_debug_log(log_data, _id,
-  "%s vec4 shader: %d inst, %d loops, %u cycles, "
-  "%d:%d spills:fills, compacted %d to %d bytes.",
-  stage_abbrev, before_size / 16,
-  loop_count, cfg->cycle_count, spill_count,
-  fill_count, before_size, after_size);
-
+  "%s", buf);
+   free(buf);
 }
 
 extern "C" const unsigned *
-- 
2.19.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 09/11] i965: make shader_debug_log_mesa callable

[Mark Janes]

Debug output of IR/assemblies is likely to exceed the 4k limit in
KHR_debug.

shader_debug_log_mesa splits large messages, providing the full
content through KHR_debug.  Expose the function, so it can be called
from the shader cache and the linker.
---
 src/mesa/drivers/dri/i965/intel_screen.c | 2 +-
 src/mesa/drivers/dri/i965/intel_screen.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
b/src/mesa/drivers/dri/i965/intel_screen.c
index ed0eaa783db..84f472814bc 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -2388,7 +2388,7 @@ intel_device_get_revision(int fd)
return revision;
 }
 
-static void
+void
 shader_debug_log_mesa(void *data, GLuint *msg_id, const char *buf)
 {
struct brw_context *brw = (struct brw_context *)data;
diff --git a/src/mesa/drivers/dri/i965/intel_screen.h 
b/src/mesa/drivers/dri/i965/intel_screen.h
index 8d56fcd9e7a..8c1101d6b08 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.h
+++ b/src/mesa/drivers/dri/i965/intel_screen.h
@@ -178,6 +178,9 @@ can_do_exec_capture(const struct intel_screen *screen)
return screen->kernel_features & KERNEL_ALLOWS_EXEC_CAPTURE;
 }
 
+void
+shader_debug_log_mesa(void *data, GLuint *msg_id, const char *buf);
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.19.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 05/11] i965: provide stable message id's for shader_debug_log

[Mark Janes]

KHR_debug indicates that message id's can be used to filter debug
logs.  Mesa incorrectly generates incrementing message id's for each
message in shader_debug_log.  The ids must be stable if they are to be
used for filtering.

_mesa_gl_vdebug expects the address of a static GLuint, which is
assigned at the first log event and remains stable for subsequent
events.
---
 src/intel/compiler/brw_compiler.h | 2 +-
 src/intel/compiler/brw_fs_generator.cpp   | 3 ++-
 src/intel/compiler/brw_vec4_generator.cpp | 3 ++-
 src/intel/vulkan/anv_device.c | 2 +-
 src/mesa/drivers/dri/i965/intel_screen.c  | 5 ++---
 5 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/intel/compiler/brw_compiler.h 
b/src/intel/compiler/brw_compiler.h
index e4f4d83c8e8..ca9d41ca7a8 100644
--- a/src/intel/compiler/brw_compiler.h
+++ b/src/intel/compiler/brw_compiler.h
@@ -89,7 +89,7 @@ struct brw_compiler {
   int aligned_pairs_class;
} fs_reg_sets[3];
 
-   void (*shader_debug_log)(void *, const char *str, ...) PRINTFLIKE(2, 3);
+   void (*shader_debug_log)(void *, GLuint *msg_id, const char *str, ...) 
PRINTFLIKE(3, 4);
void (*shader_perf_log)(void *, const char *str, ...) PRINTFLIKE(2, 3);
 
bool scalar_stage[MESA_SHADER_STAGES];
diff --git a/src/intel/compiler/brw_fs_generator.cpp 
b/src/intel/compiler/brw_fs_generator.cpp
index ebf63b07837..a8618912cf4 100644
--- a/src/intel/compiler/brw_fs_generator.cpp
+++ b/src/intel/compiler/brw_fs_generator.cpp
@@ -2476,7 +2476,8 @@ fs_generator::generate_code(const cfg_t *cfg, int 
dispatch_width)
ralloc_free(disasm_info);
assert(validated);
 
-   compiler->shader_debug_log(log_data,
+   static GLuint msg_id = 0;
+   compiler->shader_debug_log(log_data, _id,
   "%s SIMD%d shader: %d inst, %d loops, %u cycles, 
"
   "%d:%d spills:fills, Promoted %u constants, "
   "compacted %d to %d bytes.",
diff --git a/src/intel/compiler/brw_vec4_generator.cpp 
b/src/intel/compiler/brw_vec4_generator.cpp
index ee232cbf96e..00c89b21daf 100644
--- a/src/intel/compiler/brw_vec4_generator.cpp
+++ b/src/intel/compiler/brw_vec4_generator.cpp
@@ -2211,7 +2211,8 @@ generate_code(struct brw_codegen *p,
ralloc_free(disasm_info);
assert(validated);
 
-   compiler->shader_debug_log(log_data,
+   static GLuint msg_id = 0;
+   compiler->shader_debug_log(log_data, _id,
   "%s vec4 shader: %d inst, %d loops, %u cycles, "
   "%d:%d spills:fills, compacted %d to %d bytes.",
   stage_abbrev, before_size / 16,
diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 6b5ba25c6bc..e60f7327ec1 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -45,7 +45,7 @@
 #include "genxml/gen7_pack.h"
 
 static void
-compiler_debug_log(void *data, const char *fmt, ...)
+compiler_debug_log(void *data, GLuint *id, const char *fmt, ...)
 { }
 
 static void
diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
b/src/mesa/drivers/dri/i965/intel_screen.c
index c0201357869..6b54767f8dd 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -2389,14 +2389,13 @@ intel_device_get_revision(int fd)
 }
 
 static void
-shader_debug_log_mesa(void *data, const char *fmt, ...)
+shader_debug_log_mesa(void *data, GLuint *msg_id, const char *fmt, ...)
 {
struct brw_context *brw = (struct brw_context *)data;
va_list args;
 
va_start(args, fmt);
-   GLuint msg_id = 0;
-   _mesa_gl_vdebugf(>ctx, _id,
+   _mesa_gl_vdebugf(>ctx, msg_id,
 MESA_DEBUG_SOURCE_SHADER_COMPILER,
 MESA_DEBUG_TYPE_OTHER,
 MESA_DEBUG_SEVERITY_NOTIFICATION, fmt, args);
-- 
2.19.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 03/11] mesa: add logging function for formatted string

[Mark Janes]

---
 src/mesa/main/errors.c | 27 +++
 src/mesa/main/errors.h |  8 
 2 files changed, 35 insertions(+)

diff --git a/src/mesa/main/errors.c b/src/mesa/main/errors.c
index fad8cb59cae..995b0510575 100644
--- a/src/mesa/main/errors.c
+++ b/src/mesa/main/errors.c
@@ -253,6 +253,33 @@ _mesa_gl_debugf(struct gl_context *ctx,
va_end(args);
 }
 
+size_t
+_mesa_gl_debug(struct gl_context *ctx,
+   GLuint *id,
+   enum mesa_debug_source source,
+   enum mesa_debug_type type,
+   enum mesa_debug_severity severity,
+   const char *msg)
+{
+   _mesa_debug_get_id(id);
+
+   size_t len = strnlen(msg, MAX_DEBUG_MESSAGE_LENGTH);
+   if (len < MAX_DEBUG_MESSAGE_LENGTH) {
+  _mesa_log_msg(ctx, source, type, *id, severity, len, msg);
+  return len;
+   }
+
+   /* limit the message to fit within KHR_debug buffers */
+   char s[MAX_DEBUG_MESSAGE_LENGTH];
+   strncpy(s, msg, MAX_DEBUG_MESSAGE_LENGTH);
+   s[MAX_DEBUG_MESSAGE_LENGTH - 1] = '\0';
+   len = MAX_DEBUG_MESSAGE_LENGTH - 1;
+   _mesa_log_msg(ctx, source, type, *id, severity, len, s);
+
+   /* report the number of characters that were logged */
+   return len;
+}
+
 
 /**
  * Record an OpenGL state error.  These usually occur when the user
diff --git a/src/mesa/main/errors.h b/src/mesa/main/errors.h
index 3e2d56e7741..17fe380f26a 100644
--- a/src/mesa/main/errors.h
+++ b/src/mesa/main/errors.h
@@ -90,6 +90,14 @@ _mesa_gl_debugf(struct gl_context *ctx,
 enum mesa_debug_severity severity,
 const char *fmtString, ...) PRINTFLIKE(6, 7);
 
+extern size_t
+_mesa_gl_debug(struct gl_context *ctx,
+   GLuint *id,
+   enum mesa_debug_source source,
+   enum mesa_debug_type type,
+   enum mesa_debug_severity severity,
+   const char *msg);
+
 #define _mesa_perf_debug(ctx, sev, ...) do {  \
static GLuint msg_id = 0;  \
if (unlikely(ctx->Const.ContextFlags & GL_CONTEXT_FLAG_DEBUG_BIT)) {   \
-- 
2.19.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 08/11] intel/nir: provide SSA form to KHR_debug

[Mark Janes]

---
 src/intel/compiler/brw_fs.cpp  | 11 -
 src/intel/compiler/brw_nir.c   | 26 +-
 src/intel/compiler/brw_nir.h   |  1 +
 src/intel/compiler/brw_shader.cpp  |  2 +-
 src/intel/compiler/brw_vec4.cpp|  2 +-
 src/intel/compiler/brw_vec4_gs_visitor.cpp |  2 +-
 src/intel/compiler/brw_vec4_tcs.cpp|  2 +-
 7 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp
index 43b920ae33d..f361d7aad9d 100644
--- a/src/intel/compiler/brw_fs.cpp
+++ b/src/intel/compiler/brw_fs.cpp
@@ -7139,7 +7139,7 @@ brw_compile_fs(const struct brw_compiler *compiler, void 
*log_data,
if (!key->multisample_fbo)
   NIR_PASS_V(shader, demote_sample_qualifiers);
NIR_PASS_V(shader, move_interpolation_to_top);
-   shader = brw_postprocess_nir(shader, compiler, true);
+   shader = brw_postprocess_nir(shader, compiler, log_data, true);
 
/* key->alpha_test_func means simulating alpha testing via discards,
 * so the shader definitely kills pixels.
@@ -7384,6 +7384,7 @@ cs_set_simd_size(struct brw_cs_prog_data *cs_prog_data, 
unsigned size)
 
 static nir_shader *
 compile_cs_to_nir(const struct brw_compiler *compiler,
+  void *log_data,
   void *mem_ctx,
   const struct brw_cs_prog_key *key,
   const nir_shader *src_shader,
@@ -7392,7 +7393,7 @@ compile_cs_to_nir(const struct brw_compiler *compiler,
nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
shader = brw_nir_apply_sampler_key(shader, compiler, >tex, true);
brw_nir_lower_cs_intrinsics(shader, dispatch_width);
-   return brw_postprocess_nir(shader, compiler, true);
+   return brw_postprocess_nir(shader, compiler, log_data, true);
 }
 
 const unsigned *
@@ -7425,7 +7426,7 @@ brw_compile_cs(const struct brw_compiler *compiler, void 
*log_data,
/* Now the main event: Visit the shader IR and generate our CS IR for it.
 */
if (min_dispatch_width <= 8) {
-  nir_shader *nir8 = compile_cs_to_nir(compiler, mem_ctx, key,
+  nir_shader *nir8 = compile_cs_to_nir(compiler, log_data, mem_ctx, key,
src_shader, 8);
   v8 = new fs_visitor(compiler, log_data, mem_ctx, key, _data->base,
   NULL, /* Never used in core profile */
@@ -7446,7 +7447,7 @@ brw_compile_cs(const struct brw_compiler *compiler, void 
*log_data,
if (likely(!(INTEL_DEBUG & DEBUG_NO16)) &&
!fail_msg && min_dispatch_width <= 16) {
   /* Try a SIMD16 compile */
-  nir_shader *nir16 = compile_cs_to_nir(compiler, mem_ctx, key,
+  nir_shader *nir16 = compile_cs_to_nir(compiler, mem_ctx, log_data, key,
 src_shader, 16);
   v16 = new fs_visitor(compiler, log_data, mem_ctx, key, _data->base,
NULL, /* Never used in core profile */
@@ -7479,7 +7480,7 @@ brw_compile_cs(const struct brw_compiler *compiler, void 
*log_data,
 
if (!fail_msg && (min_dispatch_width > 16 || (INTEL_DEBUG & DEBUG_DO32))) {
   /* Try a SIMD32 compile */
-  nir_shader *nir32 = compile_cs_to_nir(compiler, mem_ctx, key,
+  nir_shader *nir32 = compile_cs_to_nir(compiler, log_data, mem_ctx, key,
 src_shader, 32);
   v32 = new fs_visitor(compiler, log_data, mem_ctx, key, _data->base,
NULL, /* Never used in core profile */
diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
index aa6788b9fe5..9c7877b920f 100644
--- a/src/intel/compiler/brw_nir.c
+++ b/src/intel/compiler/brw_nir.c
@@ -775,7 +775,7 @@ brw_nir_link_shaders(const struct brw_compiler *compiler,
  */
 nir_shader *
 brw_postprocess_nir(nir_shader *nir, const struct brw_compiler *compiler,
-bool is_scalar)
+void *log_data, bool is_scalar)
 {
const struct gen_device_info *devinfo = compiler->devinfo;
bool debug_enabled =
@@ -812,9 +812,17 @@ brw_postprocess_nir(nir_shader *nir, const struct 
brw_compiler *compiler,
 nir_index_ssa_defs(function->impl);
   }
 
-  fprintf(stderr, "NIR (SSA form) for %s shader:\n",
+  char *buf;
+  size_t buf_size;
+  FILE * log_fp = open_memstream(, _size);
+  fprintf(log_fp, "NIR (SSA form) for %s shader:\n",
   _mesa_shader_stage_to_string(nir->info.stage));
-  nir_print_shader(nir, stderr);
+  nir_print_shader(nir, log_fp);
+  fclose(log_fp);
+  static GLuint msg_id = 0;
+  compiler->shader_debug_log(log_data, _id, buf);
+  fputs(buf, stderr);
+  free(buf);
}
 
OPT(nir_convert_from_ssa, true);
@@ -837,9 +845,17 @@ brw_postprocess_nir(nir_shader *nir, const struct 
brw_compiler *compiler,
nir_sweep(nir);
 
if (unlikely(debug_enabled)) {
-  fprintf(stderr, "NIR (final form) for %s 

[Mesa-dev] [PATCH 07/11] i965/compiler: provide formatted strings to the shader debug log

[Mark Janes]

---
 src/intel/compiler/brw_compiler.h |  2 +-
 src/intel/compiler/brw_fs_generator.cpp   |  2 +-
 src/intel/compiler/brw_vec4_generator.cpp |  3 +--
 src/intel/vulkan/anv_device.c |  2 +-
 src/mesa/drivers/dri/i965/intel_screen.c  | 19 ++-
 5 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/intel/compiler/brw_compiler.h 
b/src/intel/compiler/brw_compiler.h
index ca9d41ca7a8..816b96b24e6 100644
--- a/src/intel/compiler/brw_compiler.h
+++ b/src/intel/compiler/brw_compiler.h
@@ -89,7 +89,7 @@ struct brw_compiler {
   int aligned_pairs_class;
} fs_reg_sets[3];
 
-   void (*shader_debug_log)(void *, GLuint *msg_id, const char *str, ...) 
PRINTFLIKE(3, 4);
+   void (*shader_debug_log)(void *, GLuint *msg_id, const char *buf);
void (*shader_perf_log)(void *, const char *str, ...) PRINTFLIKE(2, 3);
 
bool scalar_stage[MESA_SHADER_STAGES];
diff --git a/src/intel/compiler/brw_fs_generator.cpp 
b/src/intel/compiler/brw_fs_generator.cpp
index 6b80f8433b2..10cc6b2ff15 100644
--- a/src/intel/compiler/brw_fs_generator.cpp
+++ b/src/intel/compiler/brw_fs_generator.cpp
@@ -2487,7 +2487,7 @@ fs_generator::generate_code(const cfg_t *cfg, int 
dispatch_width)
assert(validated);
 
static GLuint msg_id = 0;
-   compiler->shader_debug_log(log_data, _id, "%s", buf);
+   compiler->shader_debug_log(log_data, _id, buf);
free(buf);
return start_offset;
 }
diff --git a/src/intel/compiler/brw_vec4_generator.cpp 
b/src/intel/compiler/brw_vec4_generator.cpp
index f56ec6aa9a1..0ed1c8d99fa 100644
--- a/src/intel/compiler/brw_vec4_generator.cpp
+++ b/src/intel/compiler/brw_vec4_generator.cpp
@@ -2220,8 +2220,7 @@ generate_code(struct brw_codegen *p,
 
fclose(log_fp);
static GLuint msg_id = 0;
-   compiler->shader_debug_log(log_data, _id,
-  "%s", buf);
+   compiler->shader_debug_log(log_data, _id, buf);
free(buf);
 }
 
diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index e60f7327ec1..f8ecf2f6991 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -45,7 +45,7 @@
 #include "genxml/gen7_pack.h"
 
 static void
-compiler_debug_log(void *data, GLuint *id, const char *fmt, ...)
+compiler_debug_log(void *data, GLuint *id, const char *buf)
 { }
 
 static void
diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
b/src/mesa/drivers/dri/i965/intel_screen.c
index 6b54767f8dd..ed0eaa783db 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -2389,17 +2389,18 @@ intel_device_get_revision(int fd)
 }
 
 static void
-shader_debug_log_mesa(void *data, GLuint *msg_id, const char *fmt, ...)
+shader_debug_log_mesa(void *data, GLuint *msg_id, const char *buf)
 {
struct brw_context *brw = (struct brw_context *)data;
-   va_list args;
-
-   va_start(args, fmt);
-   _mesa_gl_vdebugf(>ctx, msg_id,
-MESA_DEBUG_SOURCE_SHADER_COMPILER,
-MESA_DEBUG_TYPE_OTHER,
-MESA_DEBUG_SEVERITY_NOTIFICATION, fmt, args);
-   va_end(args);
+   const size_t len = strlen(buf);
+   const char *buf_ptr = buf;
+   while (buf_ptr < buf + len) {
+  buf_ptr += _mesa_gl_debug(>ctx, msg_id,
+MESA_DEBUG_SOURCE_SHADER_COMPILER,
+MESA_DEBUG_TYPE_OTHER,
+MESA_DEBUG_SEVERITY_NOTIFICATION,
+buf_ptr);
+   }
 }
 
 static void
-- 
2.19.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 02/11] mesa: rename logging functions to reflect that they format strings

[Mark Janes]

In preparation for the definition of a function to log a formatted
string.
---
 src/mesa/drivers/dri/i915/intel_context.h | 18 +--
 src/mesa/drivers/dri/i915/intel_fbo.c | 10 +++---
 src/mesa/drivers/dri/i965/brw_context.h   | 18 +--
 src/mesa/drivers/dri/i965/genX_state_upload.c | 14 
 src/mesa/drivers/dri/i965/intel_fbo.c | 10 +++---
 src/mesa/drivers/dri/i965/intel_screen.c  | 16 +-
 src/mesa/main/bufferobj.c | 10 +++---
 src/mesa/main/errors.c| 28 
 src/mesa/main/errors.h| 32 +--
 src/mesa/main/fbobject.c  | 10 +++---
 src/mesa/main/pipelineobj.c   | 16 +-
 src/mesa/state_tracker/st_debug.c |  2 +-
 12 files changed, 92 insertions(+), 92 deletions(-)

diff --git a/src/mesa/drivers/dri/i915/intel_context.h 
b/src/mesa/drivers/dri/i915/intel_context.h
index b79288d29bc..2ed1a1444e5 100644
--- a/src/mesa/drivers/dri/i915/intel_context.h
+++ b/src/mesa/drivers/dri/i915/intel_context.h
@@ -304,11 +304,11 @@ extern int INTEL_DEBUG;
if (unlikely(INTEL_DEBUG & DEBUG_PERF))  \
   dbg_printf(__VA_ARGS__);  \
if (intel->perf_debug)   \
-  _mesa_gl_debug(>ctx, _id,  \
- MESA_DEBUG_SOURCE_API, \
- MESA_DEBUG_TYPE_PERFORMANCE,   \
- MESA_DEBUG_SEVERITY_MEDIUM,\
- __VA_ARGS__);  \
+  _mesa_gl_debugf(>ctx, _id, \
+  MESA_DEBUG_SOURCE_API,\
+  MESA_DEBUG_TYPE_PERFORMANCE,  \
+  MESA_DEBUG_SEVERITY_MEDIUM,   \
+  __VA_ARGS__); \
 } while(0)
 
 #define WARN_ONCE(cond, fmt...) do {\
@@ -320,10 +320,10 @@ extern int INTEL_DEBUG;
  fprintf(stderr, fmt);  \
  _warned = true;\
 \
- _mesa_gl_debug(ctx, _id,   \
-MESA_DEBUG_SOURCE_API,  \
-MESA_DEBUG_TYPE_OTHER,  \
-MESA_DEBUG_SEVERITY_HIGH, fmt); \
+ _mesa_gl_debugf(ctx, _id,  \
+ MESA_DEBUG_SOURCE_API, \
+ MESA_DEBUG_TYPE_OTHER, \
+ MESA_DEBUG_SEVERITY_HIGH, fmt);\
   } \
}\
 } while (0)
diff --git a/src/mesa/drivers/dri/i915/intel_fbo.c 
b/src/mesa/drivers/dri/i915/intel_fbo.c
index 78e2c1e6614..880fd7cfba1 100644
--- a/src/mesa/drivers/dri/i915/intel_fbo.c
+++ b/src/mesa/drivers/dri/i915/intel_fbo.c
@@ -536,11 +536,11 @@ intel_finish_render_texture(struct gl_context * ctx, 
struct gl_renderbuffer *rb)
 #define fbo_incomplete(fb, ...) do {  \
   static GLuint msg_id = 0;   \
   if (unlikely(ctx->Const.ContextFlags & GL_CONTEXT_FLAG_DEBUG_BIT)) {\
- _mesa_gl_debug(ctx, _id, \
-MESA_DEBUG_SOURCE_API,\
-MESA_DEBUG_TYPE_OTHER,\
-MESA_DEBUG_SEVERITY_MEDIUM,   \
-__VA_ARGS__); \
+ _mesa_gl_debugf(ctx, _id,\
+ MESA_DEBUG_SOURCE_API,   \
+ MESA_DEBUG_TYPE_OTHER,   \
+ MESA_DEBUG_SEVERITY_MEDIUM,  \
+ __VA_ARGS__);\
   }   \
   DBG(__VA_ARGS__);   \
   fb->_Status = GL_FRAMEBUFFER_UNSUPPORTED;   \
diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
b/src/mesa/drivers/dri/i965/brw_context.h
index e5d57dd8df1..7374eef9fb5 100644
--- a/src/mesa/drivers/dri/i965/brw_context.h
+++ b/src/mesa/drivers/dri/i965/brw_context.h
@@ -388,11 +388,11 @@ struct brw_cache {
if (unlikely(INTEL_DEBUG & DEBUG_PERF))  \
   dbg_printf(__VA_ARGS__);

Re: [Mesa-dev] [PATCH] mesa/main: fix up _mesa_has_rg_textures for gles2

[Mark Janes]

Choose a well-named branch on your mesa repository that you will use to
trigger i965 CI builds.  Provide me with the branch name and the repo
details, and I will configure the build.

When you force-push to the branch, your build will start.  You will get
a notification when the build starts, and when it finishes.  Results
will appear on mesa-ci.01.org a few minutes after the final
notification.

Erik Faye-Lund  writes:

> On Tue, 2018-12-04 at 10:33 -0800, Mark Janes wrote:
>> Tested-by: Mark Janes 
>> 
>> Erik Faye-Lund  writes:
>> 
>> > rg-textures are supported in GLES 2.0 if EXT_texture_rg, so let's
>> > make
>> > sure the enums are accepted.
>> > 
>> > Fixes: 510b6424607 "mesa/main: do not allow rg-textures enums
>> > before gles3"
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108936
>> > Signed-off-by: Erik Faye-Lund 
>> > ---
>> > Whoops, seems I missed an extension to enable RG-textures. Sorry
>> > for the
>> > breakage!
>> 
>> You are welcome to test on the i965 CI, which will quickly catch
>> these
>> errors for you.
>
> I would love to! How do I do that?
>
> I can't see an obvious way of triggering something here:
> https://mesa-ci.01.org/
>
>> 
>> >  src/mesa/main/context.h | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/src/mesa/main/context.h b/src/mesa/main/context.h
>> > index cdda8cf2012..7de10e9924b 100644
>> > --- a/src/mesa/main/context.h
>> > +++ b/src/mesa/main/context.h
>> > @@ -378,7 +378,8 @@ _mesa_has_packed_float(const struct gl_context
>> > *ctx)
>> >  static inline bool
>> >  _mesa_has_rg_textures(const struct gl_context *ctx)
>> >  {
>> > -   return _mesa_has_ARB_texture_rg(ctx) || _mesa_is_gles3(ctx);
>> > +   return _mesa_has_ARB_texture_rg(ctx) ||
>> > _mesa_has_EXT_texture_rg(ctx) ||
>> > +  _mesa_is_gles3(ctx);
>> >  }
>> >  
>> >  static inline bool
>> > -- 
>> > 2.19.2
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] mesa/main: fix up _mesa_has_rg_textures for gles2

[Mark Janes]

Tested-by: Mark Janes 

Erik Faye-Lund  writes:

> rg-textures are supported in GLES 2.0 if EXT_texture_rg, so let's make
> sure the enums are accepted.
>
> Fixes: 510b6424607 "mesa/main: do not allow rg-textures enums before gles3"
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108936
> Signed-off-by: Erik Faye-Lund 
> ---
> Whoops, seems I missed an extension to enable RG-textures. Sorry for the
> breakage!

You are welcome to test on the i965 CI, which will quickly catch these
errors for you.

>  src/mesa/main/context.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/main/context.h b/src/mesa/main/context.h
> index cdda8cf2012..7de10e9924b 100644
> --- a/src/mesa/main/context.h
> +++ b/src/mesa/main/context.h
> @@ -378,7 +378,8 @@ _mesa_has_packed_float(const struct gl_context *ctx)
>  static inline bool
>  _mesa_has_rg_textures(const struct gl_context *ctx)
>  {
> -   return _mesa_has_ARB_texture_rg(ctx) || _mesa_is_gles3(ctx);
> +   return _mesa_has_ARB_texture_rg(ctx) || _mesa_has_EXT_texture_rg(ctx) ||
> +  _mesa_is_gles3(ctx);
>  }
>  
>  static inline bool
> -- 
> 2.19.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] mesa/main: fixup requirements for GL_PRIMITIVES_GENERATED

[Mark Janes]

Tested-by: Mark Janes 

Erik Faye-Lund  writes:

> This enum is also allowed by EXT_tessellation_shader, which is supported
> on older i965 HW (as opposed to OES_geometry_shader). This was missed
> when narrowing this code-path, leading to dEQP regressions.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108868
> Fixes: f09d94fbd11 "mesa/main: fix validation of transform-feedback queries"
> Signed-off-by: Erik Faye-Lund 
> ---
> Sorry about this! As I mentioned on the ticket, I don't have old enough
> intel HW to trigger this, but I think this should do the trick.
>
>  src/mesa/main/queryobj.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/mesa/main/queryobj.c b/src/mesa/main/queryobj.c
> index af558f0e227..67d1f76a21a 100644
> --- a/src/mesa/main/queryobj.c
> +++ b/src/mesa/main/queryobj.c
> @@ -190,6 +190,7 @@ get_query_binding_point(struct gl_context *ctx, GLenum 
> target, GLuint index)
>   return NULL;
> case GL_PRIMITIVES_GENERATED:
>if (_mesa_has_EXT_transform_feedback(ctx) ||
> +  _mesa_has_EXT_tessellation_shader(ctx) ||
>_mesa_has_OES_geometry_shader(ctx))
>   return >Query.PrimitivesGenerated[index];
>else
> -- 
> 2.19.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] Revert "nir/builder: Assert that intN_t immediates fit"

[Mark Janes]

"Juan A. Suarez Romero"  writes:

> On Tue, 2018-11-20 at 11:13 +0100, Juan A. Suarez Romero wrote:
>> On Tue, 2018-11-20 at 09:47 +0100, Iago Toral Quiroga wrote:
>> > This reverts commit .
>> > 
>> > For this to work the compiler must ensure that it never puts
>> > the values that arrive to this helper into unsigned variables
>> > at any point in its processing, since that would not apply sign
>> > extension to the value and it would break the expectations here.
>> > Unfortunately, we use uint64_t extensively to pass and copy
>> > things around, so some times we get to this helper with values
>> > that are not properly sign extended to 64-bit. Here is an example
>> > for an 8-bit value that comes from a switch case:
>> > 
>> > (gdb) p /x x
>> > $1 = 0xffd6
>> > 
>> > The value seems to have been copied to a 32-bit value at some point
>> > getting proper sign extension, but then copied into a uint64_t
>> > which wont' apply sign extension, breaking the expectations of
>> > the assertion.
>> 
>> This is fixing dEQP-VK.spirv_assembly.type.scalar.i16.switch_vert, which 
>> failing
>> this assertion.
>> 
>> Specifically, it is failing when dealing with this SPIR-V instruction:
>> 
>> OpSwitch %input_val %default -3221 %case0 3210 %case1 19597 %case2
>> 
>> And more specifically, when dealing with the -3221 value. Just to point out 
>> the
>> values are int16.
>> 
>> 
>> Additional, just to mention this wasn't caught by the Intel CI because the
>> Vulkan CTS reference it uses to test master points to Vulkan CTS 1.1.0, which
>> does not contain this test.
>> 
>
> I'm realizing this is not entirely true. There are right now two kind of test
> branches for master: mesa_master and mesa_master_daily. The former uses 
> vulkan-
> cts-1.0.2 as reference (plus some changes), and the later uses vulkan-cts-
> 1.1.2.1 as reference (plus some changes).
>
> https://mesa-ci.01.org/mesa_master/builds/14578/group/63a9f0ea7bb98050796b649e85481845
>
> https://mesa-ci.01.org/mesa_master_daily/builds/4596/group/63a9f0ea7bb98050796b649e85481845

Neither one of these test vulkan, and they do not specify a revision to
use for vulkancts.  If you look in the subgroups section of those
builds, you will not see any vulkan suites.

Vulkan is tested by the vulkacts and vulkancts_daily builds:

https://mesa-ci.01.org/vulkancts/builds/9626/group/63a9f0ea7bb98050796b649e85481845
https://mesa-ci.01.org/vulkancts_daily/builds/1228/group/63a9f0ea7bb98050796b649e85481845

The vulkan suites are separated out because we didn't want vulkan tests
to be triggered by piglit changes.  When mesa is pushed, both the
vulkancts and mesa_master builds are triggered.

The naming is confusing.  We should rename the builds:

 mesa_master*  -> mesa_master_opengl*
 vulkancts*-> mesa_master_vulkan*
 
> So in theory the later should catch the error. But apparently, they aren't
> running the vulkancts testsuite, or at least, I don't see it in the list of
> testsuites run.
>
>
>
>
>
>> What is the policty to update the testsuite references for master testing?
>> Shouldn't use a more recent version of the test suite? The latest version is
>> 1.1.2.2, which makes the version used as reference a bit old.
>> 
>> 
>> Reviewed-by: Juan A. Suarez 
>> 
>> > ---
>> > 
>> > FWIW, I tracked down the case where we were producing this and fixed
>> > it, then the test that triggered this problem started to pass... but
>> > others started to fail so I think it is best that we remove the assert
>> > for now. If we want to keep this then I think we might need to do a
>> > careful review of the compiler code and make sure we don't use any
>> > unsigned types to copy things around to ensure that sign extension
>> > happens consistently throughout.
>> > 
>> >  src/compiler/nir/nir_builder.h | 4 
>> >  1 file changed, 4 deletions(-)
>> > 
>> > diff --git a/src/compiler/nir/nir_builder.h 
>> > b/src/compiler/nir/nir_builder.h
>> > index e37aba23dc..30fa1d7ec8 100644
>> > --- a/src/compiler/nir/nir_builder.h
>> > +++ b/src/compiler/nir/nir_builder.h
>> > @@ -330,10 +330,6 @@ nir_imm_intN_t(nir_builder *build, uint64_t x, 
>> > unsigned bit_size)
>> >  {
>> > nir_const_value v;
>> >  
>> > -   assert(bit_size == 64 ||
>> > -  (int64_t)x >> bit_size == 0 ||
>> > -  (int64_t)x >> bit_size == -1);
>> > -
>> > memset(, 0, sizeof(v));
>> > assert(bit_size <= 64);
>> > v.i64[0] = x & (~0ull >> (64 - bit_size));
>> 
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/4] i965: Add support for and expose EXT_texture_sRGB_R8

[Mark Janes]

Eric Engestrom  writes:

> On Thursday, 2018-11-15 19:01:24 +0100, Gert Wollny wrote:
>> Emulate MESA_FORMAT_R_SRGB8 by using L8_UNORM_SRGB. This is possible
>> because component swizzling is handled based on the mesa format and,
>> hence, the a r001 swizzling can be used to correct the components.
>> 
>> Enables and makes pass (tested on Kabylake)
>> 
>>   dEQP-GLES31.functional.srgb_texture_decode.skip_decode.sr8.*
>>   dEQP-GLES31.functional.texture.filtering.cube_array.formats.sr8*
>> 
>> Signed-off-by: Gert Wollny 
>
> Patches 1-3 are:
> Reviewed-by: Eric Engestrom 
>
> Patch 4 is:
> Acked-by: Eric Engestrom 

For external contributors, patches like this should not be R-B or Acked
unless they have been tested in the i965 CI.

This series failed to build when pushed, and regressed many tests when
the compilation error was corrected.

  https://bugs.freedesktop.org/show_bug.cgi?id=108805

Gert, we can set up a CI build for you if you feel like it would be
helpful to your work.  Eric can also test your patches by pushing them
to his CI project.

>> ---
>>  src/mesa/drivers/dri/i965/brw_surface_formats.c | 1 +
>>  src/mesa/drivers/dri/i965/intel_extensions.c| 1 +
>>  2 files changed, 2 insertions(+)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/brw_surface_formats.c 
>> b/src/mesa/drivers/dri/i965/brw_surface_formats.c
>> index 879cb42453..77476a8c7e 100644
>> --- a/src/mesa/drivers/dri/i965/brw_surface_formats.c
>> +++ b/src/mesa/drivers/dri/i965/brw_surface_formats.c
>> @@ -67,6 +67,7 @@ brw_isl_format_for_mesa_format(mesa_format mesa_format)
>>[MESA_FORMAT_B8G8R8A8_SRGB] = ISL_FORMAT_B8G8R8A8_UNORM_SRGB,
>>[MESA_FORMAT_R8G8B8A8_SRGB] = ISL_FORMAT_R8G8B8A8_UNORM_SRGB,
>>[MESA_FORMAT_B8G8R8X8_SRGB] = ISL_FORMAT_B8G8R8X8_UNORM_SRGB,
>> +  [MESA_FORMAT_R_SRGB8] = ISL_FORMAT_L8_UNORM_SRGB,
>>[MESA_FORMAT_L_SRGB8] = ISL_FORMAT_L8_UNORM_SRGB,
>>[MESA_FORMAT_L8A8_SRGB] = ISL_FORMAT_L8A8_UNORM_SRGB,
>>[MESA_FORMAT_SRGB_DXT1] = ISL_FORMAT_BC1_UNORM_SRGB,
>> diff --git a/src/mesa/drivers/dri/i965/intel_extensions.c 
>> b/src/mesa/drivers/dri/i965/intel_extensions.c
>> index d7e02efb54..6ec6248a70 100644
>> --- a/src/mesa/drivers/dri/i965/intel_extensions.c
>> +++ b/src/mesa/drivers/dri/i965/intel_extensions.c
>> @@ -113,6 +113,7 @@ intelInitExtensions(struct gl_context *ctx)
>> ctx->Extensions.EXT_texture_snorm = true;
>> ctx->Extensions.EXT_texture_sRGB = true;
>> ctx->Extensions.EXT_texture_sRGB_decode = true;
>> +   ctx->Extensions.EXT_texture_sRGB_R8 = true;
>> ctx->Extensions.EXT_texture_swizzle = true;
>> ctx->Extensions.EXT_texture_type_2_10_10_10_REV = true;
>> ctx->Extensions.EXT_vertex_array_bgra = true;
>> -- 
>> 2.19.1
>> 
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v4 1/2] i965/batch: avoid reverting batch buffer if saved state is an empty

[Mark Janes]

andrey simiklit  writes:

> Hello,
>
> I tested this patch few times and have the following results:
> https://mesa-ci.01.org/global_logic/builds/38/group/63a9f0ea7bb98050796b649e85481845
> https://mesa-ci.01.org/global_logic/builds/41/group/63a9f0ea7bb98050796b649e85481845
> https://mesa-ci.01.org/global_logic/builds/43/group/63a9f0ea7bb98050796b649e85481845
>
> All test runs are finished with the following issue on G965:
>
> https://mesa-ci.01.org/global_logic/builds/38/results/46496330
> ext_framebuffer_multisample-accuracy all_samples srgb small depthstencil
> linear -auto -fbo
> 
> ERROR: This test passed when it expected failure

If you test the exact same revision twice in i965 CI, it will simply
publish the results from the previous run.  Often, a single machine
fails in a run, and we send the build a second time to re-test only that
machine (all other results are re-used for faster results).  We re-use
results based the commits of the source projects.

Instead of alternating two commits through the CI, you should trivially
amend the commit message of the patch you want to test, so it gets a
different sha.  This will cause a complete re-test of the patch.

For this test, it may be that you are looking at an intermittent result.
However, it is never a problem to unexpectedly fix a test.

> Note: Also I checked latest mesa master few times and there was no
> regression:
>
> https://mesa-ci.01.org/global_logic/builds/42/group/63a9f0ea7bb98050796b649e85481845
>
> Regards,
> Andrii.
>
> On Mon, Nov 5, 2018 at 9:48 AM  wrote:
>
>> From: Andrii Simiklit 
>>
>> There's no point reverting to the last saved point if that save point is
>> the empty batch, we will just repeat ourselves.
>>
>> v2: Merge with new commits, changes was minimized, added the 'fixes' tag
>> v3: Added in to patch series
>> v4: Fixed the regression which was introduced by this patch
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108630
>> Reported-by:  Mark Janes 
>> The solution provided by: Jordan Justen 
>>
>> CC: Chris Wilson 
>> Fixes: 3faf56ffbdeb "intel: Add an interface for saving/restoring
>>  the batchbuffer state."
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107626
>> Signed-off-by: Andrii Simiklit 
>> ---
>>  src/mesa/drivers/dri/i965/brw_compute.c   | 3 ++-
>>  src/mesa/drivers/dri/i965/brw_draw.c  | 3 ++-
>>  src/mesa/drivers/dri/i965/genX_blorp_exec.c   | 1 +
>>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 7 +++
>>  src/mesa/drivers/dri/i965/intel_batchbuffer.h | 1 +
>>  5 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_compute.c
>> b/src/mesa/drivers/dri/i965/brw_compute.c
>> index de08fc3ac1..5c8e3a5d4d 100644
>> --- a/src/mesa/drivers/dri/i965/brw_compute.c
>> +++ b/src/mesa/drivers/dri/i965/brw_compute.c
>> @@ -167,7 +167,7 @@ static void
>>  brw_dispatch_compute_common(struct gl_context *ctx)
>>  {
>> struct brw_context *brw = brw_context(ctx);
>> -   bool fail_next = false;
>> +   bool fail_next;
>>
>> if (!_mesa_check_conditional_render(ctx))
>>return;
>> @@ -185,6 +185,7 @@ brw_dispatch_compute_common(struct gl_context *ctx)
>> intel_batchbuffer_require_space(brw, 600);
>> brw_require_statebuffer_space(brw, 2500);
>> intel_batchbuffer_save_state(brw);
>> +   fail_next = intel_batchbuffer_saved_state_is_empty(brw);
>>
>>   retry:
>> brw->batch.no_wrap = true;
>> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
>> b/src/mesa/drivers/dri/i965/brw_draw.c
>> index 8536c04010..19ee3962d7 100644
>> --- a/src/mesa/drivers/dri/i965/brw_draw.c
>> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
>> @@ -885,7 +885,7 @@ brw_draw_single_prim(struct gl_context *ctx,
>>  {
>> struct brw_context *brw = brw_context(ctx);
>> const struct gen_device_info *devinfo = >screen->devinfo;
>> -   bool fail_next = false;
>> +   bool fail_next;
>>
>> /* Flag BRW_NEW_DRAW_CALL on every draw.  This allows us to have
>>  * atoms that happen on every draw call.
>> @@ -898,6 +898,7 @@ brw_draw_single_prim(struct gl_context *ctx,
>> intel_batchbuffer_require_space(brw, 1500);
>> brw_require_statebuffer_space(brw, 2400);
>> intel_batchbuffer_save_state(brw);
>> +   fail_next = intel_batchbuffer_saved_state_is_empty(brw);
>>
>> if (brw->num_instances != prim->num_instances ||
>> brw->basevertex != prim->basevertex ||
>> diff --git a/src

Re: [Mesa-dev] [PATCH] anv: Don't advertise ASTC support on BSW

[Mark Janes]

Tested-by: Mark Janes 

Jason Ekstrand  writes:

> ---
>  src/intel/vulkan/anv_formats.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/src/intel/vulkan/anv_formats.c b/src/intel/vulkan/anv_formats.c
> index 33faf7cc37f..9199567f445 100644
> --- a/src/intel/vulkan/anv_formats.c
> +++ b/src/intel/vulkan/anv_formats.c
> @@ -521,6 +521,14 @@ get_image_format_features(const struct gen_device_info 
> *devinfo,
> isl_format_get_layout(plane_format.isl_format)->txc == ISL_TXC_ASTC)
>return 0;
>  
> +   /* ASTC requires nasty workarounds on BSW so we just disable it for now.
> +*
> +* TODO: Figure out the ASTC workarounds and re-enable on BSW.
> +*/
> +   if (devinfo->gen < 9 &&
> +   isl_format_get_layout(plane_format.isl_format)->txc == ISL_TXC_ASTC)
> +  return 0;
> +
> if (isl_format_supports_sampling(devinfo, plane_format.isl_format)) {
>flags |= VK_FORMAT_FEATURE_SAMPLED_IMAGE_BIT;
>  
> -- 
> 2.19.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] intel/fs: Fix a typo in need_matching_subreg_offset

[Mark Janes]

Tested-by: Mark Janes 

Jason Ekstrand  writes:

> This fixes a bunch of Vulkan subgroup tests on little core platforms.
>
> Fixes: 4150920b95 "intel/fs: Add a helper for emitting scan operations"
> ---
>  src/intel/compiler/brw_fs_builder.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/intel/compiler/brw_fs_builder.h 
> b/src/intel/compiler/brw_fs_builder.h
> index 7bee2aa0b9b..0cafaf50e56 100644
> --- a/src/intel/compiler/brw_fs_builder.h
> +++ b/src/intel/compiler/brw_fs_builder.h
> @@ -795,7 +795,7 @@ namespace brw {
>   !gen_device_info_is_9lp(shader->devinfo))
>  return false;
>  
> - if (type_sz(type > 4))
> + if (type_sz(type) > 4)
>  return true;
>  
>   if (opcode == BRW_OPCODE_MUL &&
> -- 
> 2.17.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] mesa: only update framebuffer-state for clears

[Mark Janes]

This patch broke all the intel systems also.

Ilia Mirkin  writes:

> On Wed, Sep 19, 2018 at 7:04 AM, Jakob Bornecrantz  
> wrote:
>> This patch regresses about 3000 dEQP [2,3,3.1] tests on virgl. Full
>> setup is dEQP running on virgl with vtest that is running RadeonSI. So
>> QEMU is not in the picture. All instances of the Mesa driver is from
>> the same tree and have the patch applied.
>
> Does this happen with dEQP on just radeonsi? If not, that definitely
> points in the virglrenderer direction for bugs...
>
>   -ilia
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [ANNOUNCE] mesa 18.2.0-rc5

[Mark Janes]

Hi Andres,

The final blockers have been resolved.  You should be able to make an RC
that passes all Intel validation, if you pick up:

904c2a617d8 * i965/gen7_urb: Re-emit PUSH_CONSTANT_ALLOC on some gen9
d9cf4308cee * i965/screen: Allow modifiers on sRGB formats

Looking forward to the release,

-Mark

Andres Gomez  writes:

> Hello list,
>
> The fifth release candidate for the Mesa 18.2.0 is now available.
>
> As per the issue tracker [1] we still have a number of outstanding bugs
> blocking the release.
>
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=107457
>
> Currently we have:
>  - 23 queued
>  - 11 nominated (outstanding)
>  - and 0 rejected patches
>
>
> We have in the queue:
>
> In Mesa Core we have included a correction to actually expose the
> GL_EXT_robustness extension for GLES.
>
> The GLSL compiler has received a fix preventing an incorrect linking
> error when having allowed unused in blocks with not corresponding out
> blocks in the previous stage. Also, its tests have been made sure that
> they should be executed from the build system check target or, at the
> very least, with an explicitly versioned python executable.
>
> AMD's drivers have received multiple fixes, including one to correct
> some rendering with radv for Super Mario Sunshine with the Dolphin
> emulator and another one detected due to a segmentation fault in Rise
> of the Tomb Raider.
>
> Intel's drivers have also received multiple fixes, including one to
> correct a GPU hang in DOOM 2016 running under wine.
>
> The documentation has also gotten a couple of fixes, to note 0.8.0 as
> the minimum required mako version, and to add 3 more features into the
> 18.2's release notes.
>
> Finally, from build and integration point of view, we have multiple
> fixes for meson, correcting EGL's compilation, making the GLSL tests to
> explicitly run with python, and to actually load translation files.
>
>
> Take a look at section "Mesa stable queue" for more information.
>
>
> Testing reports/general approval
> 
>
> Any testing reports (or general approval of the state of the branch)
> will be greatly appreciated.
>
> The plan is to have the sixth release candidate for 18.2.0 next
> Wednesday (2018/09/05), around or shortly after 18:00 EEST.
>
> If you have any questions or suggestions - be that about the current
> patch queue or otherwise, please go ahead.
>
>
> Trivial merge conflicts
> ---
>
> commit f958837964784315c1dc633f41f1ef5d2a17aea9
> Author: Dylan Baker 
>
> meson: Actually load translation files
>
> (cherry picked from commit 7c00db9527245d80cb748ec3442163585a5463a6)
>
> commit f6dccf66865c31b13f48b50891a9f5a0d9949b1c
> Author: Emil Velikov 
>
> glsl: remove execute bit and shebang from python tests
>
> (cherry picked from commit 48820ed8da0ad50d51a58f26e156d82b685492e2)
>
>
> Br,
> Andres
>
>
> Mesa stable queue
> -
>
> Nominated (11)
> ==
>
> Andrii Simiklit (1):
>   1b0df8a4602 i965/gen6/xfb: handle case where transform feedback is not 
> active
>
> Bas Nieuwenhuizen (1):
>   4738b6ac814 radv: Add missing checks in 
> radv_get_image_format_properties.
>
> Jason Ekstrand (6):
>   4ffb575da59 vulkan/alloc: Add a vk_strdup helper
>   8c048af5890 anv: Copy the appliation info into the instance
>   c92a463d234 anv: Claim to support depthBounds for ID games
>   cdea5d996ed anv: Free the app and engine name
>   116b47fe3c0 nir/algebraic: Be more careful converting ushr to 
> extract_u8/16
>   7cdf8f93390 nir/format_convert: Fix a bitmask in unpack_11f11f10f
>
> Lionel Landwerlin (1):
>   5a1c23d1502 anv: blorp: support multiple aspect blits
>
> Marek Olšák (1):
>   1e40f694831 ac/surface: fix CMASK fast clear for NPOT textures with 
> mipmapping on SI/CI/VI
>
> Tapani Pälli (1):
>   a72dbc461bd mesa: allow GL_UNSIGNED_BYTE type for SNORM reads
>
>
> Queued (23)
> ===
>
> Andres Gomez (1):
>   Update version to 18.2.0-rc5
>
> Dylan Baker (1):
>   meson: Actually load translation files
>
> Emil Velikov (2):
>   docs: update required mako version
>   glsl: remove execute bit and shebang from python tests
>
> Grazvydas Ignotas (1):
>   radv: place pointer length into cache uuid
>
> Gurchetan Singh (2):
>   meson: fix egl build for surfaceless
>   meson: fix egl build for android
>
> Jason Ekstrand (4):
>   anv: Fill holes in the VF VUE to zero
>   intel/decoder: Clean up field iteration and fix sub-dword fields
>   intel/batch_decoder: Fix dynamic state printing
>   intel/batch_decoder: Print blend states properly
>
> Lionel Landwerlin (2):
>   intel: decoder: unify MI_BB_START field naming
>   intel: decoder: handle 0 sized structs
>
> Marek Olšák (2):
>   ac: completely remove +auto-waitcnt-before-barrier
>   glapi: actually implement GL_EXT_robustness for GLES
>

Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering

[Mark Janes]

Kevin,

Your email messages are missing the in-reply-to header, making this
thread completely incomprehensible on the mailing list.

Please fix your mail client to include the standard headers so we can
follow what you are saying.

-Mark

"Rogovin, Kevin"  writes:

> Hi,
>
>> Having the same underlying compiler intrinsic and having the same behavior
>> are not the same thing.  The INTEL extension allows strictly more
>> functionality than the ARB extension so it needs even more testing.  In
>> particular, it allows you to do the barrier in non-uniform control-flow
>> which is a very different thing than at the top of a shader like is allowed
>> by ARB_fragment_shader_interlock.  I expect the INTEL extension to need
>> substantially more testing than the ARB one.
>
> Just an FYI: the Mesa implementation for the ARB (and thus NV)
> extension accept shaders where the begin function takes place inside
> of control flow actually.
>
>  -Kevin
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering

[Mark Janes]

"Manolova, Plamena"  writes:

> Hi Mark,
> AFAIK there is no piglit test for this specific extension. However
> underneath the hood it reuses the functionality of
> ARB_fragment_shader_interlock, which has a test. I believe the only
> major difference between the two extensions is that unlike
> beginInvocationInterlockARB, beginFragmentShaderOrderingINTEL can be
> called from functions other than main(). If necessary it would be
> pretty straightforward to reuse most of the code for the
> ARB_fragment_shader_interlock test to create one for
> INTEL_fragment_shader_ordering.

Sounds like a good plan!

> Thank you,
> Pam
>
> On Tue, Aug 28, 2018 at 6:41 PM Mark Janes  wrote:
>
>> Is there a piglit test that verifies that this feature works properly?
>>
>>  writes:
>>
>> > From: Kevin Rogovin 
>> >
>> > INTEL_fragment_shader_ordering provides the ability for shaders
>> > to issue a call to gaurnantee memory write operation ordering
>> > of overlapping pixels or samples. In contrast to
>> > ARB_fragment_shader_interlock, INTEL_fragment_shader_ordering
>> > instead of defining a critical region (which must be in main() and
>> > under no flow control) provides a single function that acts like
>> > a memory barrier that can be called under any control flow.
>> >
>> > Kevin Rogovin (2):
>> >   mesa: Add GL/GLSL plumbing for INTEL_fragment_shader_ordering.
>> >   i965: Add INTEL_fragment_shader_ordering support.
>> >
>> >  docs/relnotes/18.3.0.html|  1 +
>> >  src/compiler/glsl/builtin_functions.cpp  | 17 +
>> >  src/compiler/glsl/glsl_parser_extras.cpp |  1 +
>> >  src/compiler/glsl/glsl_parser_extras.h   |  2 ++
>> >  src/compiler/glsl/glsl_to_nir.cpp|  6 ++
>> >  src/compiler/glsl/ir.h   |  1 +
>> >  src/compiler/nir/nir_intrinsics.py   |  1 +
>> >  src/intel/compiler/brw_fs_nir.cpp|  1 +
>> >  src/mesa/drivers/dri/i965/intel_extensions.c |  1 +
>> >  src/mesa/main/extensions_table.h |  1 +
>> >  src/mesa/main/mtypes.h   |  1 +
>> >  11 files changed, 33 insertions(+)
>> >
>> > --
>> > 2.17.1
>> >
>> > ___
>> > mesa-dev mailing list
>> > mesa-dev@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering

[Mark Janes]

Is there a piglit test that verifies that this feature works properly?

 writes:

> From: Kevin Rogovin 
>
> INTEL_fragment_shader_ordering provides the ability for shaders
> to issue a call to gaurnantee memory write operation ordering
> of overlapping pixels or samples. In contrast to
> ARB_fragment_shader_interlock, INTEL_fragment_shader_ordering
> instead of defining a critical region (which must be in main() and
> under no flow control) provides a single function that acts like
> a memory barrier that can be called under any control flow.
>
> Kevin Rogovin (2):
>   mesa: Add GL/GLSL plumbing for INTEL_fragment_shader_ordering.
>   i965: Add INTEL_fragment_shader_ordering support.
>
>  docs/relnotes/18.3.0.html|  1 +
>  src/compiler/glsl/builtin_functions.cpp  | 17 +
>  src/compiler/glsl/glsl_parser_extras.cpp |  1 +
>  src/compiler/glsl/glsl_parser_extras.h   |  2 ++
>  src/compiler/glsl/glsl_to_nir.cpp|  6 ++
>  src/compiler/glsl/ir.h   |  1 +
>  src/compiler/nir/nir_intrinsics.py   |  1 +
>  src/intel/compiler/brw_fs_nir.cpp|  1 +
>  src/mesa/drivers/dri/i965/intel_extensions.c |  1 +
>  src/mesa/main/extensions_table.h |  1 +
>  src/mesa/main/mtypes.h   |  1 +
>  11 files changed, 33 insertions(+)
>
> -- 
> 2.17.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] Revert "intel/nir: Call nir_lower_io_to_scalar_early"

[Mark Janes]

This corrects all of the performance regressions that I can see on our
perf CI.

Please make sure to add the fixes tag before pushing.

Tested-by: Mark Janes 

Jason Ekstrand  writes:

> Commit 4434591bf56a6b0 caused substantially more URB messages in
> geometry and tessellation shaders.  Before we can really enable this
> sort of optimization,  We either need some way of combining them back
> together into vectors or we need to do cross-stage vector element
> elimination without splitting everything into scalars.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107510
> ---
>  src/intel/compiler/brw_nir.c | 17 +
>  1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
> index 31ffbe613ec..29ad68fdb2a 100644
> --- a/src/intel/compiler/brw_nir.c
> +++ b/src/intel/compiler/brw_nir.c
> @@ -713,18 +713,6 @@ brw_nir_link_shaders(const struct brw_compiler *compiler,
> nir_validate_shader(*producer);
> nir_validate_shader(*consumer);
>  
> -   const bool p_is_scalar =
> -  compiler->scalar_stage[(*producer)->info.stage];
> -   const bool c_is_scalar =
> -  compiler->scalar_stage[(*consumer)->info.stage];
> -
> -   if (p_is_scalar && c_is_scalar) {
> -  NIR_PASS_V(*producer, nir_lower_io_to_scalar_early, 
> nir_var_shader_out);
> -  NIR_PASS_V(*consumer, nir_lower_io_to_scalar_early, nir_var_shader_in);
> -  *producer = brw_nir_optimize(*producer, compiler, p_is_scalar);
> -  *consumer = brw_nir_optimize(*consumer, compiler, c_is_scalar);
> -   }
> -
> NIR_PASS_V(*producer, nir_remove_dead_variables, nir_var_shader_out);
> NIR_PASS_V(*consumer, nir_remove_dead_variables, nir_var_shader_in);
>  
> @@ -741,7 +729,12 @@ brw_nir_link_shaders(const struct brw_compiler *compiler,
>NIR_PASS_V(*consumer, nir_lower_indirect_derefs,
>   brw_nir_no_indirect_mask(compiler, 
> (*consumer)->info.stage));
>  
> +  const bool p_is_scalar =
> + compiler->scalar_stage[(*producer)->info.stage];
>*producer = brw_nir_optimize(*producer, compiler, p_is_scalar);
> +
> +  const bool c_is_scalar =
> + compiler->scalar_stage[(*consumer)->info.stage];
>*consumer = brw_nir_optimize(*consumer, compiler, c_is_scalar);
> }
>  }
> -- 
> 2.17.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Mesa-stable] [PATCH] nir/serialize: Alloc constants off the variable

[Mark Janes]

Please add the bugzilla reference for 107366

Tested-by: Mark Janes 

Jason Ekstrand  writes:

> nir_sweep assumes that constants area always allocated off the variable
> to which they belong.  Violating this assumption causes them to get
> freed early and leads to use-after-free bugs.
>
> Cc: mesa-sta...@lists.freedesktop.org
> ---
>  src/compiler/nir/nir_serialize.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/compiler/nir/nir_serialize.c 
> b/src/compiler/nir/nir_serialize.c
> index 6a30738c2d7..43016310048 100644
> --- a/src/compiler/nir/nir_serialize.c
> +++ b/src/compiler/nir/nir_serialize.c
> @@ -124,7 +124,7 @@ read_constant(read_ctx *ctx, nir_variable *nvar)
>  
> blob_copy_bytes(ctx->blob, (uint8_t *)c->values, sizeof(c->values));
> c->num_elements = blob_read_uint32(ctx->blob);
> -   c->elements = ralloc_array(ctx->nir, nir_constant *, c->num_elements);
> +   c->elements = ralloc_array(nvar, nir_constant *, c->num_elements);
> for (unsigned i = 0; i < c->num_elements; i++)
>c->elements[i] = read_constant(ctx, nvar);
>  
> -- 
> 2.17.1
>
> ___
> mesa-stable mailing list
> mesa-sta...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/3] i965: Sweep NIR after linking phase to free held memory

[Mark Janes]

This patch segfaults when both of the following environment variables
are set:

NIR_TEST_CLONE=true
NIR_TEST_SERIALIZE=true

Dylan -- please do not apply this to any stable branch until this bug
has been resolved:

https://bugs.freedesktop.org/show_bug.cgi?id=107366

-Mark

Danylo Piliaiev  writes:

> After optimization passes and many trasfromations most of memory
> NIR holds is a garbage which was being freed only after shader deletion.
> Freeing it at the end of linking will save memory which would be useful
> in case there are a lot of complex shaders being compiled.
> The common case for this issue is 32bit game running under Wine.
>
> The cost of the optimization is around ~3-5% of compilation speed
> with complex shaders.
>
> V2: by Jason Ekstrand
> - Move nir_sweep up, right after the last change of NIR
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103274
>
> Signed-off-by: Danylo Piliaiev 
> ---
>  src/mesa/drivers/dri/i965/brw_link.cpp | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp 
> b/src/mesa/drivers/dri/i965/brw_link.cpp
> index 1071056f14..378426101b 100644
> --- a/src/mesa/drivers/dri/i965/brw_link.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_link.cpp
> @@ -317,6 +317,8 @@ brw_link_shader(struct gl_context *ctx, struct 
> gl_shader_program *shProg)
>NIR_PASS_V(prog->nir, nir_lower_atomics_to_ssbo,
>   prog->nir->info.num_abos);
>  
> +  nir_sweep(prog->nir);
> +
>infos[stage] = >nir->info;
>  
>update_xfb_info(prog->sh.LinkedTransformFeedback, infos[stage]);
> -- 
> 2.17.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] i965/misc: Use depth/stencil surf's tiling on gen4-5

[Mark Janes]

Tested-by: Mark Janes 

Nanley Chery  writes:

> Make the 3D engine aware of the depth/stencil surface's tiling before
> doing any render operations.
>
> Fixes fbe01625f6bf2cef6742e1ff0d3d44a2afec003e
> ("i965/miptree: Share tiling_flags in miptree_create").
>
> Reported-by: Mark Janes 
> ---
>  src/mesa/drivers/dri/i965/brw_misc_state.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c 
> b/src/mesa/drivers/dri/i965/brw_misc_state.c
> index 9a663b1d61c..5cf704ff0e9 100644
> --- a/src/mesa/drivers/dri/i965/brw_misc_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
> @@ -267,6 +267,7 @@ brw_emit_depth_stencil_hiz(struct brw_context *brw,
> uint32_t depthbuffer_format = BRW_DEPTHFORMAT_D32_FLOAT;
> uint32_t depth_offset = 0;
> uint32_t width = 1, height = 1;
> +   bool tiled_surface = true;
>  
> /* If there's a packed depth/stencil bound to stencil only, we need to
>  * emit the packed depth/stencil buffer packet.
> @@ -282,6 +283,7 @@ brw_emit_depth_stencil_hiz(struct brw_context *brw,
>depth_offset = brw->depthstencil.depth_offset;
>width = depth_irb->Base.Base.Width;
>height = depth_irb->Base.Base.Height;
> +  tiled_surface = depth_mt->surf.tiling != ISL_TILING_LINEAR;
> }
>  
> const struct gen_device_info *devinfo = >screen->devinfo;
> @@ -292,7 +294,7 @@ brw_emit_depth_stencil_hiz(struct brw_context *brw,
> OUT_BATCH((depth_mt ? depth_mt->surf.row_pitch - 1 : 0) |
>   (depthbuffer_format << 18) |
>   (BRW_TILEWALK_YMAJOR << 26) |
> - (1 << 27) |
> + (tiled_surface << 27) |
>   (depth_surface_type << 29));
>  
> if (depth_mt) {
> -- 
> 2.18.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [ANNOUNCE] Mesa 18.1.4 release candidate

[Mark Janes]

Oops -- sorry, I was looking at the 18.1 branch, which is already
updated with most (but not all) of the commits in the staging/18.1
branch.

Mark Janes  writes:

> I think you've included a patch that caused a regression, but not the fix
>
> https://bugs.freedesktop.org/show_bug.cgi?id=107193
>
> Dylan Baker  writes:
>
>> Hi List,
>>
>> Mesa 18.1.4 is planned for release this Friday, July 13th, at or around 10 AM
>> PDT.
>>
>> There are currently:
>>  - 27 queued
>>  - 1 nominated
>>  - 0 rejected
>>
>> In the mesa repo, the branch "staging/18.1" in the mesa gitlab will (unless 
>> bugs
>> are found) be rebased into the 18.1 branch for the release on Friday. This 
>> has
>> been run though the Intel CI already and passes there, anyone who wants to 
>> test
>> on other hardware/drivers please do so and report any bugs.
>>
>> Dylan
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [ANNOUNCE] Mesa 18.1.4 release candidate

[Mark Janes]

I think you've included a patch that caused a regression, but not the fix

https://bugs.freedesktop.org/show_bug.cgi?id=107193

Dylan Baker  writes:

> Hi List,
>
> Mesa 18.1.4 is planned for release this Friday, July 13th, at or around 10 AM
> PDT.
>
> There are currently:
>  - 27 queued
>  - 1 nominated
>  - 0 rejected
>
> In the mesa repo, the branch "staging/18.1" in the mesa gitlab will (unless 
> bugs
> are found) be rebased into the 18.1 branch for the release on Friday. This has
> been run though the Intel CI already and passes there, anyone who wants to 
> test
> on other hardware/drivers please do so and report any bugs.
>
> Dylan
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3] i965/gen6/gs: Handle case where a GS doesn't allocate VUE

[Mark Janes]

Tested-by: Mark Janes 

Iago Toral  writes:

> Thanks Andrii, this version looks good to me.
>
> Mark: this change fixes a GPU hang in sandy bridge with geometry
> shaders (the change itself affects a path in the driver that is only
> executed in SNB with GS, so nothing else is affected). While I think
> the change in here is correct according to the PRMs and in fact it
> seems to fix the GPU hang reported in Bugzilla, since this is tinkering
> with the way in which we allocate and free VUEs for SNB GS I believe
> that if this breaks anything it might produce a GPU hang and in that
> case I would rather not hang Jenkins for everyone else until you have a
> chance to restore it, so in order to minimize that risk, could you run
> this through Jenkins when you are available? If that is inconvenient
> for you just let me know and I will send it myself late in my day on
> Monday to minimize the risk.
>
> Thanks,
> Iago
>
> On Fri, 2018-06-22 at 10:59 +0300, Andrii Simiklit wrote:
>> We can not use the VUE Dereference flags combination for EOT
>> message under ILK and SNB because the threads are not initialized
>> there with initial VUE handle unlike Pre-IL.
>> So to avoid GPU hangs on SNB and ILK we need
>> to avoid usage of the VUE Dereference flags combination.
>> (Was tested only on SNB but according to the specification
>> SNB Volume 2 Part 1: 1.6.5.3, 1.6.5.6
>> the ILK must behave itself in the similar way)
>> 
>> v2: Approach to fix this issue was changed.
>> Instead of different EOT flags in the program end
>> we will create VUE every time even if GS produces no output.
>> 
>> v3: Clean up the patch.
>> Signed-off-by: Andrii Simiklit 
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105399
>> 
>> ---
>>  src/intel/compiler/gen6_gs_visitor.cpp | 42 +---
>> --
>>  1 file changed, 21 insertions(+), 21 deletions(-)
>> 
>> diff --git a/src/intel/compiler/gen6_gs_visitor.cpp
>> b/src/intel/compiler/gen6_gs_visitor.cpp
>> index 66c69fb..c9571cc 100644
>> --- a/src/intel/compiler/gen6_gs_visitor.cpp
>> +++ b/src/intel/compiler/gen6_gs_visitor.cpp
>> @@ -350,27 +350,27 @@ gen6_gs_visitor::emit_thread_end()
>> int max_usable_mrf = FIRST_SPILL_MRF(devinfo->gen);
>>  
>> /* Issue the FF_SYNC message and obtain the initial VUE handle.
>> */
>> +   this->current_annotation = "gen6 thread end: ff_sync";
>> +
>> +   vec4_instruction *inst = NULL;
>> +   if (prog->info.has_transform_feedback_varyings) {
>> +  src_reg sol_temp(this, glsl_type::uvec4_type);
>> +  emit(GS_OPCODE_FF_SYNC_SET_PRIMITIVES,
>> +   dst_reg(this->svbi),
>> +   this->vertex_count,
>> +   this->prim_count,
>> +   sol_temp);
>> +  inst = emit(GS_OPCODE_FF_SYNC,
>> +  dst_reg(this->temp), this->prim_count, this-
>> >svbi);
>> +   } else {
>> +  inst = emit(GS_OPCODE_FF_SYNC,
>> +  dst_reg(this->temp), this->prim_count,
>> brw_imm_ud(0u));
>> +   }
>> +   inst->base_mrf = base_mrf;
>> +
>> emit(CMP(dst_null_ud(), this->vertex_count, brw_imm_ud(0u),
>> BRW_CONDITIONAL_G));
>> emit(IF(BRW_PREDICATE_NORMAL));
>> {
>> -  this->current_annotation = "gen6 thread end: ff_sync";
>> -
>> -  vec4_instruction *inst;
>> -  if (prog->info.has_transform_feedback_varyings) {
>> - src_reg sol_temp(this, glsl_type::uvec4_type);
>> - emit(GS_OPCODE_FF_SYNC_SET_PRIMITIVES,
>> -  dst_reg(this->svbi),
>> -  this->vertex_count,
>> -  this->prim_count,
>> -  sol_temp);
>> - inst = emit(GS_OPCODE_FF_SYNC,
>> - dst_reg(this->temp), this->prim_count, this-
>> >svbi);
>> -  } else {
>> - inst = emit(GS_OPCODE_FF_SYNC,
>> - dst_reg(this->temp), this->prim_count,
>> brw_imm_ud(0u));
>> -  }
>> -  inst->base_mrf = base_mrf;
>> -
>>/* Loop over all buffered vertices and emit URB write messages
>> */
>>this->current_annotation = "gen6 thread end: urb writes init";
>>src_reg vertex(this, glsl_type::uint_type);
>> @@ -414,7 +414,7 @@ gen6_gs_visitor::emit_thread_end()
>> dst_reg reg = dst_reg(MRF, mrf);
>> reg.type = output_reg[varying][0].type;
>> data.type = reg.t

Re: [Mesa-dev] [PATCH v3] i965/gen6/gs: Handle case where a GS doesn't allocate VUE

[Mark Janes]

Hi Iago,

It is usually safe to push changes to CI which provoke GPU hangs.
Occasionally, systems do not recover, so I appreciate the extra
notification.

I've sent this commit for testing and will let you know how it goes.

-Mark

Iago Toral  writes:

> Thanks Andrii, this version looks good to me.
>
> Mark: this change fixes a GPU hang in sandy bridge with geometry
> shaders (the change itself affects a path in the driver that is only
> executed in SNB with GS, so nothing else is affected). While I think
> the change in here is correct according to the PRMs and in fact it
> seems to fix the GPU hang reported in Bugzilla, since this is tinkering
> with the way in which we allocate and free VUEs for SNB GS I believe
> that if this breaks anything it might produce a GPU hang and in that
> case I would rather not hang Jenkins for everyone else until you have a
> chance to restore it, so in order to minimize that risk, could you run
> this through Jenkins when you are available? If that is inconvenient
> for you just let me know and I will send it myself late in my day on
> Monday to minimize the risk.
>
> Thanks,
> Iago
>
> On Fri, 2018-06-22 at 10:59 +0300, Andrii Simiklit wrote:
>> We can not use the VUE Dereference flags combination for EOT
>> message under ILK and SNB because the threads are not initialized
>> there with initial VUE handle unlike Pre-IL.
>> So to avoid GPU hangs on SNB and ILK we need
>> to avoid usage of the VUE Dereference flags combination.
>> (Was tested only on SNB but according to the specification
>> SNB Volume 2 Part 1: 1.6.5.3, 1.6.5.6
>> the ILK must behave itself in the similar way)
>> 
>> v2: Approach to fix this issue was changed.
>> Instead of different EOT flags in the program end
>> we will create VUE every time even if GS produces no output.
>> 
>> v3: Clean up the patch.
>> Signed-off-by: Andrii Simiklit 
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105399
>> 
>> ---
>>  src/intel/compiler/gen6_gs_visitor.cpp | 42 +---
>> --
>>  1 file changed, 21 insertions(+), 21 deletions(-)
>> 
>> diff --git a/src/intel/compiler/gen6_gs_visitor.cpp
>> b/src/intel/compiler/gen6_gs_visitor.cpp
>> index 66c69fb..c9571cc 100644
>> --- a/src/intel/compiler/gen6_gs_visitor.cpp
>> +++ b/src/intel/compiler/gen6_gs_visitor.cpp
>> @@ -350,27 +350,27 @@ gen6_gs_visitor::emit_thread_end()
>> int max_usable_mrf = FIRST_SPILL_MRF(devinfo->gen);
>>  
>> /* Issue the FF_SYNC message and obtain the initial VUE handle.
>> */
>> +   this->current_annotation = "gen6 thread end: ff_sync";
>> +
>> +   vec4_instruction *inst = NULL;
>> +   if (prog->info.has_transform_feedback_varyings) {
>> +  src_reg sol_temp(this, glsl_type::uvec4_type);
>> +  emit(GS_OPCODE_FF_SYNC_SET_PRIMITIVES,
>> +   dst_reg(this->svbi),
>> +   this->vertex_count,
>> +   this->prim_count,
>> +   sol_temp);
>> +  inst = emit(GS_OPCODE_FF_SYNC,
>> +  dst_reg(this->temp), this->prim_count, this-
>> >svbi);
>> +   } else {
>> +  inst = emit(GS_OPCODE_FF_SYNC,
>> +  dst_reg(this->temp), this->prim_count,
>> brw_imm_ud(0u));
>> +   }
>> +   inst->base_mrf = base_mrf;
>> +
>> emit(CMP(dst_null_ud(), this->vertex_count, brw_imm_ud(0u),
>> BRW_CONDITIONAL_G));
>> emit(IF(BRW_PREDICATE_NORMAL));
>> {
>> -  this->current_annotation = "gen6 thread end: ff_sync";
>> -
>> -  vec4_instruction *inst;
>> -  if (prog->info.has_transform_feedback_varyings) {
>> - src_reg sol_temp(this, glsl_type::uvec4_type);
>> - emit(GS_OPCODE_FF_SYNC_SET_PRIMITIVES,
>> -  dst_reg(this->svbi),
>> -  this->vertex_count,
>> -  this->prim_count,
>> -  sol_temp);
>> - inst = emit(GS_OPCODE_FF_SYNC,
>> - dst_reg(this->temp), this->prim_count, this-
>> >svbi);
>> -  } else {
>> - inst = emit(GS_OPCODE_FF_SYNC,
>> - dst_reg(this->temp), this->prim_count,
>> brw_imm_ud(0u));
>> -  }
>> -  inst->base_mrf = base_mrf;
>> -
>>/* Loop over all buffered vertices and emit URB write messages
>> */
>>this->current_annotation = "gen6 thread end: urb writes init";
>>src_reg vertex(this, glsl_type::uint_type);
>> @@ -414,7 +414,7 @@ gen6_gs_visitor::emit_thread_end()
>> dst_reg reg = dst_reg(MRF, mrf);
>> reg.type = output_reg[varying][0].type;
>> data.type = reg.type;
>> -   vec4_instruction *inst = emit(MOV(reg, data));
>> +   inst = emit(MOV(reg, data));
>> inst->force_writemask_all = true;
>>  
>> mrf++;
>> @@ -460,7 +460,7 @@ gen6_gs_visitor::emit_thread_end()
>>  *
>>  * However, this would lead us to end the program with an ENDIF
>> opcode,
>>  * which we want to avoid, so what we do is that we always
>> request a new
>> -* VUE handle every time we do 

Re: [Mesa-dev] [Mesa-stable] [PATCH 2/7] intel/isl: Add bounds-checking assertions for the format_info table

[Mark Janes]

This is cc'd to stable, but causes compile errors when to stable
branches:

src/intel/isl/isl_format.c:372:20: error: ‘ISL_NUM_FORMATS’ undeclared
(first use in this function); did you mean ‘ISL_UFLOAT’?

Is it dependent on another patch?

Jason Ekstrand  writes:

> We follow the same convention as isl_format_get_layout in having two
> assertions to ensure that only valid formats are passed in.  We also
> check against the array size of the table because some valid formats
> such as CCS formats will may be past the end of the table.  This fixes
> some potential out-of-bounds array access even in valid cases.
>
> Cc: mesa-sta...@lists.freedesktop.org
> ---
>  src/intel/isl/isl_format.c | 24 
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/src/intel/isl/isl_format.c b/src/intel/isl/isl_format.c
> index 52997cf2ebb..968f981cdee 100644
> --- a/src/intel/isl/isl_format.c
> +++ b/src/intel/isl/isl_format.c
> @@ -374,11 +374,19 @@ format_gen(const struct gen_device_info *devinfo)
> return devinfo->gen * 10 + (devinfo->is_g4x || devinfo->is_haswell) * 5;
>  }
>  
> +static bool
> +format_info_exists(enum isl_format format)
> +{
> +   assert(format != ISL_FORMAT_UNSUPPORTED);
> +   assert(format < ISL_NUM_FORMATS);
> +   return format < ARRAY_SIZE(format_info) && format_info[format].exists;
> +}
> +
>  bool
>  isl_format_supports_rendering(const struct gen_device_info *devinfo,
>enum isl_format format)
>  {
> -   if (!format_info[format].exists)
> +   if (!format_info_exists(format))
>return false;
>  
> return format_gen(devinfo) >= format_info[format].render_target;
> @@ -388,7 +396,7 @@ bool
>  isl_format_supports_alpha_blending(const struct gen_device_info *devinfo,
> enum isl_format format)
>  {
> -   if (!format_info[format].exists)
> +   if (!format_info_exists(format))
>return false;
>  
> return format_gen(devinfo) >= format_info[format].alpha_blend;
> @@ -398,7 +406,7 @@ bool
>  isl_format_supports_sampling(const struct gen_device_info *devinfo,
>   enum isl_format format)
>  {
> -   if (!format_info[format].exists)
> +   if (!format_info_exists(format))
>return false;
>  
> if (devinfo->is_baytrail) {
> @@ -431,7 +439,7 @@ bool
>  isl_format_supports_filtering(const struct gen_device_info *devinfo,
>enum isl_format format)
>  {
> -   if (!format_info[format].exists)
> +   if (!format_info_exists(format))
>return false;
>  
> if (devinfo->is_baytrail) {
> @@ -464,7 +472,7 @@ bool
>  isl_format_supports_vertex_fetch(const struct gen_device_info *devinfo,
>   enum isl_format format)
>  {
> -   if (!format_info[format].exists)
> +   if (!format_info_exists(format))
>return false;
>  
> /* For vertex fetch, Bay Trail supports the same set of formats as Haswell
> @@ -483,7 +491,7 @@ bool
>  isl_format_supports_typed_writes(const struct gen_device_info *devinfo,
>   enum isl_format format)
>  {
> -   if (!format_info[format].exists)
> +   if (!format_info_exists(format))
>return false;
>  
> return format_gen(devinfo) >= format_info[format].typed_write;
> @@ -504,7 +512,7 @@ bool
>  isl_format_supports_typed_reads(const struct gen_device_info *devinfo,
>  enum isl_format format)
>  {
> -   if (!format_info[format].exists)
> +   if (!format_info_exists(format))
>return false;
>  
> return format_gen(devinfo) >= format_info[format].typed_read;
> @@ -542,7 +550,7 @@ bool
>  isl_format_supports_ccs_e(const struct gen_device_info *devinfo,
>enum isl_format format)
>  {
> -   if (!format_info[format].exists)
> +   if (!format_info_exists(format))
>return false;
>  
> /* For simplicity, only report that a format supports CCS_E if blorp can
> -- 
> 2.17.1
>
> ___
> mesa-stable mailing list
> mesa-sta...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Gitlab migration

[Mark Janes]

Daniel Stone  writes:

> On 26 May 2018 at 00:47, Mark Janes  wrote:
>
>> I am eager to make Intel CI results more visible.  Can I push our
>> artifacts to an fd.o directory, and have GitLab display them to users as
>> if the build ran on fd.o?  A typical build produces ~500MB of junit test
>> output with ~4M results.
>
> Not currently, though it is on their radar:
> https://gitlab.com/gitlab-org/gitlab-ce/issues/17081
>
> Even if it were though, I don't think parsing a 500MB XML file
> server-side is a particularly good idea; same goes for Piglit's JSON,
> which is nicely human readable but far too verbose to be usable at
> scale. Think about scaling it out to the hundreds of projects (with
> however many repos) hosted by fd.o as a whole; all the forks thereof,
> the platforms and configurations they care about, and the number of
> developers and submissions. Even if we write off the 1.5 petabytes
> required to store results from this year's Mesa commits (ignoring
> pre-commit submissions) as workable, the server load to parse and
> display those results is just not workable for a large public service.

The files would be much smaller without the XML formatting.  Jenkins
parses the XML server-side whenever the website needs to display
results, which is why their implementation is not scalable.

We need something like Android's Cherry tool that stores results in a
database, and displays them to users:

  https://android.googlesource.com/platform/external/cherry

No one here has looked into how scalable that implementation is.  If
anyone has suggestions on how to provide really large test sets in a web
ui, I'm eager to get advice.

I would like to get *something* like this hosted externally, so
developers can get more out of our CI.

> There are some other things we could do though, such as:
>   - share common build configurations, build those for MRs and commits
> providing full feedback to developers for build errors in various
> configurations, and expose them (for a limited time) as downloadable
> artifacts
>   - have Intel Jenkins reuse those builds in order to run its various
> test suites on real hardware
>   - accept and display summary results (e.g. 'all tests successful' or
> a note of which tests regressed, etc)
>   - a link to download a reasonably-compressed result set stored as an
> artifact (e.g. the last 10MB results.tar.xz I got mailed back from the
> Intel CI is fine)
>
> Does that sound like something reasonable to aim for?

Maybe?  I'm not sure how valuable it is to share compiled binaries
between 2 systems.  Compiling is nearly free for us, but our external
network connection is not as good as I'd like.

We can publish matching test artifacts to your system if you think that
would help.

IMO the tarball is not very useful compared to a mature test result UI.
In particular, viewing the history of test results for a mesa branch is
a common use case for figuring out when regressions were introduced.
Unfortunately, we need a scalable implementation...

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Gitlab migration

[Mark Janes]

Daniel Stone  writes:
> We had a go at using Jenkins for some of this: Intel's been really
> quite successful at doing it internally, but our community efforts
> have been a miserable failure. After a few years I've concluded that
> it's not going to change - even with Jenkins 2.0.
>
> Firstly, Jenkins configuration is an absolute dumpster fire. Working
> out how to configure it and create the right kind of jobs (and debug
> it!) is surprisingly difficult, and involves a lot of clicking through
> the web UI, or using external tools like jenkins-job-builder which
> seem to be in varying levels of disrepair. If you have dedicated 'QA
> people' whose job is driving Jenkins for you, then great! Jenkins will
> probably work well for you. This doesn't scale to a community model
> though. Especially when people have different usecases and need to
> install different plugins.
>
> Jenkins security is also a tyre fire. Plugins are again in varying
> levels of disrepair, and seem remarkably prone to CVEs. There's no
> real good model for updating plugins (and doing so is super fragile).
> Worse still, Jenkins 2.0 really pushes you to be writing scripts in
> Groovy, which can affect Jenkins in totally arbitrary ways, and
> subvert the security model entirely. The way upstream deals with this
> is to enforce a 'sandbox' model preventing most scripts from doing
> anything useful unless manually audited and approved by an admin.
> Again, this is fine for companies or small teams where you trust
> people to not screw up, but doesn't scale to something like fd.o.
>
> Adding to these is the permission model, which again requires painful
> configuration and a lot of admin clicking. It doesn't integrate well
> with external services, and granularity is mostly at an instance
> rather than a project level: again not suitable for something like
> fd.o.
>
> From the UI and workflow perspective, something I've never liked is
> that the first-order view is very specific pipelines, e.g. 'Mesa
> master build', 'daily Piglit run', etc etc. If all you care about is
> master, then this is fine. You _can_ make those pipelines run against
> arbitrary branches and commits you pick up from MRs or similar, but
> you really are trying to jam it sideways into the UI it wants to
> present. Again this is so deeply baked into how Jenkins works that I
> don't see it as really being fixable.
>
> I have a pile of other gripes, like how difficult their remote API is
> to use, and the horrible race conditions it has. For instance, when
> you schedule a run of a particular job, it doesn't report the run ID
> back to you: you have to poll the last job number before you submit,
> then poll again for a few seconds to find the next run ID. Good luck
> to you if two runs of the same job (e.g. 'build specific Mesa commit')
> get scheduled at the same time.

I agree with some of your Jenkins critiques.  I have implemented CI on
*many* different frameworks over the past 15 years, and I think that
every implementation has its fans and haters.

It is wise to create automation which is mostly independent of the CI
framework.  Mesa i965 CI could immediately switch from Jenkins to
BuildBot or GitLab, if there was a reason to do so.  It may be that
GitLab is superior to Jenkins by now, but the selection of the CI
framework is a minor detail anyways.

CI frameworks are often based on build/test pipelines, which I think is
exactly the wrong concept for the domain.  Flexible CI is best thought
of as a multiplatform `make` system.  Setting up a "pipeline" is similar
to building your project with a shell script instead of a makefile.

I disagree with your critique of the Jenkins remote API.  It is more
flexible than any other API that I have seen for CI.  We implement our
multiplatform-make system on top of it.  It would be nice to have an ID
returned when triggering a job, but you can work around by including a
GUID as a build parameter, then polling for the GUID.

The reasons I chose Jenkins over what was available at the time:

  - job/system configuration is saved as XML for backup/diff/restore
  - huge number of users -> fewer quality issues

> GitLab CI fixes all of these things. Pipelines are strongly and
> directly correlated with commits in repositories, though you can also
> trigger them manually or on a schedule. Permissions are that of the
> repository, and just like Travis, people can fork and work on CI
> improvements in their own sandbox without impacting anything else. The
> job configuration is in relatively clean YAML, and it strongly
> suggests idiomatic form rather than a forest of thousands of
> unmaintained plugins.
>
> Jobs get run in clean containers, rather than special unicorn workers
> pre-configured just so, meaning that the builds are totally
> reproducible locally and you can use whatever build dependencies you
> want without having to bug the admins to install LLVM in some
> particular chroot. Those containers can be 

Re: [Mesa-dev] [Mesa-stable] [PATCH 1/2] radv: Fix up 2_10_10_10 alpha sign.

[Mark Janes]

Bas Nieuwenhuizen  writes:

> Pre-Vega HW always interprets the alpha for this format as unsigned,
> so we have to implement a fixup to do the sign correctly for signed
> formats.
>
> CC: 18.0 18.1 

I couldn't apply this to 18.0.  You will probably need to provide a
backport for Juan.

> ---
>  src/amd/vulkan/radv_nir_to_llvm.c | 61 ---
>  src/amd/vulkan/radv_pipeline.c| 30 +--
>  src/amd/vulkan/radv_private.h |  1 +
>  src/amd/vulkan/radv_shader.h  | 12 ++
>  4 files changed, 95 insertions(+), 9 deletions(-)
>
> diff --git a/src/amd/vulkan/radv_nir_to_llvm.c 
> b/src/amd/vulkan/radv_nir_to_llvm.c
> index f98940f0d8f..9d9ecda63cf 100644
> --- a/src/amd/vulkan/radv_nir_to_llvm.c
> +++ b/src/amd/vulkan/radv_nir_to_llvm.c
> @@ -1865,6 +1865,47 @@ static LLVMValueRef radv_get_sampler_desc(struct 
> ac_shader_abi *abi,
>   return ac_build_load_to_sgpr(>ac, list, index);
>  }
>  
> +/* For 2_10_10_10 formats the alpha is handled as unsigned by pre-vega HW.
> + * so we may need to fix it up. */
> +static LLVMValueRef
> +adjust_vertex_fetch_alpha(struct radv_shader_context *ctx,
> +  unsigned adjustment,
> +  LLVMValueRef alpha)
> +{
> + if (adjustment == RADV_ALPHA_ADJUST_NONE)
> + return alpha;
> +
> + LLVMValueRef c30 = LLVMConstInt(ctx->ac.i32, 30, 0);
> +
> + if (adjustment == RADV_ALPHA_ADJUST_SSCALED)
> + alpha = LLVMBuildFPToUI(ctx->ac.builder, alpha, ctx->ac.i32, 
> "");
> + else
> + alpha = LLVMBuildBitCast(ctx->ac.builder, alpha, ctx->ac.i32, 
> "");
> +
> + /* For the integer-like cases, do a natural sign extension.
> + *
> + * For the SNORM case, the values are 0.0, 0.333, 0.666, 1.0
> + * and happen to contain 0, 1, 2, 3 as the two LSBs of the
> + * exponent.
> + */
> + alpha = LLVMBuildShl(ctx->ac.builder, alpha,
> +  adjustment == RADV_ALPHA_ADJUST_SNORM ?
> +  LLVMConstInt(ctx->ac.i32, 7, 0) : c30, "");
> + alpha = LLVMBuildAShr(ctx->ac.builder, alpha, c30, "");
> +
> + /* Convert back to the right type. */
> + if (adjustment == RADV_ALPHA_ADJUST_SNORM) {
> + LLVMValueRef clamp;
> + LLVMValueRef neg_one = LLVMConstReal(ctx->ac.f32, -1.0);
> + alpha = LLVMBuildSIToFP(ctx->ac.builder, alpha, ctx->ac.f32, 
> "");
> + clamp = LLVMBuildFCmp(ctx->ac.builder, LLVMRealULT, alpha, 
> neg_one, "");
> + alpha = LLVMBuildSelect(ctx->ac.builder, clamp, neg_one, alpha, 
> "");
> + } else if (adjustment == RADV_ALPHA_ADJUST_SSCALED) {
> + alpha = LLVMBuildSIToFP(ctx->ac.builder, alpha, ctx->ac.f32, 
> "");
> + }
> +
> + return alpha;
> +}
>  
>  static void
>  handle_vs_input_decl(struct radv_shader_context *ctx,
> @@ -1884,9 +1925,11 @@ handle_vs_input_decl(struct radv_shader_context *ctx,
>  
>   variable->data.driver_location = idx * 4;
>  
> - for (unsigned i = 0; i < attrib_count; ++i, ++idx) {
> - if (ctx->options->key.vs.instance_rate_inputs & (1u << (index + 
> i))) {
> - uint32_t divisor = 
> ctx->options->key.vs.instance_rate_divisors[index + i];
> + for (unsigned i = 0; i < attrib_count; ++i, ++idx, ++index) {
> + LLVMValueRef output[4];
> +
> + if (ctx->options->key.vs.instance_rate_inputs & (1u << index)) {
> + uint32_t divisor = 
> ctx->options->key.vs.instance_rate_divisors[index];
>  
>   if (divisor) {
>   buffer_index = LLVMBuildAdd(ctx->ac.builder, 
> ctx->abi.instance_id,
> @@ -1910,7 +1953,7 @@ handle_vs_input_decl(struct radv_shader_context *ctx,
>   } else
>   buffer_index = LLVMBuildAdd(ctx->ac.builder, 
> ctx->abi.vertex_id,
>   ctx->abi.base_vertex, "");
> - t_offset = LLVMConstInt(ctx->ac.i32, index + i, false);
> + t_offset = LLVMConstInt(ctx->ac.i32, index, false);
>  
>   t_list = ac_build_load_to_sgpr(>ac, t_list_ptr, t_offset);
>  
> @@ -1923,9 +1966,15 @@ handle_vs_input_decl(struct radv_shader_context *ctx,
>  
>   for (unsigned chan = 0; chan < 4; chan++) {
>   LLVMValueRef llvm_chan = LLVMConstInt(ctx->ac.i32, 
> chan, false);
> + output[chan] = LLVMBuildExtractElement(ctx->ac.builder, 
> input, llvm_chan, "");
> + }
> +
> + unsigned alpha_adjust = (ctx->options->key.vs.alpha_adjust >> 
> (index * 2)) & 3;
> + output[3] = adjust_vertex_fetch_alpha(ctx, alpha_adjust, 
> output[3]);
> +
> + for (unsigned chan = 0; chan < 4; chan++) {
>   ctx->inputs[ac_llvm_reg_index_soa(idx, chan)] =
> - 

Re: [Mesa-dev] [PATCH] mesa: revert GL_[SECONDARY_]COLOR_ARRAY_SIZE glGet type to TYPE_INT

[Mark Janes]

Tested-by: Mark Janes <mark.a.ja...@intel.com>

Brian Paul <bri...@vmware.com> writes:

> Since size can be 3, 4 or GL_BGRA we need to keep these glGet types
> as TYPE_INT, not TYPE_UBYTE.
>
> Fixes: d07466fe18522 ("mesa: fix glGetInteger/Float/etc queries for
> vertex arrays attribs")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106462
> cc: mesa-sta...@lists.freedesktop.org
> ---
>  src/mesa/main/get_hash_params.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/main/get_hash_params.py 
> b/src/mesa/main/get_hash_params.py
> index bb7d23a..a97b948 100644
> --- a/src/mesa/main/get_hash_params.py
> +++ b/src/mesa/main/get_hash_params.py
> @@ -219,7 +219,7 @@ descriptor=[
>[ "NORMAL_ARRAY_TYPE", 
> "ARRAY_ENUM16(VertexAttrib[VERT_ATTRIB_NORMAL].Type), NO_EXTRA" ],
>[ "NORMAL_ARRAY_STRIDE", 
> "ARRAY_SHORT(VertexAttrib[VERT_ATTRIB_NORMAL].Stride), NO_EXTRA" ],
>[ "COLOR_ARRAY", "ARRAY_BOOL(VertexAttrib[VERT_ATTRIB_COLOR0].Enabled), 
> NO_EXTRA" ],
> -  [ "COLOR_ARRAY_SIZE", "LOC_CUSTOM, TYPE_UBYTE, 0, NO_EXTRA" ],
> +  [ "COLOR_ARRAY_SIZE", "LOC_CUSTOM, TYPE_INT, 0, NO_EXTRA" ],
>[ "COLOR_ARRAY_TYPE", 
> "ARRAY_ENUM16(VertexAttrib[VERT_ATTRIB_COLOR0].Type), NO_EXTRA" ],
>[ "COLOR_ARRAY_STRIDE", 
> "ARRAY_SHORT(VertexAttrib[VERT_ATTRIB_COLOR0].Stride), NO_EXTRA" ],
>[ "TEXTURE_COORD_ARRAY", "LOC_CUSTOM, TYPE_BOOLEAN, offsetof(struct 
> gl_array_attributes, Enabled), NO_EXTRA" ],
> @@ -825,7 +825,7 @@ descriptor=[
>[ "SECONDARY_COLOR_ARRAY", 
> "ARRAY_BOOL(VertexAttrib[VERT_ATTRIB_COLOR1].Enabled), NO_EXTRA" ],
>[ "SECONDARY_COLOR_ARRAY_TYPE", 
> "ARRAY_ENUM16(VertexAttrib[VERT_ATTRIB_COLOR1].Type), NO_EXTRA" ],
>[ "SECONDARY_COLOR_ARRAY_STRIDE", 
> "ARRAY_SHORT(VertexAttrib[VERT_ATTRIB_COLOR1].Stride), NO_EXTRA" ],
> -  [ "SECONDARY_COLOR_ARRAY_SIZE", "LOC_CUSTOM, TYPE_UBYTE, 0, NO_EXTRA" ],
> +  [ "SECONDARY_COLOR_ARRAY_SIZE", "LOC_CUSTOM, TYPE_INT, 0, NO_EXTRA" ],
>  
>  # GL_EXT_fog_coord
>[ "CURRENT_FOG_COORDINATE", 
> "CONTEXT_FLOAT(Current.Attrib[VERT_ATTRIB_FOG][0]), extra_flush_current" ],
> -- 
> 2.7.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Mesa-stable] [PATCH 2/2] eg/compute: Drop reference to kernel_param bo in destructor

[Mark Janes]

Dylan Baker <dy...@pnwbakers.com> writes:

> I have both pulled into the 18.1-proposed tree now. I think we need to have a
> wider discussion about better ways to propose patches to stable after the fact
> like ea1fff4416036066cff51826f95b4703d7211008. Thanks for helping get this
> resolved so quickly.

We have to expect that stable annotations will be missed for some
patches.  No one is perfect.  The patch author could help a lot by
making sure the commit gets into the correct stable branches, because
they are the ones with the most awareness of the situation.

> Dylan
>
> Quoting Jan Vesely (2018-05-08 10:24:53)
>> Hi,
>> 
>> the code_bo line was added in ea1fff4416036066cff51826f95b4703d7211008
>> Which was also requested for stable [0].
>> sorry for the confusion. Is there a way to indicate dependencies that I
>> missed?
>> 
>> regards,
>> Jan
>> 
>> [0] https://lists.freedesktop.org/archives/mesa-stable/2018-May/008249.
>> html
>> 
>> On Tue, 2018-05-08 at 10:10 -0700, Dylan Baker wrote:
>> > Indeed. This is currently not queued for 18.1 as it doesn't apply cleanly. 
>> > The
>> > 18.1-proposed branch at git://people.freedesktop.org/~dbaker/mesa is where 
>> > it
>> > needs to apply if that's what needs to happen.
>> > 
>> > Thanks,
>> > Dylan
>> > 
>> > Quoting Mark Janes (2018-05-08 09:28:24)
>> > > Hi Jan,
>> > > 
>> > > 
>> > > Jan Vesely <jan.ves...@rutgers.edu> writes:
>> > > 
>> > > > CC: <mesa-sta...@lists.freedesktop.org>
>> > > > Signed-off-by: Jan Vesely <jan.ves...@rutgers.edu>
>> > > > ---
>> > > >  src/gallium/drivers/r600/evergreen_compute.c | 1 +
>> > > >  1 file changed, 1 insertion(+)
>> > > > 
>> > > > diff --git a/src/gallium/drivers/r600/evergreen_compute.c 
>> > > > b/src/gallium/drivers/r600/evergreen_compute.c
>> > > > index 027930b586..5070243914 100644
>> > > > --- a/src/gallium/drivers/r600/evergreen_compute.c
>> > > > +++ b/src/gallium/drivers/r600/evergreen_compute.c
>> > > > @@ -463,6 +463,7 @@ static void evergreen_delete_compute_state(struct 
>> > > > pipe_context *ctx, void *state
>> > > >  #ifdef HAVE_OPENCL
>> > > >   radeon_shader_binary_clean(>binary);
>> > > >   pipe_resource_reference(>code_bo, NULL);
>> > > 
>> > > The stable branches do not have this ^^^ pipe_resource_reference call,
>> > > so the patch does not apply.  Can you make a proper backport of the fix
>> > > to clarify your intentions for stable?
>> > > 
>> > > > + pipe_resource_reference(>kernel_param, NULL);
>> > > >  #endif
>> > > >   r600_destroy_shader(>bc);
>> > > >   }
>> > > > -- 
>> > > > 2.17.0
>> > > > 
>> > > > ___
>> > > > mesa-stable mailing list
>> > > > mesa-sta...@lists.freedesktop.org
>> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-stable
>> > > 
>> > > ___
>> > > mesa-stable mailing list
>> > > mesa-sta...@lists.freedesktop.org
>> > > https://lists.freedesktop.org/mailman/listinfo/mesa-stable
> ___
> mesa-stable mailing list
> mesa-sta...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Mesa-stable] [PATCH 2/2] eg/compute: Drop reference to kernel_param bo in destructor

[Mark Janes]

Hi Jan,


Jan Vesely  writes:

> CC: 
> Signed-off-by: Jan Vesely 
> ---
>  src/gallium/drivers/r600/evergreen_compute.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/gallium/drivers/r600/evergreen_compute.c 
> b/src/gallium/drivers/r600/evergreen_compute.c
> index 027930b586..5070243914 100644
> --- a/src/gallium/drivers/r600/evergreen_compute.c
> +++ b/src/gallium/drivers/r600/evergreen_compute.c
> @@ -463,6 +463,7 @@ static void evergreen_delete_compute_state(struct 
> pipe_context *ctx, void *state
>  #ifdef HAVE_OPENCL
>   radeon_shader_binary_clean(>binary);
>   pipe_resource_reference(>code_bo, NULL);

The stable branches do not have this ^^^ pipe_resource_reference call,
so the patch does not apply.  Can you make a proper backport of the fix
to clarify your intentions for stable?

> + pipe_resource_reference(>kernel_param, NULL);
>  #endif
>   r600_destroy_shader(>bc);
>   }
> -- 
> 2.17.0
>
> ___
> mesa-stable mailing list
> mesa-sta...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/4] anv/device: expose shaderInt16 support in gen8+

[Mark Janes]

Patches 1-4 are

Tested-by: Mark Janes <mark.a.ja...@intel.com>

Iago Toral Quiroga <ito...@igalia.com> writes:

> This rollbacks the revert of this patch introduced with
> commit 7cf284f18e6774c810ed6db17b98e597bf96f8a5.
> ---
>  src/intel/vulkan/anv_device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index 0563eae5c1..fd516fb846 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -759,7 +759,7 @@ void anv_GetPhysicalDeviceFeatures(
>
> pdevice->info.has_64bit_types,
>.shaderInt64  = pdevice->info.gen >= 8 &&
>
> pdevice->info.has_64bit_types,
> -  .shaderInt16  = false,
> +  .shaderInt16  = pdevice->info.gen >= 8,
>.shaderResourceMinLod = false,
>.variableMultisampleRate  = true,
>.inheritedQueries = true,
> -- 
> 2.14.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 00/18] anv: add shaderInt16 support

[Mark Janes]

Clayton Craft  writes:

> Quoting Iago Toral Quiroga (2018-04-30 07:18:08)
>> This version addresses the feedback received to v1, which includes moving the
>> bit-size lowering pass from intel to core NIR (patch 8) and a separate patch
>> to add Intel's specific configuration for int16 (patch 9), and then it also
>> adds a few things that were missing in the first version, namely, a fix for
>> 16-bit comparisons to emit 32-bit booleans (patch 10 -a patch to optimize the
>> resulting code will come later-) and 16-bit pack/unpack which is needed for
>> 16-bit bitcasts (patches 11-15).
>> 
> Since this patch series was merged, we are seeing a number of failures in CI 
> on
> BSW, GLK, and BXT platforms:
>
> dEQP-VK.spirv_assembly.instruction.compute.sconvert.int16_to_int64
> dEQP-VK.spirv_assembly.instruction.compute.uconvert.uint16_to_uint64
> dEQP-VK.spirv_assembly.instruction.compute.sconvert.int16_to_uint64
>

I've reverted this patch, due to:
 https://bugs.freedesktop.org/show_bug.cgi?id=106393

We can't execute CI tests due to that bug, so I couldn't leave the patch
in place.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vcn: fix mpeg4 msg buffer settings

[Mark Janes]

Leo Liu  writes:

> Reviewed-by: Leo Liu 
>
> And Cc Mesa-stable as well.

Please include stable tags in your commit message, eg:

Cc: 18.0 18.1 

> On 2018-04-24 04:49 PM, boyuan.zh...@amd.com wrote:
>> From: Boyuan Zhang 
>>
>> Previous bit-fields assignments are incorrect and will result certain mpeg4
>> decode failed due to wrong flag values. This patch fixes these assignments.
>>
>> Signed-off-by: Boyuan Zhang 
>> ---
>>   src/gallium/drivers/radeon/radeon_vcn_dec.c | 18 +-
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/gallium/drivers/radeon/radeon_vcn_dec.c 
>> b/src/gallium/drivers/radeon/radeon_vcn_dec.c
>> index f83e9e5..4bc922d 100644
>> --- a/src/gallium/drivers/radeon/radeon_vcn_dec.c
>> +++ b/src/gallium/drivers/radeon/radeon_vcn_dec.c
>> @@ -554,15 +554,15 @@ static rvcn_dec_message_mpeg4_asp_vld_t 
>> get_mpeg4_msg(struct radeon_decoder *dec
>>   
>>  result.vop_time_increment_resolution = 
>> pic->vop_time_increment_resolution;
>>   
>> -result.short_video_header |= pic->short_video_header << 0;
>> -result.interlaced |= pic->interlaced << 2;
>> -result.load_intra_quant_mat |= 1 << 3;
>> -result.load_nonintra_quant_mat |= 1 << 4;
>> -result.quarter_sample |= pic->quarter_sample << 5;
>> -result.complexity_estimation_disable |= 1 << 6;
>> -result.resync_marker_disable |= pic->resync_marker_disable << 7;
>> -result.newpred_enable |= 0 << 10; //
>> -result.reduced_resolution_vop_enable |= 0 << 11;
>> +result.short_video_header = pic->short_video_header;
>> +result.interlaced = pic->interlaced;
>> +result.load_intra_quant_mat = 1;
>> +result.load_nonintra_quant_mat = 1;
>> +result.quarter_sample = pic->quarter_sample;
>> +result.complexity_estimation_disable = 1;
>> +result.resync_marker_disable = pic->resync_marker_disable;
>> +result.newpred_enable = 0;
>> +result.reduced_resolution_vop_enable = 0;
>>   
>>  result.quant_type = pic->quant_type;
>>   
>
> ___
> mesa-stable mailing list
> mesa-sta...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/4] intel/compiler: Add scheduler deps for instructions that implicitly read g0

[Mark Janes]

Ian Romanick <i...@freedesktop.org> writes:

> On 04/23/2018 09:19 AM, Mark Janes wrote:
>> I enabled these tests, and could not make them fail in CI.
>
> Can I consider that a Tested-by?

Sorry, I should have been more explicit.  Yes, tested-by.

>> This bug may also be related:
>> 
>> https://bugs.freedesktop.org/show_bug.cgi?id=95009
>
> If none of these tests fail with this patch, I'm included to close them
> with a note to re-open if the failures return.
>
> I also plan to nominate this for stable.  I forgot to do that when I
> sent the patch originally.
>
>> Ian Romanick <i...@freedesktop.org> writes:
>> 
>>> From: Ian Romanick <ian.d.roman...@intel.com>
>>>
>>> Otherwise the scheduler can move the writes after the reads.
>>>
>>> Signed-off-by: Ian Romanick <ian.d.roman...@intel.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95012
>>> Cc: Mark Janes <mark.a.ja...@intel.com>
>>> Cc: Clayton A Craft <clayton.a.cr...@intel.com>
>>> ---
>>> I'd really like to have this run on the CI with the tests from bugzill
>>> #95012 re-enabled.  I don't know how to do this on my own.  This series
>>> is also available at:
>>>
>>> https://cgit.freedesktop.org/~idr/mesa/log/?h=bug-95012.
>>>
>>>  src/intel/compiler/brw_ir_vec4.h | 25 
>>> 
>>>  src/intel/compiler/brw_schedule_instructions.cpp |  3 +++
>>>  2 files changed, 28 insertions(+)
>>>
>>> diff --git a/src/intel/compiler/brw_ir_vec4.h 
>>> b/src/intel/compiler/brw_ir_vec4.h
>>> index 95c5119c6c0..e401d8b4d16 100644
>>> --- a/src/intel/compiler/brw_ir_vec4.h
>>> +++ b/src/intel/compiler/brw_ir_vec4.h
>>> @@ -334,6 +334,31 @@ public:
>>>opcode != BRW_OPCODE_IF &&
>>>opcode != BRW_OPCODE_WHILE));
>>> }
>>> +
>>> +   bool reads_g0_implicitly() const
>>> +   {
>>> +  switch (opcode) {
>>> +  case SHADER_OPCODE_TEX:
>>> +  case SHADER_OPCODE_TXL:
>>> +  case SHADER_OPCODE_TXD:
>>> +  case SHADER_OPCODE_TXF:
>>> +  case SHADER_OPCODE_TXF_CMS_W:
>>> +  case SHADER_OPCODE_TXF_CMS:
>>> +  case SHADER_OPCODE_TXF_MCS:
>>> +  case SHADER_OPCODE_TXS:
>>> +  case SHADER_OPCODE_TG4:
>>> +  case SHADER_OPCODE_TG4_OFFSET:
>>> +  case SHADER_OPCODE_SAMPLEINFO:
>>> +  case VS_OPCODE_PULL_CONSTANT_LOAD:
>>> +  case GS_OPCODE_SET_PRIMITIVE_ID:
>>> +  case GS_OPCODE_GET_INSTANCE_ID:
>>> +  case SHADER_OPCODE_GEN4_SCRATCH_READ:
>>> +  case SHADER_OPCODE_GEN4_SCRATCH_WRITE:
>>> + return true;
>>> +  default:
>>> + return false;
>>> +  }
>>> +   }
>>>  };
>>>  
>>>  /**
>>> diff --git a/src/intel/compiler/brw_schedule_instructions.cpp 
>>> b/src/intel/compiler/brw_schedule_instructions.cpp
>>> index fa85045de74..f817142a8b5 100644
>>> --- a/src/intel/compiler/brw_schedule_instructions.cpp
>>> +++ b/src/intel/compiler/brw_schedule_instructions.cpp
>>> @@ -1267,6 +1267,9 @@ vec4_instruction_scheduler::calculate_deps()
>>>   }
>>>}
>>>  
>>> +  if (inst->reads_g0_implicitly())
>>> + add_dep(last_fixed_grf_write, n);
>>> +
>>>if (!inst->is_send_from_grf()) {
>>>   for (int i = 0; i < inst->mlen; i++) {
>>>  /* It looks like the MRF regs are released in the send
>>> -- 
>>> 2.14.3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/4] intel/compiler: Add scheduler deps for instructions that implicitly read g0

[Mark Janes]

I enabled these tests, and could not make them fail in CI.

This bug may also be related:

https://bugs.freedesktop.org/show_bug.cgi?id=95009

Ian Romanick <i...@freedesktop.org> writes:

> From: Ian Romanick <ian.d.roman...@intel.com>
>
> Otherwise the scheduler can move the writes after the reads.
>
> Signed-off-by: Ian Romanick <ian.d.roman...@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95012
> Cc: Mark Janes <mark.a.ja...@intel.com>
> Cc: Clayton A Craft <clayton.a.cr...@intel.com>
> ---
> I'd really like to have this run on the CI with the tests from bugzill
> #95012 re-enabled.  I don't know how to do this on my own.  This series
> is also available at:
>
> https://cgit.freedesktop.org/~idr/mesa/log/?h=bug-95012.
>
>  src/intel/compiler/brw_ir_vec4.h | 25 
> 
>  src/intel/compiler/brw_schedule_instructions.cpp |  3 +++
>  2 files changed, 28 insertions(+)
>
> diff --git a/src/intel/compiler/brw_ir_vec4.h 
> b/src/intel/compiler/brw_ir_vec4.h
> index 95c5119c6c0..e401d8b4d16 100644
> --- a/src/intel/compiler/brw_ir_vec4.h
> +++ b/src/intel/compiler/brw_ir_vec4.h
> @@ -334,6 +334,31 @@ public:
>opcode != BRW_OPCODE_IF &&
>opcode != BRW_OPCODE_WHILE));
> }
> +
> +   bool reads_g0_implicitly() const
> +   {
> +  switch (opcode) {
> +  case SHADER_OPCODE_TEX:
> +  case SHADER_OPCODE_TXL:
> +  case SHADER_OPCODE_TXD:
> +  case SHADER_OPCODE_TXF:
> +  case SHADER_OPCODE_TXF_CMS_W:
> +  case SHADER_OPCODE_TXF_CMS:
> +  case SHADER_OPCODE_TXF_MCS:
> +  case SHADER_OPCODE_TXS:
> +  case SHADER_OPCODE_TG4:
> +  case SHADER_OPCODE_TG4_OFFSET:
> +  case SHADER_OPCODE_SAMPLEINFO:
> +  case VS_OPCODE_PULL_CONSTANT_LOAD:
> +  case GS_OPCODE_SET_PRIMITIVE_ID:
> +  case GS_OPCODE_GET_INSTANCE_ID:
> +  case SHADER_OPCODE_GEN4_SCRATCH_READ:
> +  case SHADER_OPCODE_GEN4_SCRATCH_WRITE:
> + return true;
> +  default:
> + return false;
> +  }
> +   }
>  };
>  
>  /**
> diff --git a/src/intel/compiler/brw_schedule_instructions.cpp 
> b/src/intel/compiler/brw_schedule_instructions.cpp
> index fa85045de74..f817142a8b5 100644
> --- a/src/intel/compiler/brw_schedule_instructions.cpp
> +++ b/src/intel/compiler/brw_schedule_instructions.cpp
> @@ -1267,6 +1267,9 @@ vec4_instruction_scheduler::calculate_deps()
>   }
>}
>  
> +  if (inst->reads_g0_implicitly())
> + add_dep(last_fixed_grf_write, n);
> +
>if (!inst->is_send_from_grf()) {
>   for (int i = 0; i < inst->mlen; i++) {
>  /* It looks like the MRF regs are released in the send
> -- 
> 2.14.3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radv: fix radv_layout_dcc_compressed() when image doesn't have DCC

[Mark Janes]

Hi Samuel,

When this patch is applied directly to the 18.0 branch, I get a mesa
compile error:

radv_image.c:1175:9: error: implicit declaration of function
‘radv_image_has_dcc’; did you mean ‘radv_image_alloc_dcc’?

Is there another patch that this is dependent on?

-Mark

Samuel Pitoiset  writes:

> num_dcc_levels means that DCC is supported, but this doesn't
> mean that it's enabled by the driver. Instead, we should rely
> on radv_image_has_dcc().
>
> This fixes some multisample regressions since 0babc8e5d66
> ("radv: fix picking the method for resolve subpass") on Vega.
> This is because the resolve method changed from HW to FS, but
> those fails are totally unexpected, so there might some
> differences between Polaris and Vega here.
>
> Fixes: 44fcf587445 ("radv: Disable DCC for GENERAL layout and compute 
> transfer dest.")
> Signed-off-by: Samuel Pitoiset 
> ---
>  src/amd/vulkan/radv_image.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/amd/vulkan/radv_image.c b/src/amd/vulkan/radv_image.c
> index acb569203d4..a14e7c18b29 100644
> --- a/src/amd/vulkan/radv_image.c
> +++ b/src/amd/vulkan/radv_image.c
> @@ -1241,7 +1241,7 @@ bool radv_layout_dcc_compressed(const struct radv_image 
> *image,
>   (queue_mask & (1u << RADV_QUEUE_COMPUTE)))
>   return false;
>  
> - return image->surface.num_dcc_levels > 0 && layout != 
> VK_IMAGE_LAYOUT_GENERAL;
> + return radv_image_has_dcc(image) && layout != VK_IMAGE_LAYOUT_GENERAL;
>  }
>  
>  
> -- 
> 2.17.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Mesa-stable] [PATCH 1/1] i965: return the fourcc saved in __DRIimage when possible

[Mark Janes]

Hi James,

Would it be possible to create a piglit test to exercise the bug
referenced in your commit message?  If possible, we want to prevent
future regressions for this use case.

thanks,

Mark


James Xiong  writes:

> From: "Xiong, James" 
>
> When creating a image from a texture, the image's dri_format is
> set to the first plane's format, and used to look up for the
> fourcc. e.g. for FOURCC_NV12 texture, the dri_format is set to
> __DRI_IMAGE_FORMAT_R8, we end up with a wrong entry in function
> intel_lookup_fourcc():
>{ __DRI_IMAGE_FOURCC_R8, __DRI_IMAGE_COMPONENTS_R, 1,
>  { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 }, } },
> instead of the corret one:
>{ __DRI_IMAGE_FOURCC_NV12, __DRI_IMAGE_COMPONENTS_Y_UV, 2,
>  { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>{ 1, 1, 1, __DRI_IMAGE_FORMAT_GR88, 2 } } },
> as a result, a wrong fourcc __DRI_IMAGE_FOURCC_R8 was returned.
>
> To fix this bug, the image inherits the texture's planar_format that
> has the original fourcc; Upon querying, if planar_format is set,
> return the saved fourcc; Otherwise fall back to the old way.
>
> v3: add a bug description and "cc mesa-stable" tag (Jason)
>   remove abandunt null pointer check (Tapani)
>   squash 2 patches into one (James)
> v2: fall back to intel_lookup_fourcc() when planar_format is NULL
>   (Dongwon & Matt Roper)
>
> Cc: mesa-sta...@lists.freedesktop.org
> Signed-off-by: Xiong, James 
> ---
>  src/mesa/drivers/dri/i965/intel_screen.c | 13 ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
> b/src/mesa/drivers/dri/i965/intel_screen.c
> index dcb98da..29cb7ad 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -388,10 +388,16 @@ intel_image_format_lookup(int fourcc)
> return NULL;
>  }
>  
> -static boolean intel_lookup_fourcc(int dri_format, int *fourcc)
> +static boolean
> +intel_image_get_fourcc(__DRIimage *image, int *fourcc)
>  {
> +   if (image->planar_format) {
> +  *fourcc = image->planar_format->fourcc;
> +  return true;
> +   }
> +
> for (unsigned i = 0; i < ARRAY_SIZE(intel_image_formats); i++) {
> -  if (intel_image_formats[i].planes[0].dri_format == dri_format) {
> +  if (intel_image_formats[i].planes[0].dri_format == image->dri_format) {
>   *fourcc = intel_image_formats[i].fourcc;
>   return true;
>}
> @@ -578,6 +584,7 @@ intel_create_image_from_texture(__DRIcontext *context, 
> int target,
> intel_setup_image_from_mipmap_tree(brw, image, iobj->mt, level, zoffset);
> image->dri_format = driGLFormatToImageFormat(image->format);
> image->has_depthstencil = iobj->mt->stencil_mt? true : false;
> +   image->planar_format = iobj->planar_format;
> if (image->dri_format == MESA_FORMAT_NONE) {
>*error = __DRI_IMAGE_ERROR_BAD_PARAMETER;
>free(image);
> @@ -869,7 +876,7 @@ intel_query_image(__DRIimage *image, int attrib, int 
> *value)
> case __DRI_IMAGE_ATTRIB_FD:
>return !brw_bo_gem_export_to_prime(image->bo, value);
> case __DRI_IMAGE_ATTRIB_FOURCC:
> -  return intel_lookup_fourcc(image->dri_format, value);
> +  return intel_image_get_fourcc(image, value);
> case __DRI_IMAGE_ATTRIB_NUM_PLANES:
>if (isl_drm_modifier_has_aux(image->modifier)) {
>   assert(!image->planar_format || image->planar_format->nplanes == 1);
> -- 
> 2.7.4
>
> ___
> mesa-stable mailing list
> mesa-sta...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function

[Mark Janes]

Emil Velikov <emil.l.veli...@gmail.com> writes:

> (Dropping Leo, since it doesn't affect him. He's already subscribed to
> the list.)
>
> On 6 April 2018 at 19:20, Mark Janes <mark.a.ja...@intel.com> wrote:
>
>> I agree with you, however our release process still has a gap.  We
>> (Intel) test commits on master, and file bugs when we find them in i965
>> or other components.
>>
>> If those commits already have a stable tag in the commit message, they
>> will be shipped at a later date directly to customers, with no testing.
>> There is no way to blacklist broken patches in our Mesa's release
>> automation.
>>
> That's why I mentioned that the process cannot be fully automated ;-)
>
> Let me try to explain slightly differently. Amongst others you want:
>
> a) 24h (ish) buffer (getting closer to 0, as we reach the pre-release
> announcement) before landing fix in the stable branch.
>
> We had broken _badly_ a few multiple times, a balance between the two
> is essential.
>
> Looking at it from Jenkins POV:
> You don't want to test/bisect that master is broken, only to apply
> same patch and run Jenkins on the same broken patch.
>
>  - when issues to happen for example: fdo#103626 currently there's two
> ways to handle it
>
> 1) add the commit to bin/.cherry-ignore. latter of which means that
> you miss the patch when it's actually fixed up.
> See a094314340387ef2463ed8b4ddc9317bc539832b for context.

You are right.  We can just add the commit to .cherry-ignore files in
affected branches when the bug bisects to something with a stable tag.

> 2) carefully/manually git cherry-pick
> Doing this allowed me to add the regression to the tracker, as
> otherwise we would have missed it for 18.0.0 ;-)
>
> Yet we could introduce on-hold list to cherry-ignore. It's fairly trivial.
>
>
> Hope that makes things a bit clearer.
>
> -Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function

[Mark Janes]

Emil Velikov <emil.l.veli...@gmail.com> writes:

> On 5 April 2018 at 20:33, Mark Janes <mark.a.ja...@intel.com> wrote:
>> Emil Velikov <emil.l.veli...@gmail.com> writes:
>>
>>> On 4 April 2018 at 22:50, Mark Janes <mark.a.ja...@intel.com> wrote:
>>>> Leo Liu <leo@amd.com> writes:
>>>>
>>>>> On 04/04/2018 12:40 PM, Mark Janes wrote:
>>>>>> Leo Liu <leo@amd.com> writes:
>>>>>>
>>>>>>> On the CI family, firmware requires the destory command have to be the
>>>>>>> last command in the IB, moving feedback command after destroy is causing
>>>>>>> issues on CI cards, so we have to keep the previous logic that moves
>>>>>>> destroy back to the last command.
>>>>>>>
>>>>>>> But as the original issue fixed previously, with the newer family like 
>>>>>>> Vega10,
>>>>>>> feedback command have to be included inside of the task info command 
>>>>>>> along
>>>>>>> with destroy command.
>>>>>>>
>>>>>>> Fixes: 6d74cb25("radeon/vce: move destroy command before feedback 
>>>>>>> command")
>>>>>>>
>>>>>>> Signed-off-by: Leo Liu <leo@amd.com>
>>>>>>> Cc: mesa-sta...@lists.freedesktop.org
>>>>>> These tags seem ambiguous to me.  If this commit fixes a specific
>>>>>> commit, then the patch should be applied only to stable branches which
>>>>>> contain that commit.
>>>>>>
>>>>>> However, the mesa-stable CC caused this patch to be applied to 17.3,
>>>>>> which does *not* contain the broken patch.
>>>>>>
>>>>>> Leo: did you intend for the mesa-stable CC to cause this patch to be
>>>>>> applied to older stable branches?
>>>>> I would like to have this patch apply to branches "17.2", "17.3",
>>>>> "18.0", which got patch titled "radeon/vce: move destroy command before
>>>>> feedback command"
>>>>
>>>> Ok, I understand now.  You cc'd a buggy patch to stable, and the bug was
>>>> shipped in 17.3.1.
>>>>
>>> May I suggest phrasing things less personally. Mistakes happen, so
>>> let's work in providing suggestions for improvement as opposed to "you
>>> did X/Y".
>>
>> Thank you for the feedback.  I was trying to state the facts, but I
>> understand how this could be read as a criticism.
>>
> Does that mean you tested radeon/vce and observed the breakage?
> 
>
>> As you say, mistakes happen -- and when they happen on the stable
>> branches, there is very little to protect the end users.  Could we
>> enhance automation to prevent this situation?  For example:
>>
> Consistent testing/reporting is needed. I believe I've mentioned if before:
>
> You are the only one who consistently provides feedback about the state.
> There have been individuals to report, while I'm very grateful these
> reports are very rare and far between.
>
> Approx 4 years ago Carl suggested another alternative. Roughly put:
>
> Driver specific patches are _omitted_ unless team member has
> explicitly tested them.
> Needless to say plan did not go forward - see the whole thread [1].
>
> One thing that it had in common with recent discussion is a
> tester/frontman/maintainer/etc for each team.
>
> Having such a person alongside the actual testing is optional, yet
> _highly_ recommended.
>
> As you know Intel's team is the largest one, perhaps as big as all the
> others combined.  So expecting the same amount of manpower and time
> dedication is impossible.

I agree with you, however our release process still has a gap.  We
(Intel) test commits on master, and file bugs when we find them in i965
or other components.

If those commits already have a stable tag in the commit message, they
will be shipped at a later date directly to customers, with no testing.
There is no way to blacklist broken patches in our Mesa's release
automation.

> HTH
> Emil
>
> [1] https://lists.freedesktop.org/archives/mesa-dev/2014-July/062992.html
> ___
> mesa-stable mailing list
> mesa-sta...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function

[Mark Janes]

Emil Velikov <emil.l.veli...@gmail.com> writes:

> On 4 April 2018 at 22:50, Mark Janes <mark.a.ja...@intel.com> wrote:
>> Leo Liu <leo@amd.com> writes:
>>
>>> On 04/04/2018 12:40 PM, Mark Janes wrote:
>>>> Leo Liu <leo@amd.com> writes:
>>>>
>>>>> On the CI family, firmware requires the destory command have to be the
>>>>> last command in the IB, moving feedback command after destroy is causing
>>>>> issues on CI cards, so we have to keep the previous logic that moves
>>>>> destroy back to the last command.
>>>>>
>>>>> But as the original issue fixed previously, with the newer family like 
>>>>> Vega10,
>>>>> feedback command have to be included inside of the task info command along
>>>>> with destroy command.
>>>>>
>>>>> Fixes: 6d74cb25("radeon/vce: move destroy command before feedback 
>>>>> command")
>>>>>
>>>>> Signed-off-by: Leo Liu <leo@amd.com>
>>>>> Cc: mesa-sta...@lists.freedesktop.org
>>>> These tags seem ambiguous to me.  If this commit fixes a specific
>>>> commit, then the patch should be applied only to stable branches which
>>>> contain that commit.
>>>>
>>>> However, the mesa-stable CC caused this patch to be applied to 17.3,
>>>> which does *not* contain the broken patch.
>>>>
>>>> Leo: did you intend for the mesa-stable CC to cause this patch to be
>>>> applied to older stable branches?
>>> I would like to have this patch apply to branches "17.2", "17.3",
>>> "18.0", which got patch titled "radeon/vce: move destroy command before
>>> feedback command"
>>
>> Ok, I understand now.  You cc'd a buggy patch to stable, and the bug was
>> shipped in 17.3.1.
>>
> May I suggest phrasing things less personally. Mistakes happen, so
> let's work in providing suggestions for improvement as opposed to "you
> did X/Y".

Thank you for the feedback.  I was trying to state the facts, but I
understand how this could be read as a criticism.

As you say, mistakes happen -- and when they happen on the stable
branches, there is very little to protect the end users.  Could we
enhance automation to prevent this situation?  For example:

 - bin/.cherry-blacklist lists commits that should never be shipped on
   stable.

 - bisected bugs -> update the blacklist

I can't think of a way to automate that, but it would have highlighted
one of the instance where we shipped a regression in a 17.3 point
release.

> Aside from the normal stable/fixes tag, people can nominate patches by
> sending them to the list [1].
> We had patch authors, other developers and even 'random' members of
> the public to use the last method.
>
> HTH
> Emil
>
> https://www.mesa3d.org/submittingpatches.html#nominations
> ___
> mesa-stable mailing list
> mesa-sta...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function

[Mark Janes]

Leo Liu <leo@amd.com> writes:

> On 04/04/2018 12:40 PM, Mark Janes wrote:
>> Leo Liu <leo@amd.com> writes:
>>
>>> On the CI family, firmware requires the destory command have to be the
>>> last command in the IB, moving feedback command after destroy is causing
>>> issues on CI cards, so we have to keep the previous logic that moves
>>> destroy back to the last command.
>>>
>>> But as the original issue fixed previously, with the newer family like 
>>> Vega10,
>>> feedback command have to be included inside of the task info command along
>>> with destroy command.
>>>
>>> Fixes: 6d74cb25("radeon/vce: move destroy command before feedback command")
>>>
>>> Signed-off-by: Leo Liu <leo@amd.com>
>>> Cc: mesa-sta...@lists.freedesktop.org
>> These tags seem ambiguous to me.  If this commit fixes a specific
>> commit, then the patch should be applied only to stable branches which
>> contain that commit.
>>
>> However, the mesa-stable CC caused this patch to be applied to 17.3,
>> which does *not* contain the broken patch.
>>
>> Leo: did you intend for the mesa-stable CC to cause this patch to be
>> applied to older stable branches?
> I would like to have this patch apply to branches "17.2", "17.3", 
> "18.0", which got patch titled "radeon/vce: move destroy command before 
> feedback command"

Ok, I understand now.  You cc'd a buggy patch to stable, and the bug was
shipped in 17.3.1.

> And this Cc-ed patch is to fix "radeon/vce: move destroy command before 
> feedback command"
>
> Thanks,
> Leo
>
>
>>
>> Release managers: is there a protocol for how this specification should
>> be parsed, when considering a patch for stable?
>>
>>> ---
>>>   src/gallium/drivers/radeon/radeon_vce.c|  1 -
>>>   src/gallium/drivers/radeon/radeon_vce_40_2_2.c |  2 ++
>>>   src/gallium/drivers/radeon/radeon_vce_52.c | 18 ++
>>>   3 files changed, 12 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/src/gallium/drivers/radeon/radeon_vce.c 
>>> b/src/gallium/drivers/radeon/radeon_vce.c
>>> index 427bf01ed8..c84103e0ac 100644
>>> --- a/src/gallium/drivers/radeon/radeon_vce.c
>>> +++ b/src/gallium/drivers/radeon/radeon_vce.c
>>> @@ -247,7 +247,6 @@ static void rvce_destroy(struct pipe_video_codec 
>>> *encoder)
>>> enc->fb = 
>>> enc->session(enc);
>>> enc->destroy(enc);
>>> -   enc->feedback(enc);
>>> flush(enc);
>>> si_vid_destroy_buffer();
>>> }
>>> diff --git a/src/gallium/drivers/radeon/radeon_vce_40_2_2.c 
>>> b/src/gallium/drivers/radeon/radeon_vce_40_2_2.c
>>> index f1db47d4bd..04e9d7f5e1 100644
>>> --- a/src/gallium/drivers/radeon/radeon_vce_40_2_2.c
>>> +++ b/src/gallium/drivers/radeon/radeon_vce_40_2_2.c
>>> @@ -421,6 +421,8 @@ static void destroy(struct rvce_encoder *enc)
>>>   {
>>> enc->task_info(enc, 0x0001, 0, 0, 0);
>>>   
>>> +   feedback(enc);
>>> +
>>> RVCE_BEGIN(0x0201); // destroy
>>> RVCE_END();
>>>   }
>>> diff --git a/src/gallium/drivers/radeon/radeon_vce_52.c 
>>> b/src/gallium/drivers/radeon/radeon_vce_52.c
>>> index a941c476f6..421539c4bd 100644
>>> --- a/src/gallium/drivers/radeon/radeon_vce_52.c
>>> +++ b/src/gallium/drivers/radeon/radeon_vce_52.c
>>> @@ -458,14 +458,6 @@ static void config_extension(struct rvce_encoder *enc)
>>> RVCE_END();
>>>   }
>>>   
>>> -static void destroy(struct rvce_encoder *enc)
>>> -{
>>> -   enc->task_info(enc, 0x0001, 0, 0, 0);
>>> -
>>> -   RVCE_BEGIN(0x0201); // destroy
>>> -   RVCE_END();
>>> -}
>>> -
>>>   static void feedback(struct rvce_encoder *enc)
>>>   {
>>> RVCE_BEGIN(0x0505); // feedback buffer
>>> @@ -474,6 +466,16 @@ static void feedback(struct rvce_encoder *enc)
>>> RVCE_END();
>>>   }
>>>   
>>> +static void destroy(struct rvce_encoder *enc)
>>> +{
>>> +   enc->task_info(enc, 0x0001, 0, 0, 0);
>>> +
>>> +   feedback(enc);
>>> +
>>> +   RVCE_BEGIN(0x0201); // destroy
>>> +   RVCE_END();
>>> +}
>>> +
>>>   static void motion_estimation(struct rvce_encoder *enc)
>>>   {
>>> RVCE_BEGIN(0x0407); // motion estimation
>>> -- 
>>> 2.14.1
>>>
>>> ___
>>> mesa-stable mailing list
>>> mesa-sta...@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function

[Mark Janes]

Emil Velikov <emil.l.veli...@gmail.com> writes:

> On 4 April 2018 at 17:40, Mark Janes <mark.a.ja...@intel.com> wrote:
>> Leo Liu <leo@amd.com> writes:
>>
>>> On the CI family, firmware requires the destory command have to be the
>>> last command in the IB, moving feedback command after destroy is causing
>>> issues on CI cards, so we have to keep the previous logic that moves
>>> destroy back to the last command.
>>>
>>> But as the original issue fixed previously, with the newer family like 
>>> Vega10,
>>> feedback command have to be included inside of the task info command along
>>> with destroy command.
>>>
>>> Fixes: 6d74cb25("radeon/vce: move destroy command before feedback command")
>>>
>>> Signed-off-by: Leo Liu <leo@amd.com>
>>> Cc: mesa-sta...@lists.freedesktop.org
>>
>> These tags seem ambiguous to me.  If this commit fixes a specific
>> commit, then the patch should be applied only to stable branches which
>> contain that commit.
>>
>> However, the mesa-stable CC caused this patch to be applied to 17.3,
>> which does *not* contain the broken patch.
>>
>> Leo: did you intend for the mesa-stable CC to cause this patch to be
>> applied to older stable branches?
>>
>> Release managers: is there a protocol for how this specification should
>> be parsed, when considering a patch for stable?
>>
> If the Fixes tag, reference a commit that is missing in specific
> stable branch then obviously the fix is not suitable.
> Hence the stable piece than be ignored + alongside a reply to the
> patch that it will not be in $stable_branch because $reason.

OK, we have violated this policy at least a couple times in the previous
release, based on my audits:

  2f67c9b17518cf0d2fe946e39e5b8ff5ec2797c5
  i965/vec4: Fix null destination register in 3-source instructions

  6f69b63896ce2352aaa25f89287133f7f2ba1fab
  radeon/vce: move feedback command inside of destroy function

> Even if offending commit is not part of $stable_branch, getting into
> the habit and cross-referencing is very beneficial.
> Some customers may use non-stable branch, hence having track of which
> fixes they need is a smart move.
>
> HTH
> Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC] Mesa 17.3.x release problems and process improvements

[Mark Janes]

"Juan A. Suarez Romero" <jasua...@igalia.com> writes:

> On Wed, 2018-04-04 at 10:07 -0700, Mark Janes wrote:
>> Emil Velikov <emil.l.veli...@gmail.com> writes:
>> > In detail:
>> >  * make the patch queue, release date and blockers accessible at any
>> >point in time:
>> > * queued patches can be accessed, via a branch - say wip/17.3,
>> >   wip/18.0, wip/18.1, etc. The branch _will be_ rebased, although
>> >   normally reverts are recommended.
>> 
>> I created a bot that applies commits from master to wip stable branches
>> and tests in CI.  It runs several times a day and identifies patches
>> that do not cherry-pick cleanly.  Branches are here:
>> 
>>  https://github.com/janesma/mesa/tree/wip/17.3
>>  https://github.com/janesma/mesa/tree/wip/18.0
>> 
>> I've sent a couple of mails to developers when their recent patches
>> don't apply.  So far it handles about 85% of the commits containing
>> stable tags without intervention.
>> 
>
> Cool! I was thinking on a similar approach here:
>
> * Everytime a push happens, a job/bot scans the pushed patches, and creates a
> pull request with the stable patches. If some of the patches that does not
> apply, then it sends an email to the authors informing.

IMO, automating messages to developers will generate undesired noise on
mailing lists.  We should have a real person moderate the notifications,
until they prove to be consistently actionable.

> I group all the stable patches in one PR because when a push is done,
> I assume that all the patches belong to the same feature/bugfix, and
> thus it makes sense to deal with them as in one PR.

PRs seem like an unnecessary complication.  My proof-of-concept script
simply applies all commits with stable tags to an arbitrary wip branch,
based on the stable branch policies.

CI triggers from the wip branch as with any developer CI branch.

> * There's a bot that is listening for the PR. Everytime a new PR
> arrives, it starts the proper testing. If test is successful, it
> automatically merges the PR; otherwise it just sends an email
> informing the failure. An important point here is that if a PR is
> under testing, then the bot waits until the test finishes and the PR
> under testing is either merged or rejected. If it is merged, the bot
> rebases the new PR and starts the test. This way, we guarantee the
> test is done with a version that won't change after while the test is
> happening. If you are more interested, I was thinking on using Marge
> bot for this.

Fewer/smaller dependencies is better.  What does gitlab + marge buy us
over a single script?

To be clear, my script is gross at the moment.  I just hacked to get
something that would match the stable policy, and provide immediate
feedback when commits are pushed.  It could be cleaned up and shared if
anyone wants it.

>   J.A.
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC] Mesa 17.3.x release problems and process improvements

[Mark Janes]

Emil Velikov  writes:

> Hi all,
>
> Having gone through the thread a few times, I believe it can be
> summarised as follows:
>  * Greater transparency is needed.
>  * Subsystem/team maintainers.
>  * Unfit and late nominations.
>  * Developers/everyone should be more involved.
>  * Greater automation must be explored.
>
>
> NOTES:
>  * Some of the details are not listed in the thread, but have been
>raised in one form or another.
>  * The details focuses more on the goals, than the actual means.
>  * Above said, some details may have been missed - I'm mere human.
>
>
> In detail:
>  * make the patch queue, release date and blockers accessible at any
>point in time:
> * queued patches can be accessed, via a branch - say wip/17.3,
>   wip/18.0, wip/18.1, etc. The branch _will be_ rebased, although
>   normally reverts are recommended.

I created a bot that applies commits from master to wip stable branches
and tests in CI.  It runs several times a day and identifies patches
that do not cherry-pick cleanly.  Branches are here:

 https://github.com/janesma/mesa/tree/wip/17.3
 https://github.com/janesma/mesa/tree/wip/18.0

I've sent a couple of mails to developers when their recent patches
don't apply.  So far it handles about 85% of the commits containing
stable tags without intervention.

> * rejected patches must be listed alongside the reason why and
>   author+reviewer must be informed (email & IRC?) ASAP.
>* we already document and track those in .cherry-ignore. can we
>  reuse that?
>
> * patches with trivial conflicts can be merged to the wip branch
>   after another release manager, or patch author/reviewer has
>   confirmed the changes.
>
> * patches that require backports will be rejected. usual rejection
>   procedure applies (described above).
>
> * if there is delay due to extra testing time or otherwise, the
>   release manager must list the blocking issues and ETA must be
>   provided. ETA must be updated before it's reached. it may be
>   worth having the ETA and rejections in a single place - inside
>   the wip/ branch, html page, elsewhere.
>
> * the current metabug with release blockers must be made more
>   obvious.
>
> * release manager can contact Phoronix and/or similar media to
>   publicise expected delays, blockers or seek request for testing.
>
>
>  * teams are encouraged to have one or multiple maintainers. some of
>the goals of having such people include:
> * individuals that have greater interaction with the team and
>   knowledge about the team plans. rough examples include:
>* backport/bug is needed, yet person is not available - on a
>  leave (sick, sabbatical, other) or busy with other things.
>* team has higher priority with details not publicly available.
>
> * can approve unfit or late nominations - see next section.
> * to ensure cover and minimise stress it's encouraged to have
>   multiple maintainers per team and they are rotated regularly.
> * list of maintainers must be documented
>
>
>  * unfit and late nominations:
> * any rejections that are unfit based on the existing criteria can
>   be merged as long as:
>* subsystem specific patches are approved by the team
>  maintainer(s).
>* patches that cover multiple subsystems are approved by 50%+1
>  of the maintainers of the affected subsystems.
>
> * late nominations can be made after the pre-release announcement.
>   they must be approved by the subsystem maintainers up-to X hours
>   before the actual release. approval specifics are identical to the
>   ones listed in 'unfit' section just above.
>
>
>  * developers/everyone should be more involved:
> * with the patch queue accessible at any point, everyone is
>   encouraged to keep an eye open and report issues.
>
> * developers should be more active in providing backports and
>   updating the status of release blocking bugs.
>
> * release managers and team maintainers must check with developer
>   (via email, IRC, other) if no action has been made for X days.
>
> * everyone is encouraged to provide a piglit/dEQP/etc testing
>   summary (via email, attachment, html page., etc). if they do,
>   please ensure that summary consistently available, regardless if
>   there's any regressions or not. if extra time is needed reply to
>   the list informing release managers
>
> * in case of regressions bisection must be provided.
>
>
>  * testing - pre and post merge, automation:
>
>NOTE: implementation specifics is up-to each team, with goals of:
>a) results must be accessible reasonably easy
>b) high level documentation of the setup and contact points are
>   documented
>
> * with over 120 developers contributing to mesa, ambiguous patch
>   nominations will always exist.

Re: [Mesa-dev] [Mesa-stable] [PATCH] radeon/vce: move feedback command inside of destroy function

[Mark Janes]

Leo Liu  writes:

> On the CI family, firmware requires the destory command have to be the
> last command in the IB, moving feedback command after destroy is causing
> issues on CI cards, so we have to keep the previous logic that moves
> destroy back to the last command.
>
> But as the original issue fixed previously, with the newer family like Vega10,
> feedback command have to be included inside of the task info command along
> with destroy command.
>
> Fixes: 6d74cb25("radeon/vce: move destroy command before feedback command")
>
> Signed-off-by: Leo Liu 
> Cc: mesa-sta...@lists.freedesktop.org

These tags seem ambiguous to me.  If this commit fixes a specific
commit, then the patch should be applied only to stable branches which
contain that commit.

However, the mesa-stable CC caused this patch to be applied to 17.3,
which does *not* contain the broken patch.

Leo: did you intend for the mesa-stable CC to cause this patch to be
applied to older stable branches?

Release managers: is there a protocol for how this specification should
be parsed, when considering a patch for stable?

> ---
>  src/gallium/drivers/radeon/radeon_vce.c|  1 -
>  src/gallium/drivers/radeon/radeon_vce_40_2_2.c |  2 ++
>  src/gallium/drivers/radeon/radeon_vce_52.c | 18 ++
>  3 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/src/gallium/drivers/radeon/radeon_vce.c 
> b/src/gallium/drivers/radeon/radeon_vce.c
> index 427bf01ed8..c84103e0ac 100644
> --- a/src/gallium/drivers/radeon/radeon_vce.c
> +++ b/src/gallium/drivers/radeon/radeon_vce.c
> @@ -247,7 +247,6 @@ static void rvce_destroy(struct pipe_video_codec *encoder)
>   enc->fb = 
>   enc->session(enc);
>   enc->destroy(enc);
> - enc->feedback(enc);
>   flush(enc);
>   si_vid_destroy_buffer();
>   }
> diff --git a/src/gallium/drivers/radeon/radeon_vce_40_2_2.c 
> b/src/gallium/drivers/radeon/radeon_vce_40_2_2.c
> index f1db47d4bd..04e9d7f5e1 100644
> --- a/src/gallium/drivers/radeon/radeon_vce_40_2_2.c
> +++ b/src/gallium/drivers/radeon/radeon_vce_40_2_2.c
> @@ -421,6 +421,8 @@ static void destroy(struct rvce_encoder *enc)
>  {
>   enc->task_info(enc, 0x0001, 0, 0, 0);
>  
> + feedback(enc);
> +
>   RVCE_BEGIN(0x0201); // destroy
>   RVCE_END();
>  }
> diff --git a/src/gallium/drivers/radeon/radeon_vce_52.c 
> b/src/gallium/drivers/radeon/radeon_vce_52.c
> index a941c476f6..421539c4bd 100644
> --- a/src/gallium/drivers/radeon/radeon_vce_52.c
> +++ b/src/gallium/drivers/radeon/radeon_vce_52.c
> @@ -458,14 +458,6 @@ static void config_extension(struct rvce_encoder *enc)
>   RVCE_END();
>  }
>  
> -static void destroy(struct rvce_encoder *enc)
> -{
> - enc->task_info(enc, 0x0001, 0, 0, 0);
> -
> - RVCE_BEGIN(0x0201); // destroy
> - RVCE_END();
> -}
> -
>  static void feedback(struct rvce_encoder *enc)
>  {
>   RVCE_BEGIN(0x0505); // feedback buffer
> @@ -474,6 +466,16 @@ static void feedback(struct rvce_encoder *enc)
>   RVCE_END();
>  }
>  
> +static void destroy(struct rvce_encoder *enc)
> +{
> + enc->task_info(enc, 0x0001, 0, 0, 0);
> +
> + feedback(enc);
> +
> + RVCE_BEGIN(0x0201); // destroy
> + RVCE_END();
> +}
> +
>  static void motion_estimation(struct rvce_encoder *enc)
>  {
>   RVCE_BEGIN(0x0407); // motion estimation
> -- 
> 2.14.1
>
> ___
> mesa-stable mailing list
> mesa-sta...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

[Mark Janes]

This patch passes Intel's CI suites.
It needs a CC for stable in the commit message.

Tested-by: Mark Janes <mark.a.ja...@intel.com>

Sergii Romantsov <sergii.romant...@gmail.com> writes:

> Gen8+ use 48-bit address relocations so need to extend the sign
> to 64-bit return value. Without it we have higher bits zeroed
> and missing the negavive values.
> Haswell and older use 32-bit deltas so are unaffected by this issue.
>
> v2:
>   used int32_t fucntion parameter instead of explicit type conversion.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101408
> Signed-off-by: Sergii Romantsov <sergii.romant...@globallogic.com>
> Tested-by: Andriy Khulap <andriy.khu...@globallogic.com>
> ---
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index ebc02ff..7286140 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -1079,7 +1079,7 @@ brw_batch_references(struct intel_batchbuffer *batch, 
> struct brw_bo *bo)
>  static uint64_t
>  emit_reloc(struct intel_batchbuffer *batch,
> struct brw_reloc_list *rlist, uint32_t offset,
> -   struct brw_bo *target, uint32_t target_offset,
> +   struct brw_bo *target, int32_t target_offset,
> unsigned int reloc_flags)
>  {
> assert(target != NULL);
> -- 
> 2.7.4
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] mesa: ensure that variable is initialized

[Mark Janes]

Tested-by: Mark Janes <mark.a.ja...@intel.com>

Dylan Baker <dy...@pnwbakers.com> writes:

> locally I've added:
> Fixes: 16f6634e7fb5ada308e55b852cd49251e7f3f8b1 
>("mesa/program: Link SPIR-V shaders using the SPIR-V code-path")
>
> Quoting Dylan Baker (2018-04-02 15:36:01)
>> Otherwise this variable will only be set if there are spirv shaders
>> present, but it's used regardless, resulting in undefined behavior.
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105820
>> CC: Alejandro Piñeiro <apinhe...@igalia.com>
>> CC: Mark Janes <mark.a.ja...@intel.com>
>> Signed-off-by: Dylan Baker <dylan.c.ba...@intel.com>
>> ---
>>  src/mesa/program/ir_to_mesa.cpp | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/src/mesa/program/ir_to_mesa.cpp 
>> b/src/mesa/program/ir_to_mesa.cpp
>> index 49ef5ea52d8..60fb24bf664 100644
>> --- a/src/mesa/program/ir_to_mesa.cpp
>> +++ b/src/mesa/program/ir_to_mesa.cpp
>> @@ -3126,7 +3126,7 @@ void
>>  _mesa_glsl_link_shader(struct gl_context *ctx, struct gl_shader_program 
>> *prog)
>>  {
>> unsigned int i;
>> -   bool spirv;
>> +   bool spirv = false;
>>  
>> _mesa_clear_shader_program_data(ctx, prog);
>>  
>> -- 
>> 2.16.3
>> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2 v2] nir: mako all the intrinsics

[Mark Janes]

Rob Herring  writes:

> On Wed, Mar 28, 2018 at 10:18 AM, Rob Clark  wrote:
>> On Wed, Mar 28, 2018 at 10:43 AM, Rob Herring  wrote:
>>> On Sun, Mar 25, 2018 at 1:10 PM, Rob Clark  wrote:
 I threatened to do this a long time ago.. I probably *should* have done
 it a long time ago when there where many fewer intrinsics.  But the
 system of macro/#include magic for dealing with intrinsics is a bit
 annoying, and python has the nice property of optional fxn params,
 making it possible to define new intrinsics while ignoring parameters
 that are not applicable (and naming optional params).  And not having to
 specify various array lengths explicitly is nice too.

 I think the end result makes it easier to add new intrinsics.

 v2: couple small fixes found with a test program to compare the old and
 new tables
 v3: misc comments, don't rely on capture=true for meson.build, get rid
 of system_values table to avoid return value of intrinsic() and
 *mostly* remove side-effects, add autotools build support

 Signed-off-by: Rob Clark 
 ---
 So, new scheme is, I think, a reasonable compromise between keeping the
 python "clean" and keeping the intrinsic declarations easy to follow.
 It still has the side-effect that intrinsic() adds to the table, but
 drops the separate system_values table so that intrinsic() doesn't
 return a value.  The alternative would require the helper for various
 specialized intrinsic categories to be declared far from where they are
 used, which is, I think, suboptimal.  And it keeps intrinsic() and
 various wrappers pretty straightforward, so I don't think this should
 ever pose a problem for refactoring (and certainly less of a problem
 than the previous solution using cpp macros, so regardless of what your
 opinion about the py code, I guess anyone could agree that this is an
 improvement over the current state ;-))

 Also added autotools build support.  Sorry scons and android.  (Are we
 ready to drop either of these in favor of nir?)
>>>
>>> You mean meson? For Android, no. I don't see that happening anytime
>>> soon. I looked into it some by having a prebuilt target in Android.mk
>>> that calls meson. The problem is getting all the Android build
>>> environment such as include paths out of Android build system and
>>> passed into meson. I don't know how to do that in a way that is not
>>> manual and fragile.
>>>
>>> It looks like you'd just need to do some copy-n-paste of rules for
>>> Android. And you know you can push an 'android/*' branch to trigger an
>>> Android build of mesa?
>>>
>>
>> no, I didn't realize that.. on the main git tree?
>
> Yep. No one uses it AFAICT.

I was told that this mechanism was not useful because it builds with
-Werror.  Is that still true?

Clayton implemented a buildtest for android within the i965 CI, so
anyone testing there will be notified when they break android.  We are
waiting on some additional hardware before enabling it for developer
branches.

> Rob
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] clover/llvm: Fix build against LLVM/Clang 4.0

[Mark Janes]

This patch fixes the clover build for Clang 4.0, which is what the Intel
CI uses.

Tested-by: Mark Janes <mark.a.ja...@intel.com>

Aaron Watry <awa...@gmail.com> writes:

> The opencl 1.0 langstandard was renamed in 5.0+
>
> Cc: Mark Janes <mark.a.ja...@intel.com>
> ---
>  src/gallium/state_trackers/clover/llvm/invocation.cpp | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp 
> b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> index af78c2ae28..2fb3ce2365 100644
> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> @@ -85,7 +85,11 @@ namespace {
>  };
>  
>  const clc_version_lang_std cl_version_lang_stds[] = {
> +#if HAVE_LLVM >= 0x0500
> { 100, clang::LangStandard::lang_opencl10},
> +#else
> +   { 100, clang::LangStandard::lang_opencl},
> +#endif
> { 110, clang::LangStandard::lang_opencl11},
> { 120, clang::LangStandard::lang_opencl12},
> { 200, clang::LangStandard::lang_opencl20},
> -- 
> 2.14.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 5/5] clover: Dynamically calculate __OPENCL_VERSION__ and CLC language version

[Mark Janes]

Aaron, this patch breaks the meson build-test in our CI:

 ../src/gallium/state_trackers/clover/llvm/invocation.cpp:88:36: error: 
‘lang_opencl10’ is not a member of ‘clang::LangStandard’
 { 100, clang::LangStandard::lang_opencl10},

configured with:

meson -Dbuild-tests=true 
-Dgallium-drivers=r300,r600,radeonsi,nouveau,swrast,swr,freedreno,vc4,pl111,etnaviv,imx,svga,virgl
 -Dgallium-vdpau=true -Dgallium-xvmc=true -Dgallium-xa=true -Dgallium-va=true 
-Dgallium-nine=true -Dgallium-opencl=standalone -Dgallium-omx=bellagio

Pierre Moreau  writes:

> Oops, sorry.
>
> Reviewed-by: Pierre Moreau 
>
> Thanks again for the series!
> Pierre
>
> On 2018-03-20 — 20:23, Aaron Watry wrote:
>> ping.
>> 
>> This is the last of the series that still needs review.
>> 
>> --Aaron
>> 
>> On Thu, Mar 1, 2018 at 1:39 PM, Aaron Watry  wrote:
>> > Use get_language_version to calculate default cl standard based on
>> > device capabilities and -cl-std specified in build options.
>> >
>> > v4: Squash the __OPENCL_VERSION__ and CLC language version patches
>> > v3: (Jan) Allow device_version up to 2.2 while device_clc_version
>> > only goes to 2.0
>> > Use get_cl_version to calculate version instead
>> > v2: Split out from the previous patch (Pierre)
>> >
>> > Signed-off-by: Aaron Watry 
>> > CC: Pierre Moreau 
>> > CC: Jan Vesely 
>> > ---
>> >  src/gallium/state_trackers/clover/llvm/invocation.cpp | 6 --
>> >  1 file changed, 4 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp 
>> > b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> > index 8d76f203de..f146695585 100644
>> > --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> > +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> > @@ -194,7 +194,7 @@ namespace {
>> >compat::set_lang_defaults(c->getInvocation(), c->getLangOpts(),
>> >  compat::ik_opencl, 
>> > ::llvm::Triple(target.triple),
>> >  c->getPreprocessorOpts(),
>> > -clang::LangStandard::lang_opencl11);
>> > +get_language_version(opts, 
>> > device_clc_version));
>> >
>> >c->createDiagnostics(new clang::TextDiagnosticPrinter(
>> >*new raw_string_ostream(r_log),
>> > @@ -225,7 +225,9 @@ namespace {
>> >c.getPreprocessorOpts().Includes.push_back("clc/clc.h");
>> >
>> >// Add definition for the OpenCL version
>> > -  c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=110");
>> > +  c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=" +
>> > +  std::to_string(get_cl_version(
>> > +  dev.device_version()).version_number));
>> >
>> >// clc.h requires that this macro be defined:
>> >
>> > c.getPreprocessorOpts().addMacroDef("cl_clang_storage_class_specifiers");
>> > --
>> > 2.14.1
>> >
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3] i965: android: pass the correct header name to the python script

[Mark Janes]

I tested this, and still got an error:

FAILED: 
out/target/product/androidia_64/gen/SHARED_LIBRARIES/i965_dri_intermediates/brw_oa_metrics.c
 
/bin/bash -c "python 
vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa.py 
--code=out/target/product/androidia_64/gen/SHARED_LIBRARIES/i965_dri_intermediates/brw_oa_metrics.c
 
--header=out/target/common/obj/PACKAGING/boot-jars-package-check_intermediates/brw_oa_metrics.h
 vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa_hsw.xml 
vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa_bdw.xml 
vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa_chv.xml 
vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa_sklgt2.xml
 
vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa_sklgt3.xml
 
vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa_sklgt4.xml
 vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa_bxt.xml 
vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa_kblgt2.xml
 
vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa_kblgt3.xml
 vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa_glk.xml 
vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa_cflgt2.xml
 
vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa_cflgt3.xml"
Traceback (most recent call last):
  File 
"vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa.py", 
line 734, in 
main()
  File 
"vendor/intel/external/android_ia/mesa/src/mesa/drivers/dri/i965/brw_oa.py", 
line 561, in main
header_file = open(args.header, 'w')
IOError: [Errno 2] No such file or directory: 
'out/target/common/obj/PACKAGING/boot-jars-package-check_intermediates/brw_oa_metrics.h'

Emil Velikov  writes:

> v2: Pass the actual filename instead of $(word ...) magic
> v3: Drop duplicate $(i965_oa_xml_FILES)
>
> Fixes: 2d2b15fbcab ("i965: fix autotools/android build
> Cc: Lionel Landwerlin 
> Cc: Clayton Craft 
> Signed-off-by: Emil Velikov 
> ---
>  src/mesa/drivers/dri/i965/Android.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/Android.mk 
> b/src/mesa/drivers/dri/i965/Android.mk
> index 8c4a613bcf3..a1738a8afa2 100644
> --- a/src/mesa/drivers/dri/i965/Android.mk
> +++ b/src/mesa/drivers/dri/i965/Android.mk
> @@ -315,7 +315,7 @@ i965_oa_xml_FILES := $(addprefix $(LOCAL_PATH)/, \
>  $(intermediates)/brw_oa_metrics.c: $(LOCAL_PATH)/brw_oa.py 
> $(i965_oa_xml_FILES)
>   @echo "target Generated: $(PRIVATE_MODULE) <= $(notdir $(@))"
>   @mkdir -p $(dir $@)
> - $(hide) $(MESA_PYTHON2) $< --code=$@ $(i965_oa_xml_FILES) --header=$@ 
> $(i965_oa_xml_FILES)
> + $(hide) $(MESA_PYTHON2) $< --code=$@ 
> --header=$(intermediates)/brw_oa_metrics.h $(i965_oa_xml_FILES)
>  
>  $(intermediates)/brw_oa_metrics.h: $(intermediates)/brw_oa_metrics.c
>  
> -- 
> 2.16.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [ANNOUNCE] Mesa 17.3.7 release candidate

[Mark Janes]

"Juan A. Suarez Romero"  writes:

> On Fri, 2018-03-16 at 12:03 -0400, Ilia Mirkin wrote:
>> On Fri, Mar 16, 2018 at 11:52 AM, Juan A. Suarez Romero
>>  wrote:
>> > On Fri, 2018-03-16 at 11:38 -0400, Ilia Mirkin wrote:
>> > > On Fri, Mar 16, 2018 at 11:27 AM, Juan A. Suarez Romero
>> > >  wrote:
>> > > > On Fri, 2018-03-16 at 09:51 -0400, Ilia Mirkin wrote:
>> > > > > On Fri, Mar 16, 2018 at 8:40 AM, Juan A. Suarez Romero
>> > > > >  wrote:
>> > > > > > Nominated means that these patches does not enter in this release 
>> > > > > > due they
>> > > > > > arrived a bit late, but they are proposed to cherry-pick them for 
>> > > > > > the next
>> > > > > > release (in 1 or 2 weeks).
>> > > > > > 
>> > > > > > The reason is that some days before this pre-announcement is sent, 
>> > > > > > we close the
>> > > > > > list of patches that enter in the release, and we do a lot of 
>> > > > > > testing to verify
>> > > > > > nothing is broken). If there's some regression, we just try to fix 
>> > > > > > them. And
>> > > > > > when everything is ready, we send the pre-announcement email.
>> > > > > > 
>> > > > > > The nominated patches are those that arrive after we close the 
>> > > > > > list, and we are
>> > > > > > under the testing process. As we don't want to restart the full 
>> > > > > > process again
>> > > > > > and again, we just nominate them for the next release. Otherwise 
>> > > > > > that would
>> > > > > > delay the release too much.
>> > > > > 
>> > > > > Why not send the pre-announcement right when it's closed? Since your
>> > > > > testing doesn't cover all drivers, shouldn't everyone just be able to
>> > > > > test at the same time, and then be able to add to the existing list
>> > > > > with additional fixes (or removals of picked commits)?
>> > > > > 
>> > > > 
>> > > > 
>> > > > Because we want to propose a release candidate that has been tested as 
>> > > > much as
>> > > > possible, to avoid bothering people with a proposal that we need to 
>> > > > change
>> > > > because it is causing regressions (and believe me this is quite 
>> > > > common). So we
>> > > > do different tests, thanks to Intel CI which covers a lot of tests and
>> > > > configurations, before doing the pre-announcement.
>> > > > 
>> > > > Note that between we start the testing and do the pre-announcement 
>> > > > there is a
>> > > > difference of few hours, and hence the list of nominated patches is 
>> > > > quite
>> > > > reduced (either none, or a couple of them). This time wasn't the case, 
>> > > > and it
>> > > > took some days. But it is not the usual case.
>> > > 
>> > > Hm. It feels like it's the usual case. Perhaps it's not meant to be.
>> > > Why not just pick up all the nominated patches right before you do the
>> > > couple hours of testing?
>> > > 
>> > 
>> > That is what we do. We pick up all the patches until the last minute, just
>> > before starting the test. And as soon as we verify it is correct, we write 
>> > and
>> > send the pre-announcement email.
>> 
>> Then why can it be days in between? If the tests get delayed/messed
>> up/have to be done over again, just pick up all the nominated patches
>> and go again.
>> 
>
> I think that days is very infrequent, and due exceptionally reasons. Usually
> less than a day, and mainly due different time zones.

Examples of issues that can delay CI confirmation of the stable branch:

 - Regressions detected in the proposed release, which need to be
   bisected, fixed, and retested.

 - Intel CI downtime.  This can be due to lab maintenance, or
   instability with systems.

In this release iteration, we encountered both of these issues.  The
first proposed 17.3.7 was broken, as was the first fix after bisect.
Resolving the issue was delayed because we upgraded CI to Linux 4.14 to
support Vulkan 1.1, and that kernel has a bug that impacted our results.

> If we detect issues in the branch under testing, and we need to prepare a new
> one, usually I include all the new stable patches that arrived as part of the
> new queue to test. So we try to test all the nominated patches.
>
> Of course, if a new patch arrives after the testing started, and the test
> success, we just send the announcement, putting the patch as Nominated in the
> email. Because we don't want to start again, as it always arrives patches, and
> we want to avoid delaying the release.
>
>
>> > > > And finally, that is the reason why there is a couple of days between 
>> > > > the pre-
>> > > > announcement and the final announcement: for people to do more tests 
>> > > > with
>> > > > different configurations, and propose inclusions/removals.
>> > > 
>> > > What are the criteria for such inclusions? (Why do the nominated
>> > > patches not meet them?) IME such requests for inclusion are met with
>> > > "next release" replies (or "never").
>> > > 
>> > 
>> > I think this depends on the authors, and how 

Re: [Mesa-dev] [RFC] Mesa 17.3.x release problems and process improvements

[Mark Janes]

Timothy Arceri  writes:

> I'd just like to point out that as an outside user of Intels CI I have 
> missed regressions on a couple of occasions. However this was not due to 
> "Lazy behaviour", having a CI system is fantastic and I'm very grateful 
> to have access to it. However, it's not uncommon to run into issues and 
> have no idea what is going on with the system.
>
> Some examples are getting no emails back from the system after pushing, 
> results that look like a successful run even though things have failed 
> or just no results email at all, results with tonnes and tonnes of fails 
> which are clearly unrelated to my latest branch, on occasion the system 
> seems to have crashed? and been unresponsive for a whole weekend (which 
> means down for me on a Monday in Australia).

There is some down-time that we can't control.  Intel has turned off our
lab for days at a time to perform power upgrades.

In cases where our jenkins automation is at fault, we do work to improve
reliability.

Other failure modes for CI are similar to what happened this past
weekend, where we were forced to update to linux 4.14 which generated
thousands of vulkan failures (fixed in 4.15, and the CI is back).

Would it help to have an announcement list where we can indicate any
current issues affecting CI?  If devs have examples of CI that are more
reliable than ours, I'm eager to learn how to improve.

> If the change is for i965 I either wait or try bug someone at Intel (if 
> anyone happens to be around) to find out what is going on, but for core 
> mesa changes having run piglit on radeonsi locally I tend to push my 
> changes. I get that regressions are frustrating at times but using the 
> CI system as an outside user can be frustrating also when you have no 
> idea whats going on in the black box after pushing a branch, especially 
> when you need to wait an hour or so to try again in between runs.

I hear you.  We are prioritizing better visibility for external users,
and have added staff to implement that.

> I gave feedback at first on ways the system could be better, or errors I 
> seemed to hit but was told there wouldn't be and improvements made for 
> the foreseeable future so I stopped giving feedback and instead switched 
> to relying on my own local testing when the CI system seemed to have 
> lost its mind.
>
> Anyway this is not meant to be a criticism, I just wanted to share my 
> experience as an outside user.

The feedback is appreciated.
 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC] Mesa 17.3.x release problems and process improvements

[Mark Janes]

Daniel Vetter <dan...@ffwll.ch> writes:

> On Tue, Mar 13, 2018 at 4:46 PM, Mark Janes <mark.a.ja...@intel.com> wrote:
>> Daniel Vetter <dan...@ffwll.ch> writes:
>>
>>> On Mon, Mar 12, 2018 at 11:54:45PM -0700, Kenneth Graunke wrote:
>>>> On Friday, March 9, 2018 12:12:28 PM PDT Mark Janes wrote:
>>>> [snip]
>>>> > I've been doing this for Intel.  Developers are on the hook to fix their
>>>> > bugs, but you can't make them do it.  They have many pressures on them,
>>>> > and a maintainer can't make the call as to whether a rendering bug is
>>>> > more important than day-1 vulkan conformance, for example.
>>>> >
>>>> > We could heighten the transparency of what is blocking the build by
>>>> > publicizing the authors of bisected blocking bugs to Phoronix, which
>>>> > might get things moving.
>>>>
>>>> I hope you're being sarcastic here, or else I'm misunderstanding your
>>>> proposal.  Public shaming of developers who create bugs has absolutely
>>>> no place in the Mesa community, IMHO.  It would foster the kind of toxic
>>>> community that none of us want to be a part of.
>>>>
>>>> Sometimes, people who create bugs are the very people that work the
>>>> hardest, who the project may not even exist without.  Would you want
>>>> to chew out someone for creating a bug in a Vulkan driver when...if it
>>>> weren't for that person, you wouldn't have a Vulkan driver at all?  Or,
>>>> maybe they caused a couple bad bugs...but also fixed hundreds of them.
>>>>
>>>> Other times, they're new contributors or volunteers who do this, not as
>>>> their day job.  Frankly, those people are under no obligation to help us
>>>> at all, so we need to thank them and appreciate the time and effort they
>>>> spend - and give them a hand fixing things when they're too busy, or
>>>> don't have the relevant hardware or skill to track down a regression.
>>>>
>>>> It's easy to be pissed off when there are bugs, and things seem to not
>>>> be making progress, but let's try and keep things positive and work
>>>> together to make Mesa the best we can.
>>>
>>> I'd like to second this with my experience from the kernel community. The
>>> public shaming game for when you create a regression is very strong there,
>>> lead by Linus Torvalds. In my experience this directly causes:
>>>
>>> - Maintainers to hide bug reports and regressions reports at all costs,
>>>   because having Linus destroy you just aint never worth it. The meta game
>>>   becomes "avoid getting railed" instead of "deliver quality code", and
>>>   there's lots of ways to easily achieve the former that serious hurt the
>>>   latter.
>>>
>>> - Best practice (in my experience) is to not mention the dreaded
>>>   "REGRESSION" tag when you need another maintainer's help to fix a
>>>   regression, because it's too likely they'll just panic. That means they
>>>   start screaming at you to go away, or brain locks up and they can't
>>>   effectively help you track down the bug (seen both cases).
>>>
>>> - Creates a culture where talking about process/tooling improvements to
>>>   prevent regressions and/or handle them quicker becomes too dangerous,
>>>   because it all turns into a personal shaming game of who maintains the
>>>   worst subsystem.
>>>
>>> Long term you end up with a culture fucked up for good :-/
>>>
>>> Imo the only way to make this better is to try analyzing why a regressions
>>> happened, and fix the tooling to prevent that in the future. Maybe better
>>> test coverage (and long term efforts to fix known gaps), maybe better
>>> presentation of automated checks (stuff like github pull requests that
>>> automatically run CI and report full results, blocking the merge if
>>> anything is amiss).
>>
>> You have to have a very strong CI to use it to block commits.  i965 Mesa
>> has a big CI which identifies many regressions, but I wouldn't want to
>> checkpoint commits in an automated way.  A large pool of obsolete
>> CI hardware will have lower reliability than the mesa master branch --
>> which generates noise for developers and impedes progress.
>
> This was all in general about blaming regressions on people, not
> specifically for the stable-backporting-from-master issue here.
>
> A

Re: [Mesa-dev] [RFC] Mesa 17.3.x release problems and process improvements

[Mark Janes]

Daniel Vetter <dan...@ffwll.ch> writes:

> On Mon, Mar 12, 2018 at 11:54:45PM -0700, Kenneth Graunke wrote:
>> On Friday, March 9, 2018 12:12:28 PM PDT Mark Janes wrote:
>> [snip]
>> > I've been doing this for Intel.  Developers are on the hook to fix their
>> > bugs, but you can't make them do it.  They have many pressures on them,
>> > and a maintainer can't make the call as to whether a rendering bug is
>> > more important than day-1 vulkan conformance, for example.
>> > 
>> > We could heighten the transparency of what is blocking the build by
>> > publicizing the authors of bisected blocking bugs to Phoronix, which
>> > might get things moving.
>> 
>> I hope you're being sarcastic here, or else I'm misunderstanding your
>> proposal.  Public shaming of developers who create bugs has absolutely
>> no place in the Mesa community, IMHO.  It would foster the kind of toxic
>> community that none of us want to be a part of.
>> 
>> Sometimes, people who create bugs are the very people that work the
>> hardest, who the project may not even exist without.  Would you want
>> to chew out someone for creating a bug in a Vulkan driver when...if it
>> weren't for that person, you wouldn't have a Vulkan driver at all?  Or,
>> maybe they caused a couple bad bugs...but also fixed hundreds of them.
>> 
>> Other times, they're new contributors or volunteers who do this, not as
>> their day job.  Frankly, those people are under no obligation to help us
>> at all, so we need to thank them and appreciate the time and effort they
>> spend - and give them a hand fixing things when they're too busy, or
>> don't have the relevant hardware or skill to track down a regression.
>> 
>> It's easy to be pissed off when there are bugs, and things seem to not
>> be making progress, but let's try and keep things positive and work
>> together to make Mesa the best we can.
>
> I'd like to second this with my experience from the kernel community. The
> public shaming game for when you create a regression is very strong there,
> lead by Linus Torvalds. In my experience this directly causes:
>
> - Maintainers to hide bug reports and regressions reports at all costs,
>   because having Linus destroy you just aint never worth it. The meta game
>   becomes "avoid getting railed" instead of "deliver quality code", and
>   there's lots of ways to easily achieve the former that serious hurt the
>   latter.
>
> - Best practice (in my experience) is to not mention the dreaded
>   "REGRESSION" tag when you need another maintainer's help to fix a
>   regression, because it's too likely they'll just panic. That means they
>   start screaming at you to go away, or brain locks up and they can't
>   effectively help you track down the bug (seen both cases).
>
> - Creates a culture where talking about process/tooling improvements to
>   prevent regressions and/or handle them quicker becomes too dangerous,
>   because it all turns into a personal shaming game of who maintains the
>   worst subsystem.
>
> Long term you end up with a culture fucked up for good :-/
>
> Imo the only way to make this better is to try analyzing why a regressions
> happened, and fix the tooling to prevent that in the future. Maybe better
> test coverage (and long term efforts to fix known gaps), maybe better
> presentation of automated checks (stuff like github pull requests that
> automatically run CI and report full results, blocking the merge if
> anything is amiss).

You have to have a very strong CI to use it to block commits.  i965 Mesa
has a big CI which identifies many regressions, but I wouldn't want to
checkpoint commits in an automated way.  A large pool of obsolete
CI hardware will have lower reliability than the mesa master branch --
which generates noise for developers and impedes progress.

> Personally I have high hopes for gitlab.fd.o to enable us to do a lot of
> that automation in a much better and much more discoverable way, but it's
> some ways in the future still. Besides better quality that would also help
> us ramp up new contributors, since instead of unwritten rules they'd not
> just get documented merge criteria, but have a pile of bots that
> interactively walk them through everything (the best projects auto-insert
> a comment from the bot with instructions how to repro results if anything
> fails, with links to further docs).
>
> Assume that people try to do the best and fix the tooling/support
> infrastructure to allow them to, and they will deliver. Blaming them just
> drives them into hiding and looking for better places to have fun.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC] Mesa 17.3.x release problems and process improvements

[Mark Janes]

Kenneth Graunke <kenn...@whitecape.org> writes:

> On Friday, March 9, 2018 12:12:28 PM PDT Mark Janes wrote:
> [snip]
>> I've been doing this for Intel.  Developers are on the hook to fix their
>> bugs, but you can't make them do it.  They have many pressures on them,
>> and a maintainer can't make the call as to whether a rendering bug is
>> more important than day-1 vulkan conformance, for example.
>> 
>> We could heighten the transparency of what is blocking the build by
>> publicizing the authors of bisected blocking bugs to Phoronix, which
>> might get things moving.
>
> I hope you're being sarcastic here, or else I'm misunderstanding your
> proposal.  Public shaming of developers who create bugs has absolutely
> no place in the Mesa community, IMHO.  It would foster the kind of toxic
> community that none of us want to be a part of.

You are correct, it was a stupid thing to write.  I don't have any
*good* ideas for getting the release branch to a point where we can meet
our goals for users.

> Sometimes, people who create bugs are the very people that work the
> hardest, who the project may not even exist without.  Would you want
> to chew out someone for creating a bug in a Vulkan driver when...if it
> weren't for that person, you wouldn't have a Vulkan driver at all?  Or,
> maybe they caused a couple bad bugs...but also fixed hundreds of them.
>
> Other times, they're new contributors or volunteers who do this, not as
> their day job.  Frankly, those people are under no obligation to help us
> at all, so we need to thank them and appreciate the time and effort they
> spend - and give them a hand fixing things when they're too busy, or
> don't have the relevant hardware or skill to track down a regression.
>
> It's easy to be pissed off when there are bugs, and things seem to not
> be making progress, but let's try and keep things positive and work
> together to make Mesa the best we can.

It's true that I spend too much time looking at bugs.  I will often lose
whole days to coping with regressions, and it has made me cranky over
time.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC] Mesa 17.3.x release problems and process improvements

[Mark Janes]

Dylan Baker <dy...@pnwbakers.com> writes:

> Quoting Mark Janes (2018-03-12 12:40:47)
>> Handling a screw-up could be done by maintainers by force-pushing the
>> commits off the WIP branch, and adding some annotations that prevent the
>> broken commit from being re-applied to WIP by automation.
>> 
>
> That sounds like introducing a lot of developer headaches, the kind that make
> people not want to use the system. Take this scenario:
>
> 1. I push patches

In this case the person is a developer, not a release manager

> 2. CI starts
> 3. you push patches

I'll call this person "developer 2" below.

> 4. My CI fails

At this point, developer 1 needs feedback that their patch nearly
created a problem for many end-users.  Frankly, it's unacceptable for
developers to annotate a commit for the stable branches unless they are
confident that it is a *safe and necessary* fix for end-users.  We have
almost zero verification between the developer and millions of users.

> 5. I force-push

A release manager would have to resolve the failure manually, not
developer 1.  Developers can't force-push anything in my proposal.

> Now both of our patches are removed, even though yours haven't gone through CI
> at all.

Release manager would manually drop patches from developer 1, and leave
the patches from developer 2.  CI would re-test patches from developer 2.

> And if our tool isn't smart enough it will block your patches as well.
> In fact, I can't think of a way to make force pushes on a branch that
> multiple people work on *not* have race conditions.

I agree that there is a race condition here.  Right now our race
condition covers a weeks long window between the time a developer CC's
stable, and when the release manager starts applying patches.  With an
automated implementation, the window narrows to a day or so.

> I think that we should either:
> 1. Use gitlab and have CI run on PRs as well as on merged code. Either the PR
>will be red and gitlab can block the merge, or it will be green. It should 
> be
>possible to have gitlab block code that cannot be cleanly merged.
> 2. Use merges and reverts.

My 2 cents: choosing a specific git service is a step in the wrong
direction for mesa.  I agree that providing a branch to a release
manager may be preferable to email, in the cases where a developer has
to backport patches.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC] Mesa 17.3.x release problems and process improvements

[Mark Janes]

Emil Velikov <emil.l.veli...@gmail.com> writes:

> On 12 March 2018 at 11:31, Juan A. Suarez Romero <jasua...@igalia.com> wrote:
>> On Fri, 2018-03-09 at 12:12 -0800, Mark Janes wrote:
>>>  - Patches are applied to proposed stable branch by automation when the
>>>associated commit is pushed to master.  The existing commit message
>>>annotations drive this process.  There must be zero ambiguity in the
>>>annotations (eg which stable branches need the patch).
>>>
>
> I would recommend a delay between the patch landing in master and the
> wip branch. In the past, we have multiple cases where a fix lands in
> master, which causes severe regressions.
> IMHO having a 24-48h period sounds reasonable, although it can be
> tweaked based on feedback.

Having a delay means developers cannot quickly verify that their
stable-branch annotations correctly resulted in their patch being
applied where they wanted.

In the rare case that we have bad patches applied through the process,
we can treat it like a CI failure, where the maintainer steps in,
force-pushes the bad patches off the WIP branch, and adds annotations to
prevent automation from re-applying the commits later.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC] Mesa 17.3.x release problems and process improvements

[Mark Janes]

Dylan Baker <dy...@pnwbakers.com> writes:

> Quoting Emil Velikov (2018-03-12 08:38:31)
>> On 12 March 2018 at 11:31, Juan A. Suarez Romero <jasua...@igalia.com> wrote:
>> > On Fri, 2018-03-09 at 12:12 -0800, Mark Janes wrote:
>> >> Ilia Mirkin <imir...@alum.mit.edu> writes:
>> >>
>> >> > On Tue, Mar 6, 2018 at 2:34 PM, Emil Velikov <emil.l.veli...@gmail.com> 
>> >> > wrote:
>> >> > > So while others explore ways of improving the testing, let me propose
>> >> > > a few ideas for improving the actual releasing process.
>> >> > >
>> >> > >
>> >> > >  - Making the current state always visible - have a web page, git
>> >> > >branch and other ways for people to see which patches are picked,
>> >> > >require backports, etc.
>> >> >
>> >> > Yes please! A git branch that's available (and force-pushed freely)
>> >> > before the "you're screwed" announcement is going to help clear a lot
>> >> > of things up.
>> >>
>> >> I agree that early information is good.  I don't agree that anyone
>> >> should force push.  Release branches need to be protected.  Proposed
>> >> release branches should only accept patches that have already been
>> >> vetted by the process on mesa master.
>> >>
>> Agreed - release branches should not be force-pushed. We can use
>> "wip/" ones instead.
>
> I also strongly agree with this, force pushes to live branches are
> *bad* (force pushing a pull request of a features branch are perfectly
> fine). I would much rather have reverts than force pushes. If we're
> going to automate this in such way that we think we need force pushes
> I'd much rather use merges (only for stable), so that we can simply
> revert the merge commit.
>
> Or, as other have suggested, not allowing the proposed patches to be
> pushed until CI has come back green would be even better. I've used
> this approach in several github based projects and it works very well
> for keeping the branch in question in good shape.

The patches need to be in a branch for any CI to test them.  A WIP
branch seems like a good thing for CI to poll.

If CI fails at this point, then it means the developer messed up.  No
one should add a fixes/cc tag to a commit unless they have some
confidence that it will work on top of the WIP branch (by *testing* it).

Handling a screw-up could be done by maintainers by force-pushing the
commits off the WIP branch, and adding some annotations that prevent the
broken commit from being re-applied to WIP by automation.

> Dylan
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC] Mesa release improvements - Release schedule

[Mark Janes]

Andres Gomez  writes:

> Hi,
>
>  * Release schedule: move from pre-announce Wed, announcement Fri [0]
>to pre-announce Mon, announcement Wed.

This seems like an improvement to me.

> * Why would we want to do this?
>* We have delays in the release every now and then. When this
>  happens, we step already into the weekend which is bad for the
>  release team but also for the developers of specific drivers
>  who will have to test the patches in the queue before the
>  actual release.
>* Moving the dates to Mon and Wed gives a bigger margin to a
>  delay without stepping into the weekend. It also gives a bit
>  more of margin in case we found straight away that a release
>  is problematic and needs immediate correction.
>
>
> [0] https://www.mesa3d.org/releasing.html#schedule
> -- 
> Br,
>
> Andres
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC] Mesa 17.3.x release problems and process improvements

[Mark Janes]

"Juan A. Suarez Romero" <jasua...@igalia.com> writes:

> On Fri, 2018-03-09 at 12:12 -0800, Mark Janes wrote:
>> Ilia Mirkin <imir...@alum.mit.edu> writes:
>> 
>> > On Tue, Mar 6, 2018 at 2:34 PM, Emil Velikov <emil.l.veli...@gmail.com> 
>> > wrote:
>> > > So while others explore ways of improving the testing, let me propose
>> > > a few ideas for improving the actual releasing process.
>> > > 
>> > > 
>> > >  - Making the current state always visible - have a web page, git
>> > >branch and other ways for people to see which patches are picked,
>> > >require backports, etc.
>> > 
>> > Yes please! A git branch that's available (and force-pushed freely)
>> > before the "you're screwed" announcement is going to help clear a lot
>> > of things up.
>> 
>> I agree that early information is good.  I don't agree that anyone
>> should force push.  Release branches need to be protected.  Proposed
>> release branches should only accept patches that have already been
>> vetted by the process on mesa master.
>> 
>> I would propose:
>> 
>>  - Patches are applied to proposed stable branch by automation when the
>>associated commit is pushed to master.  The existing commit message
>>annotations drive this process.  There must be zero ambiguity in the
>>annotations (eg which stable branches need the patch).
>> 
>
> This would be an awesome improvement. Bear in mind that actually we (or at 
> least
> I) are doing that process in a manually way: cherry-pick all the commits with
> stable tag into the stable branch. But this could be done in automation way.
>
> The difference is that if a patch does not apply, we try to resolve the
> conflicts at our best, and only if we can't then we reject the patch.
>
> In an automated way, the patch would be plainly rejected and informed to the
> author.
>
> In any case, the ambiguity is also that we need to solve, either if we go with
> automation or keep manually. Lot of times (specially when the RC for next 
> major
> stable release is being created) we don't know in which branch we need to 
> apply
> the patch (eg, current 17.3 or next 18.0?) and thus just guess from the patch
> itself. It would be great if this ambiguity could be removed, and just clearly
> states the goal stable branch).
>
>
>>  - Committer is responsible for ensuring that the automation executed.
>> 
>
> IMHO, with automation this shouldn't be required, as this should happen always
> automatically.

What I mean to say is that release managers are not responsible for
constantly polling the status of the proposed release branch.  If the
process is automatic and immediate, patch authors should be the ones to
verify they applied to stable when they were pushed to master.

>>  - Failure-to-apply generates mail to release managers and committer.
>>The committer *should* have been able to test that the commit would
>>not apply before pushing the patch to master, and should get dinged
>>for this.
>> 
>>  - Failures to apply are resolved by committer, who must backport.  Each
>>backported commit references associated patch(es) on master in commit
>>message.  Backport is submitted by committer to release managers in
>>PR or email.
>> 
>
>
> Ideally, this should be automated too. If PR is possible, an approach would be
> the author creating a PR with the backported patch. The system would check 
> that
> everything works correctly (doing sanity check, running proper builds and
> testing), and if that works, then it would merge the patch.
>
> This would help Ilia proposal to have a way to push patches directly to 
> stable.
> Actually, it wouldn't be pushing directly, but indirectly. But the result at 
> the
> end is the same: pushing particular patches without breaking stuff.
>
>
> Also, both the automated way to cherry-pick from master plus this PR approach
> could help to solve another problem also mentioned: status of the patches in 
> the
> stable mailing list.
>
>
>>  - CI Automation immediately builds/tests the proposed stable branch
>>whenever it changes.  Release managers verify the results.
>> 
>
> This is kind of automation is something we were doing at Igalia since the
> beginning.
>
> We are actually using Travis[0][1] to exhaustively run different builds, and
> generate different Docker images[2] that contains the version under testing
> together with several testsuites.
>
> These docker images are lately distributed over a couple of different mach

Re: [Mesa-dev] [RFC] Mesa 17.3.x release problems and process improvements

[Mark Janes]

Ilia Mirkin  writes:

> On Tue, Mar 6, 2018 at 2:34 PM, Emil Velikov  wrote:
>> So while others explore ways of improving the testing, let me propose
>> a few ideas for improving the actual releasing process.
>>
>>
>>  - Making the current state always visible - have a web page, git
>>branch and other ways for people to see which patches are picked,
>>require backports, etc.
>
> Yes please! A git branch that's available (and force-pushed freely)
> before the "you're screwed" announcement is going to help clear a lot
> of things up.

I agree that early information is good.  I don't agree that anyone
should force push.  Release branches need to be protected.  Proposed
release branches should only accept patches that have already been
vetted by the process on mesa master.

I would propose:

 - Patches are applied to proposed stable branch by automation when the
   associated commit is pushed to master.  The existing commit message
   annotations drive this process.  There must be zero ambiguity in the
   annotations (eg which stable branches need the patch).

 - Committer is responsible for ensuring that the automation executed.

 - Failure-to-apply generates mail to release managers and committer.
   The committer *should* have been able to test that the commit would
   not apply before pushing the patch to master, and should get dinged
   for this.

 - Failures to apply are resolved by committer, who must backport.  Each
   backported commit references associated patch(es) on master in commit
   message.  Backport is submitted by committer to release managers in
   PR or email.

 - CI Automation immediately builds/tests the proposed stable branch
   whenever it changes.  Release managers verify the results.

 - Any CI failures are resolved by force-pushing the series off of the
   proposed branch.  Release manager communicates status to committer.

I think automation could be implemented to generate proposed stable
branches on an arbitrary branch point in a local git repo.  Including
this policy mechanism in our git tree would make the process
transparent, verifiable, debuggable, and extensible.

Separately, I think we want to limit the ability of release manager to
unilaterally change the stable build.  Release managers should not
author any new commit in the stable branch that is not reviewed by
another release manager, from a different company.

>>  - Per team maintainer - each team has person (a few people) to check,
>>coordinate and remind people for high priority bugs and backports.

I've been doing this for Intel.  Developers are on the hook to fix their
bugs, but you can't make them do it.  They have many pressures on them,
and a maintainer can't make the call as to whether a rendering bug is
more important than day-1 vulkan conformance, for example.

We could heighten the transparency of what is blocking the build by
publicizing the authors of bisected blocking bugs to Phoronix, which
might get things moving.

> Yes! I think this role is highly necessary, but not for the reasons
> you outline here. My main issue thus far with the stable release
> process has been that I don't appear to have any control over what
> patches are included in a particular release. Reasons for rejection
> have varied from "well, there will be another release in 2+ weeks" to
> "this patch doesn't fit some arbitrary criterion".

If the patches are immediately merged to proposed stable branches, then
they should be included by default.  It makes sense to have a standard
window by which patches must be applied before a release.

> What I'd like to propose is a system where the team maintainer is able
> to request any particular patch to be included in a stable release (as
> long as it's done by a certain pre-announced deadline, which happens
> *after* the initial release nominations email). If a release manager
> disagrees, they can make that disagreement known, but it's ultimately
> up to the team maintainer for driver code. If it's in shared code
> (i.e. not driver-specific - winsys, core mesa, state trackers used by
> multiple drivers), the override would be 1 additional team maintainer
> acking, and conversely any team maintainer would be able to nak it (so
> the condition in case of disagreement would be at least 2 team
> maintainers in favor, and 0 against).
>
> As you may know, I've stopped tagging things for stable since a while
> back since it seems completely pointless - nothing I can do to
> actually cause a patch to be included in a release. Telling people to
> just use HEAD seems way more reliable. I'm not particularly happy with
> this arrangement, but it's at least supportable.
>
> Hopefully others here agree with me. Otherwise I'll just go back to
> doing what I'm doing (and perhaps my contribution level has dropped
> sufficiently to not worry about it).
>
> Cheers,
>
>   -ilia
> ___
> mesa-dev mailing 

Re: [Mesa-dev] [PATCH 1/5] i965: Hard code scratch_ids_per_subslice for Cherryview

[Mark Janes]

Could this be the reason that BSW systems never reliably passed all unit
tests?  Up to now, we re-execute each failing test, and mark it as a
pass if it succeeds a second time.

I'd like to remove that crutch if possible.

Jordan Justen  writes:

> Ken suggested that we might be underallocating scratch space on HD
> 400. Allocating scratch space as though there was actually 8 EUs
> seems to help with a GPU hang seen on synmark CSDof.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104636
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105290
> Cc: Kenneth Graunke 
> Cc: Eero Tamminen 
> Cc: 
> Signed-off-by: Jordan Justen 
> ---
>  src/mesa/drivers/dri/i965/brw_program.c | 44 
> -
>  1 file changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_program.c 
> b/src/mesa/drivers/dri/i965/brw_program.c
> index 527f003977b..c121136c439 100644
> --- a/src/mesa/drivers/dri/i965/brw_program.c
> +++ b/src/mesa/drivers/dri/i965/brw_program.c
> @@ -402,23 +402,33 @@ brw_alloc_stage_scratch(struct brw_context *brw,
>if (devinfo->gen >= 9)
>   subslices = 4 * brw->screen->devinfo.num_slices;
>  
> -  /* WaCSScratchSize:hsw
> -   *
> -   * Haswell's scratch space address calculation appears to be sparse
> -   * rather than tightly packed.  The Thread ID has bits indicating
> -   * which subslice, EU within a subslice, and thread within an EU
> -   * it is.  There's a maximum of two slices and two subslices, so these
> -   * can be stored with a single bit.  Even though there are only 10 EUs
> -   * per subslice, this is stored in 4 bits, so there's an effective
> -   * maximum value of 16 EUs.  Similarly, although there are only 7
> -   * threads per EU, this is stored in a 3 bit number, giving an 
> effective
> -   * maximum value of 8 threads per EU.
> -   *
> -   * This means that we need to use 16 * 8 instead of 10 * 7 for the
> -   * number of threads per subslice.
> -   */
> -  const unsigned scratch_ids_per_subslice =
> - devinfo->is_haswell ? 16 * 8 : devinfo->max_cs_threads;
> +  unsigned scratch_ids_per_subslice;
> +  if (devinfo->is_haswell) {
> + /* WaCSScratchSize:hsw
> +  *
> +  * Haswell's scratch space address calculation appears to be sparse
> +  * rather than tightly packed. The Thread ID has bits indicating
> +  * which subslice, EU within a subslice, and thread within an EU it
> +  * is. There's a maximum of two slices and two subslices, so these
> +  * can be stored with a single bit. Even though there are only 10 
> EUs
> +  * per subslice, this is stored in 4 bits, so there's an effective
> +  * maximum value of 16 EUs. Similarly, although there are only 7
> +  * threads per EU, this is stored in a 3 bit number, giving an
> +  * effective maximum value of 8 threads per EU.
> +  *
> +  * This means that we need to use 16 * 8 instead of 10 * 7 for the
> +  * number of threads per subslice.
> +  */
> + scratch_ids_per_subslice = 16 * 8;
> +  } else if (devinfo->is_cherryview) {
> + /* For Cherryview, it appears that the scratch addresses for the 6 
> EU
> +  * devices may still generate compute scratch addresses covering the
> +  * same range as 8 EU.
> +  */
> + scratch_ids_per_subslice = 8 * 7;
> +  } else {
> + scratch_ids_per_subslice = devinfo->max_cs_threads;
> +  }
>  
>thread_count = scratch_ids_per_subslice * subslices;
>break;
> -- 
> 2.16.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965/sbe: fix number of inputs for active components

[Mark Janes]

Please add the Fixes: tag, so this patch gets applied to the appropriate
stable branches.

Iago Toral Quiroga  writes:

> In 16631ca30ea6 we fixed gen9 active components to account for padded
> inputs in the URB, which we can have with SSO programs. To do that,
> instead of going through the bitfield of inputs (which doesn't include
> padding information), we compute the number of inputs from the size
> of the URB entry.
>
> Unfortunately, there are some special inputs that are not stored in
> the URB and that we also need to account for. These special inputs
> are identified and handled during calculate_attr_overrides(), so this
> patch modifies this function to return a value with the total number
> of inputs, including the ones that are not stored in the URB, so we
> can use that number to program the correct number of active components.
>
> This fixes a regression in a WebGL program that uses Point Sprite
> functionality (specifically, VARYING_SLOT_PNTC).
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105224
> ---
>  src/mesa/drivers/dri/i965/genX_state_upload.c | 31 
> ---
>  1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c 
> b/src/mesa/drivers/dri/i965/genX_state_upload.c
> index 8668abd591..617f0ab2c4 100644
> --- a/src/mesa/drivers/dri/i965/genX_state_upload.c
> +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
> @@ -1015,7 +1015,7 @@ genX(get_attr_override)(struct 
> GENX(SF_OUTPUT_ATTRIBUTE_DETAIL) *attr,
>  }
>  
>  
> -static void
> +static int
>  genX(calculate_attr_overrides)(const struct brw_context *brw,
> struct GENX(SF_OUTPUT_ATTRIBUTE_DETAIL) 
> *attr_overrides,
> uint32_t *point_sprite_enables,
> @@ -1064,6 +1064,7 @@ genX(calculate_attr_overrides)(const struct brw_context 
> *brw,
>  */
> bool drawing_points = brw_is_drawing_points(brw);
>  
> +   uint32_t no_vue_inputs = 0;
> for (int attr = 0; attr < VARYING_SLOT_MAX; attr++) {
>int input_index = wm_prog_data->urb_setup[attr];
>  
> @@ -1097,6 +1098,12 @@ genX(calculate_attr_overrides)(const struct 
> brw_context *brw,
>   _source_attr);
>}
>  
> +  if (point_sprite ||
> +  (attr == VARYING_SLOT_PRIMITIVE_ID &&
> +   brw->vue_map_geom_out.varying_to_slot[attr] == -1)) {
> + no_vue_inputs++;
> +  }
> +
>/* The hardware can only do the overrides on 16 overrides at a
> * time, and the other up to 16 have to be lined up so that the
> * input index = the output index.  We'll need to do some
> @@ -1124,6 +1131,8 @@ genX(calculate_attr_overrides)(const struct brw_context 
> *brw,
>  * Similar text exists for Ivy Bridge.
>  */
> *urb_entry_read_length = DIV_ROUND_UP(max_source_attr + 1, 2);
> +
> +   return *urb_entry_read_length * 2 + no_vue_inputs;
>  }
>  #endif
>  
> @@ -3434,11 +3443,12 @@ genX(upload_sbe)(struct brw_context *brw)
> * BRW_NEW_GS_PROG_DATA | BRW_NEW_PRIMITIVE | BRW_NEW_TES_PROG_DATA |
> * BRW_NEW_VUE_MAP_GEOM_OUT
> */
> -  genX(calculate_attr_overrides)(brw,
> - attr_overrides,
> - _sprite_enables,
> - _entry_read_length,
> - _entry_read_offset);
> +  int num_inputs =
> + genX(calculate_attr_overrides)(brw,
> +attr_overrides,
> +_sprite_enables,
> +_entry_read_length,
> +_entry_read_offset);
>  
>/* Typically, the URB entry read length and offset should be programmed
> * in 3DSTATE_VS and 3DSTATE_GS; SBE inherits it from the last active
> @@ -3459,8 +3469,13 @@ genX(upload_sbe)(struct brw_context *brw)
>  #endif
>  
>  #if GEN_GEN >= 9
> -  /* prepare the active component dwords */
> -  const int num_inputs = urb_entry_read_length * 2;
> +  /* prepare the active component dwords
> +   *
> +   * For this, we need to account for padded inputs (that we can have 
> with
> +   * SSO programs), so we take the number of inputs from the size of the
> +   * URB entry. To that, we need to add the number of special inputs, if
> +   * any, that are not in the URB (such as point sprite inputs).
> +   */
>for (int input_index = 0; input_index < num_inputs; input_index++) {
>   sbe.AttributeActiveComponentFormat[input_index] = 
> ACTIVE_COMPONENT_XYZW;
>}
> -- 
> 2.14.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] mesa: Update vertex processing mode on _mesa_UseProgram.

[Mark Janes]

 writes:

> From: Mathias Fröhlich 
>
> Hi Clayton,
>
> The following change fixes the reported problem on my site.
> Please test/review!!
>
> best
>
> Mathias
>
>
> The change is a bug fix for 92d76a169:
>   mesa: Provide an alternative to get_vp_mode()
> that actually got exposed through 4562a7b0:
>   vbo: Make use of _DrawVAO from the dlist code.
>
> Fixes: KHR-GLES31.core.shader_image_load_store.advanced-sso-simple

The "Fixes:" tag is supposed to indicate the git commit which regresses
Mesa.  Release automation processes it when preparing branches for the
stable release.

https://www.mesa3d.org/submittingpatches.html#formatting

In this case, the patch doesn't need to be applied to a stable branch.
However, it may confuse the automation.

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105229
> Signed-off-by: Mathias Fröhlich 
> ---
>  src/mesa/main/shaderapi.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index d8a3031a38..b7bd7167e1 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -2069,6 +2069,8 @@ use_program(GLuint program, bool no_error)
>  _mesa_BindProgramPipeline(ctx->Pipeline.Current->Name);
>}
> }
> +
> +   _mesa_update_vertex_processing_mode(ctx);
>  }
>  
>  
> -- 
> 2.14.3
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [ANNOUNCE] Mesa 17.3.4 release candidate

[Mark Janes]

Kenneth Graunke  writes:

> On Thursday, February 8, 2018 8:47:00 PM PST Emil Velikov wrote:
>> Rejected (9)
>> 
>> Jason Ekstrand (2):
>>   e52a9f18d69c94b7cb7f81361cdb9e2582c3d742 i965: Replace
>> draw_aux_buffer_disabled with draw_aux_usage
>>   20f70ae3858bc213e052a8434f0e637eb36203c4 i965/draw: Set
>> NEW_AUX_STATE when draw aux changes
>> Reason: Introduce multiple regressions in the piglit compute shader tests.
>
> Hi Emil,
>
> These are absolutely critical fixes.  These patches fix GPU hangs and
> crashes in Glamor which cause people's X session to die when doing
> exciting things like using their text editor, IDE, or desktop panel.
> It's responsible for a huge swath of our GPU hang bugs on i965.
>
> Did Jason or I miss an email from you about these being rejected,
> other than at the bottom of a large changelog in an RC announcement?
> Which Piglit tests are regressing?  My guess is that we just need to
> nominate another patch, as they aren't broken in master.
>
> At this point, we've done 5 point releases in the 17.3.x series, which
> have had DRI3 crashes when pageflipping (in all drivers), and X server
> hangs and crashes galore in i965/Gen9+.  Worse, we fixed those hangs a
> month ago and haven't managed to ship them yet.  We also managed to
> ship a radv that broke completely.
>
> At this point, 17.3.x is looking like the worst Mesa release in recent
> memory, and I'm about on the verge of advising people to just go back
> to 17.2 until 18.0 comes out.  It's pretty frustrating, and I feel bad
> for our users, who depend on our software for their computer to work.
>
> We have to do better, somehow - myself included.  Ideally, we'd find a
> way to avoid major bugs like this in the first place.  Barring that,
> do we need to have developers take a more active role in backporting
> fixes again?  It seems like our nomination process works for simple
> things, but for more complex series, it doesn't work as well.  Maybe
> we need to proactively put together (tested) pull requests for stable?

It seems to me that patches CCing stable or containing 'Fixes:' could be
automatically be applied to a branch for incremental test.  Anything
that doesn't apply (or generates regressions) could be bounced to the
committer for a manual backport.

The recent issues all stem from patches that do not apply cleanly to the
release branch.  Developers should be on the hook for resolving the
conflicts *and* testing the result.

Finding conflicts shortly before release means the developers have no
chance to fix them.  It also generates pressure on the release manager
to fix up other people's patches.  Personally, I wouldn't want to be in
a position where I could easily impact large numbers of users with a
simple typo.

> --Ken
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [ANNOUNCE] mesa 17.3.4

[Mark Janes]

Bas Nieuwenhuizen  writes:

> (-mesa-announce + Mark, Dave and James)
>
> Hi Emil,
>
> radv is broken for nearly all commercial games in 17.3.4. The cause is
>
> commit ad764e365beb8a119369b97f5cb95fc7ea8c
> Author: Bas Nieuwenhuizen 
> Date:   Mon Jan 22 09:01:29 2018 +0100
>
> ac/nir: Use instance_rate_inputs per attribute, not per variable.
>
> This did the wrong thing if we had e.g. an array for which only some
> of the attributes use the instance index. Tripped up some new CTS
> tests.
>
> CC: 
> Reviewed-by: Samuel Pitoiset 
> Reviewed-by: Dave Airlie 
> (cherry picked from commit 5a4dc285002e1924dbc8c72d17481a3dbc4c0142)
>
> Conflicts:
> src/amd/common/ac_nir_to_llvm.c
>
> A typo was introduced during the conflict resolution while
> cherrypicking to 17.3.
>
> First things first, can we mitigate this? Would it be possible to get
> a 17.3.5 with this fixed ASAP? This can be fixed by either rolling
> back ad764e365beb8a119369b97f5cb95fc7ea8c or by applying
> https://patchwork.freedesktop.org/patch/204260/ (obviously not applied
> to master as the issue did not occur there).
>
> Secondly, I'd like to talk about process and how to prevent this in
> the future. Bugfix releases are supposed to be stable so downstream
> maintainers don't have to to deal with this kind of stuff happening,
> so I think that breaking radv pretty much completely is particularly
> egregious and we should look at how to prevent this happnening another
> time.
>
> First a short summary of what happened and when (all times in UTC):
>
> 2018-02-09 4:47: Emil sends out the pre-release announcement, git
> branch contains the faulty commit
> 2018-02-13 16:05: James Legg replies to the pre-release announcement:
> https://lists.freedesktop.org/archives/mesa-dev/2018-February/185355.html
> 2018-02-13 16:06: James Legg sends a patch to fix it:
> https://lists.freedesktop.org/archives/mesa-dev/2018-February/185356.html
> 2018-02-13 16:33: Bas reviews the patch.
> 2018-02-15 12:56: Emil releases 17.3.4 without the fix:
> https://lists.freedesktop.org/archives/mesa-dev/2018-February/185691.html
> 2018-02-16 21:09: Someone else replies with a fix to the pre-release
> announcement: 
> https://lists.freedesktop.org/archives/mesa-dev/2018-February/185908.html
> 2018-02-17 ~13:00: Bas notices the attached fix has "mismerge" in the
> name and decides to investigate the issue
>
>
> So I think there have been a couple of issues in communication here:
>
> 1) Essentially none of the communication talked about this being an
> issue on the branch only. This made me underestimate the severity of
> the issue, since the games kept working on master. This also meant no
> manual pings from my side to the release manager for not applying this
> to master first.
> 2) The reply to the pre-release announcement only said there was an
> issue, not that there was a fix sent out, nor any details like
> severity.
> 3) As far as I can tell no action had been taken by the release
> manager. James' reply did not get a response nor was the fix included
> in the release.
>
> My question would be how to improve the communication here. Could you
> elaborate the reason for (3)? Was it because you thought it would have
> to be picked up by the radv developers first? Would it help if I
> replied to James' reply in the announcement to link to the patch?
>
> The remaining issue is mainly about testing. The initial detection of
> this issue by James was already more than the 24-48 hours after the
> pre-release announcement that is recommended between the announcement
> and release, so a faster release would have prevented timely detection
> in the first place. I suspect radv does not get tested at all on
> releases except maybe a build test?
>
> Since manually testing all games on every release is not scalable for
> either the radv developers or the release manager I'm thinking of
> getting the automated tests from the vulkan CTS and crucible running
> before a release. That said I don't think keeping track of what tests
> are supposed to pass is something we should be pushing on the release
> manager, so I suppose we should be setting up our own CI and surfacing
> results of the stable branches to the release manager?

Tracking results is at best a significant investment which attenuates as
the driver becomes more stable and automation improves.  It takes us
perhaps an hour per day on average to track i965 status in CI.  This
investment pays bigger dividends for larger developer teams.  There is
constant demand to expand the coverage of CI.

Tracking results requires developer time when the driver has been
broken, but this cost is less than any other alternative.
Unfortunately, this cost is often borne by whoever runs the CI, and can
become burdensome.

> I'm assuming there is already a 

Re: [Mesa-dev] [PATCH] meson: freedreno depends on nir

[Mark Janes]

Acked-by: Mark Janes <mark.a.ja...@intel.com>

Dylan Baker <dy...@pnwbakers.com> writes:

> This fixes a race condition in building targets that link in freedreno.
>
> cc: Mark Janes <mark.a.ja...@intel.com>
> Fixes: 0bbecc5a8548883f76a7 ("meson: define driver dependencies")
> Signed-off-by: Dylan Baker <dylan.c.ba...@intel.com>
> ---
>  src/gallium/drivers/freedreno/meson.build | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/gallium/drivers/freedreno/meson.build 
> b/src/gallium/drivers/freedreno/meson.build
> index 8094e141b4b..ba1a21c9e0e 100644
> --- a/src/gallium/drivers/freedreno/meson.build
> +++ b/src/gallium/drivers/freedreno/meson.build
> @@ -230,6 +230,7 @@ libfreedreno = static_library(
>  driver_freedreno = declare_dependency(
>compile_args : '-DGALLIUM_FREEDRENO',
>link_with : [libfreedrenowinsys, libfreedreno],
> +  dependencies : idep_nir,
>  )
>  
>  ir3_compiler = executable(
> -- 
> 2.16.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] i965: perf: use drmIoctl() instead of ioctl()

[Mark Janes]

Tested-by: Mark Janes <mark.a.ja...@intel.com>

Lionel Landwerlin <lionel.g.landwer...@intel.com> writes:

> ioctl() might be interrupted, use drmIoctl() instead as it'll retry
> automatically.
>
> Fixes: 27ee83eaf7e "i965: perf: add support for userspace configurations"
> Cc: "18.0" <mesa-sta...@lists.freedesktop.org>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwer...@intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_performance_query.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_performance_query.c 
> b/src/mesa/drivers/dri/i965/brw_performance_query.c
> index 5b8f30db2f8..f4ff858bd5d 100644
> --- a/src/mesa/drivers/dri/i965/brw_performance_query.c
> +++ b/src/mesa/drivers/dri/i965/brw_performance_query.c
> @@ -1842,7 +1842,7 @@ kernel_has_dynamic_config_support(struct brw_context 
> *brw,
>   config.n_mux_regs = 1;
>   config.mux_regs_ptr = (uintptr_t) mux_regs;
>  
> - if (ioctl(screen->fd, DRM_IOCTL_I915_PERF_REMOVE_CONFIG, 
> _id) < 0 &&
> + if (drmIoctl(screen->fd, DRM_IOCTL_I915_PERF_REMOVE_CONFIG, 
> _id) < 0 &&
>   errno == ENOENT)
>  return true;
>  
> @@ -1889,7 +1889,7 @@ init_oa_configs(struct brw_context *brw, const char 
> *sysfs_dev_dir)
>config.n_flex_regs = query->n_flex_regs;
>config.flex_regs_ptr = (uintptr_t) query->flex_regs;
>  
> -  ret = ioctl(screen->fd, DRM_IOCTL_I915_PERF_ADD_CONFIG, );
> +  ret = drmIoctl(screen->fd, DRM_IOCTL_I915_PERF_ADD_CONFIG, );
>if (ret < 0) {
>   DBG("Failed to load \"%s\" (%s) metrics set in kernel: %s\n",
>   query->name, query->guid, strerror(errno));
> -- 
> 2.16.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 7/8] mesa: rename some 'client' array functions

[Mark Janes]

This series that was just pushed to master generates ~200 assertions on
all platforms, eg:

piglit.spec.ext_framebuffer_multisample.formats 2

ext_framebuffer_multisample-formats: ../src/mesa/main/varray.c:1068:
_mesa_enable_vertex_array_attrib: Assertion `attrib >=
VERT_ATTRIB_GENERIC0' failed.

Should we revert the series?  This needs to be corrected immediately.

Brian Paul  writes:

> A long time ago gl_vertex_array was gl_client_array.  Update some function
> names to be consistent.
> ---
>  src/mesa/main/arrayobj.c | 6 +++---
>  src/mesa/main/arrayobj.h | 4 ++--
>  src/mesa/main/attrib.c   | 2 +-
>  src/mesa/main/state.c| 2 +-
>  src/mesa/main/varray.c   | 4 ++--
>  src/mesa/main/varray.h   | 4 ++--
>  6 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/src/mesa/main/arrayobj.c b/src/mesa/main/arrayobj.c
> index b862f0f..ea9b316 100644
> --- a/src/mesa/main/arrayobj.c
> +++ b/src/mesa/main/arrayobj.c
> @@ -311,8 +311,8 @@ _mesa_initialize_vao(struct gl_context *ctx,
>   * or a gl_vertex_buffer_binding has changed.
>   */
>  void
> -_mesa_update_vao_client_arrays(struct gl_context *ctx,
> -   struct gl_vertex_array_object *vao)
> +_mesa_update_vao_derived_arrays(struct gl_context *ctx,
> +struct gl_vertex_array_object *vao)
>  {
> GLbitfield arrays = vao->NewArrays;
>  
> @@ -324,7 +324,7 @@ _mesa_update_vao_client_arrays(struct gl_context *ctx,
>const struct gl_vertex_buffer_binding *buffer_binding =
>   >BufferBinding[attrib_array->BufferBindingIndex];
>  
> -  _mesa_update_client_array(ctx, client_array, attrib_array,
> +  _mesa_update_vertex_array(ctx, client_array, attrib_array,
>  buffer_binding);
> }
>  }
> diff --git a/src/mesa/main/arrayobj.h b/src/mesa/main/arrayobj.h
> index 1b9900c..ff26157 100644
> --- a/src/mesa/main/arrayobj.h
> +++ b/src/mesa/main/arrayobj.h
> @@ -78,8 +78,8 @@ _mesa_initialize_vao(struct gl_context *ctx,
>  
>  
>  extern void
> -_mesa_update_vao_client_arrays(struct gl_context *ctx,
> -   struct gl_vertex_array_object *vao);
> +_mesa_update_vao_derived_arrays(struct gl_context *ctx,
> +struct gl_vertex_array_object *vao);
>  
>  /* Returns true if all varying arrays reside in vbos */
>  extern bool
> diff --git a/src/mesa/main/attrib.c b/src/mesa/main/attrib.c
> index 3c12fd6..0b4b63f 100644
> --- a/src/mesa/main/attrib.c
> +++ b/src/mesa/main/attrib.c
> @@ -1503,7 +1503,7 @@ copy_array_object(struct gl_context *ctx,
> /* skip RefCount */
>  
> for (i = 0; i < ARRAY_SIZE(src->VertexAttrib); i++) {
> -  _mesa_copy_client_array(ctx, >_VertexAttrib[i], 
> >_VertexAttrib[i]);
> +  _mesa_copy_vertex_array(ctx, >_VertexAttrib[i], 
> >_VertexAttrib[i]);
>_mesa_copy_vertex_attrib_array(ctx, >VertexAttrib[i], 
> >VertexAttrib[i]);
>_mesa_copy_vertex_buffer_binding(ctx, >BufferBinding[i], 
> >BufferBinding[i]);
> }
> diff --git a/src/mesa/main/state.c b/src/mesa/main/state.c
> index dd61cc7..df694d0 100644
> --- a/src/mesa/main/state.c
> +++ b/src/mesa/main/state.c
> @@ -357,7 +357,7 @@ _mesa_update_state_locked( struct gl_context *ctx )
> }
>  
> if (new_state & _NEW_ARRAY)
> -  _mesa_update_vao_client_arrays(ctx, ctx->Array.VAO);
> +  _mesa_update_vao_derived_arrays(ctx, ctx->Array.VAO);
>  
>   out:
> new_prog_state |= update_program_constants(ctx);
> diff --git a/src/mesa/main/varray.c b/src/mesa/main/varray.c
> index 2801a6f..6c022b4 100644
> --- a/src/mesa/main/varray.c
> +++ b/src/mesa/main/varray.c
> @@ -2794,10 +2794,10 @@ _mesa_VertexArrayBindingDivisor(GLuint vaobj, GLuint 
> bindingIndex,
>  
>  
>  /**
> - * Copy one client vertex array to another.
> + * Copy one vertex array to another.
>   */
>  void
> -_mesa_copy_client_array(struct gl_context *ctx,
> +_mesa_copy_vertex_array(struct gl_context *ctx,
>  struct gl_vertex_array *dst,
>  struct gl_vertex_array *src)
>  {
> diff --git a/src/mesa/main/varray.h b/src/mesa/main/varray.h
> index 19506bf..79ecd9a 100644
> --- a/src/mesa/main/varray.h
> +++ b/src/mesa/main/varray.h
> @@ -53,7 +53,7 @@ _mesa_vertex_attrib_address(const struct 
> gl_array_attributes *array,
>   * gl_array_attributes and a gl_vertex_buffer_binding.
>   */
>  static inline void
> -_mesa_update_client_array(struct gl_context *ctx,
> +_mesa_update_vertex_array(struct gl_context *ctx,
>struct gl_vertex_array *dst,
>const struct gl_array_attributes *attribs,
>const struct gl_vertex_buffer_binding *binding)
> @@ -475,7 +475,7 @@ _mesa_VertexArrayBindingDivisor(GLuint vaobj, GLuint 
> bindingIndex,
>  GLuint divisor);
>  
>  extern void
> -_mesa_copy_client_array(struct gl_context *ctx,
>