From patchwork Thu Apr 23 12:25:38 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dave Gordon X-Patchwork-Id: 6262011 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 425E99F313 for ; Thu, 23 Apr 2015 12:25:45 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 003302038C for ; Thu, 23 Apr 2015 12:25:43 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id A1DAE20380 for ; Thu, 23 Apr 2015 12:25:42 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 95B016E773; Thu, 23 Apr 2015 05:25:41 -0700 (PDT) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTP id EC2596E773 for ; Thu, 23 Apr 2015 05:25:40 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP; 23 Apr 2015 05:25:41 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,630,1422950400"; d="scan'208,223";a="718076430" Received: from dsgordon-linux.isw.intel.com (HELO [10.102.226.149]) ([10.102.226.149]) by orsmga002.jf.intel.com with ESMTP; 23 Apr 2015 05:25:39 -0700 Message-ID: <5538E4C2.6000709@intel.com> Date: Thu, 23 Apr 2015 13:25:38 +0100 From: Dave Gordon Organization: Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: yu.dai@intel.com, intel-gfx@lists.freedesktop.org References: <1429305680-4990-1-git-send-email-yu.dai@intel.com> <1429305680-4990-6-git-send-email-yu.dai@intel.com> In-Reply-To: <1429305680-4990-6-git-send-email-yu.dai@intel.com> Subject: Re: [Intel-gfx] [PATCH v3 05/15] drm/i915: Defer default hardware context initialisation until first open X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 17/04/15 22:21, yu.dai@intel.com wrote: > From: Dave Gordon > > In order to fully initialise the default contexts, we have to execute > batchbuffer commands on the GPU engines. But we can't do that until any > required firmware has been loaded, which may not be possible during > driver load, because the filesystem(s) containing the firmware may not > be mounted until later. > > Therefore, we now allow the first call to the firmware-loading code to > return -EAGAIN to indicate that it's not yet ready, and that it should > be retried when the device is first opened from user code, by which > time we expect that all required filesystems will have been mounted. > The late-retry code will then re-attempt to load the firmware if the > early attempt failed. > > Issue: VIZ-4884 > Signed-off-by: Dave Gordon > Signed-off-by: Alex Dai > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gem.c | 9 ++++++++- > drivers/gpu/drm/i915/i915_gem_context.c | 35 ++++++++++++++++++++++++++++----- > drivers/gpu/drm/i915/intel_guc.h | 2 +- > drivers/gpu/drm/i915/intel_guc_loader.c | 25 +++++++++++++++++++++-- > 5 files changed, 63 insertions(+), 9 deletions(-) Hi Alex, the code is fine :) but I'd rather that this functionality went in ahead of the GuC loader - it's actually a useful generic standalone that could provide recovery from other early initialisation issues, even before we consider GuC submission (there's a reason that EIO was already considered nonfatal during startup). The other reason I don't like this being added after the GuC loader is because it changes the interface to intel_guc_load_ucode() which was only just added in the patch before. If we swap the order then the interface is defined the same way from day one, and no-one need consider the half-baked state inbetween. So I suggest this patch should be moved to the beginning of the series (or even submitted independently), and include only the change to i915_drv.h, the second block of changes to i915_gem.c, and all the changes to i915_gem_context.c EXCEPT the three lines around the call to intel_guc_load_ucode() [see attachment for clarification and a new commit message]. Then the other changes -- which are all GuC-specific -- can be added back in the *later* patch that adds the GuC loader. > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 235fc08..d128ac4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1760,6 +1760,7 @@ struct drm_i915_private { > /* hda/i915 audio component */ > bool audio_component_registered; > > + bool contexts_ready; > uint32_t hw_context_size; > struct list_head context_list; > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 44154fe..b7bd288 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4888,8 +4888,15 @@ i915_gem_init_hw(struct drm_device *dev) > i915_gem_cleanup_ringbuffer(dev); > } > > + /* We can't enable contexts until all firmware is loaded */ > + ret = intel_guc_load_ucode(dev, false); > + if (ret == -EAGAIN) > + return 0; /* too early */ > + Don't include these five lines in this patch > ret = i915_gem_context_enable(dev_priv); > - if (ret && ret != -EIO) { > + if (ret == 0) { > + dev_priv->contexts_ready = true; > + } else if (ret && ret != -EIO) { > DRM_ERROR("Context enable failed %d\n", ret); > i915_gem_cleanup_ringbuffer(dev); > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index e4c57a3..3795df2 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -447,23 +447,48 @@ static int context_idr_cleanup(int id, void *p, void *data) > return 0; > } > > +/* Complete any late initialisation here */ > +static int i915_gem_context_first_open(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + int ret; > + > + /* We can't enable contexts until all firmware is loaded */ > + ret = intel_guc_load_ucode(dev, true); > + WARN_ON(ret == -EAGAIN); Don't include these four either > + > + ret = i915_gem_context_enable(dev_priv); > + if (ret == 0) > + dev_priv->contexts_ready = true; > + return ret; > +} > + > int i915_gem_context_open(struct drm_device *dev, struct drm_file *file) > { > + struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_i915_file_private *file_priv = file->driver_priv; > struct intel_context *ctx; > + int ret = 0; > > idr_init(&file_priv->context_idr); > > mutex_lock(&dev->struct_mutex); > - ctx = i915_gem_create_context(dev, file_priv); > + > + if (!dev_priv->contexts_ready) > + ret = i915_gem_context_first_open(dev); > + > + if (ret == 0) { > + ctx = i915_gem_create_context(dev, file_priv); > + if (IS_ERR(ctx)) > + ret = PTR_ERR(ctx); > + } > + > mutex_unlock(&dev->struct_mutex); > > - if (IS_ERR(ctx)) { > + if (ret) > idr_destroy(&file_priv->context_idr); > - return PTR_ERR(ctx); > - } > > - return 0; > + return ret; > } > > void i915_gem_context_close(struct drm_device *dev, struct drm_file *file) Then fold everything else into the patch(es) that create intel_guc.h and intel_guc_loader.c ... I think that will provide a simpler and more logical split between generic and GuC-specific code :) .Dave. From 160d40ab80d982aff479e3524a0442bb92f78b3d Mon Sep 17 00:00:00 2001 From: Dave Gordon Date: Fri, 17 Apr 2015 22:21:10 +0100 Subject: [PATCH 01/15] drm/i915: Defer default hardware context initialisation until first open To fully initialise the default contexts in i915_gem_context_enable(), we have to execute batchbuffer commands on the GPU engines. But that might not succeed during early initialisation, so we already treat a return of EIO as nonfatal in gem_init_hw(), allowing the driver to load despite not (yet) being able to submit batch commands. This commit adds a flag to record whether the default contexts have successfully been initialised; if they haven't, then we retry the call to i915_gem_context_enable() when the device is first opened, which should be late enough for all required resources to have become available and any setup helper code to have run. Issue: VIZ-4884 Signed-off-by: Dave Gordon Signed-off-by: Alex Dai --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem.c | 4 +++- drivers/gpu/drm/i915/i915_gem_context.c | 31 ++++++++++++++++++++++++++----- 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 79da7f3..0e88ef2 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1751,6 +1751,7 @@ struct drm_i915_private { /* hda/i915 audio component */ bool audio_component_registered; + bool contexts_ready; uint32_t hw_context_size; struct list_head context_list; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 89d9ebe..1fef0bb 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4889,7 +4889,9 @@ i915_gem_init_hw(struct drm_device *dev) } ret = i915_gem_context_enable(dev_priv); - if (ret && ret != -EIO) { + if (ret == 0) { + dev_priv->contexts_ready = true; + } else if (ret && ret != -EIO) { DRM_ERROR("Context enable failed %d\n", ret); i915_gem_cleanup_ringbuffer(dev); diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index e4c57a3..92baba0 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -447,23 +447,44 @@ static int context_idr_cleanup(int id, void *p, void *data) return 0; } +/* Complete any late initialisation here */ +static int i915_gem_context_first_open(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + int ret; + + ret = i915_gem_context_enable(dev_priv); + if (ret == 0) + dev_priv->contexts_ready = true; + return ret; +} + int i915_gem_context_open(struct drm_device *dev, struct drm_file *file) { + struct drm_i915_private *dev_priv = dev->dev_private; struct drm_i915_file_private *file_priv = file->driver_priv; struct intel_context *ctx; + int ret = 0; idr_init(&file_priv->context_idr); mutex_lock(&dev->struct_mutex); - ctx = i915_gem_create_context(dev, file_priv); + + if (!dev_priv->contexts_ready) + ret = i915_gem_context_first_open(dev); + + if (ret == 0) { + ctx = i915_gem_create_context(dev, file_priv); + if (IS_ERR(ctx)) + ret = PTR_ERR(ctx); + } + mutex_unlock(&dev->struct_mutex); - if (IS_ERR(ctx)) { + if (ret) idr_destroy(&file_priv->context_idr); - return PTR_ERR(ctx); - } - return 0; + return ret; } void i915_gem_context_close(struct drm_device *dev, struct drm_file *file) -- 1.7.9.5