Re: [PATCH 02/18] drm/i915: preliminary context support

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Mar 28, 2012 at 03:59:17PM -0700, Ben Widawsky wrote:
> On Thu, 29 Mar 2012 00:43:00 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Sun, Mar 18, 2012 at 01:39:42PM -0700, Ben Widawsky wrote:
> > > Very basic code for context setup/destruction in the driver.
> > > 
> > > There are 4 entry points into the contexts, load, unload, open,
> > > close. The names are self-explanatory except that load can be
> > > called during reset, and also during pm thaw/resume. As we expect
> > > our context to be preserved across these events, we do not
> > > reinitialize in this case.
> > > 
> > > Also an important note, as I intend to use contexts for ILK RC6, the
> > > context initialization must always come before RC6 initialization.
> > > 
> > > As Adam Jackson pointed out, I picked an arbitrary cutoff of 1MB
> > > where I decide the HW context is too big. The reason for this is
> > > even though context sizes are increasing with every generation,
> > > they are still measured in pages. If we somehow read back way more
> > > than that, it probably means BIOS has done something strange, or
> > > we're running on a platform that wasn't designed for this.
> > > 
> > > The 1MB was just a nice round number. I'm open to changing it to
> > > something sensible if someone has a better idea.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > <bikeshed> I see not that much precedence for _load and _unload for
> > setup/teardown ...
> > 
> > Also this patch is imo way too early in the series - you just add
> > empty functions so I have no idea what they're doing. And hence can't
> > check whether you add them at the right place. Whereas if this comes
> > later I already know what they're doing and can check without
> > applying whether they're all called at the right place.
> > </bikeshed>
> 
> I understand that you get no greater pleasure than bikeshedding my
> patches until I want to cry... but seriously with the precedent, it's
> in our driver already. So what do you want instead, setup()/teardown()
> - init/fini?
> 
> Anyway, here is the precedent:
> i915_driver_UNLOAD()->i915_gem_context_unload()
> i915_driver_LOAD()->i915_gem_LOAD() // which used to call my function
> i915_driver_LOAD()->...->i915)gem_context_load()

Well, I've fixed up gem_load, that's now also called init ;-) And
driver_load and _unload are remnants from the stoneage, when these two
functions could essentially only be called a moduel load/unload time. Now
we have hotplug and drm drivers don't use stealth attach any more ...

Anyway, I've seen a few things while reading your patches yesterday that
looked puzzling and strange, but I didn't really know what to do with
them. So I just added some easy bikeshed comments. After a nights worth of
sleep I think I'm seeing clearer, so expect some real feedback soon.

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48