Quoting Prathap Kumar Valsan (2019-10-15 08:31:29)
> +int intel_mocs_emit_all_engines(struct intel_gt *gt)
> +{
> + struct drm_i915_private *i915 = gt->i915;
> + struct intel_engine_cs *engine;
> + enum intel_engine_id id;
> + int err;
> +
> + for_each_engine(engine, i915, id) {
Pass i915, and use for_each_uabi_engine(engine, i915) instead.
> + struct i915_request *rq;
> + struct drm_i915_mocs_table t;
> +
> + rq = i915_request_create(engine->kernel_context);
> + if (IS_ERR(rq))
> + return PTR_ERR(rq);
> +
> + get_mocs_settings(rq->engine->gt, &t);
> + err = emit_mocs_control_table(rq, &t);
> + if (err) {
> + i915_request_skip(rq, err);
> + i915_request_add(rq);
> + return err;
> + }
> +
> + i915_request_add(rq);
> + }
> +
> + return 0;
> +}
> +
> void intel_mocs_init(struct intel_gt *gt)
> {
> intel_mocs_init_l3cc_table(gt);
> diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.h b/drivers/gpu/drm/i915/gt/intel_mocs.h
> index 2ae816b7ca19..6a584aa36370 100644
> --- a/drivers/gpu/drm/i915/gt/intel_mocs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_mocs.h
> @@ -49,13 +49,20 @@
> * context handling keep the MOCS in step.
> */
>
> +#include <linux/types.h>
> +
> struct i915_request;
> struct intel_engine_cs;
> +struct intel_context;
> struct intel_gt;
>
> void intel_mocs_init(struct intel_gt *gt);
> void intel_mocs_init_engine(struct intel_engine_cs *engine);
> +void intel_mocs_init_reg_state(const struct intel_context *ce);
>
> int intel_mocs_emit(struct i915_request *rq);
> +int intel_mocs_emit_all_engines(struct intel_gt *gt);
> +
> +int intel_mocs_store_clos(struct i915_request *rq, struct intel_context *ce);
>
> #endif
> diff --git a/drivers/gpu/drm/i915/i915_clos.c b/drivers/gpu/drm/i915/i915_clos.c
> new file mode 100644
> index 000000000000..ead6fadcb5b3
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_clos.c
> @@ -0,0 +1,128 @@
> +/*
> + * SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include "gem/i915_gem_context.h"
> +#include "gt/intel_context.h"
> +#include "gt/intel_mocs.h"
> +
> +#include "i915_clos.h"
> +#include "i915_drv.h"
> +#include "intel_sideband.h"
> +
> +#define GEN11_DEFAULT_CLOS 0
> +
> +static int clos_modify_context(struct intel_context *ce)
> +{
> + struct i915_request *rq;
> + int err;
> +
> + lockdep_assert_held(&ce->pin_mutex);
> +
> + rq = i915_request_create(ce->engine->kernel_context);
> + if (IS_ERR(rq))
> + return PTR_ERR(rq);
> +
> + /* Serialise with the remote context */
> + err = intel_context_prepare_remote_request(ce, rq);
> + if (err == 0)
> + err = intel_mocs_store_clos(rq, ce);
> +
> + i915_request_add(rq);
> + return err;
> +}
> +
> +static int clos_configure_context(struct i915_gem_context *ctx)
> +{
> + struct i915_gem_engines_iter it;
> + struct intel_context *ce;
> + int err = 0;
> +
> + for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
> + GEM_BUG_ON(ce == ce->engine->kernel_context);
> +
> + if (ce->engine->class != RENDER_CLASS)
> + continue;
> +
> + err = intel_context_lock_pinned(ce);
> + if (err)
> + break;
> +
> + if (intel_context_is_pinned(ce))
> + err = clos_modify_context(ce);
> +
> + intel_context_unlock_pinned(ce);
> + if (err)
> + break;
> + }
> + i915_gem_context_unlock_engines(ctx);
> +
> + return err;
> +}
> +
> +static int clos_configure_all_contexts(struct drm_i915_private *i915)
> +{
> + struct i915_gem_context *ctx, *cn;
> + int err;
> +
> + /*
> + * MOCS registers of render engine are context saved and restored to and
> + * from a context image.
> + * So for any MOCS update to reflect on the existing contexts requires
> + * updating the context image.
> + */
> + spin_lock(&i915->gem.contexts.lock);
> + list_for_each_entry_safe(ctx, cn, &i915->gem.contexts.list, link) {
> + if (ctx == i915->kernel_context)
> + continue;
Did you forget something here?
> +
> + spin_unlock(&i915->gem.contexts.lock);
> +
> + err = clos_configure_context(ctx);
> + if (err)
> + return err;
> +
> + spin_lock(&i915->gem.contexts.lock);
> + list_safe_reset_next(ctx, cn, link);
> + i915_gem_context_put(ctx);
... Something that pairs with the put?
> + }
> + spin_unlock(&i915->gem.contexts.lock);
> + /*
> + * After updating all other contexts, update render context image of
> + * kernel context. Also update the MOCS of non-render engines.
> + */
> + err = intel_mocs_emit_all_engines(&i915->gt);
> +
> + return err;
> +}
> +
> +int i915_mocs_update_clos(struct drm_i915_private *i915)
> +{
> + return clos_configure_all_contexts(i915);
> +}
> +
> +void i915_read_clos_way_mask(struct drm_i915_private *i915)
> +{
> + int ret, i;
> + u32 val;
> +
> + i915->clos.active_clos = GEN11_DEFAULT_CLOS;
> +
> + for (i = 0; i < NUM_OF_CLOS; i++) {
> + val = i;
> + ret = sandybridge_pcode_read(i915,
> + ICL_PCODE_LLC_COS_WAY_MASK_INFO,
> + &val, NULL);
> + if (ret) {
> + DRM_ERROR("Mailbox read error = %d\n", ret);
> + return;
> + }
> +
> + i915->clos.way_mask[i] = val;
> + }
> +
> + i915->clos.support_way_mask_read = true;
So you opt for boot failure on 10 generations?
> +}
> +
> diff --git a/drivers/gpu/drm/i915/i915_clos.h b/drivers/gpu/drm/i915/i915_clos.h
> new file mode 100644
> index 000000000000..02bfbdaf0ca3
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_clos.h
> @@ -0,0 +1,15 @@
> +/*
> + * SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef __I915_CLOS_H__
> +#define __I915_CLOS_H__
> +
> +struct drm_i915_private;
> +
> +int i915_mocs_update_clos(struct drm_i915_private *i915);
> +void i915_read_clos_way_mask(struct drm_i915_private *i915);
> +
> +#endif /* __I915_CLOS_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c46b339064c0..73d1f79f347e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1313,6 +1313,16 @@ struct drm_i915_private {
> bool distrust_bios_wm;
> } wm;
>
> + /* Last Level Cache Class of Service */
> + struct {
> + bool support_way_mask_read;
> + u32 active_clos;
> +#define NUM_OF_CLOS 4
We don't use _OF_
> + u16 way_mask[NUM_OF_CLOS];
> + /* Lock to serialize updating device clos via sysfs interface.*/
> + struct mutex lock;
At least try to reorder this so the holes are less obvious.
> + } clos;
> +
> struct dram_info {
> bool valid;
> bool is_16gb_dimm;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0ddbd3a5fb8d..b2b89ca8c726 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -54,6 +54,7 @@
> #include "gt/intel_renderstate.h"
> #include "gt/intel_workarounds.h"
>
> +#include "i915_clos.h"
> #include "i915_drv.h"
> #include "i915_scatterlist.h"
> #include "i915_trace.h"
> @@ -1281,6 +1282,8 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
>
> intel_uc_init(&dev_priv->gt.uc);
>
> + i915_read_clos_way_mask(dev_priv);
This seems very out of place.
> ret = intel_gt_init_hw(&dev_priv->gt);
> if (ret)
> goto err_uc_init;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e24991e54897..d9689a494910 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8869,6 +8869,7 @@ enum {
> #define ICL_PCODE_MEM_SUBSYSYSTEM_INFO 0xd
> #define ICL_PCODE_MEM_SS_READ_GLOBAL_INFO (0x0 << 8)
> #define ICL_PCODE_MEM_SS_READ_QGV_POINT_INFO(point) (((point) << 16) | (0x1 << 8))
> +#define ICL_PCODE_LLC_COS_WAY_MASK_INFO 0x1d
> #define GEN6_PCODE_READ_D_COMP 0x10
> #define GEN6_PCODE_WRITE_D_COMP 0x11
> #define HSW_PCODE_DE_WRITE_FREQ_REQ 0x17
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index bf039b8ba593..cb90cee474fb 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -32,10 +32,12 @@
>
> #include "gt/intel_rc6.h"
>
> +#include "i915_clos.h"
> #include "i915_drv.h"
> #include "i915_sysfs.h"
> #include "intel_pm.h"
> #include "intel_sideband.h"
> +#include "gt/intel_mocs.h"
>
> static inline struct drm_i915_private *kdev_minor_to_i915(struct device *kdev)
> {
> @@ -255,6 +257,92 @@ static const struct bin_attribute dpf_attrs_1 = {
> .private = (void *)1
> };
>
> +static ssize_t llc_clos_show(struct device *kdev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> + ssize_t len = 0;
> + int active_clos;
> +
> + active_clos = dev_priv->clos.active_clos;
> + len += snprintf(buf + len, PAGE_SIZE, "0x%x\n",
> + dev_priv->clos.way_mask[active_clos]);
Go through this pair with READ/WRITE_ONCE to highlight the races to
yourself and work out if they are legal (and prevent the compiler from
fouling up your race prevention logic).
> + return len;
> +}
> +
> +/*
> + * This will tie the GPU device to a class-of-service. Each class-of-service
> + * has way_mask associated with it. A way-mask determines the LLC cache ways
> + * that wil be used to allocate cachelines for the GPU memory accesses.
> + */
> +static ssize_t llc_clos_store(struct device *kdev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> + u8 active_clos, new_clos, clos_index;
> + bool valid_mask = false;
> + ssize_t ret;
> + u16 way_mask;
> +
> + ret = kstrtou16(buf, 0, &way_mask);
> + if (ret)
> + return ret;
> +
> + active_clos = dev_priv->clos.active_clos;
> +
> + if (dev_priv->clos.way_mask[active_clos] == way_mask)
> + return count;
> +
> + for (clos_index = 0; clos_index < NUM_OF_CLOS; clos_index++) {
> + if (dev_priv->clos.way_mask[clos_index] == way_mask) {
> + new_clos = clos_index;
> + valid_mask = true;
> + break;
> + }
> + }
> +
> + if (!valid_mask)
> + return -EINVAL;
> +
> + ret = mutex_lock_interruptible(&dev_priv->clos.lock);
> + if (ret)
> + return ret;
> +
> + dev_priv->clos.active_clos = new_clos;
> + ret = i915_mocs_update_clos(dev_priv);
> + if (ret) {
> + DRM_ERROR("Failed to update Class of service\n");
> + dev_priv->clos.active_clos = active_clos;
Half the contexts are using new_clos. One way out might be to set_wedged
and burn all the evidence.
> + mutex_unlock(&dev_priv->clos.lock);
> + return ret;
> + }
> +
> + mutex_unlock(&dev_priv->clos.lock);
> +
> + return count;
> +}
> +
> +static ssize_t llc_clos_modes_show(struct device *kdev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> + ssize_t len = 0;
> + int i;
> +
> + for (i = 0; i < NUM_OF_CLOS; i++)
> + len += snprintf(buf + len, PAGE_SIZE, "0x%x ",
> + dev_priv->clos.way_mask[i]);
> +
> + len += snprintf(buf + len, PAGE_SIZE, "\n");
One way other sysfs use for a single interface is to show the active
mode with an asterisk [*].
> +
> + return len;
> +}
> +
> +static DEVICE_ATTR_RW(llc_clos);
> +static DEVICE_ATTR_RO(llc_clos_modes);
> +
> static ssize_t gt_act_freq_mhz_show(struct device *kdev,
> struct device_attribute *attr, char *buf)
> {
> @@ -574,6 +662,18 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
> struct device *kdev = dev_priv->drm.primary->kdev;
> int ret;
>
> + if (dev_priv->clos.support_way_mask_read) {
> + ret = sysfs_create_file(&kdev->kobj,
> + &dev_attr_llc_clos.attr);
> + if (ret)
> + DRM_ERROR("LLC COS sysfs setup failed\n");
> +
> + ret = sysfs_create_file(&kdev->kobj,
> + &dev_attr_llc_clos_modes.attr);
> + if (ret)
> + DRM_ERROR("LLC COS sysfs setup failed\n");
On the scale of 0-9, how serious is this actually?
More like a 4 than 6?
> + }
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
