Re: Mesa >= 23.3.x and python 2.6 ...
I dropped dri-devel from this reply. On 2024-01-20 05:56:23, Stefan Dirsch wrote: > Hi Jordan > > On Sat, Jan 20, 2024 at 01:47:58AM -0800, Jordan Justen wrote: > > > > It was "fun" finding a way to get python 3.6 :), but after that, I > > think I found a way to make Python 3.6 work. I guess you can try it > > out: > > > > https://gitlab.freedesktop.org/jljusten/mesa/-/commits/intel-genxml-python3.6 > > > > In my light testing, Python 3.6 through 3.13 seemed to work. Python > > 3.5 did *not* work. > > Wow! Thanks a lot! Indeed these two patches fix this build issue for me! :-) > > In addition I needed to add the attached patch to fix some more errors I saw > with a python 3.6 build. With that I can build again Mesa 23.3.3. It looks like you could turn your changes into ~2 patches. I do find this change potentially concerning in that it seems like it actually would change behavior when newer Python versions are used. - subparsers = parser.add_subparsers(required=True) + subparsers = parser.add_subparsers() With my two patches, and your ~2 patches, you could open a merge request for Mesa. To be honest, I don't know how it would be received. (Maybe someone would push back on supporting an outdated Python, or perhaps not.) One other question, do you only care about Mesa 23.3? If so, you could target the merge request for the 23.3 branch. It seems like there potentially could be less chance of getting push back for supporting this for just the 23.3 release. -Jordan
Re: Mesa >= 23.3.x and python 2.6 ...
On 2024-01-19 12:24:24, Stefan Dirsch wrote:
> Hi Jordan
>
> Thanks for digging into this!
>
> On Fri, Jan 19, 2024 at 12:10:37PM -0800, Jordan Justen wrote:
> > On 2024-01-18 04:37:52, Stefan Dirsch wrote:
> > > Hi
> > >
> > > I noticed that with version 23.3.x Mesa no longer can be built with python
> > > 2.6. It still worked with Mesa 23.2.1.
> >
> > As mentioned in other emails, this was typo where 3.6 was intended.
> >
> > >
> > > It fails with
> > >
> > > [ 95s] Traceback (most recent call last):
> > > [ 95s] File "../src/intel/genxml/gen_bits_header.py", line 23, in
> > >
> > > [ 95s] import intel_genxml
> > > [ 95s] File
> > > "/home/abuild/rpmbuild/BUILD/mesa-23.3.3/src/intel/genxml/intel_
> > > genxml.py", line 5
> > > [ 95s] from __future__ import annotations
> > > [ 95s] ^
> > > [ 95s] SyntaxError: future feature annotations is not defined
> > >
> >
> > I guess this code first appeared in Dylan's:
> >
> > 4fd2e15855d ("intel/genxml: add type annotations to gen_sort_tags.py")
> >
> > and then became part of the standard tests a few commits later in:
> >
> > 1f0a0a46d97 ("meson: run genxml sort tests")
> >
> > back in Oct 2022. So, I guess at that point 'ninja test' would have
> > failed with Python 3.6.
> >
> > Then, I suppose I propagated this to being used on every build in:
> >
> > 0495f952d48 ("intel/genxml: Add genxml_import.py script")
> >
> > in Sept 2023.
>
> Thanks. This explains why I've found this code already in older releases, but
> it didn't fail for me yet. You said tests. Is this just a test, I could
> disable (as dirty hack)? I was assuming it would generate code ...
In 0495f952d48, I moved in to a common file, and essentially, now it's
used by our script that runs during the build in addition to the test.
It was "fun" finding a way to get python 3.6 :), but after that, I
think I found a way to make Python 3.6 work. I guess you can try it
out:
https://gitlab.freedesktop.org/jljusten/mesa/-/commits/intel-genxml-python3.6
In my light testing, Python 3.6 through 3.13 seemed to work. Python
3.5 did *not* work.
-Jordan
Re: Mesa >= 23.3.x and python 2.6 ...
On 2024-01-18 04:37:52, Stefan Dirsch wrote:
> Hi
>
> I noticed that with version 23.3.x Mesa no longer can be built with python
> 2.6. It still worked with Mesa 23.2.1.
As mentioned in other emails, this was typo where 3.6 was intended.
>
> It fails with
>
> [ 95s] Traceback (most recent call last):
> [ 95s] File "../src/intel/genxml/gen_bits_header.py", line 23, in
> [ 95s] import intel_genxml
> [ 95s] File
> "/home/abuild/rpmbuild/BUILD/mesa-23.3.3/src/intel/genxml/intel_
> genxml.py", line 5
> [ 95s] from __future__ import annotations
> [ 95s] ^
> [ 95s] SyntaxError: future feature annotations is not defined
>
I guess this code first appeared in Dylan's:
4fd2e15855d ("intel/genxml: add type annotations to gen_sort_tags.py")
and then became part of the standard tests a few commits later in:
1f0a0a46d97 ("meson: run genxml sort tests")
back in Oct 2022. So, I guess at that point 'ninja test' would have
failed with Python 3.6.
Then, I suppose I propagated this to being used on every build in:
0495f952d48 ("intel/genxml: Add genxml_import.py script")
in Sept 2023.
Maybe Dylan knows how we might make this compatible with Python 3.6,
assuming we want to. :)
https://devguide.python.org/versions/
-Jordan
Re: [PATCH v2 01/12] drm/doc: add rfc section for small BAR uapi
On 2022-06-21 11:31:39, Lionel Landwerlin wrote: > On 21/06/2022 13:44, Matthew Auld wrote: > > Add an entry for the new uapi needed for small BAR on DG2+. > > > > v2: > >- Some spelling fixes and other small tweaks. (Akeem & Thomas) > >- Rework error capture interactions, including no longer needing > > NEEDS_CPU_ACCESS for objects marked for capture. (Thomas) > >- Add probed_cpu_visible_size. (Lionel) > > v3: > >- Drop the vma query for now. > >- Add unallocated_cpu_visible_size as part of the region query. > >- Improve the docs some more, including documenting the expected > > behaviour on older kernels, since this came up in some offline > > discussion. > > v4: > >- Various improvements all over. (Tvrtko) > > > > v5: > >- Include newer integrated platforms when applying the non-recoverable > > context and error capture restriction. (Thomas) > > > > Signed-off-by: Matthew Auld > > Cc: Thomas Hellström > > Cc: Lionel Landwerlin > > Cc: Tvrtko Ursulin > > Cc: Jon Bloomfield > > Cc: Daniel Vetter > > Cc: Jordan Justen > > Cc: Kenneth Graunke > > Cc: Akeem G Abodunrin > > Cc: mesa-dev@lists.freedesktop.org > > Acked-by: Tvrtko Ursulin > > Acked-by: Akeem G Abodunrin > > > With Jordan with have changes for Anv/Iris : > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/16739 > > Acked-by: Lionel Landwerlin > Acked-by: Jordan Justen
Re: [PATCH v3] uapi/drm/i915: Document memory residency and Flat-CCS capability of obj
On 2022-05-16 00:47:43, Lionel Landwerlin wrote: > On 14/05/2022 00:06, Jordan Justen wrote: >> >> Acked-by: Jordan Justen >> >> I think Nanley has accounted for this on iris with: >> >> >> https://gitlab.freedesktop.org/mesa/mesa/-/commit/42a865730ef72574e179b56a314f30fdccc6cba8 >> > > Thanks Jordan, > > We might want to through in an additional : assert((flags & BO_ALLOC_SMEM) == > 0); in the CCS case Yeah. I noticed this potential for concern when looking at the small-bar uapi on iris. I added an assert, and I haven't seen it get triggered yet. -Jordan
Re: [PATCH v3] uapi/drm/i915: Document memory residency and Flat-CCS capability of obj
On 2022-05-13 05:31:00, Lionel Landwerlin wrote:
> On 02/05/2022 17:15, Ramalingam C wrote:
> > Capture the impact of memory region preference list of the objects, on
> > their memory residency and Flat-CCS capability.
> >
> > v2:
> >Fix the Flat-CCS capability of an obj with {lmem, smem} preference
> >list [Thomas]
> > v3:
> >Reworded the doc [Matt]
> >
> > Signed-off-by: Ramalingam C
> > cc: Matthew Auld
> > cc: Thomas Hellstrom
> > cc: Daniel Vetter
> > cc: Jon Bloomfield
> > cc: Lionel Landwerlin
> > cc: Kenneth Graunke
> > cc: mesa-dev@lists.freedesktop.org
> > cc: Jordan Justen
> > cc: Tony Ye
> > Reviewed-by: Matthew Auld
> > ---
> > include/uapi/drm/i915_drm.h | 16
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index a2def7b27009..b7e1c2fe08dc 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -3443,6 +3443,22 @@ struct drm_i915_gem_create_ext {
> >* At which point we get the object handle in
> > _i915_gem_create_ext.handle,
> >* along with the final object size in _i915_gem_create_ext.size,
> > which
> >* should account for any rounding up, if required.
> > + *
> > + * Note that userspace has no means of knowing the current backing region
> > + * for objects where @num_regions is larger than one. The kernel will only
> > + * ensure that the priority order of the @regions array is honoured, either
> > + * when initially placing the object, or when moving memory around due to
> > + * memory pressure
> > + *
> > + * On Flat-CCS capable HW, compression is supported for the objects
> > residing
> > + * in I915_MEMORY_CLASS_DEVICE. When such objects (compressed) has other
> > + * memory class in @regions and migrated (by I915, due to memory
> > + * constrain) to the non I915_MEMORY_CLASS_DEVICE region, then I915 needs
> > to
> > + * decompress the content. But I915 dosen't have the required information
> > to
> > + * decompress the userspace compressed objects.
> > + *
> > + * So I915 supports Flat-CCS, only on the objects which can reside only on
> > + * I915_MEMORY_CLASS_DEVICE regions.
>
> I think it's fine to assume Flat-CSS surface will always be in lmem.
>
> I see no issue for the Anv Vulkan driver.
>
> Maybe Nanley or Ken can speak for the Iris GL driver?
>
Acked-by: Jordan Justen
I think Nanley has accounted for this on iris with:
https://gitlab.freedesktop.org/mesa/mesa/-/commit/42a865730ef72574e179b56a314f30fdccc6cba8
-Jordan
Re: [PATCH v8 5/5] drm/i915/uapi: document behaviour for DG2 64K support
Ramalingam C writes: > On 2022-02-18 at 18:06:00 +, Robert Beckett wrote: >> >> If desired, we can make the wording clearer, maybe something like: >> >> "To keep things simple for userland, we mandate that any GTT mappings >> must be aligned to 2MB. The kernel will internally pad them out to the next >> 2MB boundary" > > Added the extra information in next version @ > https://patchwork.freedesktop.org/patch/475166/?series=100419=1 > > Jordan, hope this explanation clears your doubt. Ok. It sounds like what we are doing in Mesa matches what is required by hardware and the kernel. Thanks. -Jordan
Re: [PATCH v8 5/5] drm/i915/uapi: document behaviour for DG2 64K support
Robert Beckett writes:
> From: Matthew Auld
>
> On discrete platforms like DG2, we need to support a minimum page size
> of 64K when dealing with device local-memory. This is quite tricky for
> various reasons, so try to document the new implicit uapi for this.
>
> v3: fix typos and less emphasis
> v2: Fixed suggestions on formatting [Daniel]
>
> Signed-off-by: Matthew Auld
> Signed-off-by: Ramalingam C
> Signed-off-by: Robert Beckett
> Acked-by: Jordan Justen
> Reviewed-by: Ramalingam C
> Reviewed-by: Thomas Hellström
> cc: Simon Ser
> cc: Pekka Paalanen
> Cc: Jordan Justen
> Cc: Kenneth Graunke
> Cc: mesa-dev@lists.freedesktop.org
> Cc: Tony Ye
> Cc: Slawomir Milczarek
> ---
> include/uapi/drm/i915_drm.h | 44 -
> 1 file changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 5e678917da70..77e5e74c32c1 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1118,10 +1118,16 @@ struct drm_i915_gem_exec_object2 {
> /**
>* When the EXEC_OBJECT_PINNED flag is specified this is populated by
>* the user with the GTT offset at which this object will be pinned.
> + *
>* When the I915_EXEC_NO_RELOC flag is specified this must contain the
>* presumed_offset of the object.
> + *
>* During execbuffer2 the kernel populates it with the value of the
>* current GTT offset of the object, for future presumed_offset writes.
> + *
> + * See struct drm_i915_gem_create_ext for the rules when dealing with
> + * alignment restrictions with I915_MEMORY_CLASS_DEVICE, on devices with
> + * minimum page sizes, like DG2.
>*/
> __u64 offset;
>
> @@ -3145,11 +3151,39 @@ struct drm_i915_gem_create_ext {
>*
>* The (page-aligned) allocated size for the object will be returned.
>*
> - * Note that for some devices we have might have further minimum
> - * page-size restrictions(larger than 4K), like for device local-memory.
> - * However in general the final size here should always reflect any
> - * rounding up, if for example using the
> I915_GEM_CREATE_EXT_MEMORY_REGIONS
> - * extension to place the object in device local-memory.
> + *
> + * DG2 64K min page size implications:
> + *
> + * On discrete platforms, starting from DG2, we have to contend with GTT
> + * page size restrictions when dealing with I915_MEMORY_CLASS_DEVICE
> + * objects. Specifically the hardware only supports 64K or larger GTT
> + * page sizes for such memory. The kernel will already ensure that all
> + * I915_MEMORY_CLASS_DEVICE memory is allocated using 64K or larger page
> + * sizes underneath.
> + *
> + * Note that the returned size here will always reflect any required
> + * rounding up done by the kernel, i.e 4K will now become 64K on devices
> + * such as DG2.
> + *
> + * Special DG2 GTT address alignment requirement:
> + *
> + * The GTT alignment will also need to be at least 2M for such objects.
> + *
> + * Note that due to how the hardware implements 64K GTT page support, we
> + * have some further complications:
> + *
> + * 1) The entire PDE (which covers a 2MB virtual address range), must
> + * contain only 64K PTEs, i.e mixing 4K and 64K PTEs in the same
> + * PDE is forbidden by the hardware.
> + *
> + * 2) We still need to support 4K PTEs for I915_MEMORY_CLASS_SYSTEM
> + * objects.
> + *
> + * To keep things simple for userland, we mandate that any GTT mappings
> + * must be aligned to and rounded up to 2MB.
Could I get a clarification about this "rounded up" part.
Currently Mesa is aligning the start of each and every buffer VMA to be
2MiB aligned. But, we are *not* taking any steps to "round up" the size
of buffers to 2MiB alignment.
Bob's Mesa MR from a while ago,
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14599
was trying to add this "round up" size for buffers. We didn't accept
this MR because we thought if we have ensured that no other buffer will
use the same 2MiB VMA range, then it should not be required.
If what we are doing is ok, then maybe this "round up" language should
be dropped? Or, perhaps the "round up" mentioned here isn't implying we
must align the size of buffers that we create, and I'm misinterpreting
this.
-Jordan
> As this only wastes virtual
> + * address space and avoids userland having to copy any needlessly
> + * complicated PDE sharing scheme (coloring) and only affects DG2, this
> + * is deemed to be a good compromise.
>*/
> __u64 size;
> /**
> --
> 2.25.1
Re: [PATCH v2 4/4] drm/i915/uapi: document behaviour for DG2 64K support
Robert Beckett writes:
> From: Matthew Auld
>
> On discrete platforms like DG2, we need to support a minimum page size
> of 64K when dealing with device local-memory. This is quite tricky for
> various reasons, so try to document the new implicit uapi for this.
>
> v2: Fixed suggestions on formatting [Daniel]
>
> Signed-off-by: Matthew Auld
> Signed-off-by: Ramalingam C
> Signed-off-by: Robert Beckett
> cc: Simon Ser
> cc: Pekka Paalanen
> Cc: Jordan Justen
> Cc: Kenneth Graunke
> Cc: mesa-dev@lists.freedesktop.org
> Cc: Tony Ye
> Cc: Slawomir Milczarek
> ---
> include/uapi/drm/i915_drm.h | 44 -
> 1 file changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 5e678917da70..486b7b96291e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1118,10 +1118,16 @@ struct drm_i915_gem_exec_object2 {
> /**
>* When the EXEC_OBJECT_PINNED flag is specified this is populated by
>* the user with the GTT offset at which this object will be pinned.
> + *
>* When the I915_EXEC_NO_RELOC flag is specified this must contain the
>* presumed_offset of the object.
> + *
>* During execbuffer2 the kernel populates it with the value of the
>* current GTT offset of the object, for future presumed_offset writes.
> + *
> + * See struct drm_i915_gem_create_ext for the rules when dealing with
> + * alignment restrictions with I915_MEMORY_CLASS_DEVICE, on devices with
> + * minimum page sizes, like DG2.
>*/
> __u64 offset;
>
> @@ -3145,11 +3151,39 @@ struct drm_i915_gem_create_ext {
>*
>* The (page-aligned) allocated size for the object will be returned.
>*
> - * Note that for some devices we have might have further minimum
> - * page-size restrictions(larger than 4K), like for device local-memory.
> - * However in general the final size here should always reflect any
> - * rounding up, if for example using the
> I915_GEM_CREATE_EXT_MEMORY_REGIONS
> - * extension to place the object in device local-memory.
> + *
> + * **DG2 64K min page size implications:**
Long term, I'm not sure that the "**" (for emphasis) is needed here or
below. It's interesting at the moment, but will be just another thing
baked into the kernel/user code in a month from now. :)
> + *
> + * On discrete platforms, starting from DG2, we have to contend with GTT
> + * page size restrictions when dealing with I915_MEMORY_CLASS_DEVICE
> + * objects. Specifically the hardware only supports 64K or larger GTT
> + * page sizes for such memory. The kernel will already ensure that all
> + * I915_MEMORY_CLASS_DEVICE memory is allocated using 64K or larger page
> + * sizes underneath.
> + *
> + * Note that the returned size here will always reflect any required
> + * rounding up done by the kernel, i.e 4K will now become 64K on devices
> + * such as DG2.
> + *
> + * **Special DG2 GTT address alignment requirement:**
> + *
> + * The GTT alignment will also need be at least 2M for such objects.
> + *
> + * Note that due to how the hardware implements 64K GTT page support, we
> + * have some further complications:
> + *
> + * 1) The entire PDE(which covers a 2MB virtual address range), must
> + * contain only 64K PTEs, i.e mixing 4K and 64K PTEs in the same
> + * PDE is forbidden by the hardware.
> + *
> + * 2) We still need to support 4K PTEs for I915_MEMORY_CLASS_SYSTEM
> + * objects.
> + *
> + * To keep things simple for userland, we mandate that any GTT mappings
> + * must be aligned to and rounded up to 2MB. As this only wastes virtual
> + * address space and avoids userland having to copy any needlessly
> + * complicated PDE sharing scheme (coloring) and only affects GD2, this
> + * id deemed to be a good compromise.
typos: GD2, id
Isn't much of this more relavent to the vma offset at exec time? Is
there actually any new restriction on the size field during buffer
creation?
I see Matthew references these notes from the offset comments, so if the
kernel devs prefer it here, then you can add my Acked-by on this patch.
-Jordan
>*/
> __u64 size;
> /**
> --
> 2.25.1
Re: [Mesa-dev] Workflow Proposal
Daniel Stone writes: > On Wed, 13 Oct 2021 at 20:13, Jordan Justen wrote: >> Alyssa Rosenzweig writes: >> > >> > Sorry, I'll make that point more emphatic. >> > >> > Upstream must do what's best for upstream without zero regard for the >> > whims of management. Doubly so for bad management. >> >> If the r-b process ever had any notice from any company's management, I >> haven't seen it. (Actually, I think most management would rather have >> the short sighted view of skipping code review to more quickly merge >> patches.) In terms of who to "track down", that is also a tenuous >> connection. > > All of the above is true but also totally irrelevant to the actual discussion. > > When R-b as a metric came up at the time of the first switch, I wrote > a really trivial Python script which used the GitLab API to scrape MR > discussions and pull 'Reviewed-by: ...' comments out and print a > leaderboard for number of reviewed MRs over the past calendar month. > Adapting that to look at approvals rather than comments would cut it > down to about 10 LoC. > > Whether it's Reviewed-by in the trailer or an approval, both are > explicitly designed to be machine readable, which means it's trivial > to turn it into a metric if you want to. Whether or not that's a good > idea is the problem of whoever wields it. Correct. So let's not try to play whac-a-mole/manager/evil-genius. :) I think several people have already said that it's good to take the time to recognize the often overlooked and thankless job of reviewing. Potentially stripping Reviewed-by tags seems counter to that. Adding Approved-by from the web page could be helpful to simplify reviews for many merge-requests. But, retaining per-patch Reviewed-by is helpful in other cases. So hopefully we can add Approved-by *without* destroying Reviewed-by tags in the process. I would like to be able to use either in the cases where they make sense. -Jordan
Re: [Mesa-dev] Workflow Proposal
Alyssa Rosenzweig writes: >> Upstream should do what's best for upstream, not for Intel's "unique" >> management. >> >>Not sure how from Emma explaining how Rb tags were used by Intel >>management it came the conclusion that it were used in that way only by >>Intel management. Spoiler: it is not. > > Sorry, I'll make that point more emphatic. > > Upstream must do what's best for upstream without zero regard for the > whims of management. Doubly so for bad management. If the r-b process ever had any notice from any company's management, I haven't seen it. (Actually, I think most management would rather have the short sighted view of skipping code review to more quickly merge patches.) In terms of who to "track down", that is also a tenuous connection. The value of r-b is to give reviewers credit for the hard work that they do. (Which, I believe is what Matt and apinheiro are also saying.) Personally I try to make a rework log on patch commit messages to give reviewers more explicit credit for changes that are made based on their code review. I hope Marge Bot doesn't start stripping the r-b tags. But, if Marge can add Approved-by which allows the review process to flow more quickly for some types of merge-requests, then I think that's a good thing. I wouldn't be surprised if this became the most commonly used review process in Mesa. -Jordan
Re: [Mesa-dev] Workflow Proposal
Bas Nieuwenhuizen writes: > On Wed, Oct 6, 2021 at 8:49 PM Jordan Justen > wrote: >> >> I guess I missed where it was suggested that Marge should remove >> Reviewed-by tags. I don't think Marge should ever remove something from >> the commit message. > > AFAIU this is upstream Marge behavior. Once you enable the > Approval->Rb tag conversion it removes existing Rb tags. Hence why we > don't have the conversion enabled. > Ah, I guess it is documented for --add-reviewers here: https://github.com/smarkets/marge-bot#adding-reviewed-by-tested-and-part-of-to-commit-messages "All existing Reviewed-by: trailers on commits in the branch will be stripped." I hope we would wait for Marge to add a --add-approvers switch which would leave Reviewed-by tags alone, but add Approved-by tags. -Jordan
Re: [Mesa-dev] Workflow Proposal
Mike Blumenkrantz writes: > On Wed, Oct 6, 2021 at 1:27 PM Bas Nieuwenhuizen > wrote: > >> On Wed, Oct 6, 2021 at 7:07 PM Jason Ekstrand >> wrote: >> > >> > My primary grip with approvals or the button is that it's the wrong >> > granularity. It's per-MR instead of per-patch. When people are >> > regularly posting MRs that touch a bunch of different stuff, per-patch >> > review is pretty common. I'm not sure I want to lose that. :-/ Could a hybrid approach work? Marge could just add: Approved-by: @jljusten to the commit message based on the state of the MR. But, for MR's where r-b is more appropriate, the developer can still manually add Reviewed-by. Personally I don't find adding the r-b and force pushing to be much of a burden, but I could see how in some cases of a small MR, it could be nice to just click some buttons on the web-page and be done with it. But, I really would like Marge to add something to the commit messages indicating who approved it. Yeah, you can get that info today by following the Part-of link, but there's no guarantees about that being around forever. >> Would it be an option to get Marge to not remove existing Rb tags, so >> we could get the streamlined process where possible and fall back if >> the MRs turn more complicated? I guess I missed where it was suggested that Marge should remove Reviewed-by tags. I don't think Marge should ever remove something from the commit message. > If people really, truly care about per-patch Approval, couldn't they just > split out patches from bigger MRs and get Approvals there? Otherwise it > should be trivial enough to check the gitlab MR and see who reviewed which > patch if it becomes an issue at a later date. Odds are at that point you're > already going to the MR to see wtf someone was thinking... I don't like the idea of saying "just split out the MRs". That doesn't work in a lot of cases where patches have dependencies, and just causes potential reviewers to have to look in more places to see the big picture. -Jordan
[Mesa-dev] NOTICE! Renaming Mesa "master" branch to "main" tomorrow
We plan to proceed with the branch rename tomorrow mid-day (US Pacific time) tomorrow, May 5. If all goes well, this MR will merge, https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/10474 and the master branch will be locked from further changes. The main branch will then be created and all open merge-requests will be changed to target the main branch. This will change the "update time" of all open MRs, but they should remain in the same order as the oldest ones will be updated first. Additional related tasks are being tracked in: https://gitlab.freedesktop.org/mesa/mesa/-/issues/4501 -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Rename "master" branch to "main"?
On 2021-03-25 08:11:45, Jason Ekstrand wrote: > On Thu, Mar 25, 2021 at 9:41 AM Rob Clark wrote: > > > > yeah, I meant to convert a repo w/ more MRs than 7 but less than mesa > > using the script to beta test that, wasn't suggesting to do it by hand > > Plan is to convert piglit next. It's sitting at about 60 open MRs. > Piglit's conversion went through last month without any issues. Therefore I opened an MR for mesa: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/10474 This MR makes some changes to the tree to rename the branch, and it adds an empty warning message commit. This warning message will live as the last commit on the upstream master branch for the forseeable future as a notice to developers that they need to change the upstream tracking ref to main. We also have a related issue open to track some other tasks related to the rename: https://gitlab.freedesktop.org/mesa/mesa/-/issues/4501 Timing wise, it's probably about the best time around now to maximize the time before the next release branchpoint. Regarding the 21.1 release, I think there could be arguments for before or after. One reason to go before the release is to work out any kinks before the potentially more urgent 20.1.1 release. But, before or after, I think Eric and Dylan will be more than capable of doing some extra checks to verify that the scripts are working for the stable branches. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] piglit default main branch results - Re: Rename "master" branch to "main"?
On 2021-03-24 23:47:46, Jordan Justen wrote: > On 2021-03-24 21:14:57, Jason Ekstrand wrote: > > > > Jordan, is there any way you can make the script sort by last updated > > before it > > re-targets the MRs so they at least stay in the same order? Updating them > > in MR > > # order wouldn't be bad either, I guess. > > > > It already does process them sorting by the oldest "update time" > first, so roughly speaking the order sorted by update time should be > the same. > > I don't know what might happen if 2 MRs were updated within the same > second. But, the updates are actually a bit slow (maybe ~1 update per > second), so there doesn't seem to be much risk, as far as I can see, > of, for instance, 10 updates happening within the same second. > Crucible and piglit are now changed over to using main. Piglit has had at least 2 MR's merge to main now: * https://gitlab.freedesktop.org/mesa/piglit/-/merge_requests/463 * https://gitlab.freedesktop.org/mesa/piglit/-/merge_requests/497 It appears that the "last updated sort order" stayed 100% the same before and after the script ran to update the open MR branch targets. It took ~48 seconds for the script to update 58 piglit merge-requests. I have only seen one issue so far for the piglit default branch change. It appears that the piglit patchwork scripting isn't configured for the main branch. I think it's long past time to retire the piglit patchwork, so I made a request to the admins to do that. I think Mesa already retired patchwork. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Rename "master" branch to "main"?
On 2021-03-24 21:14:57, Jason Ekstrand wrote: > > Jordan, is there any way you can make the script sort by last updated before > it > re-targets the MRs so they at least stay in the same order? Updating them in > MR > # order wouldn't be bad either, I guess. > It already does process them sorting by the oldest "update time" first, so roughly speaking the order sorted by update time should be the same. I don't know what might happen if 2 MRs were updated within the same second. But, the updates are actually a bit slow (maybe ~1 update per second), so there doesn't seem to be much risk, as far as I can see, of, for instance, 10 updates happening within the same second. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Rename "master" branch to "main"?
On 2021-03-23 09:38:59, Eric Anholt wrote: > On Tue, Mar 23, 2021 at 7:02 AM Jason Ekstrand wrote: > > > > Trying to pick this discussion back up. Daniel Stone thinks it's a > > half hour of API bashing to retarget all the MRs so, if the fd.o > > admins have some heads up, it should be tractable. Should we do this > > right after branching 21.1 along with the LTS branch? > > +1 Jason, I opened a related Mesa issue: https://gitlab.freedesktop.org/mesa/mesa/-/issues/4501 I made this change in crucible, and used a script to update the 7 MR target branches: https://gitlab.freedesktop.org/mesa/crucible/-/issues/5 As mentioned in the Mesa issue, I think we should use piglit as another test run before changing Mesa: https://gitlab.freedesktop.org/mesa/piglit/-/issues/50 Piglit currently has 60 open merge requests. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] fp/int64 on gen11/12?
On 2020-11-17 16:03:31, Brian Paul wrote: > Another Intel question: It looks like gen11/gen12 don't have fp/int64 > enabled in the Vulkan driver. From gen_device_info.c: > > #define GEN11_FEATURES(_gt, _slices, _subslices, _l3) \ > GEN8_FEATURES, \ > GEN11_HW_INFO, \ > .has_64bit_float = false, \ > .has_64bit_int = false, > ... > > #define GEN12_FEATURES(_gt, _slices, _l3) \ > GEN8_FEATURES, \ > GEN12_HW_INFO, \ > .has_64bit_float = false,\ > .has_64bit_int = false, > > But gen8/9 do support it. Is this a driver and/or hardware issue? > It matches the hardware. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa: save/restore SSO flag when using ARB_get_program_binary
Reviewed-by: Jordan Justen
On 2019-07-10 17:39:24, Timothy Arceri wrote:
> Ping! The spec bug was updated and they have agreed to update the spec
> to define this behavior.
>
> I've also sent a piglit test for this:
>
> https://patchwork.freedesktop.org/patch/317112/
>
> On 1/7/19 12:25 pm, Timothy Arceri wrote:
> > Without this the restored program will fail the pipeline validation
> > checks when we attempt to use an SSO program.
> >
> > Fixes: c20fd744fef1 ("mesa: Add Mesa ARB_get_program_binary helper
> > functions")
> >
> > Cc: Jordan Justen
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111010
> > ---
> > src/mesa/main/program_binary.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/src/mesa/main/program_binary.c b/src/mesa/main/program_binary.c
> > index 7390fef5887..39537cfccce 100644
> > --- a/src/mesa/main/program_binary.c
> > +++ b/src/mesa/main/program_binary.c
> > @@ -178,6 +178,8 @@ write_program_payload(struct gl_context *ctx, struct
> > blob *blob,
> > shader->Program);
> > }
> >
> > + blob_write_uint32(blob, sh_prog->SeparateShader);
> > +
> > serialize_glsl_program(blob, ctx, sh_prog);
> >
> > for (unsigned stage = 0; stage < MESA_SHADER_STAGES; stage++) {
> > @@ -195,6 +197,8 @@ static bool
> > read_program_payload(struct gl_context *ctx, struct blob_reader *blob,
> >GLenum binary_format, struct gl_shader_program
> > *sh_prog)
> > {
> > + sh_prog->SeparateShader = blob_read_uint32(blob);
> > +
> > if (!deserialize_glsl_program(blob, ctx, sh_prog))
> > return false;
> >
> >
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Intel-gfx] [PATCH 2/3] iris: Create a composite context for both compute and render pipelines
On 2019-03-31 03:02:21, Chris Wilson wrote:
> Quoting Jordan Justen (2019-03-31 10:57:09)
> > On 2019-03-25 03:58:59, Chris Wilson wrote:
> > > iris currently uses two distinct GEM contexts to have distinct logical
> > > HW contexts for the compute and render pipelines. However, using two
> > > distinct GEM contexts implies that they are distinct timelines, yet as
> > > they are a single GL context that implies they belong to a single
> > > timeline from the client perspective. Currently, fences are occasionally
> > > inserted to order the two timelines. Using 2 GEM contexts, also implies
> > > that we keep 2 ppGTT for identical buffer state. If we can create a
> > > single GEM context, with the right capabilities, we can have a single
> > > VM, a single timeline, but 2 logical HW contexts for the 2 pipelines.
> > >
> > > This is allowed through the new context interface that allows VM to be
> > > shared, timelines to be specified, and for the logical contexts to be
> > > constructed as the user desires.
> > >
> > > Cc: Joonas Lahtinen
> > > Cc: Kenneth Graunke
> > > ---
> > > src/gallium/drivers/iris/iris_batch.c | 16 ++-
> > > src/gallium/drivers/iris/iris_batch.h | 5 +--
> > > src/gallium/drivers/iris/iris_context.c | 56 -
> > > 3 files changed, 60 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/src/gallium/drivers/iris/iris_batch.c
> > > b/src/gallium/drivers/iris/iris_batch.c
> > > index 556422c38bc..40bcd939795 100644
> > > --- a/src/gallium/drivers/iris/iris_batch.c
> > > +++ b/src/gallium/drivers/iris/iris_batch.c
> > > @@ -164,24 +164,16 @@ iris_init_batch(struct iris_batch *batch,
> > > struct iris_vtable *vtbl,
> > > struct pipe_debug_callback *dbg,
> > > struct iris_batch *all_batches,
> > > -enum iris_batch_name name,
> > > -uint8_t engine,
> > > -int priority)
> > > +uint32_t hw_id,
> > > +enum iris_batch_name name)
> > > {
> > > batch->screen = screen;
> > > batch->vtbl = vtbl;
> > > batch->dbg = dbg;
> > > batch->name = name;
> > >
> > > - /* engine should be one of I915_EXEC_RENDER, I915_EXEC_BLT, etc. */
> > > - assert((engine & ~I915_EXEC_RING_MASK) == 0);
> > > - assert(util_bitcount(engine) == 1);
> > > - batch->engine = engine;
> > > -
> > > - batch->hw_ctx_id = iris_create_hw_context(screen->bufmgr);
> > > - assert(batch->hw_ctx_id);
> > > -
> > > - iris_hw_context_set_priority(screen->bufmgr, batch->hw_ctx_id,
> > > priority);
> > > + batch->hw_ctx_id = hw_id;
> > > + batch->engine = name;
> >
> > I think this should fall-back to I915_EXEC_RENDER if the old style
> > contexts are created.
>
> It uses the legacy uABI to map both name=COMPUTE to the render ring
> and name=RENDER to the render ring.
Oh, what legacy uABI sets that? I thought if engines weren't set then
I915_EXEC_RENDER would be needed for the two contexts to execute on
the rcs. But, after this patch, isn't name 0 and 1?
I guess I915_EXEC_DEFAULT is 0 and I915_EXEC_RENDER is 1. So, will
I915_EXEC_DEFAULT execute on rcs too? If so, I'm not sure I agree with
taking advantage of this coincidence without even a comment.
-Jordan
> Should we have more or either, or
> start using the many video engines, ctx->engines[] will be the only way
> to define the execbuf mapping. So name == engine, as used today, can
> remain invariant across the legacy I915_EXEC_RING API and future
> ctx->engines[] for the remaining forseeable future of GEM_EXECBUFFER2.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Intel-gfx] [PATCH 2/3] iris: Create a composite context for both compute and render pipelines
On 2019-03-25 03:58:59, Chris Wilson wrote:
> iris currently uses two distinct GEM contexts to have distinct logical
> HW contexts for the compute and render pipelines. However, using two
> distinct GEM contexts implies that they are distinct timelines, yet as
> they are a single GL context that implies they belong to a single
> timeline from the client perspective. Currently, fences are occasionally
> inserted to order the two timelines. Using 2 GEM contexts, also implies
> that we keep 2 ppGTT for identical buffer state. If we can create a
> single GEM context, with the right capabilities, we can have a single
> VM, a single timeline, but 2 logical HW contexts for the 2 pipelines.
>
> This is allowed through the new context interface that allows VM to be
> shared, timelines to be specified, and for the logical contexts to be
> constructed as the user desires.
>
> Cc: Joonas Lahtinen
> Cc: Kenneth Graunke
> ---
> src/gallium/drivers/iris/iris_batch.c | 16 ++-
> src/gallium/drivers/iris/iris_batch.h | 5 +--
> src/gallium/drivers/iris/iris_context.c | 56 -
> 3 files changed, 60 insertions(+), 17 deletions(-)
>
> diff --git a/src/gallium/drivers/iris/iris_batch.c
> b/src/gallium/drivers/iris/iris_batch.c
> index 556422c38bc..40bcd939795 100644
> --- a/src/gallium/drivers/iris/iris_batch.c
> +++ b/src/gallium/drivers/iris/iris_batch.c
> @@ -164,24 +164,16 @@ iris_init_batch(struct iris_batch *batch,
> struct iris_vtable *vtbl,
> struct pipe_debug_callback *dbg,
> struct iris_batch *all_batches,
> -enum iris_batch_name name,
> -uint8_t engine,
> -int priority)
> +uint32_t hw_id,
> +enum iris_batch_name name)
> {
> batch->screen = screen;
> batch->vtbl = vtbl;
> batch->dbg = dbg;
> batch->name = name;
>
> - /* engine should be one of I915_EXEC_RENDER, I915_EXEC_BLT, etc. */
> - assert((engine & ~I915_EXEC_RING_MASK) == 0);
> - assert(util_bitcount(engine) == 1);
> - batch->engine = engine;
> -
> - batch->hw_ctx_id = iris_create_hw_context(screen->bufmgr);
> - assert(batch->hw_ctx_id);
> -
> - iris_hw_context_set_priority(screen->bufmgr, batch->hw_ctx_id, priority);
> + batch->hw_ctx_id = hw_id;
> + batch->engine = name;
I think this should fall-back to I915_EXEC_RENDER if the old style
contexts are created.
-Jordan
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] drm-uapi: Pull i915_drm.h changes for context cloning
Where are these changes from (repo/commit)? It could be good to
reference in the commit message.
I suspect that the answer might mean that these patches should be
labeled RFC.
-Jordan
On 2019-03-25 03:58:58, Chris Wilson wrote:
> For use in GPU recovery and pipeline construction.
> ---
> include/drm-uapi/i915_drm.h | 389 +---
> 1 file changed, 317 insertions(+), 72 deletions(-)
>
> diff --git a/include/drm-uapi/i915_drm.h b/include/drm-uapi/i915_drm.h
> index d2792ab3640..59baacd265d 100644
> --- a/include/drm-uapi/i915_drm.h
> +++ b/include/drm-uapi/i915_drm.h
> @@ -62,6 +62,28 @@ extern "C" {
> #define I915_ERROR_UEVENT "ERROR"
> #define I915_RESET_UEVENT "RESET"
>
> +/*
> + * i915_user_extension: Base class for defining a chain of extensions
> + *
> + * Many interfaces need to grow over time. In most cases we can simply
> + * extend the struct and have userspace pass in more data. Another option,
> + * as demonstrated by Vulkan's approach to providing extensions for forward
> + * and backward compatibility, is to use a list of optional structs to
> + * provide those extra details.
> + *
> + * The key advantage to using an extension chain is that it allows us to
> + * redefine the interface more easily than an ever growing struct of
> + * increasing complexity, and for large parts of that interface to be
> + * entirely optional. The downside is more pointer chasing; chasing across
> + * the boundary with pointers encapsulated inside u64.
> + */
> +struct i915_user_extension {
> + __u64 next_extension;
> + __u32 name;
> + __u32 flags; /* All undefined bits must be zero. */
> + __u32 rsvd[4]; /* Reserved for future use; must be zero. */
> +};
> +
> /*
> * MOCS indexes used for GPU surfaces, defining the cacheability of the
> * surface data and the coherency for this data wrt. CPU vs. GPU accesses.
> @@ -99,9 +121,14 @@ enum drm_i915_gem_engine_class {
> I915_ENGINE_CLASS_VIDEO = 2,
> I915_ENGINE_CLASS_VIDEO_ENHANCE = 3,
>
> + /* should be kept compact */
> +
> I915_ENGINE_CLASS_INVALID = -1
> };
>
> +#define I915_ENGINE_CLASS_INVALID_NONE -1
> +#define I915_ENGINE_CLASS_INVALID_VIRTUAL 0
> +
> /**
> * DOC: perf_events exposed by i915 through
> /sys/bus/event_sources/drivers/i915
> *
> @@ -319,6 +346,9 @@ typedef struct _drm_i915_sarea {
> #define DRM_I915_PERF_ADD_CONFIG 0x37
> #define DRM_I915_PERF_REMOVE_CONFIG0x38
> #define DRM_I915_QUERY 0x39
> +#define DRM_I915_GEM_VM_CREATE 0x3a
> +#define DRM_I915_GEM_VM_DESTROY0x3b
> +/* Must be kept compact -- no holes */
>
> #define DRM_IOCTL_I915_INITDRM_IOW( DRM_COMMAND_BASE +
> DRM_I915_INIT, drm_i915_init_t)
> #define DRM_IOCTL_I915_FLUSH DRM_IO ( DRM_COMMAND_BASE +
> DRM_I915_FLUSH)
> @@ -367,6 +397,7 @@ typedef struct _drm_i915_sarea {
> #define DRM_IOCTL_I915_GET_SPRITE_COLORKEY DRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_GET_SPRITE_COLORKEY, struct drm_intel_sprite_colorkey)
> #define DRM_IOCTL_I915_GEM_WAITDRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_GEM_WAIT, struct drm_i915_gem_wait)
> #define DRM_IOCTL_I915_GEM_CONTEXT_CREATE DRM_IOWR (DRM_COMMAND_BASE +
> DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
> +#define DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT DRM_IOWR (DRM_COMMAND_BASE +
> DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create_ext)
> #define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY DRM_IOW (DRM_COMMAND_BASE +
> DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
> #define DRM_IOCTL_I915_REG_READDRM_IOWR
> (DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
> #define DRM_IOCTL_I915_GET_RESET_STATS DRM_IOWR (DRM_COMMAND_BASE +
> DRM_I915_GET_RESET_STATS, struct drm_i915_reset_stats)
> @@ -377,6 +408,8 @@ typedef struct _drm_i915_sarea {
> #define DRM_IOCTL_I915_PERF_ADD_CONFIG DRM_IOW(DRM_COMMAND_BASE +
> DRM_I915_PERF_ADD_CONFIG, struct drm_i915_perf_oa_config)
> #define DRM_IOCTL_I915_PERF_REMOVE_CONFIG DRM_IOW(DRM_COMMAND_BASE +
> DRM_I915_PERF_REMOVE_CONFIG, __u64)
> #define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_QUERY, struct drm_i915_query)
> +#define DRM_IOCTL_I915_GEM_VM_CREATE DRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_GEM_VM_CREATE, struct drm_i915_gem_vm_control)
> +#define DRM_IOCTL_I915_GEM_VM_DESTROY DRM_IOW (DRM_COMMAND_BASE +
> DRM_I915_GEM_VM_DESTROY, struct drm_i915_gem_vm_control)
>
> /* Allow drivers to submit batchbuffers directly to hardware, relying
> * on the security mechanisms provided by hardware.
> @@ -476,6 +509,7 @@ typedef struct drm_i915_irq_wait {
> #define I915_SCHEDULER_CAP_ENABLED (1ul << 0)
> #define I915_SCHEDULER_CAP_PRIORITY (1ul << 1)
> #define I915_SCHEDULER_CAP_PREEMPTION(1ul << 2)
> +#define
Re: [Mesa-dev] [PATCH] android, autotools: st/mesa: fix location of float64_glsl.h
On 2019-03-04 16:11:33, Jordan Justen wrote:
> On 2019-03-04 15:21:26, Mauro Rossi wrote:
> > Hi,
> > On Mon, Mar 4, 2019 at 7:50 PM Mauro Rossi wrote:
> > >
> > > Il lun 4 mar 2019, 18:59 Dylan Baker ha scritto:
> > >>
> > >> I wrote c812, and I'd be very happy to revert it. The problem that I ran
> > >> into
> > >> was putting it in a place that android, meson, and the autotools dist
> > >> tarball
> > >> were happy with. I'm hoping that we can remove autotools in the very
> > >> near future
> > >> and just revert c812 at that time. Does that seem like a reasonable
> > >> solution?
> > >>
> > >> Dylan
> >
> > Hi Dylan,
> > I have checked on Android.mk and reverting you commit c812c740e6
> > would be very complex.
> >
> > The best way is to proceed now is with my patch to un-break the Android
> > build
> >
> > Then as a reference, the future solution may require chages to sources:
> >
> > #include "compiler/glsl/{header}.h"
> > to become
> > #include "glsl/{header}.h"
> >
> > in order to reach the Android generated files with
> > $(call generated-sources-dir-for,STATIC_LIBRARIES,libmesa_glsl,,)
> > as equivalent of -I$(top_builddir)/src/compiler
>
> Just wanted to point out that Jason is moving this under glsl in:
>
> https://gitlab.freedesktop.org/mesa/mesa/merge_requests/371
Mauro,
Jason pushed MR 371 in 9ab1b1d0227499b7ff6a61fdebe75693212a67f5.
> So, either that will fix this, or else cause it to break in a new,
> different way. :)
Which was it? :)
-Jordan
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] android, autotools: st/mesa: fix location of float64_glsl.h
On 2019-03-04 15:21:26, Mauro Rossi wrote:
> Hi,
> On Mon, Mar 4, 2019 at 7:50 PM Mauro Rossi wrote:
> >
> > Il lun 4 mar 2019, 18:59 Dylan Baker ha scritto:
> >>
> >> I wrote c812, and I'd be very happy to revert it. The problem that I ran
> >> into
> >> was putting it in a place that android, meson, and the autotools dist
> >> tarball
> >> were happy with. I'm hoping that we can remove autotools in the very near
> >> future
> >> and just revert c812 at that time. Does that seem like a reasonable
> >> solution?
> >>
> >> Dylan
>
> Hi Dylan,
> I have checked on Android.mk and reverting you commit c812c740e6
> would be very complex.
>
> The best way is to proceed now is with my patch to un-break the Android build
>
> Then as a reference, the future solution may require chages to sources:
>
> #include "compiler/glsl/{header}.h"
> to become
> #include "glsl/{header}.h"
>
> in order to reach the Android generated files with
> $(call generated-sources-dir-for,STATIC_LIBRARIES,libmesa_glsl,,)
> as equivalent of -I$(top_builddir)/src/compiler
Just wanted to point out that Jason is moving this under glsl in:
https://gitlab.freedesktop.org/mesa/mesa/merge_requests/371
So, either that will fix this, or else cause it to break in a new,
different way. :)
-Jordan
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] Should piglit drop bugzilla and move to using gitlab issues?
I guess piglit makes very light usage of bugzilla. Would it be simpler to just use gitlab issues in the piglit gitlab project? -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] Piglit gitlab merge requests enabled - Re: GitLab migration of Piglit
On 2019-02-20 07:36:48, Den wrote: > > Given the discussion below, I think we'll make piglit a sub-project of > > mesa. Those who need commit access to piglit but not mesa can be > > added directly to the piglit project. > > Hi list. > > Since piglit was also moved to the gitlab, same with mesa, our team is > interested in process workflow for contributing to it. Before (again, > same with mesa) we created mailing threads and after reviewing test was > pushed to master by somebody with access. > Now mesa got a new possibility for reviewing - merge requests, which > doesn't exist in piglit. Also, according to Jason's conclusion, anybody > can request commit access to piglit. But in this case how the review > process will be done? I notice that no one replied to Den with concern about using merge requests for piglit. Several of us discussed this yesterday, and it seems even more useful for the piglit project to use merge requests than Mesa. Therefore, I enabled merge requests on the piglit project. If no one objects, then I think we should document it on the piglit webpage. Does anyone have concerns about this plan? -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: Document and *require* usage of Signed-off-by
Part 3, wherein I regroup, and once again present an option where Signed-off-by is optional. (Or ... required :) https://gitlab.freedesktop.org/mesa/mesa/merge_requests/31 I turned it into 3 patches. > 21f1070b6ef docs: Add developer-certificate-of-origin.txt Adds the DCO 1.1 as a separate text file. > c6213abf55d docs: Document the optional usage of Signed-off-by 2/3 points at the DCO 1.1 text and says that if you add Signed-off-by, then your contribution follows the DCO 1.1 guidelines. But, the usage of Signed-off-by would still be optional. > f510fb75cd2 docs: Document and *require* usage of Signed-off-by Make Signed-off-by required. So, bring your pitchforks, torches, NACKs and other implements of discussion to MR #31. :) -Jordan On 2018-11-27 23:20:22, Jordan Justen wrote: > This adds the "Developer's Certificate of Origin 1.1" from the Linux > kernel. It indicates that by using Signed-off-by you are certifying > that your patch meets the DCO 1.1 guidelines. > > It also changes Signed-off-by from being optional to being required. > > Signed-off-by: Jordan Justen > Cc: Matt Turner > --- > docs/submittingpatches.html | 52 - > 1 file changed, 51 insertions(+), 1 deletion(-) > > diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html > index 9ae750d5a15..6d506b3691b 100644 > --- a/docs/submittingpatches.html > +++ b/docs/submittingpatches.html > @@ -20,6 +20,8 @@ > > Basic guidelines > Patch formatting > +Patch Signing (Signed-off-by, Developer's > + Certificate of Origin) > Testing Patches > Mailing Patches > Reviewing Patches > @@ -73,7 +75,9 @@ if needed. For example: > is necessary, and removing it causes no regressions in piglit on any > platform. > > -A "Signed-off-by:" line is not required, but not discouraged either. > +A "Signed-off-by:" line is required. The format > +and meaning of Signed-off-by is documented below in > +the patch signing section. > If a patch addresses a bugzilla issue, that should be noted in the > patch comment. For example: > > @@ -129,7 +133,53 @@ Please use common sense and do not > blindly add everyone. > > > > + > + Patch Signing (Signed-off-by, Developer's Certificate of Origin) > + > > + > + As described in the patch formatting > + section, you must sign your patch by including Signed-off-by in the > + patch commit message. The Signed-off-by must include your real name > + and email address in this format: > + > + > + Signed-off-by: Joe Hacker jhac...@foo.com > + > + > + By adding Signed-of-by to your contributed patch, you certify that > + your contribution meets the guidelines of the Developer's > + Certificate of Origin: > + > + > + > +Developer's Certificate of Origin 1.1 > +- > + > +By making a contribution to this project, I certify that: > + > + (a) The contribution was created in whole or in part by me and I > + have the right to submit it under the open source license > + indicated in the file; or > + > + (b) The contribution is based upon previous work that, to the best > + of my knowledge, is covered under an appropriate open source > + license and I have the right under that license to submit that > + work with modifications, whether created in whole or in part > + by me, under the same open source license (unless I am > + permitted to submit under a different license), as indicated > + in the file; or > + > + (c) The contribution was provided directly to me by some other > + person who certified (a), (b) or (c) and I have not modified > + it. > + > + (d) I understand and agree that this project and the contribution > + are public and that a record of the contribution (including all > + personal information I submit with it, including my sign-off) is > + maintained indefinitely and may be redistributed consistent with > + this project or the open source license(s) involved. > + > > Testing Patches > > -- > 2.20.0.rc1 > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] genxml: Consistently use a numeric "MOCS" field
Reviewed-by: Jordan Justen On 2018-12-11 20:23:06, Kenneth Graunke wrote: > When we first started using genxml, we decided to represent MOCS as an > actual structure, and pack values. However, in many places, it was more > convenient to use a numeric value rather than treating it as a struct, > so we added secondary setters in a bunch of places as well. > > We were not entirely consistent, either. Some places only had one. > Gen6 had both kinds of setters for STATE_BASE_ADDRESS, but newer gens > only had the struct-based setters. The names were sometimes "Constant > Buffer Object Control State" instead of "Memory", making it harder to > find. Many had prefixes like "Vertex Buffer MOCS"...in a vertex buffer > packet...which is a bit redundant. > > On modern hardware, MOCS is simply an index into a table, but we were > still carrying around the structure with an "Index to MOCS Table" field, > in addition to the direct numeric setters. This is clunky - we really > just want a number on new hardware. > > This patch eliminates the struct-based setters, and makes the numeric > setters be consistently called "MOCS". We leave the struct definition > around on Gen7-8 for reference purposes, but it is unused. > --- > src/intel/blorp/blorp_genX_exec.h | 2 +- > src/intel/genxml/gen10.xml| 53 + > src/intel/genxml/gen11.xml| 53 + > src/intel/genxml/gen6.xml | 28 ++- > src/intel/genxml/gen7.xml | 35 - > src/intel/genxml/gen75.xml| 38 -- > src/intel/genxml/gen8.xml | 47 +--- > src/intel/genxml/gen9.xml | 50 +--- > src/intel/isl/isl_emit_depth_stencil.c| 6 +- > src/intel/vulkan/anv_private.h| 76 --- > src/intel/vulkan/gen7_cmd_buffer.c| 2 +- > src/intel/vulkan/gen8_cmd_buffer.c| 2 +- > src/intel/vulkan/genX_cmd_buffer.c| 19 +++-- > src/intel/vulkan/genX_gpu_memcpy.c| 4 +- > src/intel/vulkan/genX_state.c | 6 +- > src/mesa/drivers/dri/i965/genX_state_upload.c | 14 ++-- > 16 files changed, 177 insertions(+), 258 deletions(-) > > diff --git a/src/intel/blorp/blorp_genX_exec.h > b/src/intel/blorp/blorp_genX_exec.h > index 065980616ec..42494ffbc86 100644 > --- a/src/intel/blorp/blorp_genX_exec.h > +++ b/src/intel/blorp/blorp_genX_exec.h > @@ -311,7 +311,7 @@ blorp_fill_vertex_buffer_state(struct blorp_batch *batch, > vb[idx].BufferPitch = stride; > > #if GEN_GEN >= 6 > - vb[idx].VertexBufferMOCS = addr.mocs; > + vb[idx].MOCS = addr.mocs; > #endif > > #if GEN_GEN >= 7 > diff --git a/src/intel/genxml/gen10.xml b/src/intel/genxml/gen10.xml > index 2d3bc39b1b9..21cd8a17d91 100644 > --- a/src/intel/genxml/gen10.xml > +++ b/src/intel/genxml/gen10.xml > @@ -219,14 +219,9 @@ > > > > - > - > - > - > > > - type="MEMORY_OBJECT_CONTROL_STATE"/> > - > + > > > > @@ -495,7 +490,6 @@ > type="bool"/> > type="bool"/> > type="bool"/> > - type="MEMORY_OBJECT_CONTROL_STATE"/> > > > > @@ -993,7 +987,7 @@ > > type="address"/> > > - type="MEMORY_OBJECT_CONTROL_STATE"/> > + > type="uint"> > > > @@ -1085,7 +1079,7 @@ > default="3"/> > default="0"/> > default="26"/> > - type="MEMORY_OBJECT_CONTROL_STATE"/> > + > > type="3DSTATE_CONSTANT_BODY"/> > > @@ -1095,7 +1089,7 @@ > default="3"/> > default="0"/> > default="22"/> > - type="MEMORY_OBJECT_CONTROL_STATE"/> > + > > type="3DSTATE_CONSTANT_BODY"/> > > @@ -1105,7 +1099,7 @@ > default="3"/> > default="0"/> > default="25"/> > - type="MEMORY_OBJECT_CONTROL_STATE"/> > + > > type="3DSTATE_CONSTANT_BODY"/> > > @@ -1116,7 +1110,7 @@ > default="0"/> > default="23"/> > type="uint"/> > - type="MEMORY_OBJECT_CONTROL_STATE"/> > + > > type="3DSTATE_CONSTANT_BODY"/>
Re: [Mesa-dev] [PATCH v2] docs: Document GitLab merge request process (email alternative)
On 2018-12-06 13:57:09, Nicolai Hähnle wrote: > On 06.12.18 00:32, Jordan Justen wrote: > > + To participate in code review, you should monitor the > > + https://lists.freedesktop.org/mailman/listinfo/mesa-dev;> > > + mesa-dev email list and the GitLab > > + Mesa > href="https://gitlab.freedesktop.org/mesa/mesa/merge_requests;>Merge > > + Requests page. > > This link is broken. Yeah, we'll have to enable merge requests on the Mesa project before it'll work. :) I just used the crucible url and s/crucible/mesa/. https://gitlab.freedesktop.org/mesa/crucible/merge_requests -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] docs: Document GitLab merge request process (email alternative)
On 2018-12-05 15:44:18, Jason Ekstrand wrote: > On Wed, Dec 5, 2018 at 5:32 PM Jordan Justen > wrote: > > -Mailing Patches > > +Submitting Patches > > > > > > -Patches should be sent to the mesa-dev mailing list for review: > > +Patches may be submitted to the Mesa project by > > +email or with a > > +GitLab merge request. To prevent > > +duplicate code review, only use one method to submit your changes. > > + > > > > Do we want to require a cover-letter to be sent to the ML? Ideally, we'd > just have a bot that makes one every time someone submits a MR and sends it > to the list. Maybe someone just needs to write that bot. > > > + > > +Mailing Patches > > + > > + > > +Patches may be sent to the mesa-dev mailing list for review: > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev;> > > mesa-dev@lists.freedesktop.org. > > When submitting a patch make sure to use > > @@ -217,8 +228,63 @@ disabled before sending your patches. (Note that you > > may need to contact > > your email administrator for this.) > > > > > > +GitLab Merge Requests > > + > > + > > + https://gitlab.freedesktop.org/mesa/mesa;>GitLab Merge > > + Requests (MR) can also be used to submit patches for Mesa. > > + > > + > > + > > + If the MR may have interest for most of the Mesa community, you can > > + send an email to the mesa-dev email list including a link to the MR. > > + Don't send the patch to mesa-dev, just the MR link. Regarding the cover-letter, I put in this weasel worded sentence. Should it instead say this is required and that it should be a git format-patch generated cover letter? Or, should we drop it entirely and assume we'll get an automated way to send an email to the list whenever a new MR is opened? Relatedly, I think it might be possible to enable an irc channel to be notified about pushes and MR's. Not sure if it'd be a good idea, or maybe too noisy. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2] docs: Document GitLab merge request process (email alternative)
This documents a process for using GitLab Merge Requests as an second way to submit code changes for Mesa. Only one of the two methods is allowed for each patch series. We will *not* require all patches to be emailed. Some code changes may be reviewed and merged without any discussion on the mesa-dev email list. v2: * No longer require email. Allow submitter to choose email or a GitLab merge request. * Various feedback from Brian, Daniel, Dylan, Eric, Erik, Jason, Matt, Michel and Rob. Signed-off-by: Jordan Justen --- docs/submittingpatches.html | 76 ++--- 1 file changed, 71 insertions(+), 5 deletions(-) diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html index 92d954a2d09..21175988d0b 100644 --- a/docs/submittingpatches.html +++ b/docs/submittingpatches.html @@ -21,7 +21,7 @@ Basic guidelines Patch formatting Testing Patches -Mailing Patches +Submitting Patches Reviewing Patches Nominating a commit for a stable branch Criteria for accepting patches to the stable branch @@ -42,8 +42,10 @@ components. git bisect.) Patches should be properly formatted. Patches should be sufficiently tested before submitting. -Patches should be submitted to mesa-dev -for review using git send-email. +Patches should be submitted +to mesa-dev or with +a merge request +for review. @@ -180,10 +182,19 @@ run. -Mailing Patches +Submitting Patches -Patches should be sent to the mesa-dev mailing list for review: +Patches may be submitted to the Mesa project by +email or with a +GitLab merge request. To prevent +duplicate code review, only use one method to submit your changes. + + +Mailing Patches + + +Patches may be sent to the mesa-dev mailing list for review: https://lists.freedesktop.org/mailman/listinfo/mesa-dev;> mesa-dev@lists.freedesktop.org. When submitting a patch make sure to use @@ -217,8 +228,63 @@ disabled before sending your patches. (Note that you may need to contact your email administrator for this.) +GitLab Merge Requests + + + https://gitlab.freedesktop.org/mesa/mesa;>GitLab Merge + Requests (MR) can also be used to submit patches for Mesa. + + + + If the MR may have interest for most of the Mesa community, you can + send an email to the mesa-dev email list including a link to the MR. + Don't send the patch to mesa-dev, just the MR link. + + + Add labels to your MR to help reviewers find it. For example: + +Mesa changes affecting all drivers: mesa +Hardware vendor specific code: amd, intel, nvidia, ... +Driver specific code: anvil, freedreno, i965, iris, radeonsi, + radv, vc4, ... +Other tag examples: gallium, util + + + + If you revise your patches based on code review and push an update + to your branch, you should maintain a clean history + in your patches. There should not be "fixup" patches in the history. + The series should be buildable and functional after every commit + whenever you push the branch. + + + It is your responsibility to keep the MR alive and making progress, + as there are no guarantees that a Mesa dev will independently take + interest in it. + + + Some other notes: + +Make changes and update your branch based on feedback +Old, stale MR may be closed, but you can reopen it if you + still want to pursue the changes +You should periodically check to see if your MR needs to be + rebased +Make sure your MR is closed if your patches get pushed outside + of GitLab + + + Reviewing Patches + + To participate in code review, you should monitor the + https://lists.freedesktop.org/mailman/listinfo/mesa-dev;> + mesa-dev email list and the GitLab + Mesa https://gitlab.freedesktop.org/mesa/mesa/merge_requests;>Merge + Requests page. + + When you've reviewed a patch on the mailing list, please be unambiguous about your review. That is, state either -- 2.20.0.rc2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Make Jordan an Owner of the mesa project?
On 2018-12-04 19:39:05, Jason Ekstrand wrote: > Given that everyone else has firmly ACKed, I'm going to click the button. > Congratulations, Jordan, you're now a mesa Owner! Thanks all! ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] GitLab merge requests, discussion summary - Re: [PATCH] docs: Document optional GitLab code review process
Lots of discussion here. :) I hoped to just dip our toes into the merge request stream, but the consensus is clearly against my idea. Oh well. :) Here's what I gathered from the discussion: == Keep email, allow duplicate MR, my idea == I don't think anyone else was in favor of this. Several were against it. == NACK merge requests altogether == I did not see this, so that seems promising for merge requests. :) I guess no one strongly disagrees with trying merge requests. == Allow submitter (or driver) to choose to email or MR, but not both == Several seemed to argue that this could be a good next step. But, in all those cases, I think they wanted to use merge requests going forward. It was mentioned that vc4 has moved to github in order to allow github pull requests to be used. == Require all email list, or all merge requests == A few argued in favor of this, and seemed to feel pretty strongly about it. In both cases I believe they wanted to more to merge requests. :) == Next step? == So, what do you all think is the best next step after the discussion? We could take the smaller step to allow the submitter, or project to decide which they prefer. It would allow those who prefer email to continue with email, while trying out gitlab merge requests. But, it could mean that devs need to check both gitlab and email. Or, we could just try to make the jump to MR only. Maybe if I write that patch someone will finally stand up to try to NACK moving away from email. :) Or, are we a little too early for this discussion, and we should just drop it for now? My opinion would be to try to move a bit slower and allow the submitter/project to choose for a trial period. (Dylan and Daniel seem to think this could be really frustrating for devs though.) But, if everyone seems to think it's reasonable to try to jump straight to using merge requests exclusively, I can type that version up... -Jordan On 2018-11-27 17:13:38, Jordan Justen wrote: > This documents a mechanism for using GitLab Merge Requests as an > optional, secondary way to get code reviews for patchsets. > > We still require all patches to be emailed. > > Aside from the potential usage for code review comments, it might also > help reviewers to find unmerged patchsets which need review. (Assuming > it doesn't fall into the same fate of patchwork with hundreds of open > MRs.) > > Signed-off-by: Jordan Justen > Cc: Jason Ekstrand > --- > docs/submittingpatches.html | 25 + > 1 file changed, 25 insertions(+) > > diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html > index 5d8ba443191..852f28c198a 100644 > --- a/docs/submittingpatches.html > +++ b/docs/submittingpatches.html > @@ -24,6 +24,7 @@ > Testing Patches > Mailing Patches > Reviewing Patches > +GitLab Merge Requests > Nominating a commit for a stable branch > Criteria for accepting patches to the stable > branch > Sending backports for the stable branch > @@ -282,6 +283,30 @@ which tells the patch author that the patch can be > committed, as long > as the issues are resolved first. > > > +GitLab Merge Requests > + > + > + https://gitlab.freedesktop.org/mesa/mesa;>GitLab Merge > + Requests (MR) can be used as an optional, secondary method of > + obtaining code review for patches. > + > + > + > + All patches should be submitted using email as well > + Consider including a link to the MR in your email based > +cover-letter > + Address code review from both email and the MR > + Add appropriate labels to your MR. For example: > + > + Mesa changes affect all drivers: mesa > + Hardware vendor specific code: amd, intel, nvidia, etc > + Driver specific code: anvil, freedreno, i965, iris, radeonsi, > radv, vc4, etc > + Other tag examples: gallium, util > + > + Never use the merge button on the GitLab page to push patches > + Add Reviewed-by tags to your commits and push using git > + Close your MR when your patches get pushed! > + > > Nominating a commit for a stable branch > > -- > 2.20.0.rc1 > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: Document and *require* usage of Signed-off-by
On 2018-11-29 15:53:09, Eric Anholt wrote: > e<#secure method=pgpmime mode=sign> > Erik Faye-Lund writes: > > > On Wed, 2018-11-28 at 13:43 -0800, Eric Anholt wrote: > >> Jordan Justen writes: > >> > >> > This adds the "Developer's Certificate of Origin 1.1" from the > >> > Linux > >> > kernel. It indicates that by using Signed-off-by you are certifying > >> > that your patch meets the DCO 1.1 guidelines. > >> > > >> > It also changes Signed-off-by from being optional to being > >> > required. > >> > > >> > Signed-off-by: Jordan Justen > >> > Cc: Matt Turner > >> > >> What problem for our project is solved by signed-off-by? Do you > >> think > >> that it has actually reduced the incidence of people submitting code > >> they don't have permission to submit in the projects where it's been > >> used? I know the behavior in the kernel is that people submit a > >> patch, > >> it's missing s-o-b, so it gets bounced, then they maybe add s-o-b or > >> just give up. I don't think anyone stops and says "Wow, that's a > >> good > >> question. Maybe I don't have permission to distribute this after > >> all?" > >> > >> /me sees s-o-b as basically just a linux kernel hazing ritual > > > > I don't think that's the purpose of s-o-b in the Kernel. AFAIK, the > > reason for the s-o-b is to have a paper-trail for how a patch landed in > > the kernel; who it went through etc. > > I get the *idea*, I just believe the idea fails in practice. The S-O-B > idea came from "wouldn't it be nice if we could make everyone think > about this issue that is important to us" but what we actually got > instead is "your patch gets bounced if you don't have a s-o-b on until > you slap one on and resend." Yeah, I can see how that can happen in some cases. Hopefully the feedback to the contributor would be, go read about patch signing in docs/submittingpatches.html rather than just "add Signed-off-by". It sounds like the biggest drawback is that some casual contributors might be turned off from making the contribution because they need to read the DCO and revise their patch. Could it be argued that such contributions are not neccessarily worth the time then? If they treat a code contribution this cavalierly, then maybe a bug report would be more their speed? > We've already got 1-2 people to contact if there's a question about > authorship, from the committer and author fields. There's also the missed opportunity for them to think about the license issue. (Maybe not 100% will seriously think about it, but will it be 0%?) Sometimes I'm shocked about how many projects I find on github with absolutely no license in sight. It's like, oh that's cool, but I guess it's not usable by anyone for anything. :\ So, maybe it's not so bad to add this if we might also be inviting in the pull/merge request crowd? :) It seems like you are annoyed, or at least skeptical about this. Is that a "NAK", or "why?", or "alright, but it's a waste of time"? -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: Document and *require* usage of Signed-off-by
On 2018-11-28 13:43:29, Eric Anholt wrote: > Jordan Justen writes: > > > This adds the "Developer's Certificate of Origin 1.1" from the Linux > > kernel. It indicates that by using Signed-off-by you are certifying > > that your patch meets the DCO 1.1 guidelines. > > > > It also changes Signed-off-by from being optional to being required. > > > > Signed-off-by: Jordan Justen > > Cc: Matt Turner > > What problem for our project is solved by signed-off-by? Do you think > that it has actually reduced the incidence of people submitting code > they don't have permission to submit in the projects where it's been > used? I know the behavior in the kernel is that people submit a patch, > it's missing s-o-b, so it gets bounced, then they maybe add s-o-b or > just give up. I don't think anyone stops and says "Wow, that's a good > question. Maybe I don't have permission to distribute this after all?" Is it possible that some of the cases of "just give up" were "Oh I didn't think about that. I guess I actually don't have permission to contribute this under that license."? It seems like contributors have to spend a little brain power once to read and ponder the ~25 line document with regards to their contributions, and then just add a -s to their commit command line. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: Document optional GitLab code review process
On 2018-11-28 10:14:49, Jason Ekstrand wrote: > On Wed, Nov 28, 2018 at 11:35 AM Jordan Justen > wrote: > > On 2018-11-28 06:59:42, Eric Engestrom wrote: > > > On Tuesday, 2018-11-27 19:45:50 -0800, Jordan Justen wrote: > > > > On 2018-11-27 19:20:09, Matt Turner wrote: > > > > > > > > > > Discussion point: I think attempting to have simultaneous review in > > > > > two places is a recipe for wasted time. > > > > > > > > That's possible. It also happens on email sometimes. But, I want to > > > > say that maybe the usual problem is too little code review, and not > > > > too much? :) > > > > > > I think duplicating the review possibilities might very well lead to > > > less review as people might (unconsciously) think "it has been/will be > > > reviewed on the other one". > > > > Maybe they were looking for an excuse to not do code review? :) > > > > I agree with your point to some extent, but I think it's better to try > > to work out a transition. To not jump immediately to forcing people to > > go to the web page. > > > > > > > > > > Strawman: maybe we can only email the cover letter to the mailing > > > > > list and include in it a link to the MR? > > > > > > > > I was hoping to make a smaller step and see what happens. Maybe this > > > > will give people the chance to try out MR based reviews, but not take > > > > away email based reviews just yet. > > > > > > > > I don't think we should move away from email based reviews until we > > > > are sure MR's actually work better. (I'm far from convinced on this > > > > point. :) > > > > > > I think one way to solve this is to allow both, but only one at a time. > > > Devs can choose to send their patches/series as either an MR _or_ as > > > emails, but not both. One can still send a cover-letter style email to > > > the ML to present the MR with a link to it. > > > > I would prefer to keep emails as required for now. Let people > > optionally try it out for some time. > > > > Perhaps as a next step we can let the poster consider using a MR and > > only a cover letter. (Assuming the MR method proves useful. If it > > doesn't we should revert back to email only.) > > > > I agree with Eric on this one. If a single submission has both types of > review then you will effectively end up with two different groups of > reviewers providing potentially conflicting feedback without seeing each > other's feedback and the submitter has to mediate. They always have to. Case in point, this patch. I want to keep all patches on the list but let people optionally post an MR so people can try it out as a first step. Dylan says it has to be all or nothing. You and Eric say the submitter should be forced to choose one or the other. (I'm not sure where to go from here. :) I think this is a worst case scenario situation. Normally people comment about separate parts of code, and not often in conflicting ways. If a major conflict comes up, then the submitter could ask the MR based reviewer to add their point on the email list discussion. (Since we'd be keeping email as the primary.) > It's better just to > have any given series have a single point for review. Yes, this means that > everyone will be forced to start doing GitHub review as soon as someone > does an MR that's relevant to them. I don't see a good way around that > which doesn't make a mess. > > Certain projects could, I suppose, have a requirement that all significant > work on that project be done on the list or on GitLab. However, people who > feel like custodians of Mesa as a whole will have to do both as long as > both are supported. I guess by 'projects', you mean sub-projects within Mesa? I'm not against Eric's idea of allowing the submitter to choose email list vs MR, but I do think we should hold off and consider that in 3~6 months. > > > Other things that need to be present in this 'GitLab MR' document are: > > > > > > - Review process: when you change something in your MR, force-push the > > > new version, don't add fixup commits. The history of the MR at any one > > > time must be "clean" in the sense that it would be acceptable to push > > > it as is. > > > > We don't document this for email based reviews, but people seem to > > figure it out. :) > > > > Is this more of a concern because they might not know that pushing the > > same branch updates the MR? > > > > I think it's worth explicitly documenting. Many people who are used to the > MR workflow are also used to having very sloppy branches because they > assume that only the final merge matters. This is why GitLab and GitHub > both have an option for "squash before merge". :-( Ok. It seems reasonable to add. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: Document optional GitLab code review process
On 2018-11-28 09:22:35, Dylan Baker wrote: > Quoting Matt Turner (2018-11-27 19:20:09) > > On Tue, Nov 27, 2018 at 5:13 PM Jordan Justen > > wrote: > > > > > > This documents a mechanism for using GitLab Merge Requests as an > > > optional, secondary way to get code reviews for patchsets. > > > > > > We still require all patches to be emailed. > > > > > > Aside from the potential usage for code review comments, it might also > > > help reviewers to find unmerged patchsets which need review. (Assuming > > > it doesn't fall into the same fate of patchwork with hundreds of open > > > MRs.) > > > > > > Signed-off-by: Jordan Justen > > > Cc: Jason Ekstrand > > > --- > > > docs/submittingpatches.html | 25 + > > > 1 file changed, 25 insertions(+) > > > > > > diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html > > > index 5d8ba443191..852f28c198a 100644 > > > --- a/docs/submittingpatches.html > > > +++ b/docs/submittingpatches.html > > > @@ -24,6 +24,7 @@ > > > Testing Patches > > > Mailing Patches > > > Reviewing Patches > > > +GitLab Merge Requests > > > Nominating a commit for a stable branch > > > Criteria for accepting patches to the stable > > > branch > > > Sending backports for the stable branch > > > @@ -282,6 +283,30 @@ which tells the patch author that the patch can be > > > committed, as long > > > as the issues are resolved first. > > > > > > > > > +GitLab Merge Requests > > > + > > > + > > > + https://gitlab.freedesktop.org/mesa/mesa;>GitLab Merge > > > + Requests (MR) can be used as an optional, secondary method of > > > + obtaining code review for patches. > > > + > > > + > > > + > > > + All patches should be submitted using email as well > > > + Consider including a link to the MR in your email based > > > +cover-letter > > > + Address code review from both email and the MR > > > > Discussion point: I think attempting to have simultaneous review in > > two places is a recipe for wasted time. Strawman: maybe we can only > > email the cover letter to the mailing list and include in it a link to > > the MR? > > I think it's a *really* bad idea to allow both. Some people will immediately > begin using MR/PR's only and never read the list, others will never check > MR/PRs and only use the list. It'll leave the rest of us forced to use both. Requiring emails seems to avoid these issues for now. We'd just be trying out using merge requests, but if you want to see all patches, use email. > We should either go all in and archive the mailing list and use only > gitlab, or not use PR/MR's IMHO. If that is the only choice, then I choose that we not use MR. Or, wait until everyone is forced to try MR via other freedesktop projects. :) One motivation I had for this patch was to see if MR could be a 'patchwork with labels'. Although, unlike patchwork, the contributor would have to take an extra step to open the MR and apply the label. One positive vs patchwork would be having the same login system as Mesa's repo. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: Document optional GitLab code review process
On 2018-11-28 06:59:42, Eric Engestrom wrote: > On Tuesday, 2018-11-27 19:45:50 -0800, Jordan Justen wrote: > > On 2018-11-27 19:20:09, Matt Turner wrote: > > > > > > Discussion point: I think attempting to have simultaneous review in > > > two places is a recipe for wasted time. > > > > That's possible. It also happens on email sometimes. But, I want to > > say that maybe the usual problem is too little code review, and not > > too much? :) > > I think duplicating the review possibilities might very well lead to > less review as people might (unconsciously) think "it has been/will be > reviewed on the other one". Maybe they were looking for an excuse to not do code review? :) I agree with your point to some extent, but I think it's better to try to work out a transition. To not jump immediately to forcing people to go to the web page. > > > > > Strawman: maybe we can only email the cover letter to the mailing > > > list and include in it a link to the MR? > > > > I was hoping to make a smaller step and see what happens. Maybe this > > will give people the chance to try out MR based reviews, but not take > > away email based reviews just yet. > > > > I don't think we should move away from email based reviews until we > > are sure MR's actually work better. (I'm far from convinced on this > > point. :) > > I think one way to solve this is to allow both, but only one at a time. > Devs can choose to send their patches/series as either an MR _or_ as > emails, but not both. One can still send a cover-letter style email to > the ML to present the MR with a link to it. I would prefer to keep emails as required for now. Let people optionally try it out for some time. Perhaps as a next step we can let the poster consider using a MR and only a cover letter. (Assuming the MR method proves useful. If it doesn't we should revert back to email only.) > I also agree with Matt that we need to disable everything that shouldn't > be used, but unfortunately we can't disable the merge button without > disabling merge requests. I guess we can't do this. Can we just tell people with push access to not do that again if they make a mistake? Can merge request approvals be used to only prevent the web page from merging things? (But, still allow pushes.) https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html Maybe require approval from "dont-merge-use-git-push"? :) > As for auto-closing issues and MRs by adding a special string to the > commit message, GitLab doesn't support that for commits that are pushed > behind its back, but it does support it it GitLab is used. Are you sure? Pushes seem to work this way on github, and my reading of https://docs.gitlab.com/ee/user/project/issues/automatic_issue_closing.html is that on gitlab pushes do the same. > Adding a post-receive hook that does that should be possible, I'm not > sure how much work that would be and if it would be worth it. > DanielS? > > Other things that need to be present in this 'GitLab MR' document are: > > - Review process: when you change something in your MR, force-push the > new version, don't add fixup commits. The history of the MR at any one > time must be "clean" in the sense that it would be acceptable to push > it as is. We don't document this for email based reviews, but people seem to figure it out. :) Is this more of a concern because they might not know that pushing the same branch updates the MR? If we only use git push, isn't this also unlikely to cause a problem in the main branch? > - WIP MRs: prefix your MR title with "WIP" to indicate that it's a work > in progress and is not ready to be merged as is (Side note: GitLab > will not offer to merge an MR that is prefixed with "WIP") If we are saying that merge should not be used from the web page, then I don't think this is in scope yet. > - I think we should split the list in two between the dev side and the > reviewer side. I'm not sure I understand what you mean, but it sounds bad. :) > - When an issue is raised, who closes it? The dev to indicate that the > new revision should fix it, or the reviewer to indicate that they have > verified the fix? The dev has to fix issues with getting their code merged. If you are talking about a case of someone posting an MR that doesn't have push access, then still the dev. In trivial cases the person with push access can choose to go out of their way to fix up a minor issue. We don't document the similar case with email patches, so I don't think this is something separate to consider documenting here. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: Document and *require* usage of Signed-off-by
On 2018-11-28 00:47:25, Erik Faye-Lund wrote: > On Tue, 2018-11-27 at 23:20 -0800, Jordan Justen wrote: > > This adds the "Developer's Certificate of Origin 1.1" from the Linux > > kernel. It indicates that by using Signed-off-by you are certifying > > that your patch meets the DCO 1.1 guidelines. > > > > It also changes Signed-off-by from being optional to being required. > > > > Signed-off-by: Jordan Justen > > Cc: Matt Turner > > --- > > docs/submittingpatches.html | 52 > > - > > 1 file changed, 51 insertions(+), 1 deletion(-) > > > > diff --git a/docs/submittingpatches.html > > b/docs/submittingpatches.html > > index 9ae750d5a15..6d506b3691b 100644 > > --- a/docs/submittingpatches.html > > +++ b/docs/submittingpatches.html > > @@ -20,6 +20,8 @@ > > > > Basic guidelines > > Patch formatting > > +Patch Signing (Signed-off-by, Developer's > > + Certificate of Origin) > > Testing Patches > > Mailing Patches > > Reviewing Patches > > @@ -73,7 +75,9 @@ if needed. For example: > > is necessary, and removing it causes no regressions in piglit on > > any > > platform. > > > > -A "Signed-off-by:" line is not required, but not discouraged > > either. > > +A "Signed-off-by:" line is required. The format > > +and meaning of Signed-off-by is documented below in > > +the patch signing section. > > If a patch addresses a bugzilla issue, that should be noted in > > the > > patch comment. For example: > > > > @@ -129,7 +133,53 @@ Please use common sense and do > > not blindly add everyone. > > > > > > > > + > > + Patch Signing (Signed-off-by, Developer's Certificate of Origin) > > + > > > > + > > + As described in the patch formatting > > + section, you must sign your patch by including Signed-off-by in > > the > > + patch commit message. The Signed-off-by must include your real > > name > > + and email address in this format: > > + > > + > > + Signed-off-by: Joe Hacker jhac...@foo.com > > + > > + > > + By adding Signed-of-by to your contributed patch, you certify that > > + your contribution meets the guidelines of the Developer's > > + Certificate of Origin: > > + > > + > > + > > +Developer's Certificate of Origin 1.1 > > +- > > + > > +By making a contribution to this project, I certify that: > > + > > + (a) The contribution was created in whole or in part by me and I > > + have the right to submit it under the open source license > > + indicated in the file; or > > + > > + (b) The contribution is based upon previous work that, to the best > > + of my knowledge, is covered under an appropriate open source > > + license and I have the right under that license to submit that > > + work with modifications, whether created in whole or in part > > + by me, under the same open source license (unless I am > > + permitted to submit under a different license), as indicated > > + in the file; or > > + > > + (c) The contribution was provided directly to me by some other > > + person who certified (a), (b) or (c) and I have not modified > > + it. > > + > > + (d) I understand and agree that this project and the contribution > > + are public and that a record of the contribution (including all > > + personal information I submit with it, including my sign-off) > > is > > + maintained indefinitely and may be redistributed consistent > > with > > + this project or the open source license(s) involved. > > + > > I don't think you can legally copy parts for this file, but not all of > it, due to this text (from here: https://developercertificate.org/) > > "Everyone is permitted to copy and distribute verbatim copies of this > license document, but changing it is not allowed." > > Removing that text (and the copyright statement above it), is changing > it. It came from the kernel Documentation/process/submitting-patches.rst, which doesn't have that specific text about "verbatim copies". I guess you prefer we copy it from https://developercertificate.org/? > I would propose you add it as a separate file and link that, to avoid > confusion about what "this license document" refers to. I do see that Eclipse had it on a page with other content. Although, the main focus of the page is the DCO. https://www.eclipse.org/legal/DCO.php It doesn't look like https://developercertificate.org/ has a filename associated with the content. So, something like docs/dco.txt or docs/developer-certificate-of-origin.txt? -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] docs: Document and *require* usage of Signed-off-by
This adds the "Developer's Certificate of Origin 1.1" from the Linux kernel. It indicates that by using Signed-off-by you are certifying that your patch meets the DCO 1.1 guidelines. It also changes Signed-off-by from being optional to being required. Signed-off-by: Jordan Justen Cc: Matt Turner --- docs/submittingpatches.html | 52 - 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html index 9ae750d5a15..6d506b3691b 100644 --- a/docs/submittingpatches.html +++ b/docs/submittingpatches.html @@ -20,6 +20,8 @@ Basic guidelines Patch formatting +Patch Signing (Signed-off-by, Developer's + Certificate of Origin) Testing Patches Mailing Patches Reviewing Patches @@ -73,7 +75,9 @@ if needed. For example: is necessary, and removing it causes no regressions in piglit on any platform. -A "Signed-off-by:" line is not required, but not discouraged either. +A "Signed-off-by:" line is required. The format +and meaning of Signed-off-by is documented below in +the patch signing section. If a patch addresses a bugzilla issue, that should be noted in the patch comment. For example: @@ -129,7 +133,53 @@ Please use common sense and do not blindly add everyone. + + Patch Signing (Signed-off-by, Developer's Certificate of Origin) + + + As described in the patch formatting + section, you must sign your patch by including Signed-off-by in the + patch commit message. The Signed-off-by must include your real name + and email address in this format: + + + Signed-off-by: Joe Hacker jhac...@foo.com + + + By adding Signed-of-by to your contributed patch, you certify that + your contribution meets the guidelines of the Developer's + Certificate of Origin: + + + +Developer's Certificate of Origin 1.1 +- + +By making a contribution to this project, I certify that: + + (a) The contribution was created in whole or in part by me and I + have the right to submit it under the open source license + indicated in the file; or + + (b) The contribution is based upon previous work that, to the best + of my knowledge, is covered under an appropriate open source + license and I have the right under that license to submit that + work with modifications, whether created in whole or in part + by me, under the same open source license (unless I am + permitted to submit under a different license), as indicated + in the file; or + + (c) The contribution was provided directly to me by some other + person who certified (a), (b) or (c) and I have not modified + it. + + (d) I understand and agree that this project and the contribution + are public and that a record of the contribution (including all + personal information I submit with it, including my sign-off) is + maintained indefinitely and may be redistributed consistent with + this project or the open source license(s) involved. + Testing Patches -- 2.20.0.rc1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: Document the optional usage of Signed-off-by
On 2018-11-27 19:53:38, Matt Turner wrote: > On Tue, Nov 27, 2018 at 7:26 PM Jordan Justen > wrote: > > On 2018-11-27 18:04:17, Matt Turner wrote: > > > By all means, require it (with a git hook) if you like. > > > > I personally don't want to push for that right now. > > > > I guess I would like it to be required someday, primarily because it > > creates a standard process for open source projects to use. (So, > > people are more likely to be used to it in general when contributing > > to open source projects.) > > > > But, I'm not confident that the consensus for Mesa would be in favor > > of making that change right now. So, as an alternative I'd like to > > remove any barriers (such as ambiguity) to its usage in Mesa. > > I don't think requiring S-o-b is onerous. Using S-o-b in a defined way > is just making explicit the current implicit promise to the community > that "I have the right to contribute this code" etc. I'm not sure why > you're hesitant. Ok. I'll type that version up. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: Document optional GitLab code review process
On 2018-11-27 19:20:09, Matt Turner wrote: > On Tue, Nov 27, 2018 at 5:13 PM Jordan Justen > wrote: > > > > This documents a mechanism for using GitLab Merge Requests as an > > optional, secondary way to get code reviews for patchsets. > > > > We still require all patches to be emailed. > > > > Aside from the potential usage for code review comments, it might also > > help reviewers to find unmerged patchsets which need review. (Assuming > > it doesn't fall into the same fate of patchwork with hundreds of open > > MRs.) > > > > Signed-off-by: Jordan Justen > > Cc: Jason Ekstrand > > --- > > docs/submittingpatches.html | 25 + > > 1 file changed, 25 insertions(+) > > > > diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html > > index 5d8ba443191..852f28c198a 100644 > > --- a/docs/submittingpatches.html > > +++ b/docs/submittingpatches.html > > @@ -24,6 +24,7 @@ > > Testing Patches > > Mailing Patches > > Reviewing Patches > > +GitLab Merge Requests > > Nominating a commit for a stable branch > > Criteria for accepting patches to the stable > > branch > > Sending backports for the stable branch > > @@ -282,6 +283,30 @@ which tells the patch author that the patch can be > > committed, as long > > as the issues are resolved first. > > > > > > +GitLab Merge Requests > > + > > + > > + https://gitlab.freedesktop.org/mesa/mesa;>GitLab Merge > > + Requests (MR) can be used as an optional, secondary method of > > + obtaining code review for patches. > > + > > + > > + > > + All patches should be submitted using email as well > > + Consider including a link to the MR in your email based > > +cover-letter > > + Address code review from both email and the MR > > Discussion point: I think attempting to have simultaneous review in > two places is a recipe for wasted time. That's possible. It also happens on email sometimes. But, I want to say that maybe the usual problem is too little code review, and not too much? :) > Strawman: maybe we can only email the cover letter to the mailing > list and include in it a link to the MR? I was hoping to make a smaller step and see what happens. Maybe this will give people the chance to try out MR based reviews, but not take away email based reviews just yet. I don't think we should move away from email based reviews until we are sure MR's actually work better. (I'm far from convinced on this point. :) -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: Document the optional usage of Signed-off-by
On 2018-11-27 18:04:17, Matt Turner wrote: > On Tue, Nov 27, 2018 at 6:00 PM Jordan Justen > wrote: > > > > On 2018-11-27 17:17:15, Matt Turner wrote: > > > On Tue, Nov 27, 2018 at 5:13 PM Jordan Justen > > > wrote: > > > > > > > > This adds the "Developer's Certificate of Origin 1.1" from the Linux > > > > kernel. It indicates that by using Signed-off-by you are certifying > > > > that your patch meets the DCO 1.1 guidelines. > > > > > > Do we gain anything if it's optional? > > > > As I recall, one thing that bothered you about Signed-off-by in Mesa > > is that it wasn't documented what it meant when it was used. > > > > Perhaps there are developers that don't want to use Signed-off-by with > > an undocumented meaning for Mesa. If that is the case, then this might > > help. I wasn't sure if you fell into that category. > > > > I use -s whenever I commit, so requiring it would not bother me. But, > > I notice that many people (such as yourself) do not, so I didn't see > > the need to push for that. > > > > If it's well documented, and becomes commonly used, then perhaps > > requiring it might be a reasonable thing to consider. I won't be > > holding my breath while waiting on that. :) > > I don't have a problem requiring it. I sign-off on commits I make to > Gentoo, to Linux, etc. If it has the same meaning as with the Linux kernel, but is not required, then you won't use it? I guess your concern might be that you are then giving something to the project that others can choose not to. ? > I'm just against cargo-culting it like we're doing now without a > defined meaning. The purpose of this patch is to give it a defined meaning. And the meaning I chose is the one that people are more likely to have in mind when they use Signed-off-by. Maybe that's too big of an assumption on my part, but I think several other open source projects have followed the kernel on this. > By all means, require it (with a git hook) if you like. I personally don't want to push for that right now. I guess I would like it to be required someday, primarily because it creates a standard process for open source projects to use. (So, people are more likely to be used to it in general when contributing to open source projects.) But, I'm not confident that the consensus for Mesa would be in favor of making that change right now. So, as an alternative I'd like to remove any barriers (such as ambiguity) to its usage in Mesa. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: Document the optional usage of Signed-off-by
On 2018-11-27 17:17:15, Matt Turner wrote: > On Tue, Nov 27, 2018 at 5:13 PM Jordan Justen > wrote: > > > > This adds the "Developer's Certificate of Origin 1.1" from the Linux > > kernel. It indicates that by using Signed-off-by you are certifying > > that your patch meets the DCO 1.1 guidelines. > > Do we gain anything if it's optional? As I recall, one thing that bothered you about Signed-off-by in Mesa is that it wasn't documented what it meant when it was used. Perhaps there are developers that don't want to use Signed-off-by with an undocumented meaning for Mesa. If that is the case, then this might help. I wasn't sure if you fell into that category. I use -s whenever I commit, so requiring it would not bother me. But, I notice that many people (such as yourself) do not, so I didn't see the need to push for that. If it's well documented, and becomes commonly used, then perhaps requiring it might be a reasonable thing to consider. I won't be holding my breath while waiting on that. :) -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] docs: Document optional GitLab code review process
This documents a mechanism for using GitLab Merge Requests as an optional, secondary way to get code reviews for patchsets. We still require all patches to be emailed. Aside from the potential usage for code review comments, it might also help reviewers to find unmerged patchsets which need review. (Assuming it doesn't fall into the same fate of patchwork with hundreds of open MRs.) Signed-off-by: Jordan Justen Cc: Jason Ekstrand --- docs/submittingpatches.html | 25 + 1 file changed, 25 insertions(+) diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html index 5d8ba443191..852f28c198a 100644 --- a/docs/submittingpatches.html +++ b/docs/submittingpatches.html @@ -24,6 +24,7 @@ Testing Patches Mailing Patches Reviewing Patches +GitLab Merge Requests Nominating a commit for a stable branch Criteria for accepting patches to the stable branch Sending backports for the stable branch @@ -282,6 +283,30 @@ which tells the patch author that the patch can be committed, as long as the issues are resolved first. +GitLab Merge Requests + + + https://gitlab.freedesktop.org/mesa/mesa;>GitLab Merge + Requests (MR) can be used as an optional, secondary method of + obtaining code review for patches. + + + + All patches should be submitted using email as well + Consider including a link to the MR in your email based +cover-letter + Address code review from both email and the MR + Add appropriate labels to your MR. For example: + + Mesa changes affect all drivers: mesa + Hardware vendor specific code: amd, intel, nvidia, etc + Driver specific code: anvil, freedreno, i965, iris, radeonsi, radv, vc4, etc + Other tag examples: gallium, util + + Never use the merge button on the GitLab page to push patches + Add Reviewed-by tags to your commits and push using git + Close your MR when your patches get pushed! + Nominating a commit for a stable branch -- 2.20.0.rc1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] docs: Document the optional usage of Signed-off-by
This adds the "Developer's Certificate of Origin 1.1" from the Linux kernel. It indicates that by using Signed-off-by you are certifying that your patch meets the DCO 1.1 guidelines. Signed-off-by: Jordan Justen Cc: Matt Turner --- docs/submittingpatches.html | 53 - 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html index 3f97c941aa5..5d8ba443191 100644 --- a/docs/submittingpatches.html +++ b/docs/submittingpatches.html @@ -20,6 +20,7 @@ Basic guidelines Patch formatting +Patch signing Testing Patches Mailing Patches Reviewing Patches @@ -72,7 +73,9 @@ if needed. For example: is necessary, and removing it causes no regressions in piglit on any platform. -A "Signed-off-by:" line is not required, but not discouraged either. +A "Signed-off-by:" line is not required, but not discouraged +either. The format and meaning of Signed-off-by is documented below in +the patch signing section. If a patch addresses a bugzilla issue, that should be noted in the patch comment. For example: @@ -128,7 +131,55 @@ Please use common sense and do not blindly add everyone. +Patch signing + + Note: Patch signing is optional for the Mesa project. + + + + As described in the patch formatting + section, you can optionally sign your patch by including + Signed-off-by in the patch commit message. The Signed-off-by must + include your real name and email address in this format: + + + Signed-off-by: Joe Hacker jhac...@foo.com + + + By adding Signed-of-by to your contributed patch, you certify that + your contribution meets the guidelines of the Developer's + Certificate of Origin: + + + +Developer's Certificate of Origin 1.1 +- + +By making a contribution to this project, I certify that: + + (a) The contribution was created in whole or in part by me and I + have the right to submit it under the open source license + indicated in the file; or + + (b) The contribution is based upon previous work that, to the best + of my knowledge, is covered under an appropriate open source + license and I have the right under that license to submit that + work with modifications, whether created in whole or in part + by me, under the same open source license (unless I am + permitted to submit under a different license), as indicated + in the file; or + + (c) The contribution was provided directly to me by some other + person who certified (a), (b) or (c) and I have not modified + it. + + (d) I understand and agree that this project and the contribution + are public and that a record of the contribution (including all + personal information I submit with it, including my sign-off) is + maintained indefinitely and may be redistributed consistent with + this project or the open source license(s) involved. + Testing Patches -- 2.20.0.rc1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/2] i965/genX_state: Add register access functions
Signed-off-by: Jordan Justen
---
src/mesa/drivers/dri/i965/genX_state_upload.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c
b/src/mesa/drivers/dri/i965/genX_state_upload.c
index 5acd0922922..6495862e700 100644
--- a/src/mesa/drivers/dri/i965/genX_state_upload.c
+++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
@@ -197,6 +197,37 @@ KSP(UNUSED struct brw_context *brw, uint32_t offset)
_brw_cmd_pack(cmd)(brw, (void *)_dst, ), \
_dst = NULL)
+#if GEN_GEN >= 7
+static void
+emit_lrm(struct brw_context *brw, uint32_t reg, struct brw_address addr)
+{
+ brw_batch_emit(brw, GENX(MI_LOAD_REGISTER_MEM), lrm) {
+ lrm.RegisterAddress = reg;
+ lrm.MemoryAddress= addr;
+ }
+}
+#endif
+
+MAYBE_UNUSED static void
+emit_lri(struct brw_context *brw, uint32_t reg, uint32_t imm)
+{
+ brw_batch_emit(brw, GENX(MI_LOAD_REGISTER_IMM), lri) {
+ lri.RegisterOffset = reg;
+ lri.DataDWord= imm;
+ }
+}
+
+#if GEN_IS_HASWELL || GEN_GEN >= 8
+MAYBE_UNUSED static void
+emit_lrr(struct brw_context *brw, uint32_t dst, uint32_t src)
+{
+ brw_batch_emit(brw, GENX(MI_LOAD_REGISTER_REG), lrr) {
+ lrr.SourceRegisterAddress= src;
+ lrr.DestinationRegisterAddress = dst;
+ }
+}
+#endif
+
/**
* Polygon stipple packet
*/
--
2.19.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/2] i965/compute: Emit GPGPU_WALKER in genX_state_upload
Signed-off-by: Jordan Justen
---
src/mesa/drivers/dri/i965/brw_compute.c | 131 +-
src/mesa/drivers/dri/i965/brw_context.h | 2 +
src/mesa/drivers/dri/i965/genX_state_upload.c | 102 ++
3 files changed, 105 insertions(+), 130 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_compute.c
b/src/mesa/drivers/dri/i965/brw_compute.c
index de08fc3ac16..6e4f5b593aa 100644
--- a/src/mesa/drivers/dri/i965/brw_compute.c
+++ b/src/mesa/drivers/dri/i965/brw_compute.c
@@ -34,135 +34,6 @@
#include "brw_defines.h"
-static void
-prepare_indirect_gpgpu_walker(struct brw_context *brw)
-{
- const struct gen_device_info *devinfo = >screen->devinfo;
- GLintptr indirect_offset = brw->compute.num_work_groups_offset;
- struct brw_bo *bo = brw->compute.num_work_groups_bo;
-
- brw_load_register_mem(brw, GEN7_GPGPU_DISPATCHDIMX, bo, indirect_offset +
0);
- brw_load_register_mem(brw, GEN7_GPGPU_DISPATCHDIMY, bo, indirect_offset +
4);
- brw_load_register_mem(brw, GEN7_GPGPU_DISPATCHDIMZ, bo, indirect_offset +
8);
-
- if (devinfo->gen > 7)
- return;
-
- /* Clear upper 32-bits of SRC0 and all 64-bits of SRC1 */
- BEGIN_BATCH(7);
- OUT_BATCH(MI_LOAD_REGISTER_IMM | (7 - 2));
- OUT_BATCH(MI_PREDICATE_SRC0 + 4);
- OUT_BATCH(0u);
- OUT_BATCH(MI_PREDICATE_SRC1 + 0);
- OUT_BATCH(0u);
- OUT_BATCH(MI_PREDICATE_SRC1 + 4);
- OUT_BATCH(0u);
- ADVANCE_BATCH();
-
- /* Load compute_dispatch_indirect_x_size into SRC0 */
- brw_load_register_mem(brw, MI_PREDICATE_SRC0, bo, indirect_offset + 0);
-
- /* predicate = (compute_dispatch_indirect_x_size == 0); */
- BEGIN_BATCH(1);
- OUT_BATCH(GEN7_MI_PREDICATE |
- MI_PREDICATE_LOADOP_LOAD |
- MI_PREDICATE_COMBINEOP_SET |
- MI_PREDICATE_COMPAREOP_SRCS_EQUAL);
- ADVANCE_BATCH();
-
- /* Load compute_dispatch_indirect_y_size into SRC0 */
- brw_load_register_mem(brw, MI_PREDICATE_SRC0, bo, indirect_offset + 4);
-
- /* predicate |= (compute_dispatch_indirect_y_size == 0); */
- BEGIN_BATCH(1);
- OUT_BATCH(GEN7_MI_PREDICATE |
- MI_PREDICATE_LOADOP_LOAD |
- MI_PREDICATE_COMBINEOP_OR |
- MI_PREDICATE_COMPAREOP_SRCS_EQUAL);
- ADVANCE_BATCH();
-
- /* Load compute_dispatch_indirect_z_size into SRC0 */
- brw_load_register_mem(brw, MI_PREDICATE_SRC0, bo, indirect_offset + 8);
-
- /* predicate |= (compute_dispatch_indirect_z_size == 0); */
- BEGIN_BATCH(1);
- OUT_BATCH(GEN7_MI_PREDICATE |
- MI_PREDICATE_LOADOP_LOAD |
- MI_PREDICATE_COMBINEOP_OR |
- MI_PREDICATE_COMPAREOP_SRCS_EQUAL);
- ADVANCE_BATCH();
-
- /* predicate = !predicate; */
- BEGIN_BATCH(1);
- OUT_BATCH(GEN7_MI_PREDICATE |
- MI_PREDICATE_LOADOP_LOADINV |
- MI_PREDICATE_COMBINEOP_OR |
- MI_PREDICATE_COMPAREOP_FALSE);
- ADVANCE_BATCH();
-}
-
-static void
-brw_emit_gpgpu_walker(struct brw_context *brw)
-{
- const struct gen_device_info *devinfo = >screen->devinfo;
- const struct brw_cs_prog_data *prog_data =
- brw_cs_prog_data(brw->cs.base.prog_data);
-
- const GLuint *num_groups = brw->compute.num_work_groups;
- uint32_t indirect_flag;
-
- if (brw->compute.num_work_groups_bo == NULL) {
- indirect_flag = 0;
- } else {
- indirect_flag =
- GEN7_GPGPU_INDIRECT_PARAMETER_ENABLE |
- (devinfo->gen == 7 ? GEN7_GPGPU_PREDICATE_ENABLE : 0);
- prepare_indirect_gpgpu_walker(brw);
- }
-
- const unsigned simd_size = prog_data->simd_size;
- unsigned group_size = prog_data->local_size[0] *
- prog_data->local_size[1] * prog_data->local_size[2];
- unsigned thread_width_max =
- (group_size + simd_size - 1) / simd_size;
-
- uint32_t right_mask = 0xu >> (32 - simd_size);
- const unsigned right_non_aligned = group_size & (simd_size - 1);
- if (right_non_aligned != 0)
- right_mask >>= (simd_size - right_non_aligned);
-
- uint32_t dwords = devinfo->gen < 8 ? 11 : 15;
- BEGIN_BATCH(dwords);
- OUT_BATCH(GPGPU_WALKER << 16 | (dwords - 2) | indirect_flag);
- OUT_BATCH(0);
- if (devinfo->gen >= 8) {
- OUT_BATCH(0); /* Indirect Data Length */
- OUT_BATCH(0); /* Indirect Data Start Address */
- }
- assert(thread_width_max <= brw->screen->devinfo.max_cs_threads);
- OUT_BATCH(SET_FIELD(simd_size / 16, GPGPU_WALKER_SIMD_SIZE) |
- SET_FIELD(thread_width_max - 1, GPGPU_WALKER_THREAD_WIDTH_MAX));
- OUT_BATCH(0);/* Thread Group ID Starting X */
- if (devinfo->gen >= 8)
- OUT_BATCH(0); /* MBZ */
- OUT_BATCH(num_groups[0]);/* Thread Group ID X Dimension */
- OUT_BATCH(0);/* Thread Group ID Starting Y */
- if (devinfo->gen >= 8)
- OUT_BATCH(0);
Re: [Mesa-dev] [PATCH v4 1/2] i965/batch: avoid reverting batch buffer if saved state is an empty
On 2018-11-05 00:11:52, andrey simiklit wrote:
> 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
>
> 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
>
It also looks like the tex3d-maxsize test is back to failing after a
few seconds on g965, rather than running forever. (So, #108630 is
fixed by this version.)
https://mesa-ci.01.org/global_logic/builds/38/results/46492597
-Jordan
>
> 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/mesa/drivers/dri/i965/genX_blorp_exec.c
> > b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > index 34bfcad03e..a62b88e166 100644
> > --- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > +++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > @@ -309,6 +309,7 @@ retry:
> > intel_batchbuffer_require_space(brw, 1400);
>
Re: [Mesa-dev] [PATCH 1/4] i965: Move some genX infrastructure to genX_boilerplate.h.
On 2018-11-01 20:04:18, Kenneth Graunke wrote: > This will let us make multiple genX_*.c files, without copy and pasting > all this boilerplate. > --- > src/mesa/drivers/dri/i965/Makefile.sources| 10 ++ > src/mesa/drivers/dri/i965/genX_boilerplate.h | 160 ++ > src/mesa/drivers/dri/i965/genX_state_upload.c | 129 +- > src/mesa/drivers/dri/i965/meson.build | 3 +- > 4 files changed, 174 insertions(+), 128 deletions(-) > create mode 100644 src/mesa/drivers/dri/i965/genX_boilerplate.h > > diff --git a/src/mesa/drivers/dri/i965/Makefile.sources > b/src/mesa/drivers/dri/i965/Makefile.sources > index 043a70029f2..63fa7b886f2 100644 > --- a/src/mesa/drivers/dri/i965/Makefile.sources > +++ b/src/mesa/drivers/dri/i965/Makefile.sources > @@ -126,42 +126,52 @@ intel_tiled_memcpy_dep_FILES = \ > > i965_gen4_FILES = \ > genX_blorp_exec.c \ > + genX_boilerplate.h \ brw_genX.h? genX_common.h? genX.h? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] intel: Use a URB start offset of 0 for disabled stages.
Reviewed-by: Jordan Justen
On 2018-11-01 20:01:36, Kenneth Graunke wrote:
> There are some cases where the VS is the only stage enabled, it uses the
> entire URB, and the URB is large enough that placing later stages after
> the VS exceeds the number of bits for "URB Starting Address".
>
> For example, on Icelake GT2, "varying-packing-simple mat2x4 array" from
> Piglit is getting a starting offset of 128 for the GS/HS/DS. But the
> field is only large enough to hold an offset of 127.
>
> i965 doesn't hit any genxml assertions because it's still using the old
> OUT_BATCH mechanism. 128 << GEN7_URB_STARTING_ADDRESS_SHIFT (57) == 0,
> with the extra bit falling off the end. So we place the disabled stage
> at the beginning of the URB (overlapping with push constants). This is
> likely okay since it's a zero size region (0 entries).
>
> It seems like the Vulkan driver might hit this assertion, however, and
> the situation seems harmless. To work around this, always place
> disabled stages at the start of the URB, so the last enabled stage can
> fill the remaining space without overflowing the field.
> ---
> src/intel/common/gen_urb_config.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/src/intel/common/gen_urb_config.c
> b/src/intel/common/gen_urb_config.c
> index 0b632149cd9..1440dd713e9 100644
> --- a/src/intel/common/gen_urb_config.c
> +++ b/src/intel/common/gen_urb_config.c
> @@ -195,8 +195,14 @@ gen_get_urb_config(const struct gen_device_info *devinfo,
> }
>
> /* Lay out the URB in pipeline order: push constants, VS, HS, DS, GS. */
> - start[0] = push_constant_chunks;
> - for (int i = MESA_SHADER_TESS_CTRL; i <= MESA_SHADER_GEOMETRY; i++) {
> - start[i] = start[i - 1] + chunks[i - 1];
> + int next = push_constant_chunks;
> + for (int i = MESA_SHADER_VERTEX; i <= MESA_SHADER_GEOMETRY; i++) {
> + if (entries[i]) {
> + start[i] = next;
> + next += chunks[i];
> + } else {
> + /* Just put disabled stages at the beginning. */
> + start[i] = 0;
> + }
> }
> }
> --
> 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 v3 1/2] i965/batch: avoid reverting batch buffer if saved state is an empty
On 2018-10-26 01:06:52, andrey simiklit wrote:
> Hi,
>
> Could you please help me with a push. I don't have a right for it :-)
>
I pushed these two patches. Thanks!
e4e0fd5ffe1 i965/batch: don't ignore the 'brw_new_batch' call for a 'new batch'
a9031bf9b55 i965/batch: avoid reverting batch buffer if saved state is an empty
-Jordan
>
> On Thu, Oct 11, 2018 at 9:22 PM Jordan Justen
> wrote:
>
> > On 2018-10-11 03:01:56, andrey simiklit wrote:
> > > Hello,
> > >
> > > Thanks for reviewing.
> > > Please find my comment below.
> > >
> > > On Thu, Oct 11, 2018 at 12:37 AM Jordan Justen <
> > jordan.l.jus...@intel.com>
> > > wrote:
> > >
> > > > On 2018-09-12 09:05:44, 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.
> > > > >
> > > > > 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
> > > > > ---
> > > > > 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 | 3 ++-
> > > > > src/mesa/drivers/dri/i965/intel_batchbuffer.c | 7 +++
> > > > > src/mesa/drivers/dri/i965/intel_batchbuffer.h | 1 +
> > > > > 5 files changed, 14 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/src/mesa/drivers/dri/i965/brw_compute.c
> > > > b/src/mesa/drivers/dri/i965/brw_compute.c
> > > > > index de08fc3..5c8e3a5 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 8536c04..19ee396 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);
> > > >
> > > > It seems like this will cause the WARN_ONCE to be hit incorrectly.
> > > > What about adding a 'bool empty_state', and checking that before
> > > > fail_next in the code that follows. If empty_state is true, I guess
> > > > you want to flush, but not emit the WARN_ONCE?
> > > >
> > >
> > > We just predict that first step (non-failed branch without WA
Re: [Mesa-dev] [PATCH] i965: Add PCI IDs for new Amberlake parts that are Coffeelake based
Reviewed-by: Jordan Justen On 2018-10-15 16:05:39, Kenneth Graunke wrote: > See commit c0c46ca461f136a0ae1ed69da6c874e850aeeb53 in the Linux kernel, > where José Roberto de Souza added this new PCI ID there. > --- > include/pci_ids/i965_pci_ids.h | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/include/pci_ids/i965_pci_ids.h b/include/pci_ids/i965_pci_ids.h > index cb33bea7d4d..7201562d824 100644 > --- a/include/pci_ids/i965_pci_ids.h > +++ b/include/pci_ids/i965_pci_ids.h > @@ -163,8 +163,9 @@ CHIPSET(0x5923, kbl_gt3, "Intel(R) Kabylake GT3") > CHIPSET(0x5926, kbl_gt3, "Intel(R) Iris Plus Graphics 640 (Kaby Lake GT3e)") > CHIPSET(0x5927, kbl_gt3, "Intel(R) Iris Plus Graphics 650 (Kaby Lake GT3e)") > CHIPSET(0x593B, kbl_gt4, "Intel(R) Kabylake GT4") > -CHIPSET(0x591C, kbl_gt2, "Intel(R) Amber Lake GT2") > -CHIPSET(0x87C0, kbl_gt2, "Intel(R) Amber Lake GT2") > +CHIPSET(0x591C, kbl_gt2, "Intel(R) Amber Lake (Kabylake) GT2") > +CHIPSET(0x87C0, kbl_gt2, "Intel(R) Amber Lake (Kabylake) GT2") > +CHIPSET(0x87CA, cfl_gt2, "Intel(R) Amber Lake (Coffeelake) GT2") > CHIPSET(0x3184, glk, "Intel(R) UHD Graphics 605 (Geminilake)") > CHIPSET(0x3185, glk_2x6, "Intel(R) UHD Graphics 600 (Geminilake 2x6)") > CHIPSET(0x3E90, cfl_gt1, "Intel(R) UHD Graphics 610 (Coffeelake 2x6 GT1)") > -- > 2.19.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] [PATCH v3 1/2] i965/batch: avoid reverting batch buffer if saved state is an empty
On 2018-10-11 03:01:56, andrey simiklit wrote:
> Hello,
>
> Thanks for reviewing.
> Please find my comment below.
>
> On Thu, Oct 11, 2018 at 12:37 AM Jordan Justen
> wrote:
>
> > On 2018-09-12 09:05:44, 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.
> > >
> > > 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
> > > ---
> > > 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 | 3 ++-
> > > src/mesa/drivers/dri/i965/intel_batchbuffer.c | 7 +++
> > > src/mesa/drivers/dri/i965/intel_batchbuffer.h | 1 +
> > > 5 files changed, 14 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_compute.c
> > b/src/mesa/drivers/dri/i965/brw_compute.c
> > > index de08fc3..5c8e3a5 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 8536c04..19ee396 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);
> >
> > It seems like this will cause the WARN_ONCE to be hit incorrectly.
> > What about adding a 'bool empty_state', and checking that before
> > fail_next in the code that follows. If empty_state is true, I guess
> > you want to flush, but not emit the WARN_ONCE?
> >
>
> We just predict that first step (non-failed branch without WARN_ONCE)
> which should make the batch an empty will not make sense
> because it is already empty and we immediately pass into a failed branch.
> All WARN_ONCE calls are conditional ('ret == -ENOSPC') there.
> I guess that if we came to the failed branch (I mean branch which contains
> WARN_ONCE)
> then regardless a reason (due to 'empty state' or 'failed try') we rather
> should log a warning
> that there isn't left space to transfer a batch if it is true.
Oh. You are right. It sounds like a reasonable warning message for
that case. So, there's no need to change the patch.
-Jordan
>
> What do you think about it?
> Should we log a warning if calls of
> 'brw_upload_render_state' + 'brw_emit_prim' functions
> for an empty batch lead to ENOSPC error?
>
> Anyway if it is unacceptable for you
> I can implement the solution which you suggested.
>
>
> >
> > With that change,
> > series Reviewed-by: Jordan Justen
> >
> > As always, it could be nice to get an r-b, or acked-by from Ken. :)
> >
> > -Jordan
> >
> > >
> > > if (brw->num_instances != prim->num_instances ||
> > > brw-&
Re: [Mesa-dev] [PATCH 1/2] anv/gen9+: Initialize new fields in STATE_BASE_ADDRESS
On 2018-10-10 14:38:23, Rafael Antognolli wrote:
> On Wed, Oct 10, 2018 at 02:04:11PM -0700, Jordan Justen wrote:
> > On 2018-10-10 13:45:13, Rafael Antognolli wrote:
> > > On Wed, Oct 10, 2018 at 01:39:25PM -0700, Jordan Justen wrote:
> > > > Ref: 263b584d5e4 "i965/skl: Emit extra zeros in STATE_BASE_ADDRESS on
> > > > Skylake."
> > > > Signed-off-by: Jordan Justen
> > > > ---
> > > > src/intel/vulkan/genX_cmd_buffer.c | 12
> > > > 1 file changed, 12 insertions(+)
> > > >
> > > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > > > b/src/intel/vulkan/genX_cmd_buffer.c
> > > > index c3a7e5c83c3..43a02f22567 100644
> > > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > > @@ -121,6 +121,18 @@ genX(cmd_buffer_emit_state_base_address)(struct
> > > > anv_cmd_buffer *cmd_buffer)
> > > >sba.IndirectObjectBufferSizeModifyEnable = true;
> > > >sba.InstructionBufferSize = 0xf;
> > > >sba.InstructionBuffersizeModifyEnable = true;
> > > > +# endif
> > > > +# if (GEN_GEN >= 9)
> > > > + sba.BindlessSurfaceStateBaseAddress = (struct anv_address) {
> > > > NULL, 0 };
> > > > + sba.BindlessSurfaceStateMemoryObjectControlState = GENX(MOCS);
> > > > + sba.BindlessSurfaceStateBaseAddressModifyEnable = true;
> > > > + sba.BindlessSurfaceStateSize = 0;
> > > > +# endif
> > > > +# if (GEN_GEN >= 10)
> > > > + sba.BindlessSamplerStateBaseAddress = (struct anv_address) {
> > > > NULL, 0 };
> > > > + sba.BindlessSamplerStateMemoryObjectControlState = GENX(MOCS);
> > > > + sba.BindlessSamplerStateBaseAddressModifyEnable = true;
> > > > + sba.BindlessSamplerStateBufferSize = 0;
> > >
> > > Do we really need to set all of these fields? AFAIK the ones we don't
> > > set should be left as 0's anyway, so at least the Address and BufferSize
> > > should be fine to be left out. I think the MOCS field should be fine
> > > too, since we are not setting any pointer here. Unless you want to
> > > be really explicit...
> >
> > Yeah. I don't know that it is helpful since the genxml already sets
> > the packet length, and I guess things should be zero by default. Maybe
> > it will make it a little easier to find for bindless in the future?
> >
> > Regarding Jason's comment about the enable bit, I was following Ken's
> > referenced commit (263b584d5e4) for the similar field in gen9+ on
> > i965. Maybe it is good to actually force the write to explicitly set
> > the size to 0?
>
> Yeah, my understanding is that we should set the "modify" bit, so it
> will actually set the address and size to 0.
>
> > I guess setting MOCS does not follow what Ken did in i965.
> >
> > If we actually do want to set the enable bit, then it might be good to
> > also leave the fields being explicitly set to zero.
> >
> > My preference would be to just set the fields explicitly. Since we
> > only specify this packet in one place, it doesn't seem like it adds
> > too much verbosity.
>
> On most of the genxml code I've seen, we only set the fields that are
> not zeroed by default.
I think there are exceptions:
$ git grep -Ee "\..* = 0;" src/intel/vulkan/genX_*
I think the rule is more like: if setting the field to 0 is notable,
then it's better to explicitly set it for informational purposes.
If the 'enable' bit is set, then I think think the fields that will be
updated are notable. If the 'enable' bit was not set, then maybe the
fields are not important. (In that case, perhaps the patch should be
dropped entirely.)
Anyway, if Jason doesn't have any further input, I'll go with your
suggestion of dropping the zeroed fields.
-Jordan
> And if the name of these fields change or
> something in a future generation, assuming we are still not using them,
> it's easier to just change the xml for that gen.
>
> So to keep the code consistent with the rest, I would leave it out, but
> regardless of what you choose,
>
> Reviewed-by: Rafael Antognolli
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 1/2] i965/batch: avoid reverting batch buffer if saved state is an empty
On 2018-09-12 09:05:44, 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.
>
> 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
> ---
> 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 | 3 ++-
> src/mesa/drivers/dri/i965/intel_batchbuffer.c | 7 +++
> src/mesa/drivers/dri/i965/intel_batchbuffer.h | 1 +
> 5 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_compute.c
> b/src/mesa/drivers/dri/i965/brw_compute.c
> index de08fc3..5c8e3a5 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 8536c04..19ee396 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);
It seems like this will cause the WARN_ONCE to be hit incorrectly.
What about adding a 'bool empty_state', and checking that before
fail_next in the code that follows. If empty_state is true, I guess
you want to flush, but not emit the WARN_ONCE?
With that change,
series Reviewed-by: Jordan Justen
As always, it could be nice to get an r-b, or acked-by from Ken. :)
-Jordan
>
> if (brw->num_instances != prim->num_instances ||
> brw->basevertex != prim->basevertex ||
> diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> index 34bfcad..fd9ce93 100644
> --- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> +++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> @@ -268,7 +268,7 @@ genX(blorp_exec)(struct blorp_batch *batch,
> assert(batch->blorp->driver_ctx == batch->driver_batch);
> struct brw_context *brw = batch->driver_batch;
> struct gl_context *ctx = >ctx;
> - bool check_aperture_failed_once = false;
> + bool check_aperture_failed_once;
>
> #if GEN_GEN >= 11
> /* The PIPE_CONTROL command description says:
> @@ -309,6 +309,7 @@ retry:
> intel_batchbuffer_require_space(brw, 1400);
> brw_require_statebuffer_space(brw, 600);
> intel_batchbuffer_save_state(brw);
> + check_aperture_failed_once = intel_batchbuffer_saved_state_is_empty(brw);
> brw->batch.no_wrap = true;
>
> #if GEN_GEN == 6
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index 4363b14..2dc6eb8 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -299,6 +299,13 @@ intel_batchbuffer_save_state(struct brw_context *brw)
> brw->batch.saved.exec_count = brw->batch.exec_count;
> }
>
> +bool
> +intel_batchbuffer_saved_state_is_empty(struct brw_context *brw)
> +{
> + struct intel_batchbuffer *batch = >batch;
> + return (batch->saved.map_next == batch->batch.map);
> +}
> +
> void
> intel_batchbuffer_reset_to_saved(struct brw_context *brw)
> {
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
Re: [Mesa-dev] [PATCH 1/2] anv/gen9+: Initialize new fields in STATE_BASE_ADDRESS
On 2018-10-10 13:45:13, Rafael Antognolli wrote:
> On Wed, Oct 10, 2018 at 01:39:25PM -0700, Jordan Justen wrote:
> > Ref: 263b584d5e4 "i965/skl: Emit extra zeros in STATE_BASE_ADDRESS on
> > Skylake."
> > Signed-off-by: Jordan Justen
> > ---
> > src/intel/vulkan/genX_cmd_buffer.c | 12
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > b/src/intel/vulkan/genX_cmd_buffer.c
> > index c3a7e5c83c3..43a02f22567 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -121,6 +121,18 @@ genX(cmd_buffer_emit_state_base_address)(struct
> > anv_cmd_buffer *cmd_buffer)
> >sba.IndirectObjectBufferSizeModifyEnable = true;
> >sba.InstructionBufferSize = 0xf;
> >sba.InstructionBuffersizeModifyEnable = true;
> > +# endif
> > +# if (GEN_GEN >= 9)
> > + sba.BindlessSurfaceStateBaseAddress = (struct anv_address) { NULL, 0
> > };
> > + sba.BindlessSurfaceStateMemoryObjectControlState = GENX(MOCS);
> > + sba.BindlessSurfaceStateBaseAddressModifyEnable = true;
> > + sba.BindlessSurfaceStateSize = 0;
> > +# endif
> > +# if (GEN_GEN >= 10)
> > + sba.BindlessSamplerStateBaseAddress = (struct anv_address) { NULL, 0
> > };
> > + sba.BindlessSamplerStateMemoryObjectControlState = GENX(MOCS);
> > + sba.BindlessSamplerStateBaseAddressModifyEnable = true;
> > + sba.BindlessSamplerStateBufferSize = 0;
>
> Do we really need to set all of these fields? AFAIK the ones we don't
> set should be left as 0's anyway, so at least the Address and BufferSize
> should be fine to be left out. I think the MOCS field should be fine
> too, since we are not setting any pointer here. Unless you want to
> be really explicit...
Yeah. I don't know that it is helpful since the genxml already sets
the packet length, and I guess things should be zero by default. Maybe
it will make it a little easier to find for bindless in the future?
Regarding Jason's comment about the enable bit, I was following Ken's
referenced commit (263b584d5e4) for the similar field in gen9+ on
i965. Maybe it is good to actually force the write to explicitly set
the size to 0?
I guess setting MOCS does not follow what Ken did in i965.
If we actually do want to set the enable bit, then it might be good to
also leave the fields being explicitly set to zero.
My preference would be to just set the fields explicitly. Since we
only specify this packet in one place, it doesn't seem like it adds
too much verbosity.
-Jordan
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/2] anv/gen9+: Initialize new fields in STATE_BASE_ADDRESS
Ref: 263b584d5e4 "i965/skl: Emit extra zeros in STATE_BASE_ADDRESS on Skylake."
Signed-off-by: Jordan Justen
---
src/intel/vulkan/genX_cmd_buffer.c | 12
1 file changed, 12 insertions(+)
diff --git a/src/intel/vulkan/genX_cmd_buffer.c
b/src/intel/vulkan/genX_cmd_buffer.c
index c3a7e5c83c3..43a02f22567 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -121,6 +121,18 @@ genX(cmd_buffer_emit_state_base_address)(struct
anv_cmd_buffer *cmd_buffer)
sba.IndirectObjectBufferSizeModifyEnable = true;
sba.InstructionBufferSize = 0xf;
sba.InstructionBuffersizeModifyEnable = true;
+# endif
+# if (GEN_GEN >= 9)
+ sba.BindlessSurfaceStateBaseAddress = (struct anv_address) { NULL, 0 };
+ sba.BindlessSurfaceStateMemoryObjectControlState = GENX(MOCS);
+ sba.BindlessSurfaceStateBaseAddressModifyEnable = true;
+ sba.BindlessSurfaceStateSize = 0;
+# endif
+# if (GEN_GEN >= 10)
+ sba.BindlessSamplerStateBaseAddress = (struct anv_address) { NULL, 0 };
+ sba.BindlessSamplerStateMemoryObjectControlState = GENX(MOCS);
+ sba.BindlessSamplerStateBaseAddressModifyEnable = true;
+ sba.BindlessSamplerStateBufferSize = 0;
# endif
}
--
2.19.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/2] i965/gen10+: Initialize new fields in STATE_BASE_ADDRESS
Ref: 263b584d5e4 "i965/skl: Emit extra zeros in STATE_BASE_ADDRESS on Skylake."
Signed-off-by: Jordan Justen
---
src/mesa/drivers/dri/i965/brw_misc_state.c | 7 ++-
1 file changed, 6 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 0895e1f2b7f..9bff2c8ac92 100644
--- a/src/mesa/drivers/dri/i965/brw_misc_state.c
+++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
@@ -688,7 +688,7 @@ brw_upload_state_base_address(struct brw_context *brw)
* to the bottom 4GB.
*/
uint32_t mocs_wb = devinfo->gen >= 9 ? SKL_MOCS_WB : BDW_MOCS_WB;
- int pkt_len = devinfo->gen >= 9 ? 19 : 16;
+ int pkt_len = devinfo->gen >= 10 ? 22 : (devinfo->gen >= 9 ? 19 : 16);
BEGIN_BATCH(pkt_len);
OUT_BATCH(CMD_STATE_BASE_ADDRESS << 16 | (pkt_len - 2));
@@ -718,6 +718,11 @@ brw_upload_state_base_address(struct brw_context *brw)
OUT_BATCH(0);
OUT_BATCH(0);
}
+ if (devinfo->gen >= 10) {
+ OUT_BATCH(1);
+ OUT_BATCH(0);
+ OUT_BATCH(0);
+ }
ADVANCE_BATCH();
} else if (devinfo->gen >= 6) {
uint8_t mocs = devinfo->gen == 7 ? GEN7_MOCS_L3 : 0;
--
2.19.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/2] anv: If softpin is supported, use it with the hiz clear value bo
Signed-off-by: Jordan Justen
Cc: Nanley Chery
Cc: Jason Ekstrand
---
src/intel/vulkan/anv_device.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 265fc4a3347..4e64f595650 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -1569,6 +1569,15 @@ static void
anv_device_init_hiz_clear_value_bo(struct anv_device *device)
{
anv_bo_init_new(>hiz_clear_bo, device, 4096);
+
+ if (device->instance->physicalDevice.has_exec_async)
+ device->hiz_clear_bo.flags |= EXEC_OBJECT_ASYNC;
+
+ if (device->instance->physicalDevice.use_softpin)
+ device->hiz_clear_bo.flags |= EXEC_OBJECT_PINNED;
+
+ anv_vma_alloc(device, >hiz_clear_bo);
+
uint32_t *map = anv_gem_mmap(device, device->hiz_clear_bo.gem_handle,
0, 4096, 0);
--
2.18.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/2] anv: s/batch/value_bo/ on anv_device_init_hiz_clear_batch
Signed-off-by: Jordan Justen
Cc: Jason Ekstrand
---
src/intel/vulkan/anv_device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 4219a073d2d..265fc4a3347 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -1566,7 +1566,7 @@ vk_priority_to_gen(int priority)
}
static void
-anv_device_init_hiz_clear_batch(struct anv_device *device)
+anv_device_init_hiz_clear_value_bo(struct anv_device *device)
{
anv_bo_init_new(>hiz_clear_bo, device, 4096);
uint32_t *map = anv_gem_mmap(device, device->hiz_clear_bo.gem_handle,
@@ -1802,7 +1802,7 @@ VkResult anv_CreateDevice(
anv_device_init_trivial_batch(device);
if (device->info.gen >= 10)
- anv_device_init_hiz_clear_batch(device);
+ anv_device_init_hiz_clear_value_bo(device);
anv_scratch_pool_init(device, >scratch_pool);
--
2.18.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] anv: If softpin is supported, use it with hiz clear batch bo
On 2018-09-25 16:02:28, Nanley Chery wrote:
> On Tue, Sep 25, 2018 at 03:22:11PM -0700, Jordan Justen wrote:
> > Signed-off-by: Jordan Justen
> > Cc: Nanley Chery
> > ---
> > src/intel/vulkan/anv_device.c | 9 +
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> > index 0ea8be052fa..4e446c3280a 100644
> > --- a/src/intel/vulkan/anv_device.c
> > +++ b/src/intel/vulkan/anv_device.c
> > @@ -1574,6 +1574,15 @@ static void
> > anv_device_init_hiz_clear_batch(struct anv_device *device)
> > {
> > anv_bo_init_new(>hiz_clear_bo, device, 4096);
> > +
> > + if (device->instance->physicalDevice.has_exec_async)
> > + device->hiz_clear_bo.flags |= EXEC_OBJECT_ASYNC;
> > +
> > + if (device->instance->physicalDevice.use_softpin)
> > + device->hiz_clear_bo.flags |= EXEC_OBJECT_PINNED;
> > +
> > + anv_vma_alloc(device, >hiz_clear_bo);
> > +
>
> Seems like we should handle the return value of this function.
> Maybe also hook into the block of gotos in anv_CreateDevice()?
I think the anv_gem_mmap call below is more likely to fail, than
anv_vma_alloc, but we don't have an error path for it. It looks like
the anv_vma_alloc call could only fail if the vma range fills, and
during device init I think that is probably impossible.
I also notice that we have a similar unchecked call in
anv_device_init_trivial_batch.
I wouldn't mind adding another potential failure path if it were
remotely possible, but anv_CreateDevice is already pretty gross with
all the labels for potential failure cases. :)
-Jordan
>
> > uint32_t *map = anv_gem_mmap(device, device->hiz_clear_bo.gem_handle,
> > 0, 4096, 0);
> >
> > --
> > 2.18.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
[Mesa-dev] [PATCH] anv: If softpin is supported, use it with hiz clear batch bo
Signed-off-by: Jordan Justen
Cc: Nanley Chery
---
src/intel/vulkan/anv_device.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 0ea8be052fa..4e446c3280a 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -1574,6 +1574,15 @@ static void
anv_device_init_hiz_clear_batch(struct anv_device *device)
{
anv_bo_init_new(>hiz_clear_bo, device, 4096);
+
+ if (device->instance->physicalDevice.has_exec_async)
+ device->hiz_clear_bo.flags |= EXEC_OBJECT_ASYNC;
+
+ if (device->instance->physicalDevice.use_softpin)
+ device->hiz_clear_bo.flags |= EXEC_OBJECT_PINNED;
+
+ anv_vma_alloc(device, >hiz_clear_bo);
+
uint32_t *map = anv_gem_mmap(device, device->hiz_clear_bo.gem_handle,
0, 4096, 0);
--
2.18.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Don't bother to call blorp if the blit is zero-sized
Reviewed-by: Jordan Justen On 2018-09-11 09:15:38, Jason Ekstrand wrote: > Cc: mesa-sta...@lists.freedesktop.org > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107892 > --- > src/mesa/drivers/dri/i965/brw_meta_util.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/mesa/drivers/dri/i965/brw_meta_util.c > b/src/mesa/drivers/dri/i965/brw_meta_util.c > index 908b0989769..6714d96237c 100644 > --- a/src/mesa/drivers/dri/i965/brw_meta_util.c > +++ b/src/mesa/drivers/dri/i965/brw_meta_util.c > @@ -247,6 +247,9 @@ brw_meta_mirror_clip_and_scissor(const struct gl_context > *ctx, > clip_src_y1, clip_dst_y1, clip_dst_y0, > scaleY, false); > > + if (*dstX0 == *dstX1 || *dstY0 == *dstY1) > + return true; > + > /* Account for the fact that in the system framebuffer, the origin is at > * the lower left. > */ > -- > 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 2/2] intel/compiler: Add brw_get_compiler_config_value for disk cache
On 2018-08-02 11:27:57, Dylan Baker wrote:
> Quoting Jordan Justen (2018-07-25 17:12:20)
> > During code review, Jason pointed out that:
> >
> > 2b3064c0731 "i965, anv: Use INTEL_DEBUG for disk_cache driver flags"
> >
> > Didn't account for INTEL_SCALER_* environment variables.
> >
> > To fix this, let the compiler return the disk_cache driver flags.
> >
> > Another possible fix would be to pull the INTEL_SCALER_* into
> > INTEL_DEBUG bits, but as we are currently using 41 of 64 bits, I
> > didn't think it was a good use of 4 more of these bits. (5 since
> > INTEL_PRECISE_TRIG needs to be accounted for as well.)
> >
> > Cc: Jason Ekstrand
> > Signed-off-by: Jordan Justen
> > ---
> > src/intel/compiler/brw_compiler.c | 26 ++
> > src/intel/compiler/brw_compiler.h | 11 +
> > src/intel/vulkan/anv_device.c | 3 ++-
> > src/mesa/drivers/dri/i965/brw_disk_cache.c | 3 ++-
> > 4 files changed, 41 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/intel/compiler/brw_compiler.c
> > b/src/intel/compiler/brw_compiler.c
> > index 6480dbefbf6..ba6b8e0cbbe 100644
> > --- a/src/intel/compiler/brw_compiler.c
> > +++ b/src/intel/compiler/brw_compiler.c
> > @@ -180,6 +180,32 @@ brw_compiler_create(void *mem_ctx, const struct
> > gen_device_info *devinfo)
> >
> > return compiler;
> > }
> > +static void
> > +insert_u64_bit(uint64_t *val, bool add)
> > +{
> > + *val = (*val << 1) | !!add;
> > +}
> > +
> > +const uint64_t
> > +brw_get_compiler_config_value(const struct brw_compiler *compiler)
> > +{
> > + uint64_t config = 0;
> > + insert_u64_bit(, compiler->precise_trig);
> > + if (compiler->devinfo->gen >= 8 && compiler->devinfo->gen < 10) {
> > + insert_u64_bit(, compiler->scalar_stage[MESA_SHADER_VERTEX]);
> > + insert_u64_bit(,
> > compiler->scalar_stage[MESA_SHADER_TESS_CTRL]);
> > + insert_u64_bit(,
> > compiler->scalar_stage[MESA_SHADER_TESS_EVAL]);
> > + insert_u64_bit(,
> > compiler->scalar_stage[MESA_SHADER_GEOMETRY]);
> > + }
> > + uint64_t debug_bits = INTEL_DEBUG;
> > + uint64_t mask = DEBUG_DISK_CACHE_MASK;
> > + while (mask != 0) {
> > + const uint64_t bit = 1ULL << (ffsll(mask) - 1);
> > + insert_u64_bit(, (debug_bits & bit) != 0);
> > + mask &= ~bit;
> > + }
> > + return config;
> > +}
> >
> > unsigned
> > brw_prog_data_size(gl_shader_stage stage)
> > diff --git a/src/intel/compiler/brw_compiler.h
> > b/src/intel/compiler/brw_compiler.h
> > index 9dfcfcc0115..efefbbca64b 100644
> > --- a/src/intel/compiler/brw_compiler.h
> > +++ b/src/intel/compiler/brw_compiler.h
> > @@ -1212,6 +1212,17 @@ DEFINE_PROG_DATA_DOWNCAST(sf)
> >
> > struct brw_compiler *
> > brw_compiler_create(void *mem_ctx, const struct gen_device_info *devinfo);
> > +/**
> > + * Returns a compiler configuration for use with shader_config
> > + *
> > + * This value only needs to change for settings that can cause different
> > + * program generation between two runs on the same hardware.
> > + *
> > + * For example, it doesn't need to be different for gen 8 and gen 9
> > hardware,
> > + * but it does need to be different if INTEL_DEBUG=nocompact is or isn't
> > used.
> > + */
> > +const uint64_t
> > +brw_get_compiler_config_value(const struct brw_compiler *compiler);
> >
> > unsigned
> > brw_prog_data_size(gl_shader_stage stage);
> > diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> > index 6b72a79a914..c40b94d69f3 100644
> > --- a/src/intel/vulkan/anv_device.c
> > +++ b/src/intel/vulkan/anv_device.c
> > @@ -286,7 +286,8 @@ anv_physical_device_init_disk_cache(struct
> > anv_physical_device *device)
> > char timestamp[41];
> > _mesa_sha1_format(timestamp, device->driver_build_sha1);
> >
> > - const uint64_t driver_flags = INTEL_DEBUG & DEBUG_DISK_CACHE_MASK;
> > + const uint64_t driver_flags =
> > + brw_get_compiler_config_value(device->compiler);
> > device->disk_cache = disk_cache_create(renderer, timestamp,
> > driver_flags);
> > #else
> > device->disk_cache = NULL;
> > diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c
> > b/src/mesa/drivers/dri/i965/brw_disk_cache.c
> > index 0797e6eac44..9a6f2ff570c 1
[Mesa-dev] [PATCH 2/2] intel/compiler: Add brw_get_compiler_config_value for disk cache
During code review, Jason pointed out that:
2b3064c0731 "i965, anv: Use INTEL_DEBUG for disk_cache driver flags"
Didn't account for INTEL_SCALER_* environment variables.
To fix this, let the compiler return the disk_cache driver flags.
Another possible fix would be to pull the INTEL_SCALER_* into
INTEL_DEBUG bits, but as we are currently using 41 of 64 bits, I
didn't think it was a good use of 4 more of these bits. (5 since
INTEL_PRECISE_TRIG needs to be accounted for as well.)
Cc: Jason Ekstrand
Signed-off-by: Jordan Justen
---
src/intel/compiler/brw_compiler.c | 26 ++
src/intel/compiler/brw_compiler.h | 11 +
src/intel/vulkan/anv_device.c | 3 ++-
src/mesa/drivers/dri/i965/brw_disk_cache.c | 3 ++-
4 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/src/intel/compiler/brw_compiler.c
b/src/intel/compiler/brw_compiler.c
index 6480dbefbf6..ba6b8e0cbbe 100644
--- a/src/intel/compiler/brw_compiler.c
+++ b/src/intel/compiler/brw_compiler.c
@@ -180,6 +180,32 @@ brw_compiler_create(void *mem_ctx, const struct
gen_device_info *devinfo)
return compiler;
}
+static void
+insert_u64_bit(uint64_t *val, bool add)
+{
+ *val = (*val << 1) | !!add;
+}
+
+const uint64_t
+brw_get_compiler_config_value(const struct brw_compiler *compiler)
+{
+ uint64_t config = 0;
+ insert_u64_bit(, compiler->precise_trig);
+ if (compiler->devinfo->gen >= 8 && compiler->devinfo->gen < 10) {
+ insert_u64_bit(, compiler->scalar_stage[MESA_SHADER_VERTEX]);
+ insert_u64_bit(, compiler->scalar_stage[MESA_SHADER_TESS_CTRL]);
+ insert_u64_bit(, compiler->scalar_stage[MESA_SHADER_TESS_EVAL]);
+ insert_u64_bit(, compiler->scalar_stage[MESA_SHADER_GEOMETRY]);
+ }
+ uint64_t debug_bits = INTEL_DEBUG;
+ uint64_t mask = DEBUG_DISK_CACHE_MASK;
+ while (mask != 0) {
+ const uint64_t bit = 1ULL << (ffsll(mask) - 1);
+ insert_u64_bit(, (debug_bits & bit) != 0);
+ mask &= ~bit;
+ }
+ return config;
+}
unsigned
brw_prog_data_size(gl_shader_stage stage)
diff --git a/src/intel/compiler/brw_compiler.h
b/src/intel/compiler/brw_compiler.h
index 9dfcfcc0115..efefbbca64b 100644
--- a/src/intel/compiler/brw_compiler.h
+++ b/src/intel/compiler/brw_compiler.h
@@ -1212,6 +1212,17 @@ DEFINE_PROG_DATA_DOWNCAST(sf)
struct brw_compiler *
brw_compiler_create(void *mem_ctx, const struct gen_device_info *devinfo);
+/**
+ * Returns a compiler configuration for use with shader_config
+ *
+ * This value only needs to change for settings that can cause different
+ * program generation between two runs on the same hardware.
+ *
+ * For example, it doesn't need to be different for gen 8 and gen 9 hardware,
+ * but it does need to be different if INTEL_DEBUG=nocompact is or isn't used.
+ */
+const uint64_t
+brw_get_compiler_config_value(const struct brw_compiler *compiler);
unsigned
brw_prog_data_size(gl_shader_stage stage);
diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 6b72a79a914..c40b94d69f3 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -286,7 +286,8 @@ anv_physical_device_init_disk_cache(struct
anv_physical_device *device)
char timestamp[41];
_mesa_sha1_format(timestamp, device->driver_build_sha1);
- const uint64_t driver_flags = INTEL_DEBUG & DEBUG_DISK_CACHE_MASK;
+ const uint64_t driver_flags =
+ brw_get_compiler_config_value(device->compiler);
device->disk_cache = disk_cache_create(renderer, timestamp, driver_flags);
#else
device->disk_cache = NULL;
diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c
b/src/mesa/drivers/dri/i965/brw_disk_cache.c
index 0797e6eac44..9a6f2ff570c 100644
--- a/src/mesa/drivers/dri/i965/brw_disk_cache.c
+++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
@@ -396,7 +396,8 @@ brw_disk_cache_init(struct intel_screen *screen)
char timestamp[41];
_mesa_sha1_format(timestamp, id_sha1);
- const uint64_t driver_flags = INTEL_DEBUG & DEBUG_DISK_CACHE_MASK;
+ const uint64_t driver_flags =
+ brw_get_compiler_config_value(screen->compiler);
screen->disk_cache = disk_cache_create(renderer, timestamp, driver_flags);
#endif
}
--
2.18.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/2] i965: Disable shader cache with INTEL_DEBUG=shader_time
Shader time hard codes an index of the shader time buffer within the
gen program.
In order to support shader time in the disk shader cache, we'd need to
add the shader time index into the program key. This should work, but
probably is not worth it for this particular debug feature.
Therefore, let's just disable the disk shader cache if the shader time
debug feature is used.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106382
Fixes: 96fe36f7acc "i965: Enable disk shader cache by default"
Cc: Eero Tamminen
Cc: Kenneth Graunke
Signed-off-by: Jordan Justen
---
src/intel/common/gen_debug.h | 7 +--
src/mesa/drivers/dri/i965/brw_disk_cache.c | 3 +++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/intel/common/gen_debug.h b/src/intel/common/gen_debug.h
index aa9f3cf80d7..72d7ca20a39 100644
--- a/src/intel/common/gen_debug.h
+++ b/src/intel/common/gen_debug.h
@@ -84,10 +84,13 @@ extern uint64_t INTEL_DEBUG;
#define DEBUG_COLOR (1ull << 40)
#define DEBUG_REEMIT (1ull << 41)
+/* These flags are not compatible with the disk shader cache */
+#define DEBUG_DISK_CACHE_DISABLE_MASK DEBUG_SHADER_TIME
+
/* These flags may affect program generation */
#define DEBUG_DISK_CACHE_MASK \
- (DEBUG_SHADER_TIME | DEBUG_NO16 | DEBUG_NO_DUAL_OBJECT_GS | DEBUG_NO8 | \
- DEBUG_SPILL_FS | DEBUG_SPILL_VEC4 | DEBUG_NO_COMPACTION | DEBUG_DO32)
+ (DEBUG_NO16 | DEBUG_NO_DUAL_OBJECT_GS | DEBUG_NO8 | DEBUG_SPILL_FS | \
+ DEBUG_SPILL_VEC4 | DEBUG_NO_COMPACTION | DEBUG_DO32)
#ifdef HAVE_ANDROID_PLATFORM
#define LOG_TAG "INTEL-MESA"
diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c
b/src/mesa/drivers/dri/i965/brw_disk_cache.c
index 8f1b064fd61..0797e6eac44 100644
--- a/src/mesa/drivers/dri/i965/brw_disk_cache.c
+++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
@@ -377,6 +377,9 @@ void
brw_disk_cache_init(struct intel_screen *screen)
{
#ifdef ENABLE_SHADER_CACHE
+ if (INTEL_DEBUG & DEBUG_DISK_CACHE_DISABLE_MASK)
+ return;
+
/* array length: print length + null char + 1 extra to verify it is unused
*/
char renderer[11];
MAYBE_UNUSED int len = snprintf(renderer, sizeof(renderer), "i965_%04x",
--
2.18.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] docs: trivial html fix
You might as well add something like ">" => "" to the subject.
Reviewed-by: Jordan Justen
On 2018-07-25 12:18:34, Eric Engestrom wrote:
> Signed-off-by: Eric Engestrom
> ---
> docs/releasing.html | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/docs/releasing.html b/docs/releasing.html
> index a022d0c484bc0f635b61..69d8656e59d567033087 100644
> --- a/docs/releasing.html
> +++ b/docs/releasing.html
> @@ -492,10 +492,10 @@ Perform basic testing
> # Drop LLVM_CONFIG, if applicable:
> # unset LLVM_CONFIG
>
> - __glxinfo_cmd='glxinfo 2>1 | egrep -o
> "Mesa.*|Gallium.*|.*dri\.so"'
> - __glxgears_cmd='glxgears 2>1 | grep -v "configuration file"'
> - __es2info_cmd='es2_info 2>1 | egrep
> "GL_VERSION|GL_RENDERER|.*dri\.so"'
> - __es2gears_cmd='es2gears_x11 2>1 | grep -v "configuration file"'
> + __glxinfo_cmd='glxinfo 21 | egrep -o
> "Mesa.*|Gallium.*|.*dri\.so"'
> + __glxgears_cmd='glxgears 21 | grep -v "configuration file"'
> + __es2info_cmd='es2_info 21 | egrep
> "GL_VERSION|GL_RENDERER|.*dri\.so"'
> + __es2gears_cmd='es2gears_x11 21 | grep -v "configuration
> file"'
> test "x$LD_LIBRARY_PATH" != 'x' __old_ld="$LD_LIBRARY_PATH"
> export LD_LIBRARY_PATH=`pwd`/test/usr/local/lib/:"${__old_ld}"
> export LIBGL_DRIVERS_PATH=`pwd`/test/usr/local/lib/dri/
> --
> Cheers,
> Eric
>
> ___
> 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: Expose ARB_base_instance extension
I think the subject should include gles3: i965: Expose ARB_base_instance extension in OpenGL ES 3.0 Reviewed-by: Jordan Justen On 2018-07-25 10:48:31, Sagar Ghuge wrote: > The extension requires at least OpenGL 3.0 and > OpenGL ES 3.0. > > Fixes two ext_base_instance tests: > > arb_base_instance-baseinstance-doesnt-affect-gl-instance-id_gles3 > arb_base_instance-drawarrays_gles3 > > Signed-off-by: Sagar Ghuge > --- > src/mesa/drivers/dri/i965/intel_extensions.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/mesa/drivers/dri/i965/intel_extensions.c > b/src/mesa/drivers/dri/i965/intel_extensions.c > index f837356478..9d119d0b4c 100644 > --- a/src/mesa/drivers/dri/i965/intel_extensions.c > +++ b/src/mesa/drivers/dri/i965/intel_extensions.c > @@ -315,7 +315,7 @@ intelInitExtensions(struct gl_context *ctx) > if (devinfo->gen >= 6) >ctx->Extensions.INTEL_performance_query = true; > > - if (ctx->API == API_OPENGL_CORE) > + if (ctx->API != API_OPENGL_COMPAT) >ctx->Extensions.ARB_base_instance = true; > if (ctx->API != API_OPENGL_CORE) >ctx->Extensions.ARB_color_buffer_float = true; > -- > 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 v2 2/2] i965, anv: Use INTEL_DEBUG for disk_cache driver flags
On 2018-07-23 03:29:21, Eero Tamminen wrote: > Hi, > > Should this fix also: > https://bugs.freedesktop.org/show_bug.cgi?id=106382 > ? > It helped a bit. For me, on a simple test program, it allowed shader time to work once, even when the cache had a non-shader-time program. But, running wit shader-time a second time, it printed 'No shader time collected yet'. Without this patch, the test program would crash if the cache had a non-shader-time program, and this patch prevented that. Regarding the bisect info in that bug, if you happen to run with INTEL_DEBUG=shader_time first after a fresh build, then shader time would probably work, since it would generate the shader time program and put it in the cache. So, the bisect info could be unreliable. I'll take at what is happening on the '2nd shader-time run' issue that I described above. -Jordan > > On 23.07.2018 07:27, Jordan Justen wrote: > > Since various options within INTEL_DEBUG could impact code generation, > > we need to set the disk cache driver_flags parameter based on the > > INTEL_DEBUG flags in use. > > > > An example that will affect the program generated by i965 is the > > INTEL_DEBUG=nocompact option. > > > > The DEBUG_DISK_CACHE_MASK value is added to mask the settings of > > INTEL_DEBUG that can affect program generation. > > > > v2: > > * Use driver_flags (Tim) > > * Also update Anvil (Jason) > > > > Signed-off-by: Jordan Justen > > Reviewed-by: Lionel Landwerlin (v1) > > --- > > src/intel/common/gen_debug.h | 5 + > > src/intel/vulkan/anv_device.c | 3 ++- > > src/mesa/drivers/dri/i965/brw_disk_cache.c | 3 ++- > > 3 files changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/src/intel/common/gen_debug.h b/src/intel/common/gen_debug.h > > index f6c44eeb912..aa9f3cf80d7 100644 > > --- a/src/intel/common/gen_debug.h > > +++ b/src/intel/common/gen_debug.h > > @@ -84,6 +84,11 @@ extern uint64_t INTEL_DEBUG; > > #define DEBUG_COLOR (1ull << 40) > > #define DEBUG_REEMIT (1ull << 41) > > > > +/* These flags may affect program generation */ > > +#define DEBUG_DISK_CACHE_MASK \ > > + (DEBUG_SHADER_TIME | DEBUG_NO16 | DEBUG_NO_DUAL_OBJECT_GS | DEBUG_NO8 | > > \ > > + DEBUG_SPILL_FS | DEBUG_SPILL_VEC4 | DEBUG_NO_COMPACTION | DEBUG_DO32) > > + > > #ifdef HAVE_ANDROID_PLATFORM > > #define LOG_TAG "INTEL-MESA" > > #if ANDROID_API_LEVEL >= 26 > > diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c > > index 247ba641336..97a71563b8a 100644 > > --- a/src/intel/vulkan/anv_device.c > > +++ b/src/intel/vulkan/anv_device.c > > @@ -286,7 +286,8 @@ anv_physical_device_init_disk_cache(struct > > anv_physical_device *device) > > char timestamp[41]; > > _mesa_sha1_format(timestamp, device->driver_build_sha1); > > > > - device->disk_cache = disk_cache_create(renderer, timestamp, 0); > > + const uint64_t driver_flags = INTEL_DEBUG & DEBUG_DISK_CACHE_MASK; > > + device->disk_cache = disk_cache_create(renderer, timestamp, > > driver_flags); > > #else > > device->disk_cache = NULL; > > #endif > > diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c > > b/src/mesa/drivers/dri/i965/brw_disk_cache.c > > index a678c355b9d..8f1b064fd61 100644 > > --- a/src/mesa/drivers/dri/i965/brw_disk_cache.c > > +++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c > > @@ -393,6 +393,7 @@ brw_disk_cache_init(struct intel_screen *screen) > > char timestamp[41]; > > _mesa_sha1_format(timestamp, id_sha1); > > > > - screen->disk_cache = disk_cache_create(renderer, timestamp, 0); > > + const uint64_t driver_flags = INTEL_DEBUG & DEBUG_DISK_CACHE_MASK; > > + screen->disk_cache = disk_cache_create(renderer, timestamp, > > driver_flags); > > #endif > > } > > > > ___ > 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] [PATCH v2 2/2] i965, anv: Use INTEL_DEBUG for disk_cache driver flags
Since various options within INTEL_DEBUG could impact code generation, we need to set the disk cache driver_flags parameter based on the INTEL_DEBUG flags in use. An example that will affect the program generated by i965 is the INTEL_DEBUG=nocompact option. The DEBUG_DISK_CACHE_MASK value is added to mask the settings of INTEL_DEBUG that can affect program generation. v2: * Use driver_flags (Tim) * Also update Anvil (Jason) Signed-off-by: Jordan Justen Reviewed-by: Lionel Landwerlin (v1) --- src/intel/common/gen_debug.h | 5 + src/intel/vulkan/anv_device.c | 3 ++- src/mesa/drivers/dri/i965/brw_disk_cache.c | 3 ++- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/intel/common/gen_debug.h b/src/intel/common/gen_debug.h index f6c44eeb912..aa9f3cf80d7 100644 --- a/src/intel/common/gen_debug.h +++ b/src/intel/common/gen_debug.h @@ -84,6 +84,11 @@ extern uint64_t INTEL_DEBUG; #define DEBUG_COLOR (1ull << 40) #define DEBUG_REEMIT (1ull << 41) +/* These flags may affect program generation */ +#define DEBUG_DISK_CACHE_MASK \ + (DEBUG_SHADER_TIME | DEBUG_NO16 | DEBUG_NO_DUAL_OBJECT_GS | DEBUG_NO8 | \ + DEBUG_SPILL_FS | DEBUG_SPILL_VEC4 | DEBUG_NO_COMPACTION | DEBUG_DO32) + #ifdef HAVE_ANDROID_PLATFORM #define LOG_TAG "INTEL-MESA" #if ANDROID_API_LEVEL >= 26 diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c index 247ba641336..97a71563b8a 100644 --- a/src/intel/vulkan/anv_device.c +++ b/src/intel/vulkan/anv_device.c @@ -286,7 +286,8 @@ anv_physical_device_init_disk_cache(struct anv_physical_device *device) char timestamp[41]; _mesa_sha1_format(timestamp, device->driver_build_sha1); - device->disk_cache = disk_cache_create(renderer, timestamp, 0); + const uint64_t driver_flags = INTEL_DEBUG & DEBUG_DISK_CACHE_MASK; + device->disk_cache = disk_cache_create(renderer, timestamp, driver_flags); #else device->disk_cache = NULL; #endif diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c b/src/mesa/drivers/dri/i965/brw_disk_cache.c index a678c355b9d..8f1b064fd61 100644 --- a/src/mesa/drivers/dri/i965/brw_disk_cache.c +++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c @@ -393,6 +393,7 @@ brw_disk_cache_init(struct intel_screen *screen) char timestamp[41]; _mesa_sha1_format(timestamp, id_sha1); - screen->disk_cache = disk_cache_create(renderer, timestamp, 0); + const uint64_t driver_flags = INTEL_DEBUG & DEBUG_DISK_CACHE_MASK; + screen->disk_cache = disk_cache_create(renderer, timestamp, driver_flags); #endif } -- 2.18.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 1/2] i965, anv: Add extra unused character in disk_cache renderer temp string
This extra character should not be used by snprintf, but we make it
available to verify that we printed the exact number we wanted, and
didn't overflow.
v2:
* Also update Anvil
Signed-off-by: Jordan Justen
Reviewed-by: Lionel Landwerlin (v1)
---
src/intel/vulkan/anv_device.c | 4 ++--
src/mesa/drivers/dri/i965/brw_disk_cache.c | 5 +++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 04fd6a829ed..247ba641336 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -278,10 +278,10 @@ static void
anv_physical_device_init_disk_cache(struct anv_physical_device *device)
{
#ifdef ENABLE_SHADER_CACHE
- char renderer[9];
+ char renderer[10];
MAYBE_UNUSED int len = snprintf(renderer, sizeof(renderer), "anv_%04x",
device->chipset_id);
- assert(len == sizeof(renderer) - 1);
+ assert(len == sizeof(renderer) - 2);
char timestamp[41];
_mesa_sha1_format(timestamp, device->driver_build_sha1);
diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c
b/src/mesa/drivers/dri/i965/brw_disk_cache.c
index c478753d4ad..a678c355b9d 100644
--- a/src/mesa/drivers/dri/i965/brw_disk_cache.c
+++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
@@ -377,10 +377,11 @@ void
brw_disk_cache_init(struct intel_screen *screen)
{
#ifdef ENABLE_SHADER_CACHE
- char renderer[10];
+ /* array length: print length + null char + 1 extra to verify it is unused
*/
+ char renderer[11];
MAYBE_UNUSED int len = snprintf(renderer, sizeof(renderer), "i965_%04x",
screen->deviceID);
- assert(len == sizeof(renderer) - 1);
+ assert(len == sizeof(renderer) - 2);
const struct build_id_note *note =
build_id_find_nhdr_for_addr(brw_disk_cache_init);
--
2.18.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965/disk_cache: Add in the INTEL_DEBUG value to the renderer string
On 2018-07-22 13:43:08, Lionel Landwerlin wrote:
> Nice catch!
>
> Reviewed-by: Lionel Landwerlin
Thanks! Tim pointed out on irc that disk_cache_create has a
driver_flags param just for this purpose. Also, Jason asked that I
update the Anvil driver too. I'll try to send out a v2 today.
-Jordan
>
> On 22/07/18 05:45, Jordan Justen wrote:
> > Since various options within INTEL_DEBUG could impact code generation,
> > we should add this into the renderer string so changing the
> > INTEL_DEBUG setting will cause the shader cache to work properly.
> >
> > An example that will affect the program generated by i965 is the
> > INTEL_DEBUG=nocompact option.
> >
> > Adding in the entire 64-bits of INTEL_DEBUG is overkill. Many
> > INTEL_DEBUG options won't actually impact the program generated by the
> > driver. Nevertheless, it is more maintainable and safer to just add
> > the entire 64-bit value.
> >
> > Cc: Timothy Arceri
> > Signed-off-by: Jordan Justen
> > ---
> > src/mesa/drivers/dri/i965/brw_disk_cache.c | 7 ---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c
> > b/src/mesa/drivers/dri/i965/brw_disk_cache.c
> > index a678c355b9d..6e50476f812 100644
> > --- a/src/mesa/drivers/dri/i965/brw_disk_cache.c
> > +++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
> > @@ -378,9 +378,10 @@ brw_disk_cache_init(struct intel_screen *screen)
> > {
> > #ifdef ENABLE_SHADER_CACHE
> > /* array length: print length + null char + 1 extra to verify it is
> > unused */
> > - char renderer[11];
> > - MAYBE_UNUSED int len = snprintf(renderer, sizeof(renderer), "i965_%04x",
> > - screen->deviceID);
> > + char renderer[28];
> > + MAYBE_UNUSED int len =
> > + snprintf(renderer, sizeof(renderer), "i965_%04x_%016" PRIx64,
> > + screen->deviceID, INTEL_DEBUG);
> > assert(len == sizeof(renderer) - 2);
> >
> > const struct build_id_note *note =
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/2] i965/disk_cache: Add in the INTEL_DEBUG value to the renderer string
Since various options within INTEL_DEBUG could impact code generation,
we should add this into the renderer string so changing the
INTEL_DEBUG setting will cause the shader cache to work properly.
An example that will affect the program generated by i965 is the
INTEL_DEBUG=nocompact option.
Adding in the entire 64-bits of INTEL_DEBUG is overkill. Many
INTEL_DEBUG options won't actually impact the program generated by the
driver. Nevertheless, it is more maintainable and safer to just add
the entire 64-bit value.
Cc: Timothy Arceri
Signed-off-by: Jordan Justen
---
src/mesa/drivers/dri/i965/brw_disk_cache.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c
b/src/mesa/drivers/dri/i965/brw_disk_cache.c
index a678c355b9d..6e50476f812 100644
--- a/src/mesa/drivers/dri/i965/brw_disk_cache.c
+++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
@@ -378,9 +378,10 @@ brw_disk_cache_init(struct intel_screen *screen)
{
#ifdef ENABLE_SHADER_CACHE
/* array length: print length + null char + 1 extra to verify it is unused
*/
- char renderer[11];
- MAYBE_UNUSED int len = snprintf(renderer, sizeof(renderer), "i965_%04x",
- screen->deviceID);
+ char renderer[28];
+ MAYBE_UNUSED int len =
+ snprintf(renderer, sizeof(renderer), "i965_%04x_%016" PRIx64,
+ screen->deviceID, INTEL_DEBUG);
assert(len == sizeof(renderer) - 2);
const struct build_id_note *note =
--
2.18.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/2] i965/disk_cache: Add extra unused character in renderer temp string
This extra character should not be used by snprintf, but we make it
available to verify that we printed the exact number we wanted, and
didn't overflow.
Signed-off-by: Jordan Justen
---
src/mesa/drivers/dri/i965/brw_disk_cache.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c
b/src/mesa/drivers/dri/i965/brw_disk_cache.c
index c478753d4ad..a678c355b9d 100644
--- a/src/mesa/drivers/dri/i965/brw_disk_cache.c
+++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
@@ -377,10 +377,11 @@ void
brw_disk_cache_init(struct intel_screen *screen)
{
#ifdef ENABLE_SHADER_CACHE
- char renderer[10];
+ /* array length: print length + null char + 1 extra to verify it is unused
*/
+ char renderer[11];
MAYBE_UNUSED int len = snprintf(renderer, sizeof(renderer), "i965_%04x",
screen->deviceID);
- assert(len == sizeof(renderer) - 1);
+ assert(len == sizeof(renderer) - 2);
const struct build_id_note *note =
build_id_find_nhdr_for_addr(brw_disk_cache_init);
--
2.18.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 3/4] intel: tools: aubwrite: fix invalid frees on finish
Reviewed-by: Jordan Justen
On 2018-07-18 10:21:31, Lionel Landwerlin wrote:
> Signed-off-by: Lionel Landwerlin
> ---
> src/intel/tools/aub_write.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/src/intel/tools/aub_write.c b/src/intel/tools/aub_write.c
> index 1224e8f6b7f..de4ce33 100644
> --- a/src/intel/tools/aub_write.c
> +++ b/src/intel/tools/aub_write.c
> @@ -255,11 +255,16 @@ align_u32(uint32_t v, uint32_t a)
> }
>
> static void
> -aub_ppgtt_table_finish(struct aub_ppgtt_table *table)
> +aub_ppgtt_table_finish(struct aub_ppgtt_table *table, int level)
> {
> + if (level == 1)
> + return;
> +
> for (unsigned i = 0; i < ARRAY_SIZE(table->subtables); i++) {
> - aub_ppgtt_table_finish(table->subtables[i]);
> - free(table->subtables[i]);
> + if (table->subtables[i]) {
> + aub_ppgtt_table_finish(table->subtables[i], level - 1);
> + free(table->subtables[i]);
> + }
> }
> }
>
> @@ -280,7 +285,7 @@ aub_file_init(struct aub_file *aub, FILE *file, uint16_t
> pci_id)
> void
> aub_file_finish(struct aub_file *aub)
> {
> - aub_ppgtt_table_finish(>pml4);
> + aub_ppgtt_table_finish(>pml4, 4);
> fclose(aub->file);
> }
>
> --
> 2.18.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
[Mesa-dev] [PATCH] intel/tools: fix segfault with intel_dump_gpu
Cc: Jason Ekstrand
Cc: Lionel Landwerlin
Fixes: 0a457d987ee "intel/tools: Refactor aub dumping to remove singletons"
Signed-off-by: Jordan Justen
---
src/intel/tools/intel_dump_gpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/intel/tools/intel_dump_gpu.c b/src/intel/tools/intel_dump_gpu.c
index 6d2c4b7f983..e0ff1245925 100644
--- a/src/intel/tools/intel_dump_gpu.c
+++ b/src/intel/tools/intel_dump_gpu.c
@@ -301,7 +301,7 @@ dump_execbuffer2(int fd, struct drm_i915_gem_execbuffer2
*execbuffer2)
}
for (int i = 0; i < ARRAY_SIZE(files); i++) {
- if (files[i] != NULL)
+ if (files[i] == NULL)
continue;
aub_write_exec([i],
--
2.18.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] anv, radv: Add support for VK_KHR_get_display_properties2
features.txt? It looks like some others have been missed recently.
-Jordan
On 2018-06-20 08:07:20, Jason Ekstrand wrote:
> Cc: Keith Packard
> ---
> src/amd/vulkan/radv_extensions.py | 1 +
> src/amd/vulkan/radv_wsi_display.c | 57 +
> src/intel/vulkan/anv_extensions.py | 1 +
> src/intel/vulkan/anv_wsi_display.c | 56 +
> src/vulkan/wsi/wsi_common_display.c | 175 +---
> src/vulkan/wsi/wsi_common_display.h | 27 +
> 6 files changed, 301 insertions(+), 16 deletions(-)
>
> diff --git a/src/amd/vulkan/radv_extensions.py
> b/src/amd/vulkan/radv_extensions.py
> index 65ce7349016..5eb63a7d5dc 100644
> --- a/src/amd/vulkan/radv_extensions.py
> +++ b/src/amd/vulkan/radv_extensions.py
> @@ -66,6 +66,7 @@ EXTENSIONS = [
> Extension('VK_KHR_external_semaphore',1,
> 'device->rad_info.has_syncobj'),
> Extension('VK_KHR_external_semaphore_capabilities', 1, True),
> Extension('VK_KHR_external_semaphore_fd', 1,
> 'device->rad_info.has_syncobj'),
> +Extension('VK_KHR_get_display_properties2', 1,
> 'VK_USE_PLATFORM_DISPLAY_KHR'),
> Extension('VK_KHR_get_memory_requirements2', 1, True),
> Extension('VK_KHR_get_physical_device_properties2', 1, True),
> Extension('VK_KHR_get_surface_capabilities2', 1,
> 'RADV_HAS_SURFACE'),
> diff --git a/src/amd/vulkan/radv_wsi_display.c
> b/src/amd/vulkan/radv_wsi_display.c
> index 84431019dbb..764180ec7b5 100644
> --- a/src/amd/vulkan/radv_wsi_display.c
> +++ b/src/amd/vulkan/radv_wsi_display.c
> @@ -56,6 +56,20 @@
> radv_GetPhysicalDeviceDisplayPropertiesKHR(VkPhysicalDevice physical_device,
> properties);
> }
>
> +VkResult
> +radv_GetPhysicalDeviceDisplayProperties2KHR(VkPhysicalDevice physical_device,
> +uint32_t *property_count,
> +VkDisplayProperties2KHR
> *properties)
> +{
> + RADV_FROM_HANDLE(radv_physical_device, pdevice, physical_device);
> +
> + return wsi_display_get_physical_device_display_properties2(
> + physical_device,
> + >wsi_device,
> + property_count,
> + properties);
> +}
> +
> VkResult
> radv_GetPhysicalDeviceDisplayPlanePropertiesKHR(
> VkPhysicalDevice physical_device,
> @@ -71,6 +85,21 @@ radv_GetPhysicalDeviceDisplayPlanePropertiesKHR(
> properties);
> }
>
> +VkResult
> +radv_GetPhysicalDeviceDisplayPlaneProperties2KHR(
> + VkPhysicalDevice physical_device,
> + uint32_t *property_count,
> + VkDisplayPlaneProperties2KHR *properties)
> +{
> + RADV_FROM_HANDLE(radv_physical_device, pdevice, physical_device);
> +
> + return wsi_display_get_physical_device_display_plane_properties2(
> + physical_device,
> + >wsi_device,
> + property_count,
> + properties);
> +}
> +
> VkResult
> radv_GetDisplayPlaneSupportedDisplaysKHR(VkPhysicalDevice physical_device,
> uint32_t plane_index,
> @@ -103,6 +132,21 @@ radv_GetDisplayModePropertiesKHR(VkPhysicalDevice
> physical_device,
>properties);
> }
>
> +VkResult
> +radv_GetDisplayModeProperties2KHR(VkPhysicalDevice physical_device,
> + VkDisplayKHR display,
> + uint32_t *property_count,
> + VkDisplayModeProperties2KHR *properties)
> +{
> + RADV_FROM_HANDLE(radv_physical_device, pdevice, physical_device);
> +
> + return wsi_display_get_display_mode_properties2(physical_device,
> + >wsi_device,
> + display,
> + property_count,
> + properties);
> +}
> +
> VkResult
> radv_CreateDisplayModeKHR(VkPhysicalDevice physical_device,
>VkDisplayKHR display,
> @@ -135,6 +179,19 @@ radv_GetDisplayPlaneCapabilitiesKHR(VkPhysicalDevice
> physical_device,
> capabilities);
> }
>
> +VkResult
> +radv_GetDisplayPlaneCapabilities2KHR(VkPhysicalDevice physical_device,
> + const VkDisplayPlaneInfo2KHR
> *pDisplayPlaneInfo,
> + VkDisplayPlaneCapabilities2KHR
> *capabilities)
> +{
> + RADV_FROM_HANDLE(radv_physical_device, pdevice, physical_device);
> +
> + return wsi_get_display_plane_capabilities2(physical_device,
> + >wsi_device,
> + pDisplayPlaneInfo,
> + capabilities);
> +}
> +
Re: [Mesa-dev] [PATCH 00/16] Move the Mesa Website to Sphinx
On 2018-06-18 17:48:26, Ilia Mirkin wrote: > On Mon, Jun 18, 2018 at 7:19 PM, Jason Ekstrand wrote: > > On Mon, Jun 18, 2018 at 3:57 PM, Ilia Mirkin wrote: > >> > >> Not sure how much voting power I have, but I really like Erik's version at > >> > >> https://codepen.io/kusma/pen/vrXppL > >> > >> It uses CSS3 animation on hover, so it's pretty low impact. Some > >> slight cleverness could even include a fallback for a browser that > >> doesn't support SVG. > > > > > > Could you be more specific about what exactly it is that you like? Do you > > like the font? colors? style? the fact that it uses css? the fact that it > > doesn't use JS? To me, the later two are immaterial and pretty trivial to > > put into any logo design. Using WebGL is a bit less trivial, obviously. > > For reference, I'm comparing these two versions: > > Erik's: https://codepen.io/kusma/pen/vrXppL > Jason's: https://codepen.io/anon/pen/gKXobw > > I like both the aesthetic and the tech in Erik's variant. In general I > prefer serif fonts for readability, but I think in this case, a > sans-serif font lends itself better to lining up with the holes (I > believe others have commented on this). Placing the biggest gear below > the M (as in Erik's variant) gives it better weighting than above the > M (as in Jason's), since the M is also taller than the other letters, > that red gear is further above the "esa" in that version. Also the top > right serif from the M covers an awkward portion of the blue gear in > Jason's version. I do like the fact that Jason's version has a heavier > font, so if it's an option, Erik, I'd suggest trying to increase the > font weight on yours to see what happens. > > Regarding the tech, svg animations appear to be on their way out: > https://css-tricks.com/smil-is-dead-long-live-smil-a-guide-to-alternatives-to-smil-features/ > (apparently it's not supported in IE or Edge[1]). So CSS3 animations > seem like the accepted thing to use all over. Does Erik or Jason's version use CSS3? Thus they should work 'all over' include Edge/IE11? I tried them on Edge/IE11: > Erik's: https://codepen.io/kusma/pen/vrXppL The 'Mesa' text overflowed the box. (Should be easy to fix.) The gears did not turn. > Jason's: https://codepen.io/anon/pen/gKXobw It appeared to render correctly on Windows and Linux. The gears did not turn. -- I also fixed the Edge/IE11 issue with: https://people.freedesktop.org/~jljusten/webgl-logo/gears.html Now the webgl works and looks the same on Linux (Firefox, Chrome, WebKit), and Windows (Chrome, Edge, IE11). -Jordan > Now triggering with js > vs a hover style -- not hugely important, but IME the > mouseover/mouseout stuff is pretty unreliable. Maybe it actually works > now, who knows, but I've never had issues with :hover. Note that it's > also easy with CSS3 to do something like animate for 5s on load and > then stop (until the mouseover). (Yeah, you can do this with JS as > well...(most) ) > > Having some easter egg, as Rob Clark suggests, which flips it to a > full webgl thing would be cool, but seems like a separate endeavor. > > [1] The annoying thing one has to deal with in web development ... > browser support. Animation is an "extra" feature, but why not just > make it work? IE11 has no trouble with (most?) CSS3 animation > features. > ___ > 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/16] Move the Mesa Website to Sphinx
On 2018-06-18 15:57:50, Ilia Mirkin wrote: > Not sure how much voting power I have, but I really like Erik's version at > > https://codepen.io/kusma/pen/vrXppL I think it's unfortunate that the logo says "3D Graphics Library", while simultaneously showing an image that couldn't look more 2D. :) I think webgl is cool because of the obvious tie in with what Mesa does, but maybe webgl is too concerning because it's crashing browsers. Well, for one, that's not great. :) But, another option would be to make an animated gif of the 3D gears to place behind the svg text. (Sample animated gif: https://gitlab.com/gears3d) -Jordan > It uses CSS3 animation on hover, so it's pretty low impact. Some > slight cleverness could even include a fallback for a browser that > doesn't support SVG. > > IMO solutions like three.js and so on are non-starters for something > as trivial as a logo. Having some mesa-demos ported using it might be > neat, but the logo should be kept simple and accessible. > > -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/16] Move the Mesa Website to Sphinx
On 2018-06-17 17:43:37, Jason Ekstrand wrote: > On Sun, Jun 17, 2018 at 5:34 PM, Jordan Justen > wrote: > > > On 2018-06-17 16:42:13, Jason Ekstrand wrote: > > > On Sun, Jun 17, 2018 at 4:21 PM, Matt Turner wrote: > > > > > > > Also, Erik's is animated not with JavaScript at all, but just CSS. > > > > That's really cool. > > > > > > > > > > The only thing the JS does is turn the animation on and off. The actual > > > animation is pure SVG. The main reason (as I understood it) for the > > > JavaScript is to make the gears *not* turn if someone has disabled > > > JavaScript. I'm sure people will disagree as to whether or not that's a > > > feature. :-) I'm not sure if you can get the turn-on-hover thing with > > just > > > SVG and not JS though... /me doesn't know either all that well. > > > > All the JS and webgl. :) > > > > https://people.freedesktop.org/~jljusten/webgl-logo/gears.html > > > > Actually, it doesn't start webgl until you move your mouse over the > > logo. Before that it is displaying a jpg of the gears. > > > > I like how the gears smoothly spin up and slow to a stop. Not sure what I > think of the font. I changed the font to Roboto Slab, which I saw you mentioned earlier. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/16] Move the Mesa Website to Sphinx
On 2018-06-17 16:42:13, Jason Ekstrand wrote: > On Sun, Jun 17, 2018 at 4:21 PM, Matt Turner wrote: > > > Also, Erik's is animated not with JavaScript at all, but just CSS. > > That's really cool. > > > > The only thing the JS does is turn the animation on and off. The actual > animation is pure SVG. The main reason (as I understood it) for the > JavaScript is to make the gears *not* turn if someone has disabled > JavaScript. I'm sure people will disagree as to whether or not that's a > feature. :-) I'm not sure if you can get the turn-on-hover thing with just > SVG and not JS though... /me doesn't know either all that well. All the JS and webgl. :) https://people.freedesktop.org/~jljusten/webgl-logo/gears.html Actually, it doesn't start webgl until you move your mouse over the logo. Before that it is displaying a jpg of the gears. (Not to say there are not issues. On windows the svg overlay image looks a bit off. It also didn't work on the Edge browser, but I thought Edge supported webgl.) -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/16] Move the Mesa Website to Sphinx
On 2018-06-15 15:16:50, Laura Ekstrand wrote: > > We are now running on our own Docker image: > https://mesa-test.freedesktop.org/. > FWIW, I think the test site now looks better than mesa3d.org and retains some consistency with it, so for the new site: Acked-by: Jordan Justen Personally, I'm not sure about the flat gears vs the 3d ones. But the flat ones still maintain some consistency with the old site, and they look pretty good. Thanks for all your work on this! -Jordan > > On Mon, Jun 11, 2018 at 3:24 PM, Laura Ekstrand > wrote: > > > I really like the rotate on hover effect for the gears. I would rather > > that we use 2D WebGL for it if we could, though, since Mesa enables WebGL > > on certain platforms. > > > > Stuart, I will try to get the favicon and rtd_theme fixed. We may have to > > make our Docker image. > > > > Thanks. > > > > Laura > > > > On Fri, Jun 8, 2018 at 7:23 AM, Erik Faye-Lund > > wrote: > > > >> On Fri, Jun 8, 2018 at 4:09 PM Rhys Perry > >> wrote: > >> > > >> > Might be good to do something like this: https://codepen.io/anon/pen/ER > >> NdYJ > >> > So that those with NoScript or something won't have gears constantly > >> > rotating on their screen. > >> > > >> > >> Yeah, good point. > >> ___ > >> 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] glsl: serialize data from glTransformFeedbackVaryings
On 2018-06-14 02:58:33, Tapani Pälli wrote: > While XFB has been enabled for cache, we did not serialize enough > data for the whole API to work (such as glGetProgramiv). > > Fixes: 6d830940f7 "Allow shader cache usage with transform feedback" > Signed-off-by: Tapani Pälli > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106907 > --- > src/compiler/glsl/serialize.cpp | 15 +++ > 1 file changed, 15 insertions(+) > > diff --git a/src/compiler/glsl/serialize.cpp b/src/compiler/glsl/serialize.cpp > index 727822633d..4cb74ddba9 100644 > --- a/src/compiler/glsl/serialize.cpp > +++ b/src/compiler/glsl/serialize.cpp > @@ -323,6 +323,12 @@ write_xfb(struct blob *metadata, struct > gl_shader_program *shProg) > > blob_write_uint32(metadata, prog->info.stage); > > + /* Data set by glTransformFeedbackVaryings. */ > + blob_write_uint32(metadata, shProg->TransformFeedback.BufferMode); > + blob_write_uint32(metadata, shProg->TransformFeedback.NumVarying); > + for (unsigned i = 0; i < shProg->TransformFeedback.NumVarying; i++) > + blob_write_string(metadata, shProg->TransformFeedback.VaryingNames[i]); I guess we need BufferStride too? > + > blob_write_uint32(metadata, ltf->NumOutputs); > blob_write_uint32(metadata, ltf->ActiveBuffers); > blob_write_uint32(metadata, ltf->NumVarying); > @@ -352,6 +358,15 @@ read_xfb(struct blob_reader *metadata, struct > gl_shader_program *shProg) > if (xfb_stage == ~0u) >return; > > + /* Data set by glTransformFeedbackVaryings. */ > + shProg->TransformFeedback.BufferMode = blob_read_uint32(metadata); > + shProg->TransformFeedback.NumVarying = blob_read_uint32(metadata); > + shProg->TransformFeedback.VaryingNames = (char **) > + malloc(shProg->TransformFeedback.NumVarying * sizeof(GLchar *)); > + for (unsigned i = 0; i < shProg->TransformFeedback.NumVarying; i++) > + shProg->TransformFeedback.VaryingNames[i] = > + strdup(blob_read_string(metadata)); I guess VaryingNames uses malloc/free rather than ralloc. Maybe worth a comment? A comment might just be extra noise though too. With BufferStride added, Reviewed-by: Jordan Justen > + > struct gl_program *prog = shProg->_LinkedShaders[xfb_stage]->Program; > struct gl_transform_feedback_info *ltf = >rzalloc(prog, struct gl_transform_feedback_info); > -- > 2.14.4 > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] features.txt: mark some extensions as done
Series Reviewed-by: Jordan Justen On 2018-06-14 04:08:09, Tapani Pälli wrote: > Signed-off-by: Tapani Pälli > --- > docs/features.txt | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/docs/features.txt b/docs/features.txt > index b32606d223..423b03a9a9 100644 > --- a/docs/features.txt > +++ b/docs/features.txt > @@ -322,12 +322,14 @@ Khronos, ARB, and OES extensions that are not part of > any OpenGL or OpenGL ES ve >GL_EXT_semaphore DONE (radeonsi) >GL_EXT_semaphore_fd DONE (radeonsi) >GL_EXT_semaphore_win32not started > + GL_EXT_texture_norm16 DONE (i965, r600, > radeonsi, nvc0) >GL_KHR_blend_equation_advanced_coherent DONE (i965/gen9+) >GL_KHR_texture_compression_astc_hdr DONE (i965/bxt) >GL_KHR_texture_compression_astc_sliced_3d DONE (i965/gen9+) >GL_OES_depth_texture_cube_map DONE (all drivers > that support GLSL 1.30+) >GL_OES_EGL_image DONE (all drivers) > - GL_OES_EGL_image_external_essl3 not started > + GL_OES_EGL_image_external DONE (all drivers) > + GL_OES_EGL_image_external_essl3 DONE (all drivers) >GL_OES_required_internalformatDONE (all drivers) >GL_OES_surfaceless_contextDONE (all drivers) >GL_OES_texture_compression_astc DONE (core only) > @@ -335,7 +337,7 @@ Khronos, ARB, and OES extensions that are not part of any > OpenGL or OpenGL ES ve >GL_OES_texture_float_linear DONE (freedreno, > i965, r300, r600, radeonsi, nv30, nv50, nvc0, softpipe, llvmpipe) >GL_OES_texture_half_float DONE (freedreno, > i965, r300, r600, radeonsi, nv30, nv50, nvc0, softpipe, llvmpipe) >GL_OES_texture_half_float_linear DONE (freedreno, > i965, r300, r600, radeonsi, nv30, nv50, nvc0, softpipe, llvmpipe) > - GL_OES_texture_view not started - based > on GL_ARB_texture_view > + GL_OES_texture_view DONE (i965/gen8+) >GL_OES_viewport_array DONE (i965, nvc0, > radeonsi) >GLX_ARB_context_flush_control not started >GLX_ARB_robustness_application_isolation not started > -- > 2.14.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 v2 2/2] i965: Implement ARB_compute_variable_group_size.
On 2018-06-07 08:34:26, Plamena Manolova wrote:
> This patch adds the implementation of ARB_compute_variable_group_size
> for i965. We do this by storing the group size in a buffer surface,
> similarly to the work group number.
>
> v2: Fix some indentation inconsistencies (Jordan, Ilia)
> Do DIV_ROUND_UP correctly in brw_nir_lower_cs_intrinsics.c (Jordan)
> Use alphabetical order in features.txt (Matt)
> Set the extension constants properly in brw_context.c
>
> Signed-off-by: Plamena Manolova
> ---
> docs/features.txt| 2 +-
> docs/relnotes/18.2.0.html| 1 +
> src/compiler/nir/nir_lower_system_values.c | 13
> src/intel/compiler/brw_compiler.h| 2 +
> src/intel/compiler/brw_fs.cpp| 45
> src/intel/compiler/brw_fs_nir.cpp| 20 ++
> src/intel/compiler/brw_nir_lower_cs_intrinsics.c | 88
> +---
> src/mesa/drivers/dri/i965/brw_compute.c | 25 ++-
> src/mesa/drivers/dri/i965/brw_context.c | 6 ++
> src/mesa/drivers/dri/i965/brw_context.h | 1 +
> src/mesa/drivers/dri/i965/brw_cs.c | 4 ++
> src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 27 +++-
> src/mesa/drivers/dri/i965/intel_extensions.c | 1 +
> 13 files changed, 193 insertions(+), 42 deletions(-)
>
> diff --git a/docs/features.txt b/docs/features.txt
> index ed4050cf98..81b6663288 100644
> --- a/docs/features.txt
> +++ b/docs/features.txt
> @@ -298,7 +298,7 @@ Khronos, ARB, and OES extensions that are not part of any
> OpenGL or OpenGL ES ve
>
>GL_ARB_bindless_texture DONE (nvc0, radeonsi)
>GL_ARB_cl_event not started
> - GL_ARB_compute_variable_group_sizeDONE (nvc0, radeonsi)
> + GL_ARB_compute_variable_group_sizeDONE (i965, nvc0,
> radeonsi)
>GL_ARB_ES3_2_compatibilityDONE (i965/gen8+)
>GL_ARB_fragment_shader_interlock DONE (i965)
>GL_ARB_gpu_shader_int64 DONE (i965/gen8+,
> nvc0, radeonsi, softpipe, llvmpipe)
> diff --git a/docs/relnotes/18.2.0.html b/docs/relnotes/18.2.0.html
> index 0db37b620d..7475a56633 100644
> --- a/docs/relnotes/18.2.0.html
> +++ b/docs/relnotes/18.2.0.html
> @@ -52,6 +52,7 @@ Note: some of the new features are only available with
> certain drivers.
>
>
> GL_ARB_fragment_shader_interlock on i965
> +GL_ARB_compute_variable_group_size on i965
>
>
> Bug fixes
> diff --git a/src/compiler/nir/nir_lower_system_values.c
> b/src/compiler/nir/nir_lower_system_values.c
> index 487da04262..7ab005b000 100644
> --- a/src/compiler/nir/nir_lower_system_values.c
> +++ b/src/compiler/nir/nir_lower_system_values.c
> @@ -57,6 +57,14 @@ convert_block(nir_block *block, nir_builder *b)
>*gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID"
>*/
>
> + /*
> + * If the local work group size is variable we can't lower the
> global
> + * invocation id here.
> + */
> + if (b->shader->info.cs.local_size_variable) {
> +break;
> + }
> +
> nir_const_value local_size;
> memset(_size, 0, sizeof(local_size));
> local_size.u32[0] = b->shader->info.cs.local_size[0];
> @@ -102,6 +110,11 @@ convert_block(nir_block *block, nir_builder *b)
>}
>
>case SYSTEM_VALUE_LOCAL_GROUP_SIZE: {
> + /* If the local work group size is variable we can't lower it here
> */
> + if (b->shader->info.cs.local_size_variable) {
> +break;
> + }
> +
> nir_const_value local_size;
> memset(_size, 0, sizeof(local_size));
> local_size.u32[0] = b->shader->info.cs.local_size[0];
> diff --git a/src/intel/compiler/brw_compiler.h
> b/src/intel/compiler/brw_compiler.h
> index 8b4e6fe2e2..f54952c28f 100644
> --- a/src/intel/compiler/brw_compiler.h
> +++ b/src/intel/compiler/brw_compiler.h
> @@ -759,6 +759,7 @@ struct brw_cs_prog_data {
> unsigned threads;
> bool uses_barrier;
> bool uses_num_work_groups;
> + bool uses_variable_group_size;
>
> struct {
>struct brw_push_const_block cross_thread;
> @@ -771,6 +772,7 @@ struct brw_cs_prog_data {
> * surface indices the CS-specific surfaces
> */
>uint32_t work_groups_start;
> + uint32_t work_group_size_start;
>/** @} */
> } binding_table;
> };
> diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp
> index d67c0a4192..28730af47b 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -7228,18 +7228,32 @@ brw_compile_cs(const struct brw_compiler *compiler,
> void *log_data,
> int shader_time_index,
> char **error_str)
> {
> -
Re: [Mesa-dev] [PATCH 00/16] Move the Mesa Website to Sphinx
On Thu, Jun 7, 2018 at 2:56 AM Eero Tamminen wrote: > On 07.06.2018 12:01, Erik Faye-Lund wrote: > > Just as a fun toy, I decided to give an animated SVG "variation" of > > this a go myself: > > > > https://codepen.io/kusma/pen/vrXppL > > > > The actual SVG can be found here: > > > > https://gitlab.freedesktop.org/snippets/492 > > > > The gears were generated by this python script, based on the glxgears > > source code: > > > > https://gitlab.freedesktop.org/snippets/491 > > > > Now, dropping this onto the black background doesn't work that well, > > as it gets a bit bland, so it's probably better to add back the colors > > then. > > > > Also, I'm not really sure if animation is a good idea or not. > > Maybe it could be a link target for the static logo? > > (Kind of website "easter egg"). > > > But I definitely think logos should be vector rather than raster ;) > > For Mesa, WebGL would be more fitting implementation than SVG though... https://github.com/gears3d/gears3d.github.io/blob/master/webgl10.js One comment I would have for any animation on the main pages (as opposed to a separate 'easter egg' page), it probably should be significantly slower moving than the traditional 70 degrees / second. The faster animation would be distracting on the main pages. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] mesa/program_binary: add implicit UseProgram after successful ProgramBinary
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106810
Fixes: b4c37ce2140 "i965: Add ARB_get_program_binary support using
nir_serialization"
Ref: 3fe8d04a6d6 "mesa: don't always set _NEW_PROGRAM when linking"
Ref: c505d6d8522 "mesa: use gl_program for CurrentProgram rather than
gl_shader_program"
Cc: Timothy Arceri
Cc:
Signed-off-by: Jordan Justen
---
src/mesa/main/program_binary.c | 33 +
1 file changed, 33 insertions(+)
diff --git a/src/mesa/main/program_binary.c b/src/mesa/main/program_binary.c
index 021f6315e72..f4aa57821c8 100644
--- a/src/mesa/main/program_binary.c
+++ b/src/mesa/main/program_binary.c
@@ -33,6 +33,8 @@
#include "compiler/glsl/serialize.h"
#include "main/errors.h"
#include "main/mtypes.h"
+#include "main/shaderapi.h"
+#include "util/bitscan.h"
#include "util/crc32.h"
#include "program_binary.h"
#include "program/prog_parameter.h"
@@ -282,10 +284,41 @@ _mesa_program_binary(struct gl_context *ctx, struct
gl_shader_program *sh_prog,
struct blob_reader blob;
blob_reader_init(, payload, length - header_size);
+ unsigned programs_in_use = 0;
+ if (ctx->_Shader)
+ for (unsigned stage = 0; stage < MESA_SHADER_STAGES; stage++) {
+ if (ctx->_Shader->CurrentProgram[stage] &&
+ ctx->_Shader->CurrentProgram[stage]->Id == sh_prog->Name) {
+programs_in_use |= 1 << stage;
+ }
+ }
+
if (!read_program_payload(ctx, , binary_format, sh_prog)) {
sh_prog->data->LinkStatus = LINKING_FAILURE;
return;
}
+ /* From section 7.3 (Program Objects) of the OpenGL 4.5 spec:
+*
+*"If LinkProgram or ProgramBinary successfully re-links a program
+* object that is active for any shader stage, then the newly generated
+* executable code will be installed as part of the current rendering
+* state for all shader stages where the program is active.
+* Additionally, the newly generated executable code is made part of
+* the state of any program pipeline for all stages where the program
+* is attached."
+*/
+ if (programs_in_use) {
+ while (programs_in_use) {
+ const int stage = u_bit_scan(_in_use);
+
+ struct gl_program *prog = NULL;
+ if (sh_prog->_LinkedShaders[stage])
+prog = sh_prog->_LinkedShaders[stage]->Program;
+
+ _mesa_use_program(ctx, stage, sh_prog, prog, ctx->_Shader);
+ }
+ }
+
sh_prog->data->LinkStatus = LINKING_SKIPPED;
}
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965: Implement ARB_compute_variable_group_size.
On 2018-06-01 15:21:34, Plamena Manolova wrote:
> This patch adds the implentation of ARB_compute_variable_group_size
> for i965. We do this by storing the group size in a buffer surface,
> similarly to the work group number.
>
> Signed-off-by: Plamena Manolova
> ---
> docs/features.txt| 2 +-
> docs/relnotes/18.2.0.html| 1 +
> src/compiler/nir/nir_lower_system_values.c | 14
> src/intel/compiler/brw_compiler.h| 2 +
> src/intel/compiler/brw_fs.cpp| 45
> src/intel/compiler/brw_fs_nir.cpp| 20 ++
> src/intel/compiler/brw_nir_lower_cs_intrinsics.c | 87
> +---
> src/mesa/drivers/dri/i965/brw_compute.c | 25 ++-
> src/mesa/drivers/dri/i965/brw_context.h | 1 +
> src/mesa/drivers/dri/i965/brw_cs.c | 4 ++
> src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 27 +++-
> src/mesa/drivers/dri/i965/intel_extensions.c | 1 +
> 12 files changed, 187 insertions(+), 42 deletions(-)
>
> diff --git a/docs/features.txt b/docs/features.txt
> index ed4050cf98..7c3c856d73 100644
> --- a/docs/features.txt
> +++ b/docs/features.txt
> @@ -298,7 +298,7 @@ Khronos, ARB, and OES extensions that are not part of any
> OpenGL or OpenGL ES ve
>
>GL_ARB_bindless_texture DONE (nvc0, radeonsi)
>GL_ARB_cl_event not started
> - GL_ARB_compute_variable_group_sizeDONE (nvc0, radeonsi)
> + GL_ARB_compute_variable_group_sizeDONE (nvc0,
> radeonsi, i965)
>GL_ARB_ES3_2_compatibilityDONE (i965/gen8+)
>GL_ARB_fragment_shader_interlock DONE (i965)
>GL_ARB_gpu_shader_int64 DONE (i965/gen8+,
> nvc0, radeonsi, softpipe, llvmpipe)
> diff --git a/docs/relnotes/18.2.0.html b/docs/relnotes/18.2.0.html
> index a3f44a29dc..4ceeb7471f 100644
> --- a/docs/relnotes/18.2.0.html
> +++ b/docs/relnotes/18.2.0.html
> @@ -45,6 +45,7 @@ Note: some of the new features are only available with
> certain drivers.
>
>
> GL_ARB_fragment_shader_interlock on i965
> +GL_ARB_compute_variable_group_size on i965
>
>
> Bug fixes
> diff --git a/src/compiler/nir/nir_lower_system_values.c
> b/src/compiler/nir/nir_lower_system_values.c
> index 487da04262..0af6d69426 100644
> --- a/src/compiler/nir/nir_lower_system_values.c
> +++ b/src/compiler/nir/nir_lower_system_values.c
> @@ -57,6 +57,15 @@ convert_block(nir_block *block, nir_builder *b)
>*gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID"
>*/
>
> +
Extra line.
> + /*
> + * If the local work group size is variable we can't lower the
> global
> + * invocation id here.
> + */
> + if (b->shader->info.cs.local_size_variable) {
> + break;
> + }
> +
The indent looks off here. One extra space?
> nir_const_value local_size;
> memset(_size, 0, sizeof(local_size));
> local_size.u32[0] = b->shader->info.cs.local_size[0];
> @@ -102,6 +111,11 @@ convert_block(nir_block *block, nir_builder *b)
>}
>
>case SYSTEM_VALUE_LOCAL_GROUP_SIZE: {
> + /* If the local work group size is variable we can't lower it here
> */
> + if (b->shader->info.cs.local_size_variable) {
> +break;
> + }
> +
> nir_const_value local_size;
> memset(_size, 0, sizeof(local_size));
> local_size.u32[0] = b->shader->info.cs.local_size[0];
> diff --git a/src/intel/compiler/brw_compiler.h
> b/src/intel/compiler/brw_compiler.h
> index 8b4e6fe2e2..f54952c28f 100644
> --- a/src/intel/compiler/brw_compiler.h
> +++ b/src/intel/compiler/brw_compiler.h
> @@ -759,6 +759,7 @@ struct brw_cs_prog_data {
> unsigned threads;
> bool uses_barrier;
> bool uses_num_work_groups;
> + bool uses_variable_group_size;
>
> struct {
>struct brw_push_const_block cross_thread;
> @@ -771,6 +772,7 @@ struct brw_cs_prog_data {
> * surface indices the CS-specific surfaces
> */
>uint32_t work_groups_start;
> + uint32_t work_group_size_start;
>/** @} */
> } binding_table;
> };
> diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp
> index d67c0a4192..28730af47b 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -7228,18 +7228,32 @@ brw_compile_cs(const struct brw_compiler *compiler,
> void *log_data,
> int shader_time_index,
> char **error_str)
> {
> - prog_data->local_size[0] = src_shader->info.cs.local_size[0];
> - prog_data->local_size[1] = src_shader->info.cs.local_size[1];
> - prog_data->local_size[2] = src_shader->info.cs.local_size[2];
> - unsigned local_workgroup_size =
> -
Re: [Mesa-dev] [PATCH 1/2] nir: Add global invocation id intrinsic.
Reviewed-by: Jordan Justen
On 2018-06-01 15:21:33, Plamena Manolova wrote:
> Add the missing nir intrinsic for the gl_GlobalInvocationID
> compute shader variable.
>
> Signed-off-by: Plamena Manolova
> ---
> src/compiler/nir/nir.c | 4
> src/compiler/nir/nir_intrinsics.py | 1 +
> 2 files changed, 5 insertions(+)
>
> diff --git a/src/compiler/nir/nir.c b/src/compiler/nir/nir.c
> index dc1c560319..36a79f57ee 100644
> --- a/src/compiler/nir/nir.c
> +++ b/src/compiler/nir/nir.c
> @@ -2051,6 +2051,8 @@ nir_intrinsic_from_system_value(gl_system_value val)
>return nir_intrinsic_load_subgroup_id;
> case SYSTEM_VALUE_LOCAL_GROUP_SIZE:
>return nir_intrinsic_load_local_group_size;
> + case SYSTEM_VALUE_GLOBAL_INVOCATION_ID:
> + return nir_intrinsic_load_global_invocation_id;
> default:
>unreachable("system value does not directly correspond to intrinsic");
> }
> @@ -2130,6 +2132,8 @@ nir_system_value_from_intrinsic(nir_intrinsic_op intrin)
>return SYSTEM_VALUE_SUBGROUP_ID;
> case nir_intrinsic_load_local_group_size:
>return SYSTEM_VALUE_LOCAL_GROUP_SIZE;
> + case nir_intrinsic_load_global_invocation_id:
> + return SYSTEM_VALUE_GLOBAL_INVOCATION_ID;
> default:
>unreachable("intrinsic doesn't produce a system value");
> }
> diff --git a/src/compiler/nir/nir_intrinsics.py
> b/src/compiler/nir/nir_intrinsics.py
> index ac8a67f44b..484b2d4fd6 100644
> --- a/src/compiler/nir/nir_intrinsics.py
> +++ b/src/compiler/nir/nir_intrinsics.py
> @@ -449,6 +449,7 @@ system_value("subgroup_lt_mask", 0)
> system_value("num_subgroups", 1)
> system_value("subgroup_id", 1)
> system_value("local_group_size", 3)
> +system_value("global_invocation_id", 3)
>
> # Blend constant color values. Float values are clamped.#
> system_value("blend_const_color_r_float", 1)
> --
> 2.11.0
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/16] Move the Mesa Website to Sphinx
On 2018-05-31 16:06:13, Laura Ekstrand wrote: > A little bit of messing around this afternoon, led to this: > https://drive.google.com/file/d/1rteTv9415XEMN1E11fyN0l3hTUCCbgwz/view?usp=sharing Nice. That confirms we can easily get back to a something closer to the older look in short order. (Unless the consensus is to actually stay with the generic look.) > The gears are from Font Awesome, which comes with the RTD format. It'd be nice to get the icon back, but the gear glyph is an improvement compared to the house glyph. Anyway, I stick by what I said below too in reply to Eric about withdrawing my feedback. Thanks, -Jordan > On Thu, May 31, 2018 at 1:55 PM, Jordan Justen > wrote: > > > On 2018-05-31 12:27:04, Eric Anholt wrote: > > > Jordan Justen writes: > > > > > > > On 2018-05-24 17:37:09, Laura Ekstrand wrote: > > > >> A few of the commits are quite large and awaiting list approval. I > > suggest > > > >> that you take a look at the code here: > > > >> https://gitlab.freedesktop.org/ldeks/mesa/tree/website1_75, > > > >> and the new website here: https://mesa-test.freedesktop. > > org/index.html > > > > > > > > I think the theme should be changed to look somewhat close to the > > > > current mesa3d.org website before changing the main site. (Color > > > > scheme and missing icons.) > > > > > > > > Right now the test website looks like a million other sphinx websites, > > > > including every readthedocs book. > > > > > > > > Based on http://www.sphinx-doc.org/en/master/ and > > > > https://www.python.org/, I would say sphinx gives a lot of > > > > opportunities for a custom look. > > > > > > I think looking like a generic other sphinx website is a step forward, > > > and I look forward to this series landing. No need for continuity with > > > the old theme. > > > > I think it is 1 step forward, 1 step back. As no else appears to > > agree, I'll withdraw my feedback. > > > > -Jordan > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/16] Move the Mesa Website to Sphinx
On 2018-05-31 12:27:04, Eric Anholt wrote: > Jordan Justen writes: > > > On 2018-05-24 17:37:09, Laura Ekstrand wrote: > >> A few of the commits are quite large and awaiting list approval. I suggest > >> that you take a look at the code here: > >> https://gitlab.freedesktop.org/ldeks/mesa/tree/website1_75, > >> and the new website here: https://mesa-test.freedesktop.org/index.html > > > > I think the theme should be changed to look somewhat close to the > > current mesa3d.org website before changing the main site. (Color > > scheme and missing icons.) > > > > Right now the test website looks like a million other sphinx websites, > > including every readthedocs book. > > > > Based on http://www.sphinx-doc.org/en/master/ and > > https://www.python.org/, I would say sphinx gives a lot of > > opportunities for a custom look. > > I think looking like a generic other sphinx website is a step forward, > and I look forward to this series landing. No need for continuity with > the old theme. I think it is 1 step forward, 1 step back. As no else appears to agree, I'll withdraw my feedback. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/16] Move the Mesa Website to Sphinx
On 2018-05-30 17:35:11, wrote: > On Wed, May 30, 2018 at 7:53 PM, Jordan Justen > wrote: > > On 2018-05-30 14:14:18, Laura Ekstrand wrote: > >> The current plan is that this first patch series should just update the > >> website. Then we can swap out the theme easily with one or two commands in > >> the next series. > > > > If it's easy to do, then why not just do it? I don't think the > > mesa3d.org website should go through a step of looking like a generic > > readthedocs book. > > > > I understand we might want to tweak the look more later on, but I'm > > guessing with a couple more simple patches it could look somewhat like > > the current site. > > tbh, I'm not sure generic rtd look is a step backwards.. For the most part, it is better, but the blandness is a step backwards. Say what you will about the current site (and you did below), at least it is recognizable. > If there were some proposals about future looks, and those could be > pushed somewhere so others can review the result, by all means go for > it. My minimum proposal would be: 1. upper left background black with white text 2. retain the gears icon somewhere near the top 3. left background gray with black text This would provide a least a minimum of continuity to the current site. A few other ideas, but more reasonable to be left for the future: 1. remove 'latest' in the upper left 2. change 'search docs' be 'search site' 3. might be nice to retain the black bar acress the top (maybe, maybe not.) > But it doesn't seem to me like something that should hold back > the process. If these are "easy to change" elements of the new site, then I don't think the comment of "holding things back" is fair. If these are not easy to change, then it should be clear that what it currently looks like may be what we are stuck with. -Jordan > I mean when was "it looks less like the late 90's" a bad > thing? > > BR, > -R > > > > > -Jordan > > > >> The goal of this series is to move the website to the new > >> infrastructure. Then we can edit content and style. > >> > >> On Tue, May 29, 2018 at 11:05 PM, Jordan Justen > >> wrote: > >> > >> > On 2018-05-24 17:37:09, Laura Ekstrand wrote: > >> > > A few of the commits are quite large and awaiting list approval. I > >> > suggest > >> > > that you take a look at the code here: > >> > > https://gitlab.freedesktop.org/ldeks/mesa/tree/website1_75, > >> > > and the new website here: https://mesa-test.freedesktop.org/index.html > >> > > >> > I think the theme should be changed to look somewhat close to the > >> > current mesa3d.org website before changing the main site. (Color > >> > scheme and missing icons.) > >> > > >> > Right now the test website looks like a million other sphinx websites, > >> > including every readthedocs book. > >> > > >> > Based on http://www.sphinx-doc.org/en/master/ and > >> > https://www.python.org/, I would say sphinx gives a lot of > >> > opportunities for a custom look. > >> > > >> > -Jordan > >> > > >> > > On Thu, May 24, 2018 at 5:27 PM, Laura Ekstrand > >> > > wrote: > >> > > > >> > > > It's time to move the Mesa-3d.org website into the 21st century. We > >> > have > >> > > > chosen to move to Sphinx and resStructured text for a number of > >> > reasons: > >> > > > 1. Syntax highlighting for code snippets. > >> > > > 2. Snazzy highlighting for variables and function names. > >> > > > 3. Consistency with the Gallium drivers, which host their > >> > documentation > >> > > > on > >> > > > ReadTheDocs. > >> > > > 4. Ultimately less work for us, because writing content in > >> > reStructured > >> > > > Text > >> > > > is simpler and Sphinx turns it into beautiful, readable pages > >> > without > >> > > > us > >> > > > having to manually fix problems in html and css. > >> > > > 5. With Gitlab, the Sphinx website is auto-generated every time the > >> > Mesa > >> > > > code repository is pushed to Gitlab without us having to > >> > > > build and deploy it. > >> > > > > >> > > > The new website is currently hosted at mesa-test.freedesktop.or
Re: [Mesa-dev] [PATCH 00/16] Move the Mesa Website to Sphinx
On 2018-05-30 14:14:18, Laura Ekstrand wrote: > The current plan is that this first patch series should just update the > website. Then we can swap out the theme easily with one or two commands in > the next series. If it's easy to do, then why not just do it? I don't think the mesa3d.org website should go through a step of looking like a generic readthedocs book. I understand we might want to tweak the look more later on, but I'm guessing with a couple more simple patches it could look somewhat like the current site. -Jordan > The goal of this series is to move the website to the new > infrastructure. Then we can edit content and style. > > On Tue, May 29, 2018 at 11:05 PM, Jordan Justen > wrote: > > > On 2018-05-24 17:37:09, Laura Ekstrand wrote: > > > A few of the commits are quite large and awaiting list approval. I > > suggest > > > that you take a look at the code here: > > > https://gitlab.freedesktop.org/ldeks/mesa/tree/website1_75, > > > and the new website here: https://mesa-test.freedesktop.org/index.html > > > > I think the theme should be changed to look somewhat close to the > > current mesa3d.org website before changing the main site. (Color > > scheme and missing icons.) > > > > Right now the test website looks like a million other sphinx websites, > > including every readthedocs book. > > > > Based on http://www.sphinx-doc.org/en/master/ and > > https://www.python.org/, I would say sphinx gives a lot of > > opportunities for a custom look. > > > > -Jordan > > > > > On Thu, May 24, 2018 at 5:27 PM, Laura Ekstrand > > > wrote: > > > > > > > It's time to move the Mesa-3d.org website into the 21st century. We > > have > > > > chosen to move to Sphinx and resStructured text for a number of > > reasons: > > > > 1. Syntax highlighting for code snippets. > > > > 2. Snazzy highlighting for variables and function names. > > > > 3. Consistency with the Gallium drivers, which host their > > documentation > > > > on > > > > ReadTheDocs. > > > > 4. Ultimately less work for us, because writing content in > > reStructured > > > > Text > > > > is simpler and Sphinx turns it into beautiful, readable pages > > without > > > > us > > > > having to manually fix problems in html and css. > > > > 5. With Gitlab, the Sphinx website is auto-generated every time the > > Mesa > > > > code repository is pushed to Gitlab without us having to > > > > build and deploy it. > > > > > > > > The new website is currently hosted at mesa-test.freedesktop.org. > > When > > > > these > > > > patches are merged, Daniel Stone will point our Gitlab CI system at > > > > mesa-3d.org. > > > > > > > > When reviewing these patches, please note: > > > > 1. This patch series does *not* touch content. Please do not > > > > bikeshed > > > >the content of webpages here. That will be addressed in > > later > > > >commits. > > > > 2. Please do *not* bikeshed website style here. We are using > > the > > > >classic ReadTheDocs style for now and we will update style > > in a > > > >future commit. > > > > 3. I've done my best to make your current content look > > beautiful. > > > > If > > > >there's a problem, please let me know. > > > > 4. There were some commits to the website between when I > > started > > > > this > > > >series and now. I've done my best to incorporate your > > changes. > > > >So if you changed your content in the past two weeks, take a > > > > look > > > >at your page. > > > > > > > > I hope you'll agree that the resulting pages are much cleaner and > > easier to > > > > read. I've learned a lot by reading these pages. > > > > > > > > Thanks. > > > > > > > > Laura Ekstrand > > > > --- > > > > > > > > Jean Hertel (3): > > > > docs: Add Sphinx configuration file. > > > > docs: Add a navigation sidebar. > > > > docs: Add toctree to relnotes > > > > > > > > Laura Ekstrand (13): > > > > Added ci yaml file for Gitlab. > >
Re: [Mesa-dev] [PATCH 00/16] Move the Mesa Website to Sphinx
On 2018-05-24 17:37:09, Laura Ekstrand wrote: > A few of the commits are quite large and awaiting list approval. I suggest > that you take a look at the code here: > https://gitlab.freedesktop.org/ldeks/mesa/tree/website1_75, > and the new website here: https://mesa-test.freedesktop.org/index.html I think the theme should be changed to look somewhat close to the current mesa3d.org website before changing the main site. (Color scheme and missing icons.) Right now the test website looks like a million other sphinx websites, including every readthedocs book. Based on http://www.sphinx-doc.org/en/master/ and https://www.python.org/, I would say sphinx gives a lot of opportunities for a custom look. -Jordan > On Thu, May 24, 2018 at 5:27 PM, Laura Ekstrand > wrote: > > > It's time to move the Mesa-3d.org website into the 21st century. We have > > chosen to move to Sphinx and resStructured text for a number of reasons: > > 1. Syntax highlighting for code snippets. > > 2. Snazzy highlighting for variables and function names. > > 3. Consistency with the Gallium drivers, which host their documentation > > on > > ReadTheDocs. > > 4. Ultimately less work for us, because writing content in reStructured > > Text > > is simpler and Sphinx turns it into beautiful, readable pages without > > us > > having to manually fix problems in html and css. > > 5. With Gitlab, the Sphinx website is auto-generated every time the Mesa > > code repository is pushed to Gitlab without us having to > > build and deploy it. > > > > The new website is currently hosted at mesa-test.freedesktop.org. When > > these > > patches are merged, Daniel Stone will point our Gitlab CI system at > > mesa-3d.org. > > > > When reviewing these patches, please note: > > 1. This patch series does *not* touch content. Please do not > > bikeshed > >the content of webpages here. That will be addressed in later > >commits. > > 2. Please do *not* bikeshed website style here. We are using the > >classic ReadTheDocs style for now and we will update style in a > >future commit. > > 3. I've done my best to make your current content look beautiful. > > If > >there's a problem, please let me know. > > 4. There were some commits to the website between when I started > > this > >series and now. I've done my best to incorporate your changes. > >So if you changed your content in the past two weeks, take a > > look > >at your page. > > > > I hope you'll agree that the resulting pages are much cleaner and easier to > > read. I've learned a lot by reading these pages. > > > > Thanks. > > > > Laura Ekstrand > > --- > > > > Jean Hertel (3): > > docs: Add Sphinx configuration file. > > docs: Add a navigation sidebar. > > docs: Add toctree to relnotes > > > > Laura Ekstrand (13): > > Added ci yaml file for Gitlab. > > docs: Add python script that converts html to rst. > > docs: Remove contents.html > > docs: Result of script fixing html with Beautiful Soup. > > docs: Add results of script - pandoc-generated rst pages. > > docs: Add command for Sphinx build. > > docs: Toctree for systems.rst as in jhertel docs. > > docs: Remove html files. > > docs: Fix Sphinx compile errors. > > docs: Edits to fix toctrees. > > docs: Major manual edits to environment vars. > > docs: Human edits to the website code for clarity. > > docs: Remove unneeded mesa css file. > > > > .gitlab-ci.yml | 11 + > > docs/application-issues.html | 83 -- > > docs/application-issues.rst | 48 + > > docs/autoconf.html | 257 > > docs/autoconf.rst| 209 +++ > > docs/bugs.html | 64 - > > docs/bugs.rst| 33 + > > docs/codingstyle.html| 142 -- > > docs/codingstyle.rst | 124 ++ > > docs/conf.py | 162 +++ > > docs/conform.html| 703 -- > > docs/conform.rst | 675 ++ > > docs/contents.html | 108 -- > > docs/debugging.html | 47 - > > docs/debugging.rst | 21 + > > docs/developers.html | 58 - > > docs/developers.rst | 25 + > > docs/devinfo.html| 83 -- > > docs/devinfo.rst | 43 + > > docs/dispatch.html | 278 > > docs/dispatch.rst| 257 > > docs/download.html | 112 -- > > docs/download.rst| 73 ++ > > docs/egl.html| 259 > > docs/egl.rst | 117 ++ > > docs/envvars.html| 383 -- > > docs/envvars.rst | 615 + > > docs/extensions.html | 51 - > > docs/extensions.rst | 31 + > > docs/faq.html| 392 -- > > docs/faq.rst | 308 + > > docs/helpwanted.html |
Re: [Mesa-dev] [PATCH v2 4/4] i965: Require softpin support for Cannonlake and later.
3-4 Reviewed-by: Jordan Justen
On 2018-05-27 21:35:43, Kenneth Graunke wrote:
> This isn't strictly necessary, but anyone running Cannonlake will
> already have Kernel 4.5 or later, so there's no reason to support
> the relocation model on Gen10+.
>
> This will let us avoid dealing with them for new features.
>
> Reviewed-by: Scott D Phillips
> ---
> src/mesa/drivers/dri/i965/brw_bufmgr.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> index 80bfc7c75db..2fb0a4453ff 100644
> --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> @@ -1738,6 +1738,16 @@ brw_bufmgr_init(struct gen_device_info *devinfo, int
> fd)
> 4096, _4GB);
> util_vma_heap_init(>vma_allocator[BRW_MEMZONE_OTHER],
> 1 * _4GB, gtt_size - 1 * _4GB);
> + } else if (devinfo->gen >= 10) {
> + /* Softpin landed in 4.5, but GVT used an aliasing PPGTT until
> + * kernel commit 6b3816d69628becb7ff35978aa0751798b4a940a in
> + * 4.14. Gen10+ GVT hasn't landed yet, so it's not actually a
> + * problem - but extending this requirement back to earlier gens
> + * might actually mean requiring 4.14.
> + */
> + fprintf(stderr, "i965 requires softpin (Kernel 4.5) on Gen10+.");
> + free(bufmgr);
> + return NULL;
>}
> }
>
> --
> 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] [PATCH v2 1/4] i965: Add virtual memory allocator infrastructure to brw_bufmgr.
Reviewed-by: Jordan Justen
On 2018-05-27 21:35:40, Kenneth Graunke wrote:
> This introduces a new fast virtual memory allocator integrated with our
> BO cache bucketing. For larger objects, it falls back to the simple
> free-list allocator (util_vma).
>
> This puts the allocators in place but doesn't enable softpin yet.
>
> v2:
> (feedback from Chris Wilson)
> - Check (bo->kflags & EXEC_OBJECT_PINNED) instead of a global flag
> - Avoid vma_free(0ull) on the err_free path.
> - Only enable if the kernel says we have full PPGTT support
> - Make bucketing allocators more resistant to failing to grow arrays
> (feedback from Scott Phillips)
> - Don't use node after popping it from the list.
> - Avoid undefined behavior in canonicalization by reusing new helper
> - Comment updates
> (feedback from myself)
> - Avoid __vma_alloc vs. vma_alloc by making a zero_high_bits helper
>to return a non-canonical address with the high bits zeroed.
> ---
> src/mesa/drivers/dri/i965/brw_bufmgr.c | 291 -
> src/mesa/drivers/dri/i965/brw_bufmgr.h | 2 +
> 2 files changed, 292 insertions(+), 1 deletion(-)
>
> I plan to land this series this week, barring any major objections.
> Scott reviewed it all (from my v1.9 branch), but I wasn't positive
> whether some of his reviews still applied...so I figured I'd play it
> safe and send it out one more time.
>
> diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> index 66828f319be..13d32255715 100644
> --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> @@ -54,12 +54,15 @@
> #endif
> #include "common/gen_clflush.h"
> #include "common/gen_debug.h"
> +#include "common/gen_gem.h"
> #include "dev/gen_device_info.h"
> #include "libdrm_macros.h"
> #include "main/macros.h"
> #include "util/macros.h"
> #include "util/hash_table.h"
> #include "util/list.h"
> +#include "util/u_dynarray.h"
> +#include "util/vma.h"
> #include "brw_bufmgr.h"
> #include "brw_context.h"
> #include "string.h"
> @@ -98,9 +101,41 @@ atomic_add_unless(int *v, int add, int unless)
> return c == unless;
> }
>
> +/**
> + * i965 fixed-size bucketing VMA allocator.
> + *
> + * The BO cache maintains "cache buckets" for buffers of various sizes.
> + * All buffers in a given bucket are identically sized - when allocating,
> + * we always round up to the bucket size. This means that virtually all
> + * allocations are fixed-size; only buffers which are too large to fit in
> + * a bucket can be variably-sized.
> + *
> + * We create an allocator for each bucket. Each contains a free-list, where
> + * each node contains a pair. Each bit
> + * represents a bucket-sized block of memory. (At the first level, each
> + * bit corresponds to a page. For the second bucket, bits correspond to
> + * two pages, and so on.) 1 means a block is free, and 0 means it's in-use.
> + * The lowest bit in the bitmap is for the first block.
> + *
> + * This makes allocations cheap - any bit of any node will do. We can pick
> + * the head of the list and use ffs() to find a free block. If there are
> + * none, we allocate 64 blocks from a larger allocator - either a bigger
> + * bucketing allocator, or a fallback top-level allocator for large objects.
> + */
> +struct vma_bucket_node {
> + uint64_t start_address;
> + uint64_t bitmap;
> +};
> +
> struct bo_cache_bucket {
> + /** List of cached BOs. */
> struct list_head head;
> +
> + /** Size of this bucket, in bytes. */
> uint64_t size;
> +
> + /** List of vma_bucket_nodes. */
> + struct util_dynarray vma_list[BRW_MEMZONE_COUNT];
> };
>
> struct brw_bufmgr {
> @@ -116,6 +151,8 @@ struct brw_bufmgr {
> struct hash_table *name_table;
> struct hash_table *handle_table;
>
> + struct util_vma_heap vma_allocator[BRW_MEMZONE_COUNT];
> +
> bool has_llc:1;
> bool has_mmap_wc:1;
> bool bo_reuse:1;
> @@ -128,6 +165,17 @@ static int bo_set_tiling_internal(struct brw_bo *bo,
> uint32_t tiling_mode,
>
> static void bo_free(struct brw_bo *bo);
>
> +static uint64_t
> +zero_high_bits(uint64_t addr)
> +{
> + /* Un-canonicalize the address, zeroing out the high bits. */
> + return addr & ((1ull << 48) - 1);
> +}
> +
> +static uint64_t vma_alloc(struct brw_bufmgr *bufmgr,
> + enum brw_memory_zone memzone,
> + uint64_t size, uint64_t alignment);
>
Re: [Mesa-dev] Gitlab migration
On 2018-05-23 15:16:58, wrote: > On Wed, May 23, 2018 at 5:48 PM, Jordan Justen > <jordan.l.jus...@intel.com> wrote: > > Why can this only be done with gitlab? > > I'm not directly involved w/ decision to move things to gitlab, but that said, > > https://gitlab.freedesktop.org/freedesktop/freedesktop/wikis/home > > does provide some rational under the "Why migrate to GitLab?" section. > There might be a better writeup from the fd.o admins somewhere, but > tl;dr version is to ease burden on admins and to eventually reduce the > physical fd.o infrastructure to just the mail server with everything > else cloud hosted. > > For mesa, I'd prefer to stick to the email based workflow (which is > still an option). And not sure about wiki, other than that it can't > be worse than the current random and out of date gpu/mesa related wiki > pages that already exist. I'm skeptical that gitlab wikis will solve content issues. :) > And I'm not strongly tied to bugzilla vs gitlab's issue tracking. Another project I'm involved with had a contingency that swore github's "issues" were completely inadequate compared to bugzilla, which is to say that I don't think there is consensus on this point. :) For that (smaller) project, I thought github's issues were fine compared to bugzilla. For (the larger) Mesa project, I'm not so sure. > Afaui cgit will remain, so if you don't like the github/gitlab UI, > you are mostly free to ignore it. That is not quite the feeling I was getting from daniels on irc. Seems like they'd be looking to drop cgit as soon as they can. It was a vibe of yeah, sure, we can keep running that if needed, but we don't want to. > And if changing the push URL for mesa is the only real downside for > making life easier for the admins, then I don't really see the > problem. Agreed. -Jordan ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Gitlab migration
On 2018-05-23 16:00:53, Jason Ekstrand wrote: > On Wed, May 23, 2018 at 2:48 PM, Jordan Justen <jordan.l.jus...@intel.com> > wrote: > > > On 2018-05-23 12:34:11, Jason Ekstrand wrote: > > > Mesa developers, > > > > > > tl;dr. Please go to gitlab.freedesktop.org, create your account, and > > > upload your SSH keys. Instructions are the bottom of this e-mail. > > > > > > The freedesktop.org admins are trying to move as many projects and > > services > > > as possible over to gitlab and somehow I got hoodwinked into > > spear-heading > > > it for mesa. > > > > I thought we'd be able to decide if or when. It sounds like this is > > decided. > > > > You are free to state your objections. No one has actually pulled the > trigger yet. :-) > Rather than "it's time to do this", how about "should we do this"? FWIW, if the fdo admins think this hosting is viable long term and at least as stable as the current servers, then I don't see a concern with changing *only* the repo location. > > > > There are a number of reasons for this change. Some of those > > > reasons have to do with the maintenance cost of our sprawling and aging > > > infrastructure. Some of those reasons provide significant benefit to the > > > project being migrated: > > > > > > * Project-led user/rights management. If we're on gitlab, project > > > maintainers can give people access and we no longer have to wait for the > > > freedesktop admins to add a new person. And the freedesktop admins don't > > > have to take the time. > > > > > > * Better web UI for git. > > > > s/Better/Worse/ > > > > > Ok, so some people will argue with me on this > > > one but it's at least how I feel. :-) > > > > /me bows > > > > > * [Optional] Integrated commit history and issue tracking. Bugzilla > > tags > > > are great but gitlab ties things together much better. > > > > > > * [Optional] Merge-request workflow. With the rise of github, there are > > > many developers out there who are used to the merge-request workflow and > > > switching to that may lower the barrier to entry for new contributors. > > > > On the down side, the contribution/review history is now tied up in > > the gitlab infrastructure rather than a simple email archive. > > > > Yup, and that is a down side. That's why I made it clear that this is > optional and not going to be a thing we do right away. Ok, why don't you save it for a future refactor? :) We should say: "Gitlab has many other features, but they won't be used at this time. Perhaps in the future we can consider using some of them after further discussion." As opposed to a sales pitch for all of the things we won't be using. :) -Jordan > > > > * [Optional] Built-in wiki support > > > > The github (and gitlab) integrated wiki's look fairly busy/crappy with > > all the gibhub/gitlab UI around the contents. > > > > Yeah, I don't expect we'll actually use it for real things but it's there > if we want it. Might be useful for piglit to keep some notes or something. > > > > > * [Optional] Built-in CI. With gitlab, we can provide a docker image > > and > > > CI tasks to run in it which can do things such as build the website, run > > > build-tests, etc. I'm not sure if build-testing Android is feasible but > > we > > > could at least build-test autotools, meson, scons, and maybe even run > > some > > > LLVMpipe tests. > > > > Why can this only be done with gitlab? > > > > It *can* be done other ways but the fd.o admins are tired of everyone > wanting some hand-rolled solution for this that or the other thing. With > the gitlab CI, we provide a docker image and a script and it "just works". > There is literally zero overhead for the fd.o admins to maintain the thing. > > > > > Before anyone freaks out about the possible changes that may be > > incoming, I > > > would like to make it crystal clear that many of the above things are > > > optional. > > > > Yeah, this doesn't feel like a steamroller at all. > > > > I'm really not trying to steamroller. :-( Like I said below, we don't need > to use any of those optional features and we'll turn them off initially so > that it's not even possible to make a PR or file an issue and the wiki > won't even exist. Once you add your key and change your push URL, you > won't even notice that it's hosted on gitlab. > >
