Re: [Mesa-dev] [PATCH] gallium/swr: update rasterizer (532172)

[Rowley, Timothy O]

> On Mar 23, 2016, at 2:19 PM, Tom Stellard  wrote:
> 
> On Wed, Mar 23, 2016 at 04:53:29PM +, Rowley, Timothy O wrote:
>> 
>> While that situation would be nice, the swr rasterizer is a subset of an 
>> internal project, and what is upstreamed publicly is not just a straight 
>> copy of our repository.  Moving to having the rasterizer’s “home” to Mesa 
>> involves some large technical and workflow challenges.
> 
> How much testing do you do on the version of swr that's in Mesa?

The internal version undergoes extensive continuous integration testing.  The 
version in Mesa isn’t currently subjected to quite the same testing; we check 
for regressions on the VTK test suite regularly, do targeted testing on our 
major target applications, and currently infrequent piglit runs.

-Tim


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

Re: [Mesa-dev] [PATCH] gallium/swr: update rasterizer (532172)

[Tom Stellard]

On Wed, Mar 23, 2016 at 04:53:29PM +, Rowley, Timothy O wrote:
> 
> > On Mar 23, 2016, at 12:52 AM, Kenneth Graunke  wrote:
> > 
> > That's an awkward situation we've not run into before.
> > 
> > If the code is going to live in the upstream Mesa git repository, then
> > it seems like the best long term plan is to reverse the workflow: make
> > upstream Mesa the canonical repository, do development upstream, and
> > pull changes from upstream into any internal repositories.
> > 
> > Obviously, that's a huge process change - presumably you have a bunch
> > of people working in some Intel perforce system - but working in the
> > public is very beneficial.  It's also the mark of a true open source
> > project, rather than simply "available source”.
> 
> While that situation would be nice, the swr rasterizer is a subset of an 
> internal project, and what is upstreamed publicly is not just a straight copy 
> of our repository.  Moving to having the rasterizer’s “home” to Mesa involves 
> some large technical and workflow challenges.
> 

How much testing do you do on the version of swr that's in Mesa?

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

Re: [Mesa-dev] [PATCH] gallium/swr: update rasterizer (532172)

[Rowley, Timothy O]

> On Mar 23, 2016, at 12:52 AM, Kenneth Graunke  wrote:
> 
> That's an awkward situation we've not run into before.
> 
> If the code is going to live in the upstream Mesa git repository, then
> it seems like the best long term plan is to reverse the workflow: make
> upstream Mesa the canonical repository, do development upstream, and
> pull changes from upstream into any internal repositories.
> 
> Obviously, that's a huge process change - presumably you have a bunch
> of people working in some Intel perforce system - but working in the
> public is very beneficial.  It's also the mark of a true open source
> project, rather than simply "available source”.

While that situation would be nice, the swr rasterizer is a subset of an 
internal project, and what is upstreamed publicly is not just a straight copy 
of our repository.  Moving to having the rasterizer’s “home” to Mesa involves 
some large technical and workflow challenges.

-Tim

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

Re: [Mesa-dev] [PATCH] gallium/swr: update rasterizer (532172)

[Rowley, Timothy O]

> On Mar 23, 2016, at 12:52 AM, Justen, Jordan L  
> wrote:
> 
> On 2016-03-22 20:55:10, Rowley, Timothy O wrote:
>> 
>> Yes, there’s a lot in this patch. I froze the public version of the
>> rasterizer when I began the upstreaming process mid February, so
>> this is syncing up with about a month’s worth of development.
>> 
>> I also have this change as a series of 81 commits. Not sure if that
>> would be preferable by the community or if people would be
>> interested in reviewing the series, as issues with early commits
>> might already be addressed later in the patch set.
> 
> There seems to be some things working against community code review.
> 
> * Expected broken commits earlier in the series (We would normally ask
>  that commits are cleaned up before posting them.)

There aren’t known broken commits in the series (besides the rare source 
control “whoops” which can happen with pretty much any development workflow, 
and which I can squash in the commit series pushed upstream), it’s just that 
with a backlog of changes improvements which might have been suggested for an 
early commit might already be addressed later in the series.

> * External development (What would happen to any code review asking
>  for reworks, given that the patches are already merged elsewhere?)

