From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CFE48C4332F for ; Tue, 31 Oct 2023 13:05:00 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2BC0A10E4B1; Tue, 31 Oct 2023 13:05:00 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.7]) by gabe.freedesktop.org (Postfix) with ESMTPS id 97CC010E4B1 for ; Tue, 31 Oct 2023 13:04:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1698757498; x=1730293498; h=message-id:date:mime-version:subject:to:references:from: in-reply-to:content-transfer-encoding; bh=LcDgp3VHO6R1vMFjSNTQ0z5weqigZREfWoVN8op7YJo=; b=TM8rASEg7/noPbWlybAJettAcUJpqlOmU6K0u2ZXLPRcwlG8zEMX2xYp 9VzzNJfpv5dp7y+Hf/oSukbEXzbQ4k6oEhoSEqm2EFL2OeEu/3GIuAAW1 ++K2wy4dVD6WmJS8UkDzZROvplG76RVwBQq2teyKoRmCrijFikPg/h5Zu Z1M3pUFWVrRmrt/J4BYq3r1Wf7sbnmLasxKuxS/3XDzb9CYADgEJ+AhFa /yoo1hUUv60DODDgmmpv9XKDZTAUNLrG5o6igt5CEm0xcGfcDSwSl93X0 IF+6xolWX8WT6epZvAuGSVUuWtQKvc1POMQetCEePxHUmOyDEek+E1Ed5 A==; X-IronPort-AV: E=McAfee;i="6600,9927,10879"; a="9812919" X-IronPort-AV: E=Sophos;i="6.03,265,1694761200"; d="scan'208";a="9812919" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmvoesa101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Oct 2023 06:04:58 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10879"; a="760609359" X-IronPort-AV: E=Sophos;i="6.03,265,1694761200"; d="scan'208";a="760609359" Received: from nirmoyda-mobl.ger.corp.intel.com (HELO [10.252.33.135]) ([10.252.33.135]) by orsmga002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Oct 2023 06:04:56 -0700 Message-ID: Date: Tue, 31 Oct 2023 14:04:54 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Content-Language: en-US To: Jani Nikula , intel-gfx@lists.freedesktop.org References: <20231031124502.1772160-1-jani.nikula@intel.com> <20231031124502.1772160-3-jani.nikula@intel.com> From: Nirmoy Das In-Reply-To: <20231031124502.1772160-3-jani.nikula@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Intel-gfx] [PATCH RESEND 3/3] drm/i915: move gpu error sysfs to i915_gpu_error.c X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 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" On 10/31/2023 1:45 PM, Jani Nikula wrote: > Hide gpu error specifics in i915_gpu_error.c. This is also cleaner wrt > conditional compilation, as i915_gpu_error.c is only built with > DRM_I915_CAPTURE_ERROR=y. > > With this, we can also make i915_first_error_state() static. > > Signed-off-by: Jani Nikula Reviewed-by: Nirmoy Das > --- > drivers/gpu/drm/i915/i915_gpu_error.c | 75 ++++++++++++++++++++++++- > drivers/gpu/drm/i915/i915_gpu_error.h | 17 +++--- > drivers/gpu/drm/i915/i915_sysfs.c | 79 +-------------------------- > 3 files changed, 86 insertions(+), 85 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > index f195df91d9e6..00559a75b798 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -57,6 +57,7 @@ > #include "i915_memcpy.h" > #include "i915_reg.h" > #include "i915_scatterlist.h" > +#include "i915_sysfs.h" > #include "i915_utils.h" > > #define ALLOW_FAIL (__GFP_KSWAPD_RECLAIM | __GFP_RETRY_MAYFAIL | __GFP_NOWARN) > @@ -2211,7 +2212,7 @@ void i915_capture_error_state(struct intel_gt *gt, > i915_gpu_coredump_put(error); > } > > -struct i915_gpu_coredump * > +static struct i915_gpu_coredump * > i915_first_error_state(struct drm_i915_private *i915) > { > struct i915_gpu_coredump *error; > @@ -2487,3 +2488,75 @@ void i915_gpu_error_debugfs_register(struct drm_i915_private *i915) > debugfs_create_file("i915_gpu_info", 0644, minor->debugfs_root, i915, > &i915_gpu_info_fops); > } > + > +static ssize_t error_state_read(struct file *filp, struct kobject *kobj, > + struct bin_attribute *attr, char *buf, > + loff_t off, size_t count) > +{ > + > + struct device *kdev = kobj_to_dev(kobj); > + struct drm_i915_private *i915 = kdev_minor_to_i915(kdev); > + struct i915_gpu_coredump *gpu; > + ssize_t ret = 0; > + > + /* > + * FIXME: Concurrent clients triggering resets and reading + clearing > + * dumps can cause inconsistent sysfs reads when a user calls in with a > + * non-zero offset to complete a prior partial read but the > + * gpu_coredump has been cleared or replaced. > + */ > + > + gpu = i915_first_error_state(i915); > + if (IS_ERR(gpu)) { > + ret = PTR_ERR(gpu); > + } else if (gpu) { > + ret = i915_gpu_coredump_copy_to_buffer(gpu, buf, off, count); > + i915_gpu_coredump_put(gpu); > + } else { > + const char *str = "No error state collected\n"; > + size_t len = strlen(str); > + > + if (off < len) { > + ret = min_t(size_t, count, len - off); > + memcpy(buf, str + off, ret); > + } > + } > + > + return ret; > +} > + > +static ssize_t error_state_write(struct file *file, struct kobject *kobj, > + struct bin_attribute *attr, char *buf, > + loff_t off, size_t count) > +{ > + struct device *kdev = kobj_to_dev(kobj); > + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > + > + drm_dbg(&dev_priv->drm, "Resetting error state\n"); > + i915_reset_error_state(dev_priv); > + > + return count; > +} > + > +static const struct bin_attribute error_state_attr = { > + .attr.name = "error", > + .attr.mode = S_IRUSR | S_IWUSR, > + .size = 0, > + .read = error_state_read, > + .write = error_state_write, > +}; > + > +void i915_gpu_error_sysfs_setup(struct drm_i915_private *i915) > +{ > + struct device *kdev = i915->drm.primary->kdev; > + > + if (sysfs_create_bin_file(&kdev->kobj, &error_state_attr)) > + drm_err(&i915->drm, "error_state sysfs setup failed\n"); > +} > + > +void i915_gpu_error_sysfs_teardown(struct drm_i915_private *i915) > +{ > + struct device *kdev = i915->drm.primary->kdev; > + > + sysfs_remove_bin_file(&kdev->kobj, &error_state_attr); > +} > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.h b/drivers/gpu/drm/i915/i915_gpu_error.h > index f851189b0ff1..fa886620d3f8 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.h > +++ b/drivers/gpu/drm/i915/i915_gpu_error.h > @@ -325,11 +325,12 @@ static inline void i915_gpu_coredump_put(struct i915_gpu_coredump *gpu) > kref_put(&gpu->ref, __i915_gpu_coredump_free); > } > > -struct i915_gpu_coredump *i915_first_error_state(struct drm_i915_private *i915); > void i915_reset_error_state(struct drm_i915_private *i915); > void i915_disable_error_state(struct drm_i915_private *i915, int err); > > void i915_gpu_error_debugfs_register(struct drm_i915_private *i915); > +void i915_gpu_error_sysfs_setup(struct drm_i915_private *i915); > +void i915_gpu_error_sysfs_teardown(struct drm_i915_private *i915); > > #else > > @@ -398,12 +399,6 @@ static inline void i915_gpu_coredump_put(struct i915_gpu_coredump *gpu) > { > } > > -static inline struct i915_gpu_coredump * > -i915_first_error_state(struct drm_i915_private *i915) > -{ > - return ERR_PTR(-ENODEV); > -} > - > static inline void i915_reset_error_state(struct drm_i915_private *i915) > { > } > @@ -417,6 +412,14 @@ static inline void i915_gpu_error_debugfs_register(struct drm_i915_private *i915 > { > } > > +static inline void i915_gpu_error_sysfs_setup(struct drm_i915_private *i915) > +{ > +} > + > +static inline void i915_gpu_error_sysfs_teardown(struct drm_i915_private *i915) > +{ > +} > + > #endif /* IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR) */ > > #endif /* _I915_GPU_ERROR_H_ */ > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c > index e88bb4f04305..613decd47760 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -155,81 +155,6 @@ static const struct bin_attribute dpf_attrs_1 = { > .private = (void *)1 > }; > > -#if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR) > - > -static ssize_t error_state_read(struct file *filp, struct kobject *kobj, > - struct bin_attribute *attr, char *buf, > - loff_t off, size_t count) > -{ > - > - struct device *kdev = kobj_to_dev(kobj); > - struct drm_i915_private *i915 = kdev_minor_to_i915(kdev); > - struct i915_gpu_coredump *gpu; > - ssize_t ret = 0; > - > - /* > - * FIXME: Concurrent clients triggering resets and reading + clearing > - * dumps can cause inconsistent sysfs reads when a user calls in with a > - * non-zero offset to complete a prior partial read but the > - * gpu_coredump has been cleared or replaced. > - */ > - > - gpu = i915_first_error_state(i915); > - if (IS_ERR(gpu)) { > - ret = PTR_ERR(gpu); > - } else if (gpu) { > - ret = i915_gpu_coredump_copy_to_buffer(gpu, buf, off, count); > - i915_gpu_coredump_put(gpu); > - } else { > - const char *str = "No error state collected\n"; > - size_t len = strlen(str); > - > - if (off < len) { > - ret = min_t(size_t, count, len - off); > - memcpy(buf, str + off, ret); > - } > - } > - > - return ret; > -} > - > -static ssize_t error_state_write(struct file *file, struct kobject *kobj, > - struct bin_attribute *attr, char *buf, > - loff_t off, size_t count) > -{ > - struct device *kdev = kobj_to_dev(kobj); > - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > - > - drm_dbg(&dev_priv->drm, "Resetting error state\n"); > - i915_reset_error_state(dev_priv); > - > - return count; > -} > - > -static const struct bin_attribute error_state_attr = { > - .attr.name = "error", > - .attr.mode = S_IRUSR | S_IWUSR, > - .size = 0, > - .read = error_state_read, > - .write = error_state_write, > -}; > - > -static void i915_setup_error_capture(struct device *kdev) > -{ > - if (sysfs_create_bin_file(&kdev->kobj, &error_state_attr)) > - drm_err(&kdev_minor_to_i915(kdev)->drm, > - "error_state sysfs setup failed\n"); > -} > - > -static void i915_teardown_error_capture(struct device *kdev) > -{ > - sysfs_remove_bin_file(&kdev->kobj, &error_state_attr); > -} > -#else > -static void i915_setup_error_capture(struct device *kdev) {} > -static void i915_teardown_error_capture(struct device *kdev) {} > -#endif > - > void i915_setup_sysfs(struct drm_i915_private *dev_priv) > { > struct device *kdev = dev_priv->drm.primary->kdev; > @@ -255,7 +180,7 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) > drm_warn(&dev_priv->drm, > "failed to register GT sysfs directory\n"); > > - i915_setup_error_capture(kdev); > + i915_gpu_error_sysfs_setup(dev_priv); > > intel_engines_add_sysfs(dev_priv); > } > @@ -264,7 +189,7 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv) > { > struct device *kdev = dev_priv->drm.primary->kdev; > > - i915_teardown_error_capture(kdev); > + i915_gpu_error_sysfs_teardown(dev_priv); > > device_remove_bin_file(kdev, &dpf_attrs_1); > device_remove_bin_file(kdev, &dpf_attrs);