From mboxrd@z Thu Jan  1 00:00:00 1970
From: Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [PATCH 02/18] drm/i915: preliminary context support
Date: Thu, 29 Mar 2012 10:43:45 +0200
Message-ID: <20120329084345.GA4106@phenom.ffwll.local>
References: <1332103198-25852-1-git-send-email-ben@bwidawsk.net>
	<1332103198-25852-3-git-send-email-ben@bwidawsk.net>
	<20120328224300.GH2046@phenom.ffwll.local>
	<20120328155917.182444ab@bwidawsk.net>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org>
Received: from mail-ee0-f49.google.com (mail-ee0-f49.google.com [74.125.83.49])
	by gabe.freedesktop.org (Postfix) with ESMTP id 015FE9EB0B
	for <intel-gfx@lists.freedesktop.org>;
	Thu, 29 Mar 2012 01:43:05 -0700 (PDT)
Received: by eekb45 with SMTP id b45so906715eek.36
	for <intel-gfx@lists.freedesktop.org>;
	Thu, 29 Mar 2012 01:43:04 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <20120328155917.182444ab@bwidawsk.net>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/intel-gfx>
List-Post: <mailto:intel-gfx@lists.freedesktop.org>
List-Help: <mailto:intel-gfx-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=subscribe>
Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org
Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org
To: Ben Widawsky <ben@bwidawsk.net>
Cc: intel-gfx@lists.freedesktop.org
List-Id: intel-gfx@lists.freedesktop.org

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