We’ve only had to deal with this in a limited fashion to this point (back when 
we were developing on github).  Depending on the impact of the suggested 
changes, I’d either do it myself or work with an internal developer to 
implement them, and amend the commit to include the changes.  Keeping the two 
repositories in sync is non-trivial, and I expect there might be times where a 
review for the rasterizer might have to addressed as “noted, will try to 
address in the future” if changes become stacked to where the commit history 
can’t be reordered/squashed before pushing.

> * A large backlog of changes. :)

I hope to do updates on a more regular basis; the upstreaming and follow-up on 
issues arising caused the long period this time and the resulting backlog.

> I still think it would be better to see the 81 commits split up in the
> history as long as they won't cause problems for others. Since most
> people are unlikely to be building openswr, I don't think the commits
> will affect them.

Once we sort out some more of the build issues we’d like to propose adding 
openswr to the default driver build list, but as you say for now people aren’t 
building this code unless they intentionally turned it on.

> We rarely use merges, but perhaps it is appropriate since openswr is
> developed externally. You could start a branch at the last openswr
> commit, add your 81 commits. Then you could merge the resulting branch
> into master.

I’ve been respecting the “no merges” mesa git workflow.  I have a script which 
can convert from our internal repository to git commits relative to master head.

- Tim

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

Re: [Mesa-dev] [PATCH] gallium/swr: update rasterizer (532172)

[Stéphane Marchesin]

On Tue, Mar 22, 2016 at 8:55 PM, Rowley, Timothy O
 wrote:
>
>> On Mar 22, 2016, at 3:51 PM, Justen, Jordan L  
>> wrote:
>>
>> What does 532172 in the subject refer to?
>
> swr rasterizer development happens in another source control system.  532172 
> is a revision id to checkpoint where we’ve pushed the changes publicly.
>
>> From this commit message, it seems clear that this single patch is
>> doing a whole lot. Usually that's a good sign that it should be split
>> into multiple patches.
>>
>> However, since this is only changing your driver, you can probably
>> take any sort of patches that you like. :)
>>
>> There is arguably little value to sending out a patch like this, since
>> it is very difficult to review. In other words, perhaps if you are
>> going to make big, unreviewable patches like this that only change
>> your driver, then you might as well just push them straight away.
>>
>> (But, it would be better, in my opinion, to try to split up the
>> changes and let them be reviewed.)
>
> Yes, there’s a lot in this patch.  I froze the public version of the 
> rasterizer when I began the upstreaming process mid February, so this is 
> syncing up with about a month’s worth of development.
>
> I also have this change as a series of 81 commits.  Not sure if that would be 
> preferable by the community or if people would be interested in reviewing the 
> series, as issues with early commits might already be addressed later in the 
> patch set.

From a consumer perspective, I am less interested in swr if I can't
bisect it to find and fix issues locally. Landing such large patches
would definitely prevent bisectability. Even if you have another
upstream repo (after all, that's what git is about), what prevents you
from turning all these commits into mesa commits, possibly with a
script if that's too tedious?

Stéphane


>
> -Tim
>
>
> ___
> 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] gallium/swr: update rasterizer (532172)

[Kenneth Graunke]

On Wednesday, March 23, 2016 3:55:10 AM PDT Rowley, Timothy O wrote:
> 
> > On Mar 22, 2016, at 3:51 PM, Justen, Jordan L  
wrote:
> > 
> > What does 532172 in the subject refer to?
> 
> swr rasterizer development happens in another source control system.  532172 
is a revision id to checkpoint where we’ve pushed the changes publicly.

That's an awkward situation we've not run into before.

If the code is going to live in the upstream Mesa git repository, then
it seems like the best long term plan is to reverse the workflow: make
upstream Mesa the canonical repository, do development upstream, and
pull changes from upstream into any internal repositories.

Obviously, that's a huge process change - presumably you have a bunch
of people working in some Intel perforce system - but working in the
public is very beneficial.  It's also the mark of a true open source
project, rather than simply "available source".

I don't know how much control you have over this, though...?


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/swr: update rasterizer (532172)

[Jordan Justen]

