On 30 March 2017 at 23:32, Nanley Chery <nanleych...@gmail.com> wrote: > On Mon, Mar 20, 2017 at 08:05:22PM -0700, Nanley Chery wrote: > > Okay, I've re-read the email. > >> On Mon, Mar 20, 2017 at 08:01:25PM -0700, Nanley Chery wrote: >> > On Thu, Mar 16, 2017 at 05:34:13PM -0700, Kenneth Graunke wrote: >> > > On Wednesday, March 8, 2017 10:27:20 AM PDT Nanley Chery wrote: >> > > > On Wed, Mar 08, 2017 at 10:07:12AM -0800, Nanley Chery wrote: >> > > > > On Wed, Mar 08, 2017 at 02:17:59AM -0800, Kenneth Graunke wrote: >> > > > > > On Thursday, March 2, 2017 4:36:08 PM PST Nanley Chery wrote: >> > > > > > > On Mon, Feb 06, 2017 at 03:55:49PM -0800, Kenneth Graunke wrote: >> > > > > > > > If a HiZ op is the first thing in the batch, we should make >> > > > > > > > sure >> > > > > > > > to select the render pipeline and emit state base address >> > > > > > > > before >> > > > > > > > proceeding. >> > > > > > > > >> > > > > > > > I believe 3DSTATE_WM_HZ_OP creates 3DPRIMITIVEs internally, and >> > > > > > > > dispatching those on the GPGPU pipeline seems a bit sketchy. >> > > > > > > > I'm >> > > > > > > >> > > > > > > Yes, it does seem like we currently allow HZ_OPs within a GPGPU >> > > > > > > pipeline. This patch should fix that problem. >> > > > > > > >> > > > > > > > not actually sure that STATE_BASE_ADDRESS is necessary, as the >> > > > > > > > depth related commands use graphics addresses, not ones >> > > > > > > > relative >> > > > > > > > to the base address...but we're likely to set it as part of the >> > > > > > > > next operation anyway, so we should just do it right away. >> > > > > > > > >> > >> > Why should we do it right away if it will happen later on? I don't see >> > why this part of the patch is necessary. >> > >> > > > > > > >> > > > > > > I agree, re-emitting STATE_BASE_ADDRESS doesn't seem necessary. >> > > > > > > I think >> > > > > > > we should drop this part of the patch and add it back in later >> > > > > > > if we get >> > > > > > > some data that it's necessary. Leaving it there may be >> > > > > > > distracting to >> > > > > > > some readers and the BDW PRM warns that it's an expensive >> > > > > > > command: >> > > > > > > >> > > > > > > Execution of this command causes a full pipeline flush, thus >> > > > > > > its >> > > > > > > use should be minimized for higher performance. >> > > > > > >> > > > > > I think it should be basically free, actually. We track a boolean, >> > > > > > brw->batch.state_base_address_emitted, to avoid emitting it >> > > > > > multiple >> > > > > > times per batch. >> > > > > > >> > > > > > Let's say the first thing in a fresh batch is a HiZ op, followed by >> > > > > > normal drawing. Previously, we'd do: >> > > > > > >> > > > > > 1. HiZ op commands >> > > > > > 2. STATE_BASE_ADDRESS (triggered by normal rendering upload) >> > > > > > 3. rest of normal drawing commands >> > > > > > >> > > > > > Now we'd do: >> > > > > > >> > > > > > 1. STATE_BASE_ADDRESS (triggered by HiZ op) >> > > > > > 2. HiZ op commands >> > > > > > 3. normal drawing commands (second SBA is skipped) >> > > > > > >> > > > > > In other words...we're just moving it a bit earlier. I suppose >> > > > > > there >> > > > > > could be a batch containing only HiZ ops, at which point we'd pay >> > > > > > for >> > > > > > a single STATE_BASE_ADDRESS...but that seems really unlikely. >> > > > > > >> > > > > >> > > > > Sorry for not stating it up front, but the special case you've >> > > > > mentioned >> > > > > is exactly what I'd like not to hurt unnecessarily. >> > > > > >> > > >> > > Why? We really think there are going to be batches with only >> > > 3DSTATE_WM_HZ_OP and no normal rendering or BLORP? It sounds >> > > really hypothetical to me. >> > > >> > >> > I've commented on the performance implications of that snippet because >> > it is the only functional change I can see from emitting SBA. That >> > unfortunately seems to have distracted us from the more important >> > question found above. Sorry about that. >> > > > I've commented on the performance implications of that snippet because > it is the only functional change I can see from emitting SBA. > Unfortunately, discussing the impact of this change is seems to have > distracted us from the more important question of why we're making this > change. Sorry about that. > >> > > > Correct me if I'm wrong, but after thinking about it some more, it >> > > > seems >> > > > that performance wouldn't suffer by emitting the SBA since the pipeline >> > > > was already flushed at the end of the preceding batch. It may also >> > > > improve since the pipelined HiZ op will likely be followed by other >> > > > pipelined commands. I'm not totally confident in my understanding on >> > > > pipeline flushes by the way. Is this why you'd like to emit the SBA >> > > > here? >> > > > I think it's fine to leave it if we expound on the rationale. >> > > >> > > Performance is not a motivation for this patch. Having the GPU do >> > > work without a pipeline selected or state base addresses in place seems >> > > potentially dangerous. I was hoping it would help with GPU hangs. I'm >> > > not certain that it does, and it might be safe to skip this, but it >> > > seems like a lot of mental gymnastics to prove that it's safe for very >> > > little upside. >> > > >> > >> > I was only referring to the portion of the patch that emits SBA. >> > >> >> Sorry, I definitely did not read this thoroughly enough. Please ignore >> my earlier reply. > > Do we have any data that suggests that we need to emit SBA before > emitting HiZ ops? If we don't, I don't think we should speculatively do > so.
Gents, I hope that the discussion had lead to a better fix - can someone point out the patch title/sha1? Alternatively, if this has fallen through the cracks - humble ping :-) -Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
