All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Gordon <david.s.gordon@intel.com>
To: Daniel Vetter <daniel@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 5/5] drm/i915: Flatten engine init control flow
Date: Mon, 01 Dec 2014 16:11:15 +0000	[thread overview]
Message-ID: <547C9323.3090506@intel.com> (raw)
In-Reply-To: <20141120091916.GA25711@phenom.ffwll.local>

On 20/11/14 09:19, Daniel Vetter wrote:
> On Thu, Nov 20, 2014 at 08:10:27AM +0000, Chris Wilson wrote:
>> On Thu, Nov 20, 2014 at 12:33:08AM +0100, Daniel Vetter wrote:
>>> Now that sanity prevails and we have the clean split between software
>>> init and starting the engines we can drop all the "have we allocate
>>> this struct already?" nonsense.
>>
>> Actually, I was hoping for something completely different when you sad
>> flattening the init...
> 
> I presume you've hoped for more ;-) Essentially this is just the legacy
> ringbuffer blueprint, applying to execlist left as an exercise for the
> reader. And it's just a very small patch.
> 
>> Compare with
>> http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/intel_ringbuffer.c#n1983
>>
>> There is a lot of duplicated code in the ringbuffer setup, and the code
>> movement here wants to be split differently so that the sequence is
>> identical to execlists. (The only difference is that we only use one
>> ring with legacy, rather than one ring in each context [per-engine].)
>> Making sure the logic is foolproof for both is esssential.
> 
> Imo we first need to beat execlist into shape (with small patches, not by
> rewriting it all). Once stuff settles we can take another look at whether
> unification makes sense, but for now I want a big piece of insulation
> between execlist and stuff we actually depend on. Or at least stuff Dave
> might rip my head off over.
> 
> So for now I still want to embrace the duplication.
> -Daniel

As a matter of policy: while we have the duplication and the split
between legacy ringbuffer and lrc versions, should developers try to
submit corresponding changes to both paths together (e.g. in the same
patchset). Or is it OK to submit improvements only to one of the two
paths, causing them to diverge and hence possibly making deduplication
more difficult?

Anyway, this change looks OK in itself, so subject to the "exercise for
the reader" approach being acceptable (on which point I defer to your
and others judgement), then:

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2014-12-01 16:11 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-19 23:33 [PATCH 1/5] drm/i915: s/init()/init_hw()/ in intel_engine_cs Daniel Vetter
2014-11-19 23:33 ` [PATCH 2/5] drm/i915: s/intel_workarouns_ring/intel_render_workarounds/ Daniel Vetter
2014-11-20  8:05   ` Chris Wilson
2014-11-20  9:14     ` Daniel Vetter
2014-11-27 15:07   ` Dave Gordon
2014-11-28 17:53     ` Daniel Vetter
2014-11-19 23:33 ` [PATCH 3/5] drm/i915: Move intel_init_pipe_control out of engine->init_hw Daniel Vetter
2014-11-27 16:02   ` Dave Gordon
2014-11-19 23:33 ` [PATCH 4/5] drm/i915: Only init engines once Daniel Vetter
2014-11-20  8:06   ` Chris Wilson
2014-11-28 12:02   ` Dave Gordon
2014-11-28 17:56     ` Daniel Vetter
2014-11-28 18:43       ` Chris Wilson
2014-11-19 23:33 ` [PATCH 5/5] drm/i915: Flatten engine init control flow Daniel Vetter
2014-11-20  8:10   ` Chris Wilson
2014-11-20  9:19     ` Daniel Vetter
2014-12-01 16:11       ` Dave Gordon [this message]
2014-12-01 16:34         ` Daniel Vetter
2014-11-20  8:03 ` [PATCH 1/5] drm/i915: s/init()/init_hw()/ in intel_engine_cs Chris Wilson
2014-11-20  9:11   ` Daniel Vetter
2014-11-20  9:15     ` Chris Wilson
2014-11-20  8:45 ` [PATCH] drm/i915: Move init_unused_rings to gem_init_hw Daniel Vetter
2014-11-21 19:01   ` Dave Gordon
2014-11-21 20:27     ` Daniel Vetter
2014-12-02 15:39       ` Dave Gordon
2014-11-27 14:36 ` [PATCH 1/5] drm/i915: s/init()/init_hw()/ in intel_engine_cs Dave Gordon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=547C9323.3090506@intel.com \
    --to=david.s.gordon@intel.com \
    --cc=daniel.vetter@intel.com \
    --cc=daniel@ffwll.ch \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.