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

[Ben Widawsky <ben@bwidawsk.net>]

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()

As for your other comment, I do agree. The point of this patch was to
show where the hooks go to allow people to review them. But without
code, or comments explaining what the code will do - it's pretty
useless. I think this can just be combined with the next patch.

Also, some people may want to review it this way (it is easier for me
to think of this way, for example).

> 
> Cheers, Daniel