From patchwork Thu Mar 8 15:46:57 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micha=C5=82_Winiarski?= X-Patchwork-Id: 10268327 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 960E9602BD for ; Thu, 8 Mar 2018 15:47:41 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 862332974B for ; Thu, 8 Mar 2018 15:47:41 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7B1972975A; Thu, 8 Mar 2018 15:47:41 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id A75872974B for ; Thu, 8 Mar 2018 15:47:40 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id D14E46E96B; Thu, 8 Mar 2018 15:47:39 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 48AE56E96B for ; Thu, 8 Mar 2018 15:47:39 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Mar 2018 07:47:39 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,441,1515484800"; d="scan'208";a="209936018" Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by fmsmga006.fm.intel.com with ESMTP; 08 Mar 2018 07:47:38 -0800 Received: from localhost (172.28.172.64) by IRSMSX102.ger.corp.intel.com (163.33.3.155) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 8 Mar 2018 15:47:37 +0000 From: =?UTF-8?q?Micha=C5=82=20Winiarski?= To: Date: Thu, 8 Mar 2018 16:46:57 +0100 Message-ID: <20180308154707.21716-5-michal.winiarski@intel.com> X-Mailer: git-send-email 2.14.3 In-Reply-To: <20180308154707.21716-1-michal.winiarski@intel.com> References: <20180308154707.21716-1-michal.winiarski@intel.com> MIME-Version: 1.0 X-Originating-IP: [172.28.172.64] Subject: [Intel-gfx] [PATCH v2 05/15] drm/i915/guc: Log runtime should consist of both mapping and relay X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.23 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-Virus-Scanned: ClamAV using ClamSMTP Currently, we're treating relay and mapping of GuC log as a separate concepts. We're also using inconsistent locking, sometimes using relay_lock, sometimes using struct mutex. Let's correct that. Anything touching the runtime is now serialized using runtime.lock, while we're still using struct mutex as inner lock for mapping. We're still racy in setting the log level - but we'll take care of that in the following patches. v2: Tidy locking (Sagar) Signed-off-by: Michał Winiarski Cc: Chris Wilson Cc: Daniele Ceraolo Spurio Cc: Sagar Arun Kamble Cc: Michal Wajdeczko Reviewed-by: Sagar Arun Kamble --- drivers/gpu/drm/i915/intel_guc_log.c | 113 +++++++++++------------------------ drivers/gpu/drm/i915/intel_guc_log.h | 3 +- 2 files changed, 36 insertions(+), 80 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 89cb3939467f..4eb3ebd8d6c3 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -155,10 +155,7 @@ static int guc_log_relay_file_create(struct intel_guc *guc) struct dentry *log_dir; int ret; - if (!i915_modparams.guc_log_level) - return 0; - - mutex_lock(&guc->log.runtime.relay_lock); + lockdep_assert_held(&guc->log.runtime.lock); /* For now create the log file in /sys/kernel/debug/dri/0 dir */ log_dir = dev_priv->drm.primary->debugfs_root; @@ -177,28 +174,16 @@ static int guc_log_relay_file_create(struct intel_guc *guc) */ if (!log_dir) { DRM_ERROR("Debugfs dir not available yet for GuC log file\n"); - ret = -ENODEV; - goto out_unlock; + return -ENODEV; } ret = relay_late_setup_files(guc->log.runtime.relay_chan, "guc_log", log_dir); if (ret < 0 && ret != -EEXIST) { DRM_ERROR("Couldn't associate relay chan with file %d\n", ret); - goto out_unlock; + return ret; } - ret = 0; - -out_unlock: - mutex_unlock(&guc->log.runtime.relay_lock); - return ret; -} - -static bool guc_log_has_relay(struct intel_guc *guc) -{ - lockdep_assert_held(&guc->log.runtime.relay_lock); - - return guc->log.runtime.relay_chan != NULL; + return 0; } static void guc_move_to_next_buf(struct intel_guc *guc) @@ -209,9 +194,6 @@ static void guc_move_to_next_buf(struct intel_guc *guc) */ smp_wmb(); - if (!guc_log_has_relay(guc)) - return; - /* All data has been written, so now move the offset of sub buffer. */ relay_reserve(guc->log.runtime.relay_chan, guc->log.vma->obj->base.size); @@ -221,9 +203,6 @@ static void guc_move_to_next_buf(struct intel_guc *guc) static void *guc_get_write_buffer(struct intel_guc *guc) { - if (!guc_log_has_relay(guc)) - return NULL; - /* * Just get the base address of a new sub buffer and copy data into it * ourselves. NULL will be returned in no-overwrite mode, if all sub @@ -284,13 +263,14 @@ static void guc_read_update_log_buffer(struct intel_guc *guc) void *src_data, *dst_data; bool new_overflow; + mutex_lock(&guc->log.runtime.lock); + if (WARN_ON(!guc->log.runtime.buf_addr)) - return; + goto out_unlock; /* Get the pointer to shared GuC log buffer */ log_buf_state = src_data = guc->log.runtime.buf_addr; - mutex_lock(&guc->log.runtime.relay_lock); /* Get the pointer to local buffer to store the logs */ log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc); @@ -302,9 +282,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc) */ DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n"); guc->log.capture_miss_count++; - mutex_unlock(&guc->log.runtime.relay_lock); - return; + goto out_unlock; } /* Actual logs are present from the 2nd page */ @@ -375,7 +354,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc) guc_move_to_next_buf(guc); - mutex_unlock(&guc->log.runtime.relay_lock); +out_unlock: + mutex_unlock(&guc->log.runtime.lock); } static void capture_logs_work(struct work_struct *work) @@ -391,20 +371,20 @@ static bool guc_log_has_runtime(struct intel_guc *guc) return guc->log.runtime.buf_addr != NULL; } -static int guc_log_runtime_create(struct intel_guc *guc) +static int guc_log_map(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); void *vaddr; int ret; - lockdep_assert_held(&dev_priv->drm.struct_mutex); + lockdep_assert_held(&guc->log.runtime.lock); if (!guc->log.vma) return -ENODEV; - GEM_BUG_ON(guc_log_has_runtime(guc)); - + mutex_lock(&dev_priv->drm.struct_mutex); ret = i915_gem_object_set_to_wc_domain(guc->log.vma->obj, true); + mutex_unlock(&dev_priv->drm.struct_mutex); if (ret) return ret; @@ -424,14 +404,9 @@ static int guc_log_runtime_create(struct intel_guc *guc) return 0; } -static void guc_log_runtime_destroy(struct intel_guc *guc) +static void guc_log_unmap(struct intel_guc *guc) { - /* - * It's possible that the runtime stuff was never allocated because - * GuC log was disabled at the boot time. - */ - if (!guc_log_has_runtime(guc)) - return; + lockdep_assert_held(&guc->log.runtime.lock); i915_gem_object_unpin_map(guc->log.vma->obj); guc->log.runtime.buf_addr = NULL; @@ -439,7 +414,7 @@ static void guc_log_runtime_destroy(struct intel_guc *guc) void intel_guc_log_init_early(struct intel_guc *guc) { - mutex_init(&guc->log.runtime.relay_lock); + mutex_init(&guc->log.runtime.lock); INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work); } @@ -450,12 +425,7 @@ static int guc_log_relay_create(struct intel_guc *guc) size_t n_subbufs, subbuf_size; int ret; - if (!i915_modparams.guc_log_level) - return 0; - - mutex_lock(&guc->log.runtime.relay_lock); - - GEM_BUG_ON(guc_log_has_relay(guc)); + lockdep_assert_held(&guc->log.runtime.lock); /* Keep the size of sub buffers same as shared log buffer */ subbuf_size = GUC_LOG_SIZE; @@ -485,12 +455,9 @@ static int guc_log_relay_create(struct intel_guc *guc) GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size); guc->log.runtime.relay_chan = guc_log_relay_chan; - mutex_unlock(&guc->log.runtime.relay_lock); - return 0; err: - mutex_unlock(&guc->log.runtime.relay_lock); /* logging will be off */ i915_modparams.guc_log_level = 0; return ret; @@ -498,20 +465,10 @@ static int guc_log_relay_create(struct intel_guc *guc) static void guc_log_relay_destroy(struct intel_guc *guc) { - mutex_lock(&guc->log.runtime.relay_lock); - - /* - * It's possible that the relay was never allocated because - * GuC log was disabled at the boot time. - */ - if (!guc_log_has_relay(guc)) - goto out_unlock; + lockdep_assert_held(&guc->log.runtime.lock); relay_close(guc->log.runtime.relay_chan); guc->log.runtime.relay_chan = NULL; - -out_unlock: - mutex_unlock(&guc->log.runtime.relay_lock); } static void guc_log_capture_logs(struct intel_guc *guc) @@ -610,7 +567,6 @@ static void guc_log_flush_irq_disable(struct intel_guc *guc) void intel_guc_log_destroy(struct intel_guc *guc) { - guc_log_runtime_destroy(guc); i915_vma_unpin_and_release(&guc->log.vma); } @@ -687,9 +643,10 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val) int intel_guc_log_register(struct intel_guc *guc) { - struct drm_i915_private *i915 = guc_to_i915(guc); int ret; + mutex_lock(&guc->log.runtime.lock); + GEM_BUG_ON(guc_log_has_runtime(guc)); /* @@ -701,35 +658,32 @@ int intel_guc_log_register(struct intel_guc *guc) if (ret) goto err; - mutex_lock(&i915->drm.struct_mutex); - ret = guc_log_runtime_create(guc); - mutex_unlock(&i915->drm.struct_mutex); - + ret = guc_log_map(guc); if (ret) goto err_relay; ret = guc_log_relay_file_create(guc); if (ret) - goto err_runtime; + goto err_unmap; guc_log_flush_irq_enable(guc); + mutex_unlock(&guc->log.runtime.lock); + return 0; -err_runtime: - mutex_lock(&i915->drm.struct_mutex); - guc_log_runtime_destroy(guc); - mutex_unlock(&i915->drm.struct_mutex); +err_unmap: + guc_log_unmap(guc); err_relay: guc_log_relay_destroy(guc); err: + mutex_unlock(&guc->log.runtime.lock); + return ret; } void intel_guc_log_unregister(struct intel_guc *guc) { - struct drm_i915_private *i915 = guc_to_i915(guc); - guc_log_flush_irq_disable(guc); /* @@ -740,9 +694,12 @@ void intel_guc_log_unregister(struct intel_guc *guc) */ guc_flush_logs(guc); - mutex_lock(&i915->drm.struct_mutex); - guc_log_runtime_destroy(guc); - mutex_unlock(&i915->drm.struct_mutex); + mutex_lock(&guc->log.runtime.lock); + GEM_BUG_ON(!guc_log_has_runtime(guc)); + + guc_log_unmap(guc); guc_log_relay_destroy(guc); + + mutex_unlock(&guc->log.runtime.lock); } diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h index 09dd2ef1933d..8c26cce77a98 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.h +++ b/drivers/gpu/drm/i915/intel_guc_log.h @@ -48,8 +48,7 @@ struct intel_guc_log { struct workqueue_struct *flush_wq; struct work_struct flush_work; struct rchan *relay_chan; - /* To serialize the access to relay_chan */ - struct mutex relay_lock; + struct mutex lock; } runtime; /* logging related stats */ u32 capture_miss_count;