On 2016-03-22 20:55:10, Rowley, Timothy O wrote:
> 
> > On Mar 22, 2016, at 3:51 PM, Justen, Jordan L  
> > wrote:
> > 
> > What does 532172 in the subject refer to?
> 
> swr rasterizer development happens in another source control system.
> 532172 is a revision id to checkpoint where we’ve pushed the changes
> publicly.
> 
> > From this commit message, it seems clear that this single patch is
> > doing a whole lot. Usually that's a good sign that it should be split
> > into multiple patches.
> > 
> > However, since this is only changing your driver, you can probably
> > take any sort of patches that you like. :)
> > 
> > There is arguably little value to sending out a patch like this, since
> > it is very difficult to review. In other words, perhaps if you are
> > going to make big, unreviewable patches like this that only change
> > your driver, then you might as well just push them straight away.
> > 
> > (But, it would be better, in my opinion, to try to split up the
> > changes and let them be reviewed.)
> 
> Yes, there’s a lot in this patch. I froze the public version of the
> rasterizer when I began the upstreaming process mid February, so
> this is syncing up with about a month’s worth of development.
> 
> I also have this change as a series of 81 commits. Not sure if that
> would be preferable by the community or if people would be
> interested in reviewing the series, as issues with early commits
> might already be addressed later in the patch set.
> 

There seems to be some things working against community code review.

* Expected broken commits earlier in the series (We would normally ask
  that commits are cleaned up before posting them.)

* External development (What would happen to any code review asking
  for reworks, given that the patches are already merged elsewhere?)

* A large backlog of changes. :)

For those reasons, I don't see much value in posting this, or the 81
patches to mesa-dev. Maybe going fwd, there won't be such a backlog,
and code review would then be possible. (And, of course anything
outside the openswr driver code would require code review.)

I still think it would be better to see the 81 commits split up in the
history as long as they won't cause problems for others. Since most
people are unlikely to be building openswr, I don't think the commits
will affect them.

We rarely use merges, but perhaps it is appropriate since openswr is
developed externally. You could start a branch at the last openswr
commit, add your 81 commits. Then you could merge the resulting branch
into master.

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

Re: [Mesa-dev] [PATCH] gallium/swr: update rasterizer (532172)

[Rowley, Timothy O]

> On Mar 22, 2016, at 3:51 PM, Justen, Jordan L  
> wrote:
> 
> What does 532172 in the subject refer to?

swr rasterizer development happens in another source control system.  532172 is 
a revision id to checkpoint where we’ve pushed the changes publicly.

> From this commit message, it seems clear that this single patch is
> doing a whole lot. Usually that's a good sign that it should be split
> into multiple patches.
> 
> However, since this is only changing your driver, you can probably
> take any sort of patches that you like. :)
> 
> There is arguably little value to sending out a patch like this, since
> it is very difficult to review. In other words, perhaps if you are
> going to make big, unreviewable patches like this that only change
> your driver, then you might as well just push them straight away.
> 
> (But, it would be better, in my opinion, to try to split up the
> changes and let them be reviewed.)

Yes, there’s a lot in this patch.  I froze the public version of the rasterizer 
when I began the upstreaming process mid February, so this is syncing up with 
about a month’s worth of development.

I also have this change as a series of 81 commits.  Not sure if that would be 
preferable by the community or if people would be interested in reviewing the 
series, as issues with early commits might already be addressed later in the 
patch set.

-Tim


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

Re: [Mesa-dev] [PATCH] gallium/swr: update rasterizer (532172)

[Jordan Justen]

What does 532172 in the subject refer to?

On 2016-03-22 12:45:48, Tim Rowley wrote:
> Highlights include:
>   * code style fixes
>   * start removing win32 types
>   * switch DC/DS rings to ringbuffer datastructure
>   * rdtsc bucket support for shaders
>   * address some coverity issues
>   * user clip planes
>   * global arena
>   * support llvm-svn

From this commit message, it seems clear that this single patch is
doing a whole lot. Usually that's a good sign that it should be split
into multiple patches.

However, since this is only changing your driver, you can probably
take any sort of patches that you like. :)

There is arguably little value to sending out a patch like this, since
it is very difficult to review. In other words, perhaps if you are
going to make big, unreviewable patches like this that only change
your driver, then you might as well just push them straight away.

(But, it would be better, in my opinion, to try to split up the
changes and let them be reviewed.)

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