Data port coherency control for UMDs.

The patch adds a parameter to control the data port coherency functionality on a per-exec call basis. When data port coherency flag value is different than what it was in previous call for the context, a command to switch data port coherency state is added before the buffer to be executed.

Quoting Tomasz Lis (2018-03-19 12:37:35)

Quoted 4 lines

The patch adds a parameter to control the data port coherency functionality on a per-exec call basis. When data port coherency flag value is different than what it was in previous call for the context, a command to switch data port coherency state is added before the buffer to be executed.

So this is part of the context? Why do it at exec level?

If exec level
is desired, why not whitelist it?
-Chris

Quoted 5 lines

Quoting Tomasz Lis (2018-03-19 12:37:35)

Quoted 4 lines

The patch adds a parameter to control the data port coherency functionality on a per-exec call basis. When data port coherency flag value is different than what it was in previous call for the context, a command to switch data port coherency state is added before the buffer to be executed.

Quoted 1 line

So this is part of the context? Why do it at exec level?

It is part of the context, stored within HDC chicken bit register. The exec level was requested by the OCL team, due to concerns about performance cost of context setparam calls.

Quoted 2 lines

If exec level
is desired, why not whitelist it?

If we have no issue in whitelisting the register, I'm sure OCL will agree to that.
I assumed the whitelisting will be unacceptable because of security concerns with some options.
The register also changes its position and content between gens, which makes whitelisting hard to manage.

Main purpose of chicken bit registers, in general, is to allow work around for hardware features which could  be buggy or could have unintended influence on the platform.
The data port coherency functionality landed there for the same reasons; then it twisted itself in a way that we now need user space to switch it. Is it really ok to whitelist chicken bit registers?

Quoting Lis, Tomasz (2018-03-19 14:14:19)

Quoted 11 lines · Chris Wilson

Quoted 6 lines

Quoting Tomasz Lis (2018-03-19 12:37:35)

Quoted 4 lines

The patch adds a parameter to control the data port coherency functionality on a per-exec call basis. When data port coherency flag value is different than what it was in previous call for the context, a command to switch data port coherency state is added before the buffer to be executed.

So this is part of the context? Why do it at exec level?

It is part of the context, stored within HDC chicken bit register. The exec level was requested by the OCL team, due to concerns about performance cost of context setparam calls.

What? Oh dear, oh dear, thrice oh dear. The context setparam would look like:

	if (arg != context->value) {
		rq = request_alloc(context, RCS);
		cs = ring_begin(rq, 4);
		cs++ = MI_LRI;
		cs++ = reg;
		cs++ = magic;
		cs++ = MI_NOOP;
		request_add(rq);
		context->value = arg
	}

The argument is whether stuffing it into a crowded, v.frequently executed execbuf is better than an irregular setparam. If they want to flip it on every batch, use execbuf. If it's going to be very infrequent, setparam.

That discussion must be part of the rationale in the commitlog.

Otoh, execbuf3 would accept it as a command packet. Hmm.

Quoted 9 lines

Quoted 2 lines

If exec level
is desired, why not whitelist it?

If we have no issue in whitelisting the register, I'm sure OCL will agree to that.
I assumed the whitelisting will be unacceptable because of security concerns with some options.
The register also changes its position and content between gens, which makes whitelisting hard to manage.

Quoted 6 lines

Main purpose of chicken bit registers, in general, is to allow work around for hardware features which could  be buggy or could have unintended influence on the platform.
The data port coherency functionality landed there for the same reasons; then it twisted itself in a way that we now need user space to switch it. Is it really ok to whitelist chicken bit registers?

It all depends on whether it breaks segregation. If the only users affected are themselves, fine. Otherwise, no.
-Chris

    Quoting Lis, Tomasz (2018-03-19 14:14:19)


        On 2018-03-19 13:43, Chris Wilson wrote:

            Quoting Tomasz Lis (2018-03-19 12:37:35)

                The patch adds a parameter to control the data port coherency functionality
                on a per-exec call basis. When data port coherency flag value is different
                than what it was in previous call for the context, a command to switch data
                port coherency state is added before the buffer to be executed.

            So this is part of the context? Why do it at exec level?

        It is part of the context, stored within HDC chicken bit register.
        The exec level was requested by the OCL team, due to concerns about
        performance cost of context setparam calls.

    What? Oh dear, oh dear, thrice oh dear. The context setparam would look
    like:

            if (arg != context->value) {
                    rq = request_alloc(context, RCS);
                    cs = ring_begin(rq, 4);
                    cs++ = MI_LRI;
                    cs++ = reg;
                    cs++ = magic;
                    cs++ = MI_NOOP;
                    request_add(rq);
                    context->value = arg
            }

    The argument is whether stuffing it into a crowded, v.frequently
    executed execbuf is better than an irregular setparam. If they want to
    flip it on every batch, use execbuf. If it's going to be very
    infrequent, setparam.

Implementing the data port coherency switch as context setparam would not be a problem, I agree.
But this is not a solution OCL is willing to accept. Any additional IOCTL call is a concern for the OCL developers.

For more explanation on switch frequency - please look at the cover letter I provided; here's the related part of it:
(note: the data port coherency is called fine grain coherency within UMD)

3. Will coherency switch be used frequently?

There are scenarios that will require frequent toggling of the coherency switch.
E.g. an application has two OCL compute kernels: kern_master and kern_worker. kern_master uses, concurrently with CPU, some fine grain SVM resources (CL_MEM_SVM_FINE_GRAIN_BUFFER). These resources contain descriptors of computational work that needs to be executed. kern_master analyzes incoming work descriptors and populates a plain OCL buffer (non-fine-grain) with payload for kern_worker. Once kern_master is done, kern_worker kicks-in and processes the payload that kern_master produced. These two kernels work in a loop, one after another. Since only kern_master requires coherency, kern_worker should not be forced to pay for it. This means that we need to have the ability to toggle coherency switch on or off per each GPU submission:
(ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> (ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> ...

That discussion must be part of the rationale in the commitlog.

Will add.
Should I place the whole text from cover letter within the commit comment?

Otoh, execbuf3 would accept it as a command packet. Hmm.

I know we have execbuf2, but execbuf3? Are you proposing to add something like that?

If exec level
is desired, why not whitelist it?

If we have no issue in whitelisting the register, I'm sure OCL will agree to that.
I assumed the whitelisting will be unacceptable because of security concerns with some options.
The register also changes its position and content between gens, which makes whitelisting hard to manage.

Main purpose of chicken bit registers, in general, is to allow work around for hardware features which could  be buggy or could have unintended influence on the platform.
The data port coherency functionality landed there for the same reasons; then it twisted itself in a way that we now need user space to switch it. Is it really ok to whitelist chicken bit registers?

It all depends on whether it breaks segregation. If the only users affected are themselves, fine. Otherwise, no.
-Chris

Chicken Bit registers are definitely not planned as safe for use. While meaning of bits within HDC_CHICKEN0 change between gens, I doubt any of the registers *can't* be used to cause GPU hung.
-Tomasz

Quoted 6 lines · Chris Wilson

Quoting Tomasz Lis (2018-03-19 12:37:35)

Quoted 4 lines

The patch adds a parameter to control the data port coherency functionality on a per-exec call basis. When data port coherency flag value is different than what it was in previous call for the context, a command to switch data port coherency state is added before the buffer to be executed.

So this is part of the context? Why do it at exec level?

It is part of the context, stored within HDC chicken bit register.
The exec level was requested by the OCL team, due to concerns about performance cost of context setparam calls.

Quoted 3 lines

  If exec level
is desired, why not whitelist it?
-Chris

If we have no issue in whitelisting the register, I'm sure OCL will agree to that.
I assumed the whitelisting will be unacceptable because of security concerns with some options. The register also changes its position and content between gens, which makes whitelisting hard to manage.

Main purpose of chicken bit registers, in general, is to allow work around for hardware features which could  be buggy or could have unintended influence on the platform. The data port coherency functionality landed there for the same reasons; then it twisted itself in a way that we now need user space to switch it. Is it really ok to whitelist chicken bit registers?
-Tomasz

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 21 lines · Chris Wilson

Quoted 9 lines

Quoting Tomasz Lis (2018-03-19 12:37:35)

Quoted 7 lines

The patch adds a parameter to control the data port coherency functionality
on a per-exec call basis. When data port coherency flag value is different
than what it was in previous call for the context, a command to switch data
port coherency state is added before the buffer to be executed.

So this is part of the context? Why do it at exec level?

It is part of the context, stored within HDC chicken bit register. The exec level was requested by the OCL team, due to concerns about performance cost of context setparam calls.

Quoted 3 lines

  If exec level
is desired, why not whitelist it?
-Chris

Quoted 7 lines

If we have no issue in whitelisting the register, I'm sure OCL will agree to that.
I assumed the whitelisting will be unacceptable because of security concerns with some options.
The register also changes its position and content between gens, which makes whitelisting hard to manage.

I think a security analysis of this register was already done, and the result was that it contains some other bits that could be dangerous. In CNL those bits were moved out of the way and the HDC_CHICKEN0 register can be whitelisted (WaAllowUMDToControlCoherency). In ICL the register should already be non-privileged.

Quoting Oscar Mateo (2018-03-20 18:43:45)

Quoted 27 lines · Tomasz

Quoted 18 lines · Chris Wilson

Quoted 9 lines

Quoting Tomasz Lis (2018-03-19 12:37:35)

Quoted 7 lines

The patch adds a parameter to control the data port coherency functionality
on a per-exec call basis. When data port coherency flag value is different
than what it was in previous call for the context, a command to switch data
port coherency state is added before the buffer to be executed.

So this is part of the context? Why do it at exec level?

It is part of the context, stored within HDC chicken bit register. The exec level was requested by the OCL team, due to concerns about performance cost of context setparam calls.

Quoted 3 lines

  If exec level
is desired, why not whitelist it?
-Chris

Quoted 7 lines

If we have no issue in whitelisting the register, I'm sure OCL will agree to that.
I assumed the whitelisting will be unacceptable because of security concerns with some options.
The register also changes its position and content between gens, which makes whitelisting hard to manage.

Quoted 5 lines

I think a security analysis of this register was already done, and the result was that it contains some other bits that could be dangerous. In CNL those bits were moved out of the way and the HDC_CHICKEN0 register can be whitelisted (WaAllowUMDToControlCoherency). In ICL the register should already be non-privileged.

The previous alternative to whitelisting was running through a command parser for validation. That's a very general mechanism suitable for a wide range of sins.
-Chris

Quoted 33 lines · Chris Wilson

Quoting Oscar Mateo (2018-03-20 18:43:45)

Quoted 32 lines · Tomasz

Quoted 18 lines · Chris Wilson

Quoted 9 lines

Quoting Tomasz Lis (2018-03-19 12:37:35)

Quoted 7 lines

The patch adds a parameter to control the data port coherency functionality
on a per-exec call basis. When data port coherency flag value is different
than what it was in previous call for the context, a command to switch data
port coherency state is added before the buffer to be executed.

So this is part of the context? Why do it at exec level?

It is part of the context, stored within HDC chicken bit register. The exec level was requested by the OCL team, due to concerns about performance cost of context setparam calls.

Quoted 3 lines

   If exec level
is desired, why not whitelist it?
-Chris

Quoted 7 lines

If we have no issue in whitelisting the register, I'm sure OCL will agree to that.
I assumed the whitelisting will be unacceptable because of security concerns with some options.
The register also changes its position and content between gens, which makes whitelisting hard to manage.

I think a security analysis of this register was already done, and the result was that it contains some other bits that could be dangerous. In CNL those bits were moved out of the way and the HDC_CHICKEN0 register can be whitelisted (WaAllowUMDToControlCoherency). In ICL the register should already be non-privileged.

Quoted 4 lines

The previous alternative to whitelisting was running through a command parser for validation. That's a very general mechanism suitable for a wide range of sins.
-Chris

Are you suggesting that we enable the cmd parser for every Gen < CNL for this particular usage only? :P

The patch adds a parameter to control the data port coherency functionality on a per-context level. When the IOCTL is called, a command to switch data port coherency state is added to the ordered list. All prior requests are executed on old coherency settings, and all exec requests after the IOCTL will use new settings.

Rationale:

The OpenCL driver develpers requested a functionality to control cache coherency at data port level. Keeping the coherency at that level is disabled by default due to its performance costs. OpenCL driver is planning to enable it for a small subset of submissions, when such functionality is required. Below are answers to basic question explaining background of the functionality and reasoning for the proposed implementation:

1. Why do we need a coherency enable/disable switch for memory that is shared between CPU and GEN (GPU)?

Memory coherency between CPU and GEN, while being a great feature that enables CL_MEM_SVM_FINE_GRAIN_BUFFER OCL capability on Intel GEN architecture, adds overhead related to tracking (snooping) memory inside different cache units (L1$, L2$, L3$, LLC$, etc.). At the same time, minority of modern OCL applications actually use CL_MEM_SVM_FINE_GRAIN_BUFFER (and hence require memory coherency between CPU and GPU). The goal of coherency enable/disable switch is to remove overhead of memory coherency when memory coherency is not needed.

2. Why do we need a global coherency switch?

In order to support I/O commands from within EUs (Execution Units), Intel GEN ISA (GEN Instruction Set Assembly) contains dedicated "send" instructions. These send instructions provide several addressing models. One of these addressing models (named "stateless") provides most flexible I/O using plain virtual addresses (as opposed to buffer_handle+offset models). This "stateless" model is similar to regular memory load/store operations available on typical CPUs. Since this model provides I/O using arbitrary virtual addresses, it enables algorithmic designs that are based on pointer-to-pointer (e.g. buffer of pointers) concepts. For instance, it allows creating tree-like data structures such as:

                   ________________
                  |      NODE1     |
                  | uint64_t data  |
                  +----------------|
                  | NODE*  |  NODE*|
                  +--------+-------+
                    /              \
   ________________/                \________________
  |      NODE2     |                |      NODE3     |
  | uint64_t data  |                | uint64_t data  |
  +----------------|                +----------------|
  | NODE*  |  NODE*|                | NODE*  |  NODE*|
  +--------+-------+                +--------+-------+

Please note that pointers inside such structures can point to memory locations in different OCL allocations - e.g. NODE1 and NODE2 can reside in one OCL allocation while NODE3 resides in a completely separate OCL allocation. Additionally, such pointers can be shared with CPU (i.e. using SVM - Shared Virtual Memory feature). Using pointers from different allocations doesn't affect the stateless addressing model which even allows scattered reading from different allocations at the same time (i.e. by utilizing SIMD-nature of send instructions).

When it comes to coherency programming, send instructions in stateless model can be encoded (at ISA level) to either use or disable coherency. However, for generic OCL applications (such as example with tree-like data structure), OCL compiler is not able to determine origin of memory pointed to by an arbitrary pointer - i.e. is not able to track given pointer back to a specific allocation. As such, it's not able to decide whether coherency is needed or not for specific pointer (or for specific I/O instruction). As a result, compiler encodes all stateless sends as coherent (doing otherwise would lead to functional issues resulting from data corruption). Please note that it would be possible to workaround this (e.g. based on allocations map and pointer bounds checking prior to each I/O instruction) but the performance cost of such workaround would be many times greater than the cost of keeping coherency always enabled. As such, enabling/disabling memory coherency at GEN ISA level is not feasible and alternative method is needed.

Such alternative solution is to have a global coherency switch that allows disabling coherency for single (though entire) GPU submission. This is beneficial because this way we:
* can enable (and pay for) coherency only in submissions that actually need coherency (submissions that use CL_MEM_SVM_FINE_GRAIN_BUFFER resources)
* don't care about coherency at GEN ISA granularity (no performance impact)

3. Will coherency switch be used frequently?

There are scenarios that will require frequent toggling of the coherency switch.
E.g. an application has two OCL compute kernels: kern_master and kern_worker. kern_master uses, concurrently with CPU, some fine grain SVM resources (CL_MEM_SVM_FINE_GRAIN_BUFFER). These resources contain descriptors of computational work that needs to be executed. kern_master analyzes incoming work descriptors and populates a plain OCL buffer (non-fine-grain) with payload for kern_worker. Once kern_master is done, kern_worker kicks-in and processes the payload that kern_master produced. These two kernels work in a loop, one after another. Since only kern_master requires coherency, kern_worker should not be forced to pay for it. This means that we need to have the ability to toggle coherency switch on or off per each GPU submission:
(ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> (ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> ...

Cc Joonas Lahtinen <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Michal Winiarski <[email protected]>

Bspec: 11419

Signed-off-by Tomasz Lis <[email protected]>

drivers/gpu/drm/i915/i915_gem_context.c +5 -0

+               __clear_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+       return ret;
+}
+
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
                                  struct drm_file *file)
 {

@@ -784,6 +802,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
                                    struct drm_file *file)
 {
+       struct drm_i915_private *dev_priv = to_i915(dev);
        struct drm_i915_file_private *file_priv = file->driver_priv;
        struct drm_i915_gem_context_param *args = data;
        struct i915_gem_context *ctx;

drivers/gpu/drm/i915/i915_gem_context.c +5 -0

+               if (INTEL_GEN(dev_priv) < 9)
+                       ret = -ENODEV;
+               else
+                       args->value = i915_gem_context_is_data_port_coherent(ctx);
+               break;
        default:
                ret = -EINVAL;
                break;

@@ -893,6 +923,17 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
                }
                break;

drivers/gpu/drm/i915/i915_gem_context.c +2 -0

+               break;
+
        default:
                ret = -EINVAL;
                break;

+++ b/drivers/gpu/drm/i915/intel_lrc.c

drivers/gpu/drm/i915/i915_gem_context.c +9 -0

@@ -258,6 +258,57 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx,
        ce->lrc_desc = desc;
 }
 
+static int emit_set_data_port_coherency(struct i915_request *req, bool enable)
+{
+       u32 *cs;
+       i915_reg_t reg;
+
+       GEM_BUG_ON(req->engine->class != RENDER_CLASS);
+       GEM_BUG_ON(INTEL_GEN(req->i915) < 9);
+
+       cs = intel_ring_begin(req, 4);

drivers/gpu/drm/i915/i915_gem_context.c +18 -0

+       if (enable)
+               *cs++ = _MASKED_BIT_DISABLE(HDC_FORCE_NON_COHERENT);
+       else
+               *cs++ = _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT);
+       *cs++ = MI_NOOP;
+
+       intel_ring_advance(req, cs);
+
+       return 0;
+}
+
+int
+intel_lr_context_modify_data_port_coherency(struct i915_gem_context *ctx,
+               bool enable)
+{
+       struct i915_request *req;
+       int ret;
+

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

+
 static struct i915_priolist *
 lookup_priolist(struct intel_engine_cs *engine, int prio)
 {

drivers/gpu/drm/i915/intel_lrc.h +4 -0

index 1593194..214e291 100644
@@ -104,4 +104,8 @@ struct i915_gem_context;
 
 void intel_lr_context_resume(struct drm_i915_private *dev_priv);
 
+int
+intel_lr_context_modify_data_port_coherency(struct i915_gem_context *ctx,
+                                            bool enable);
+
 #endif /* _INTEL_LRC_H_ */

include/uapi/drm/i915_drm.h +0 -0

index 7f5634c..fab072f 100644

Changelog would be much appreciated. And this is not the first version of the series. It helps to remind the reviewer that original implementation was changed into IOCTl based on feedback. Please see the git log in i915 for some examples.

Quoting Tomasz Lis (2018-06-20 18:03:07)

Quoted 107 lines

The patch adds a parameter to control the data port coherency functionality on a per-context level. When the IOCTL is called, a command to switch data port coherency state is added to the ordered list. All prior requests are executed on old coherency settings, and all exec requests after the IOCTL will use new settings.

Rationale:

The OpenCL driver develpers requested a functionality to control cache coherency at data port level. Keeping the coherency at that level is disabled by default due to its performance costs. OpenCL driver is planning to enable it for a small subset of submissions, when such functionality is required. Below are answers to basic question explaining background of the functionality and reasoning for the proposed implementation:

1. Why do we need a coherency enable/disable switch for memory that is shared between CPU and GEN (GPU)?

Memory coherency between CPU and GEN, while being a great feature that enables CL_MEM_SVM_FINE_GRAIN_BUFFER OCL capability on Intel GEN architecture, adds overhead related to tracking (snooping) memory inside different cache units (L1$, L2$, L3$, LLC$, etc.). At the same time, minority of modern OCL applications actually use CL_MEM_SVM_FINE_GRAIN_BUFFER (and hence require memory coherency between CPU and GPU). The goal of coherency enable/disable switch is to remove overhead of memory coherency when memory coherency is not needed.

2. Why do we need a global coherency switch?

In order to support I/O commands from within EUs (Execution Units), Intel GEN ISA (GEN Instruction Set Assembly) contains dedicated "send" instructions. These send instructions provide several addressing models. One of these addressing models (named "stateless") provides most flexible I/O using plain virtual addresses (as opposed to buffer_handle+offset models). This "stateless" model is similar to regular memory load/store operations available on typical CPUs. Since this model provides I/O using arbitrary virtual addresses, it enables algorithmic designs that are based on pointer-to-pointer (e.g. buffer of pointers) concepts. For instance, it allows creating tree-like data structures such as:

                    ________________
                   |      NODE1     |
                   | uint64_t data  |
                   +----------------|
                   | NODE*  |  NODE*|
                   +--------+-------+
                     /              \
    ________________/                \________________
   |      NODE2     |                |      NODE3     |
   | uint64_t data  |                | uint64_t data  |
   +----------------|                +----------------|
   | NODE*  |  NODE*|                | NODE*  |  NODE*|
   +--------+-------+                +--------+-------+

Please note that pointers inside such structures can point to memory locations in different OCL allocations - e.g. NODE1 and NODE2 can reside in one OCL allocation while NODE3 resides in a completely separate OCL allocation. Additionally, such pointers can be shared with CPU (i.e. using SVM - Shared Virtual Memory feature). Using pointers from different allocations doesn't affect the stateless addressing model which even allows scattered reading from different allocations at the same time (i.e. by utilizing SIMD-nature of send instructions).

When it comes to coherency programming, send instructions in stateless model can be encoded (at ISA level) to either use or disable coherency. However, for generic OCL applications (such as example with tree-like data structure), OCL compiler is not able to determine origin of memory pointed to by an arbitrary pointer - i.e. is not able to track given pointer back to a specific allocation. As such, it's not able to decide whether coherency is needed or not for specific pointer (or for specific I/O instruction). As a result, compiler encodes all stateless sends as coherent (doing otherwise would lead to functional issues resulting from data corruption). Please note that it would be possible to workaround this (e.g. based on allocations map and pointer bounds checking prior to each I/O instruction) but the performance cost of such workaround would be many times greater than the cost of keeping coherency always enabled. As such, enabling/disabling memory coherency at GEN ISA level is not feasible and alternative method is needed.

Such alternative solution is to have a global coherency switch that allows disabling coherency for single (though entire) GPU submission. This is beneficial because this way we:
* can enable (and pay for) coherency only in submissions that actually need coherency (submissions that use CL_MEM_SVM_FINE_GRAIN_BUFFER resources)
* don't care about coherency at GEN ISA granularity (no performance impact)

3. Will coherency switch be used frequently?

There are scenarios that will require frequent toggling of the coherency switch.
E.g. an application has two OCL compute kernels: kern_master and kern_worker. kern_master uses, concurrently with CPU, some fine grain SVM resources (CL_MEM_SVM_FINE_GRAIN_BUFFER). These resources contain descriptors of computational work that needs to be executed. kern_master analyzes incoming work descriptors and populates a plain OCL buffer (non-fine-grain) with payload for kern_worker. Once kern_master is done, kern_worker kicks-in and processes the payload that kern_master produced. These two kernels work in a loop, one after another. Since only kern_master requires coherency, kern_worker should not be forced to pay for it. This means that we need to have the ability to toggle coherency switch on or off per each GPU submission:
(ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> (ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> ...

Cc Joonas Lahtinen <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Michal Winiarski <[email protected]>

Bspec: 11419

Signed-off-by Tomasz Lis <[email protected]>

<SNIP>

Quoted 8 lines

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

index ccf463a..ea65ae6 100644
@@ -711,6 +711,24 @@ static bool client_is_banned(struct drm_i915_file_private *file_priv)
         return atomic_read(&file_priv->ban_score) >= I915_CLIENT_SCORE_BANNED;
  }
 

drivers/gpu/drm/i915/i915_gem_context.c static bool client_is_banned(struct drm_i915_file_private *file_priv) · line 718

Quoted 5 lines

drivers/gpu/drm/i915/i915_gem_context.c +5 -0

+static int i915_gem_context_set_data_port_coherent(struct i915_gem_context *ctx)
+{
+       int ret;
+       ret = intel_lr_context_modify_data_port_coherency(ctx, true);
+       if (!GEM_WARN_ON(ret))

I don't think there's need for the WARN as the error will be propagated back to userspace?

drivers/gpu/drm/i915/i915_gem_context.c static bool client_is_banned(struct drm_i915_file_private *file_priv) · line 727

Quoted 10 lines

drivers/gpu/drm/i915/i915_gem_context.c +9 -0

+               __set_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+       return ret;
+}
+
+static int i915_gem_context_clear_data_port_coherent(struct i915_gem_context *ctx)
+{
+       int ret;
+       ret = intel_lr_context_modify_data_port_coherency(ctx, false);
+       if (!GEM_WARN_ON(ret))

Ditto.

Quoted 18 lines

drivers/gpu/drm/i915/i915_gem_context.c +5 -0

+               __clear_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+       return ret;
+}
+
  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
                                   struct drm_file *file)
  {

@@ -784,6 +802,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
  int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
                                     struct drm_file *file)
  {
+       struct drm_i915_private *dev_priv = to_i915(dev);
         struct drm_i915_file_private *file_priv = file->driver_priv;
         struct drm_i915_gem_context_param *args = data;
         struct i915_gem_context *ctx;

@@ -818,6 +837,16 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
         case I915_CONTEXT_PARAM_PRIORITY:

drivers/gpu/drm/i915/i915_gem_context.c int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, · line 844

Quoted 7 lines

drivers/gpu/drm/i915/i915_gem_context.c +5 -0

                 args->value = ctx->sched.priority;
                 break;
+       case I915_CONTEXT_PARAM_COHERENCY:
+               /*
+                * ENODEV if the feature is not supported. This removes the need
+                * of separate IS_SUPPORTED parameter.
+                */

Code speaks for itself, the comment is not needed.

Quoted 12 lines

drivers/gpu/drm/i915/i915_gem_context.c +5 -0

+               if (INTEL_GEN(dev_priv) < 9)
+                       ret = -ENODEV;
+               else
+                       args->value = i915_gem_context_is_data_port_coherent(ctx);
+               break;
         default:
                 ret = -EINVAL;
                 break;

@@ -893,6 +923,17 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
                 }
                 break;
 

drivers/gpu/drm/i915/i915_gem_context.c int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data, · line 934

Quoted 9 lines

drivers/gpu/drm/i915/i915_gem_context.c +9 -0

+       case I915_CONTEXT_PARAM_COHERENCY:
+               if (args->size)
+                       ret = -EINVAL;
+               else if (INTEL_GEN(dev_priv) < 9)
+                       ret = -ENODEV;
+               else if (args->value)
+                       ret = i915_gem_context_set_data_port_coherent(ctx);
+               else
+                       ret = i915_gem_context_clear_data_port_coherent(ctx);

Be more strict with the uAPI. Only accept values 0 or 1, then you leave space for extension in the future.

Quoted 6 lines

drivers/gpu/drm/i915/i915_gem_context.c +2 -0

+               break;
+
         default:
                 ret = -EINVAL;
                 break;

<SNIP>

Quoted 1 line

+++ b/drivers/gpu/drm/i915/intel_lrc.c

I'm feeling this is not the right file. The bit is in hardware context, and doesn't have so much to do with LRC.

Quoted 19 lines

drivers/gpu/drm/i915/i915_gem_context.c +14 -0

@@ -258,6 +258,57 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx,
         ce->lrc_desc = desc;
  }
 
+static int emit_set_data_port_coherency(struct i915_request *req, bool enable)
+{
+       u32 *cs;
+       i915_reg_t reg;
+
+       GEM_BUG_ON(req->engine->class != RENDER_CLASS);
+       GEM_BUG_ON(INTEL_GEN(req->i915) < 9);
+
+       cs = intel_ring_begin(req, 4);
+       if (IS_ERR(cs))
+               return PTR_ERR(cs);
+
+       if (INTEL_GEN(req->i915) >= 10)
+               reg = CNL_HDC_CHICKEN0;

drivers/gpu/drm/i915/i915_gem_context.c intel_lr_context_descriptor_update(struct i915_gem_context *ctx, · line 279

Quoted 5 lines

drivers/gpu/drm/i915/i915_gem_context.c +5 -0

+       else
+               reg = HDC_CHICKEN0;
+
+       /* FIXME: this feature may be unuseable on CNL; If this checks to be
+        *  true, we should enodev for CNL. */

This is exactly why we want the IGT tests to check for effects, not for the register. Then we can get an answer by running the tests on all kind of CNL systems at hand.

drivers/gpu/drm/i915/i915_gem_context.c intel_lr_context_descriptor_update(struct i915_gem_context *ctx, · line 282

Quoted 4 lines

drivers/gpu/drm/i915/i915_gem_context.c +3 -0

+       *cs++ = MI_LOAD_REGISTER_IMM(1);
+       *cs++ = i915_mmio_reg_offset(reg);
+       /* Enabling coherency means disabling the bit which forces it off */

Code is again very self explanatory without the comment.

Quoted 27 lines

drivers/gpu/drm/i915/i915_gem_context.c +26 -0

+       if (enable)
+               *cs++ = _MASKED_BIT_DISABLE(HDC_FORCE_NON_COHERENT);
+       else
+               *cs++ = _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT);
+       *cs++ = MI_NOOP;
+
+       intel_ring_advance(req, cs);
+
+       return 0;
+}
+
+int
+intel_lr_context_modify_data_port_coherency(struct i915_gem_context *ctx,
+               bool enable)
+{
+       struct i915_request *req;
+       int ret;
+
+       req = i915_request_alloc(ctx->i915->engine[RCS], ctx);
+       if (IS_ERR(req))
+               return PTR_ERR(req);
+
+       ret = emit_set_data_port_coherency(req, enable);
+
+       i915_request_add(req);
+

drivers/gpu/drm/i915/i915_gem_context.c intel_lr_context_descriptor_update(struct i915_gem_context *ctx, · line 310

Quoted 2 lines

drivers/gpu/drm/i915/i915_gem_context.c +2 -0

+       return ret;
+}

I'm thinking we should set this value when it has changed, when we insert the requests into the command stream. So if you change back and forth, while not emitting any requests, nothing really happens. If you change the value and emit a request, we should emit a LRI before the jump to the commands. Similary if you keep setting the value to the value it already was in, nothing will happen, again.

Quoted 12 lines

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

+
  static struct i915_priolist *
  lookup_priolist(struct intel_engine_cs *engine, int prio)
  {

drivers/gpu/drm/i915/intel_lrc.h +0 -0

index 1593194..214e291 100644
@@ -104,4 +104,8 @@ struct i915_gem_context;
 
  void intel_lr_context_resume(struct drm_i915_private *dev_priv);
 

Quoted 10 lines

drivers/gpu/drm/i915/intel_lrc.h +4 -0

+int
+intel_lr_context_modify_data_port_coherency(struct i915_gem_context *ctx,
+                                            bool enable);
+
  #endif /* _INTEL_LRC_H_ */

include/uapi/drm/i915_drm.h +0 -0

index 7f5634c..fab072f 100644

include/uapi/drm/i915_drm.h struct drm_i915_gem_context_param { · line 1456

Quoted 4 lines

include/uapi/drm/i915_drm.h +1 -0

@@ -1453,6 +1453,7 @@ struct drm_i915_gem_context_param {
  #define I915_CONTEXT_PARAM_NO_ERROR_CAPTURE    0x4
  #define I915_CONTEXT_PARAM_BANNABLE    0x5
  #define I915_CONTEXT_PARAM_PRIORITY    0x6
+#define I915_CONTEXT_PARAM_COHERENCY   0x7

Please add this line after the indented context priorities.

include/uapi/drm/i915_drm.h struct drm_i915_gem_context_param { · line 1459

Quoted 4 lines

include/uapi/drm/i915_drm.h +0 -0

  #define   I915_CONTEXT_MAX_USER_PRIORITY       1023 /* inclusive */
  #define   I915_CONTEXT_DEFAULT_PRIORITY                0
  #define   I915_CONTEXT_MIN_USER_PRIORITY       -1023 /* inclusive */

Here.

Regards, Joonas

Quoted 10 lines

Quoting Tomasz Lis (2018-06-20 18:03:07)

Quoted 9 lines

int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
struct drm_file *file)
{

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

+       struct drm_i915_private *dev_priv = to_i915(dev);
         struct drm_i915_file_private *file_priv = file->driver_priv;
         struct drm_i915_gem_context_param *args = data;
         struct i915_gem_context *ctx;

@@ -818,6 +837,16 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
         case I915_CONTEXT_PARAM_PRIORITY:

Quoted 8 lines

drivers/gpu/drm/i915/i915_gem_context.c int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, · line 844

Quoted 7 lines

drivers/gpu/drm/i915/i915_gem_context.c +5 -0

                 args->value = ctx->sched.priority;
                 break;
+       case I915_CONTEXT_PARAM_COHERENCY:
+               /*
+                * ENODEV if the feature is not supported. This removes the need
+                * of separate IS_SUPPORTED parameter.
+                */

Code speaks for itself, the comment is not needed.

I don't think it is a good idea to limit comments. The current look of the code makes it hard for anyone new to work on it, as the only documentation is the history in mailing list.
I don't think it's the correct approach. I believe comments should be encouraged.

Quoted 4 lines

drivers/gpu/drm/i915/intel_lrc.c intel_lr_context_descriptor_update(struct i915_gem_context *ctx, · line 282

Quoted 3 lines

drivers/gpu/drm/i915/intel_lrc.c +3 -0

+       *cs++ = MI_LOAD_REGISTER_IMM(1);
+       *cs++ = i915_mmio_reg_offset(reg);
+       /* Enabling coherency means disabling the bit which forces it off */

Code is again very self explanatory without the comment.

The logic is reversed, so that "enable" does a "disable". I believe the comment does a great job of assuring the reader that this is not just a coding mistake.

Do we have any official guidelines for limiting comments?

Quoted 6 lines

I'm thinking we should set this value when it has changed, when we insert the requests into the command stream. So if you change back and forth, while not emitting any requests, nothing really happens. If you change the value and emit a request, we should emit a LRI before the jump to the commands. Similary if you keep setting the value to the value it already was in, nothing will happen, again.

When I considered that, my way of reasoning was:
If we execute the flag changing buffer right away, it may be sent to hardware faster if there is no job in progress.
If we use the lazy way, and trigger the change just before submission - there will be additional conditions in submission code, plus the change will be made when there is another job pending (though it's not a considerable payload to just switch a flag).
If user space switches the flag back and forth without much sense, then there is something wrong with the user space driver, and it shouldn't be up to kernel to fix that.

drivers/gpu/drm/i915/intel_lrc.c +22 -0

index 33bc914..c69dc26 100644
@@ -258,6 +258,57 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx,
        ce->lrc_desc = desc;
 }
 
+static int emit_set_data_port_coherency(struct i915_request *req, bool enable)
+{
+       u32 *cs;
+       i915_reg_t reg;
+
+       GEM_BUG_ON(req->engine->class != RENDER_CLASS);
+       GEM_BUG_ON(INTEL_GEN(req->i915) < 9);
+
+       cs = intel_ring_begin(req, 4);
+       if (IS_ERR(cs))
+               return PTR_ERR(cs);
+
+       if (INTEL_GEN(req->i915) >= 10)
+               reg = CNL_HDC_CHICKEN0;
+       else
+               reg = HDC_CHICKEN0;
+
+       /* FIXME: this feature may be unuseable on CNL; If this checks to be
+        *  true, we should enodev for CNL. */
+       *cs++ = MI_LOAD_REGISTER_IMM(1);
+       *cs++ = i915_mmio_reg_offset(reg);
+       /* Enabling coherency means disabling the bit which forces it off */

drivers/gpu/drm/i915/intel_lrc.h +4 -0

index 1593194..214e291 100644
@@ -104,4 +104,8 @@ struct i915_gem_context;
 
 void intel_lr_context_resume(struct drm_i915_private *dev_priv);
 
+int
+intel_lr_context_modify_data_port_coherency(struct i915_gem_context *ctx,
+                                            bool enable);
+
 #endif /* _INTEL_LRC_H_ */

include/uapi/drm/i915_drm.h +0 -0

index 7f5634c..fab072f 100644

Quoting Tomasz Lis (2018-06-20 16:03:07)

Quoted 30 lines

drivers/gpu/drm/i915/intel_lrc.c +22 -0

index 33bc914..c69dc26 100644
@@ -258,6 +258,57 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx,
         ce->lrc_desc = desc;
  }
 
+static int emit_set_data_port_coherency(struct i915_request *req, bool enable)
+{
+       u32 *cs;
+       i915_reg_t reg;
+
+       GEM_BUG_ON(req->engine->class != RENDER_CLASS);
+       GEM_BUG_ON(INTEL_GEN(req->i915) < 9);
+
+       cs = intel_ring_begin(req, 4);
+       if (IS_ERR(cs))
+               return PTR_ERR(cs);
+
+       if (INTEL_GEN(req->i915) >= 10)
+               reg = CNL_HDC_CHICKEN0;
+       else
+               reg = HDC_CHICKEN0;
+
+       /* FIXME: this feature may be unuseable on CNL; If this checks to be
+        *  true, we should enodev for CNL. */
+       *cs++ = MI_LOAD_REGISTER_IMM(1);
+       *cs++ = i915_mmio_reg_offset(reg);
+       /* Enabling coherency means disabling the bit which forces it off */

drivers/gpu/drm/i915/intel_lrc.c intel_lr_context_descriptor_update(struct i915_gem_context *ctx, · line 292

Quoted 10 lines

drivers/gpu/drm/i915/intel_lrc.c +10 -0

+       if (enable)
+               *cs++ = _MASKED_BIT_DISABLE(HDC_FORCE_NON_COHERENT);
+       else
+               *cs++ = _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT);
+       *cs++ = MI_NOOP;
+
+       intel_ring_advance(req, cs);
+
+       return 0;
+}

There's nothing specific to the logical ringbuffer context here afaics. It could have just been done inside the single i915_gem_context_set_data_port_coherency(). Also makes it clearer that i915_gem_context_set_data_port_coherency needs struct_mutex.

cmd = HDC_FORCE_NON_COHERENT << 16;
if (!coherent)

	cmd |= HDC_FORCE_NON_COHERENT;

*cs++ = cmd;

Does that read any clearer?

Quoted 19 lines

drivers/gpu/drm/i915/intel_lrc.h +4 -0

index 1593194..214e291 100644
@@ -104,4 +104,8 @@ struct i915_gem_context;
 
  void intel_lr_context_resume(struct drm_i915_private *dev_priv);
 
+int
+intel_lr_context_modify_data_port_coherency(struct i915_gem_context *ctx,
+                                            bool enable);
+
  #endif /* _INTEL_LRC_H_ */

include/uapi/drm/i915_drm.h +0 -0

index 7f5634c..fab072f 100644

include/uapi/drm/i915_drm.h struct drm_i915_gem_context_param { · line 1456

Quoted 4 lines

include/uapi/drm/i915_drm.h +1 -0

@@ -1453,6 +1453,7 @@ struct drm_i915_gem_context_param {
  #define I915_CONTEXT_PARAM_NO_ERROR_CAPTURE    0x4
  #define I915_CONTEXT_PARAM_BANNABLE    0x5
  #define I915_CONTEXT_PARAM_PRIORITY    0x6
+#define I915_CONTEXT_PARAM_COHERENCY   0x7

DATAPORT_COHERENCY
There are many different caches.

There should be some commentary around here telling userspace what the contract is.

include/uapi/drm/i915_drm.h struct drm_i915_gem_context_param { · line 1459

Quoted 4 lines

include/uapi/drm/i915_drm.h +0 -0

  #define   I915_CONTEXT_MAX_USER_PRIORITY       1023 /* inclusive */
  #define   I915_CONTEXT_DEFAULT_PRIORITY                0
  #define   I915_CONTEXT_MIN_USER_PRIORITY       -1023 /* inclusive */

COHERENCY has MAX/MIN_USER_PRIORITY, interesting. I thought it was just a boolean.
-Chris

I would like to add few things that were mentioned previously.

According to adoption plan.
Our plan is to drop dependency on LLVM 4.0.1 (with custom patches) and instead compile with unpatched (either system or vanilla) LLVM 6.0. Work to transition our compiler stack to LLVM 6 is expected to complete in late Q3. Additionally, we are refactoring our packaging, so instead of a single neo package with multiple components we will have multiple versioned packages with clear dependencies.

We are coordinating with ClearLinux team to get included once that happens and plan to reach out to other OSVs to do the same.

Is this plan enough to consider NEO an actual opensource client for the coherency control patch ?

PR for patch usage:
https://github.com/intel/compute-runtime/pull/53

Bartosz

Yes, once you follow through with the plan, there should be no issues about merging patches to support the driver.

You may want to squeeze your timeline to be complete before 4.19-rc5, which is the feature cutoff date for 4.20, but that is rather an ambitious goal. Your original schedule would land the patches before 4.20-rc5 resulting in inclusion to 4.21.

Regards, Joonas

PS. I'm going on a vacation for a couple of weeks.

Additionally, we are already on Arch:
https://aur.archlinux.org/packages/compute-runtime

Can I assume that adoption plan is not a blocker anymore?

Bartosz

Quoted 8 lines

Yes, once you follow through with the plan, there should be no issues about merging patches to support the driver.

You may want to squeeze your timeline to be complete before 4.19-rc5, which is the feature cutoff date for 4.20, but that is rather an ambitious goal. Your original schedule would land the patches before 4.20-rc5 resulting in inclusion to 4.21.

Regards, Joonas

PS. I'm going on a vacation for a couple of weeks.

I'm not an Arch user myself, but my impression is that AUR [1] is equivalent of Ubuntu's PPA where anybody can very much upload anything outside of the support model of the distro.

The patch adds a parameter to control the data port coherency functionality on a per-context level. When the IOCTL is called, a command to switch data port coherency state is added to the ordered list. All prior requests are executed on old coherency settings, and all exec requests after the IOCTL will use new settings.

Rationale:

The OpenCL driver develpers requested a functionality to control cache coherency at data port level. Keeping the coherency at that level is disabled by default due to its performance costs. OpenCL driver is planning to enable it for a small subset of submissions, when such functionality is required. Below are answers to basic question explaining background of the functionality and reasoning for the proposed implementation:

1. Why do we need a coherency enable/disable switch for memory that is shared between CPU and GEN (GPU)?

Memory coherency between CPU and GEN, while being a great feature that enables CL_MEM_SVM_FINE_GRAIN_BUFFER OCL capability on Intel GEN architecture, adds overhead related to tracking (snooping) memory inside different cache units (L1$, L2$, L3$, LLC$, etc.). At the same time, minority of modern OCL applications actually use CL_MEM_SVM_FINE_GRAIN_BUFFER (and hence require memory coherency between CPU and GPU). The goal of coherency enable/disable switch is to remove overhead of memory coherency when memory coherency is not needed.

2. Why do we need a global coherency switch?

In order to support I/O commands from within EUs (Execution Units), Intel GEN ISA (GEN Instruction Set Assembly) contains dedicated "send" instructions. These send instructions provide several addressing models. One of these addressing models (named "stateless") provides most flexible I/O using plain virtual addresses (as opposed to buffer_handle+offset models). This "stateless" model is similar to regular memory load/store operations available on typical CPUs. Since this model provides I/O using arbitrary virtual addresses, it enables algorithmic designs that are based on pointer-to-pointer (e.g. buffer of pointers) concepts. For instance, it allows creating tree-like data structures such as:

                    ________________
                   |      NODE1     |
                   | uint64_t data  |
                   +----------------|
                   | NODE*  |  NODE*|
                   +--------+-------+
                     /              \
    ________________/                \________________
   |      NODE2     |                |      NODE3     |
   | uint64_t data  |                | uint64_t data  |
   +----------------|                +----------------|
   | NODE*  |  NODE*|                | NODE*  |  NODE*|
   +--------+-------+                +--------+-------+

Please note that pointers inside such structures can point to memory locations in different OCL allocations - e.g. NODE1 and NODE2 can reside in one OCL allocation while NODE3 resides in a completely separate OCL allocation. Additionally, such pointers can be shared with CPU (i.e. using SVM - Shared Virtual Memory feature). Using pointers from different allocations doesn't affect the stateless addressing model which even allows scattered reading from different allocations at the same time (i.e. by utilizing SIMD-nature of send instructions).

When it comes to coherency programming, send instructions in stateless model can be encoded (at ISA level) to either use or disable coherency. However, for generic OCL applications (such as example with tree-like data structure), OCL compiler is not able to determine origin of memory pointed to by an arbitrary pointer - i.e. is not able to track given pointer back to a specific allocation. As such, it's not able to decide whether coherency is needed or not for specific pointer (or for specific I/O instruction). As a result, compiler encodes all stateless sends as coherent (doing otherwise would lead to functional issues resulting from data corruption). Please note that it would be possible to workaround this (e.g. based on allocations map and pointer bounds checking prior to each I/O instruction) but the performance cost of such workaround would be many times greater than the cost of keeping coherency always enabled. As such, enabling/disabling memory coherency at GEN ISA level is not feasible and alternative method is needed.

Such alternative solution is to have a global coherency switch that allows disabling coherency for single (though entire) GPU submission. This is beneficial because this way we:
* can enable (and pay for) coherency only in submissions that actually need coherency (submissions that use CL_MEM_SVM_FINE_GRAIN_BUFFER resources)
* don't care about coherency at GEN ISA granularity (no performance impact)

3. Will coherency switch be used frequently?

There are scenarios that will require frequent toggling of the coherency switch.
E.g. an application has two OCL compute kernels: kern_master and kern_worker. kern_master uses, concurrently with CPU, some fine grain SVM resources (CL_MEM_SVM_FINE_GRAIN_BUFFER). These resources contain descriptors of computational work that needs to be executed. kern_master analyzes incoming work descriptors and populates a plain OCL buffer (non-fine-grain) with payload for kern_worker. Once kern_master is done, kern_worker kicks-in and processes the payload that kern_master produced. These two kernels work in a loop, one after another. Since only kern_master requires coherency, kern_worker should not be forced to pay for it. This means that we need to have the ability to toggle coherency switch on or off per each GPU submission:
(ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> (ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> ...

v2: Fixed compilation warning.
v3: Refactored the patch to add IOCTL instead of exec flag.
v4: Renamed and documented the API flag. Used strict values.
Removed redundant GEM_WARN_ON()s. Improved to coding standard. Introduced a macro for checking whether hardware supports the feature.

Cc Joonas Lahtinen <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Michal Winiarski <[email protected]>

Bspec: 11419

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h         |  1 +
  drivers/gpu/drm/i915/i915_gem_context.c | 41 +++++++++++++++++++++++++++
  drivers/gpu/drm/i915/i915_gem_context.h |  6 ++++
  drivers/gpu/drm/i915/intel_lrc.c        | 49 +++++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_lrc.h        |  4 +++
  include/uapi/drm/i915_drm.h             |  6 ++++
  6 files changed, 107 insertions(+)

drivers/gpu/drm/i915/i915_drv.h +0 -0

index 09ab124..7d4bbd5 100644

drivers/gpu/drm/i915/i915_drv.h +0 -0

  #define HWS_NEEDS_PHYSICAL(dev_priv)	((dev_priv)->info.hws_needs_physical)
 

drivers/gpu/drm/i915/i915_gem_context.c +41 -0

index b10770c..6db352e 100644
@@ -711,6 +711,26 @@ static bool client_is_banned(struct drm_i915_file_private *file_priv)
  	return atomic_read(&file_priv->ban_score) >= I915_CLIENT_SCORE_BANNED;
  }
 
+static int i915_gem_context_set_data_port_coherent(struct i915_gem_context *ctx)
+{
+	int ret;
+
+	ret = intel_lr_context_modify_data_port_coherency(ctx, true);
+	if (!ret)
+		__set_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+	return ret;
+}
+
+static int i915_gem_context_clear_data_port_coherent(struct i915_gem_context *ctx)
+{
+	int ret;
+
+	ret = intel_lr_context_modify_data_port_coherency(ctx, false);
+	if (!ret)
+		__clear_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+	return ret;
+}
+
  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
  				  struct drm_file *file)
  {

@@ -784,6 +804,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
  int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
  				    struct drm_file *file)
  {
+	struct drm_i915_private *dev_priv = to_i915(dev);
  	struct drm_i915_file_private *file_priv = file->driver_priv;
  	struct drm_i915_gem_context_param *args = data;
  	struct i915_gem_context *ctx;

@@ -818,6 +839,12 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
  	case I915_CONTEXT_PARAM_PRIORITY:
  		args->value = ctx->sched.priority;
  		break;
+	case I915_CONTEXT_PARAM_DATA_PORT_COHERENCY:
+		if (!HAS_DATA_PORT_COHERENCY(dev_priv))
+			ret = -ENODEV;
+		else
+			args->value = i915_gem_context_is_data_port_coherent(ctx);
+		break;
  	default:
  		ret = -EINVAL;
  		break;

@@ -830,6 +857,7 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
  int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
  				    struct drm_file *file)
  {
+	struct drm_i915_private *dev_priv = to_i915(dev);
  	struct drm_i915_file_private *file_priv = file->driver_priv;
  	struct drm_i915_gem_context_param *args = data;
  	struct i915_gem_context *ctx;

@@ -893,6 +921,19 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
  		}
  		break;
 
+	case I915_CONTEXT_PARAM_DATA_PORT_COHERENCY:
+		if (args->size)
+			ret = -EINVAL;
+		else if (!HAS_DATA_PORT_COHERENCY(dev_priv))
+			ret = -ENODEV;
+		else if (args->value == 1)
+			ret = i915_gem_context_set_data_port_coherent(ctx);
+		else if (args->value == 0)
+			ret = i915_gem_context_clear_data_port_coherent(ctx);
+		else
+			ret = -EINVAL;
+		break;
+
  	default:
  		ret = -EINVAL;
  		break;

drivers/gpu/drm/i915/i915_gem_context.h +6 -0

index b116e49..e8ccb70 100644
@@ -126,6 +126,7 @@ struct i915_gem_context {
  #define CONTEXT_BANNABLE		3
  #define CONTEXT_BANNED			4
  #define CONTEXT_FORCE_SINGLE_SUBMISSION	5
+#define CONTEXT_DATA_PORT_COHERENT	6
 
  	/**
  	 * @hw_id: - unique identifier for the context

@@ -257,6 +258,11 @@ static inline void i915_gem_context_set_force_single_submission(struct i915_gem_
  	__set_bit(CONTEXT_FORCE_SINGLE_SUBMISSION, &ctx->flags);
  }
 
+static inline bool i915_gem_context_is_data_port_coherent(struct i915_gem_context *ctx)
+{
+	return test_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+}
+
  static inline bool i915_gem_context_is_default(const struct i915_gem_context *c)
  {
  	return c->user_handle == DEFAULT_CONTEXT_HANDLE;

drivers/gpu/drm/i915/intel_lrc.c +49 -0

index ab89dab..1f037e3 100644
@@ -259,6 +259,55 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx,
  	ce->lrc_desc = desc;
  }
 
+static int emit_set_data_port_coherency(struct i915_request *req, bool enable)
+{
+	u32 *cs;
+	i915_reg_t reg;
+
+	GEM_BUG_ON(req->engine->class != RENDER_CLASS);
+	GEM_BUG_ON(INTEL_GEN(req->i915) < 9);
+
+	cs = intel_ring_begin(req, 4);
+	if (IS_ERR(cs))
+		return PTR_ERR(cs);
+
+	if (INTEL_GEN(req->i915) >= 10)
+		reg = CNL_HDC_CHICKEN0;
+	else
+		reg = HDC_CHICKEN0;
+
+	*cs++ = MI_LOAD_REGISTER_IMM(1);
+	*cs++ = i915_mmio_reg_offset(reg);
+	/* Enabling coherency means disabling the bit which forces it off */
+	if (enable)
+		*cs++ = _MASKED_BIT_DISABLE(HDC_FORCE_NON_COHERENT);
+	else
+		*cs++ = _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT);
+	*cs++ = MI_NOOP;
+
+	intel_ring_advance(req, cs);
+
+	return 0;
+}
+
+int
+intel_lr_context_modify_data_port_coherency(struct i915_gem_context *ctx,
+					    bool enable)
+{
+	struct i915_request *req;
+	int ret;
+
+	req = i915_request_alloc(ctx->i915->engine[RCS], ctx);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
+
+	ret = emit_set_data_port_coherency(req, enable);
+
+	i915_request_add(req);
+
+	return ret;
+}
+
  static struct i915_priolist *
  lookup_priolist(struct intel_engine_cs *engine, int prio)
  {

drivers/gpu/drm/i915/intel_lrc.h +4 -0

index 1593194..f6965ae 100644
@@ -104,4 +104,8 @@ struct i915_gem_context;
 
  void intel_lr_context_resume(struct drm_i915_private *dev_priv);
 
+int
+intel_lr_context_modify_data_port_coherency(struct i915_gem_context *ctx,
+					    bool enable);
+
  #endif /* _INTEL_LRC_H_ */

include/uapi/drm/i915_drm.h +0 -0

index 7f5634c..e677bea 100644
@@ -1456,6 +1456,12 @@ struct drm_i915_gem_context_param {
  #define   I915_CONTEXT_MAX_USER_PRIORITY	1023 /* inclusive */
  #define   I915_CONTEXT_DEFAULT_PRIORITY		0

Quoted 125 lines · Tomasz Lis

The patch adds a parameter to control the data port coherency functionality on a per-context level. When the IOCTL is called, a command to switch data port coherency state is added to the ordered list. All prior requests are executed on old coherency settings, and all exec requests after the IOCTL will use new settings.

Rationale:

The OpenCL driver develpers requested a functionality to control cache coherency at data port level. Keeping the coherency at that level is disabled by default due to its performance costs. OpenCL driver is planning to enable it for a small subset of submissions, when such functionality is required. Below are answers to basic question explaining background of the functionality and reasoning for the proposed implementation:

1. Why do we need a coherency enable/disable switch for memory that is shared between CPU and GEN (GPU)?

Memory coherency between CPU and GEN, while being a great feature that enables CL_MEM_SVM_FINE_GRAIN_BUFFER OCL capability on Intel GEN architecture, adds overhead related to tracking (snooping) memory inside different cache units (L1$, L2$, L3$, LLC$, etc.). At the same time, minority of modern OCL applications actually use CL_MEM_SVM_FINE_GRAIN_BUFFER (and hence require memory coherency between CPU and GPU). The goal of coherency enable/disable switch is to remove overhead of memory coherency when memory coherency is not needed.

2. Why do we need a global coherency switch?

In order to support I/O commands from within EUs (Execution Units), Intel GEN ISA (GEN Instruction Set Assembly) contains dedicated "send" instructions. These send instructions provide several addressing models. One of these addressing models (named "stateless") provides most flexible I/O using plain virtual addresses (as opposed to buffer_handle+offset models). This "stateless" model is similar to regular memory load/store operations available on typical CPUs. Since this model provides I/O using arbitrary virtual addresses, it enables algorithmic designs that are based on pointer-to-pointer (e.g. buffer of pointers) concepts. For instance, it allows creating tree-like data structures such as:
                    ________________

                   |      NODE1     |
                   | uint64_t data  |
                   +----------------|
                   | NODE*  |  NODE*|
                   +--------+-------+
                     /              \
    ________________/                \________________
   |      NODE2     |                |      NODE3     |
   | uint64_t data  |                | uint64_t data  |
   +----------------|                +----------------|
   | NODE*  |  NODE*|                | NODE*  |  NODE*|
   +--------+-------+                +--------+-------+

Please note that pointers inside such structures can point to memory locations in different OCL allocations  - e.g. NODE1 and NODE2 can reside in one OCL allocation while NODE3 resides in a completely separate OCL allocation. Additionally, such pointers can be shared with CPU (i.e. using SVM - Shared Virtual Memory feature). Using pointers from different allocations doesn't affect the stateless addressing model which even allows scattered reading from different allocations at the same time (i.e. by utilizing SIMD-nature of send instructions).

When it comes to coherency programming, send instructions in stateless model can be encoded (at ISA level) to either use or disable coherency. However, for generic OCL applications (such as example with tree-like data structure), OCL compiler is not able to determine origin of memory pointed to by an arbitrary pointer - i.e. is not able to track given pointer back to a specific allocation. As such, it's not able to decide whether coherency is needed or not for specific pointer (or for specific I/O instruction). As a result, compiler encodes all stateless sends as coherent (doing otherwise would lead to functional issues resulting from data corruption). Please note that it would be possible to workaround this (e.g. based on allocations map and pointer bounds checking prior to each I/O instruction) but the performance cost of such workaround would be many times greater than the cost of keeping coherency always enabled. As such, enabling/disabling memory coherency at GEN ISA level is not feasible and alternative method is needed.

Such alternative solution is to have a global coherency switch that allows disabling coherency for single (though entire) GPU submission. This is beneficial because this way we:
* can enable (and pay for) coherency only in submissions that actually need coherency (submissions that use CL_MEM_SVM_FINE_GRAIN_BUFFER resources)
* don't care about coherency at GEN ISA granularity (no performance impact)

3. Will coherency switch be used frequently?

There are scenarios that will require frequent toggling of the coherency switch.
E.g. an application has two OCL compute kernels: kern_master and kern_worker. kern_master uses, concurrently with CPU, some fine grain SVM resources (CL_MEM_SVM_FINE_GRAIN_BUFFER). These resources contain descriptors of computational work that needs to be executed. kern_master analyzes incoming work descriptors and populates a plain OCL buffer (non-fine-grain) with payload for kern_worker. Once kern_master is done, kern_worker kicks-in and processes the payload that kern_master produced. These two kernels work in a loop, one after another. Since only kern_master requires coherency, kern_worker should not be forced to pay for it. This means that we need to have the ability to toggle coherency switch on or off per each GPU submission:
(ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> (ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> ...

v2: Fixed compilation warning.
v3: Refactored the patch to add IOCTL instead of exec flag.
v4: Renamed and documented the API flag. Used strict values. Removed redundant GEM_WARN_ON()s. Improved to coding standard. Introduced a macro for checking whether hardware supports the feature.

Cc Joonas Lahtinen <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Michal Winiarski <[email protected]>

Bspec: 11419

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h         |  1 + drivers/gpu/drm/i915/i915_gem_context.c | 41
+++++++++++++++++++++++++++
  drivers/gpu/drm/i915/i915_gem_context.h |  6 ++++ drivers/gpu/drm/i915/intel_lrc.c        | 49
+++++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_lrc.h        |  4 +++ include/uapi/drm/i915_drm.h             |  6 ++++

  6 files changed, 107 insertions(+)

drivers/gpu/drm/i915/i915_drv.h intel_info(const struct drm_i915_private · line 2528

Quoted 10 lines

drivers/gpu/drm/i915/i915_drv.h +1 -0

index 09ab124..7d4bbd5 100644
@@ -2524,6 +2524,7 @@ intel_info(const struct drm_i915_private 
 *dev_priv)
   #define HAS_EDRAM(dev_priv)    (!!((dev_priv)->edram_cap & EDRAM_ENABLED))
   #define HAS_WT(dev_priv)    ((IS_HASWELL(dev_priv) || \
                    IS_BROADWELL(dev_priv)) && HAS_EDRAM(dev_priv))
+#define HAS_DATA_PORT_COHERENCY(dev_priv) (INTEL_GEN(dev_priv) >= 9)

Reading the documentation it seems that the bit you want to set is gone in ICL/Gen11. Maybe limit this to >= 9 && < 11?

Cheers,

-
Lionel

Quoted 193 lines

drivers/gpu/drm/i915/i915_drv.h +0 -0

     #define HWS_NEEDS_PHYSICAL(dev_priv) ((dev_priv)->info.hws_needs_physical)

  diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index b10770c..6db352e 100644

drivers/gpu/drm/i915/i915_gem_context.c +19 -0

@@ -711,6 +711,26 @@ static bool client_is_banned(struct 
 drm_i915_file_private *file_priv)
       return atomic_read(&file_priv->ban_score) >= I915_CLIENT_SCORE_BANNED;
   }
   +static int i915_gem_context_set_data_port_coherent(struct i915_gem_context *ctx)
+{
+    int ret;
+
+    ret = intel_lr_context_modify_data_port_coherency(ctx, true);
+    if (!ret)
+        __set_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+    return ret;
+}
+
+static int i915_gem_context_clear_data_port_coherent(struct 
 i915_gem_context *ctx)
+{
+    int ret;
+
+    ret = intel_lr_context_modify_data_port_coherency(ctx, false);
+    if (!ret)
+        __clear_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+    return ret;
+}
+
   int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,

                    struct drm_file *file)
  {

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

@@ -784,6 +804,7 @@ int i915_gem_context_destroy_ioctl(struct 
 drm_device *dev, void *data,
   int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
                       struct drm_file *file)
   {
+    struct drm_i915_private *dev_priv = to_i915(dev);
       struct drm_i915_file_private *file_priv = file->driver_priv;
       struct drm_i915_gem_context_param *args = data;

      struct i915_gem_context *ctx;

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

@@ -818,6 +839,12 @@ int i915_gem_context_getparam_ioctl(struct 
 drm_device *dev, void *data,

      case I915_CONTEXT_PARAM_PRIORITY:
          args->value = ctx->sched.priority;
          break;
+    case I915_CONTEXT_PARAM_DATA_PORT_COHERENCY:
+        if (!HAS_DATA_PORT_COHERENCY(dev_priv))
+            ret = -ENODEV;
+        else
+            args->value = i915_gem_context_is_data_port_coherent(ctx); +        break;
      default:
          ret = -EINVAL;
          break;

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

@@ -830,6 +857,7 @@ int i915_gem_context_getparam_ioctl(struct 
 drm_device *dev, void *data,
   int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
                       struct drm_file *file)
   {
+    struct drm_i915_private *dev_priv = to_i915(dev);
       struct drm_i915_file_private *file_priv = file->driver_priv;
       struct drm_i915_gem_context_param *args = data;

      struct i915_gem_context *ctx;

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

@@ -893,6 +921,19 @@ int i915_gem_context_setparam_ioctl(struct 
 drm_device *dev, void *data,
           }
           break;

  +    case I915_CONTEXT_PARAM_DATA_PORT_COHERENCY:
+        if (args->size)
+            ret = -EINVAL;
+        else if (!HAS_DATA_PORT_COHERENCY(dev_priv))
+            ret = -ENODEV;
+        else if (args->value == 1)
+            ret = i915_gem_context_set_data_port_coherent(ctx); +        else if (args->value == 0)
+            ret = i915_gem_context_clear_data_port_coherent(ctx); +        else
+            ret = -EINVAL;
+        break;
+
      default:
          ret = -EINVAL;
          break;

drivers/gpu/drm/i915/i915_gem_context.h +1 -0

index b116e49..e8ccb70 100644
@@ -126,6 +126,7 @@ struct i915_gem_context {
   #define CONTEXT_BANNABLE        3
   #define CONTEXT_BANNED            4
   #define CONTEXT_FORCE_SINGLE_SUBMISSION    5
+#define CONTEXT_DATA_PORT_COHERENT    6
         /**
        * @hw_id: - unique identifier for the context

@@ -257,6 +258,11 @@ static inline void 

i915_gem_context_set_force_single_submission(struct i915_gem_ __set_bit(CONTEXT_FORCE_SINGLE_SUBMISSION, &ctx->flags);
  }
  +static inline bool i915_gem_context_is_data_port_coherent(struct i915_gem_context *ctx)

drivers/gpu/drm/i915/i915_gem_context.h +4 -0

+{
+    return test_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+}
+

  static inline bool i915_gem_context_is_default(const struct i915_gem_context *c) {
      return c->user_handle == DEFAULT_CONTEXT_HANDLE;

drivers/gpu/drm/i915/intel_lrc.c +19 -0

index ab89dab..1f037e3 100644
@@ -259,6 +259,55 @@ intel_lr_context_descriptor_update(struct 
 i915_gem_context *ctx,
       ce->lrc_desc = desc;
   }
   +static int emit_set_data_port_coherency(struct i915_request *req, bool enable)
+{
+    u32 *cs;
+    i915_reg_t reg;
+
+    GEM_BUG_ON(req->engine->class != RENDER_CLASS);
+    GEM_BUG_ON(INTEL_GEN(req->i915) < 9);
+
+    cs = intel_ring_begin(req, 4);
+    if (IS_ERR(cs))
+        return PTR_ERR(cs);
+
+    if (INTEL_GEN(req->i915) >= 10)
+        reg = CNL_HDC_CHICKEN0;
+    else
+        reg = HDC_CHICKEN0;
+
+    *cs++ = MI_LOAD_REGISTER_IMM(1);
+    *cs++ = i915_mmio_reg_offset(reg);
+    /* Enabling coherency means disabling the bit which forces it 

off */
+    if (enable)
+        *cs++ = _MASKED_BIT_DISABLE(HDC_FORCE_NON_COHERENT); +    else
+        *cs++ = _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT); +    *cs++ = MI_NOOP;

drivers/gpu/drm/i915/intel_lrc.c +8 -0

+
+    intel_ring_advance(req, cs);
+
+    return 0;
+}
+
+int
+intel_lr_context_modify_data_port_coherency(struct i915_gem_context 

*ctx,
+                        bool enable)

drivers/gpu/drm/i915/intel_lrc.c +15 -0

+{
+    struct i915_request *req;
+    int ret;
+
+    req = i915_request_alloc(ctx->i915->engine[RCS], ctx);
+    if (IS_ERR(req))
+        return PTR_ERR(req);
+
+    ret = emit_set_data_port_coherency(req, enable);
+
+    i915_request_add(req);
+
+    return ret;
+}
+

  static struct i915_priolist * lookup_priolist(struct intel_engine_cs *engine, int prio) {

drivers/gpu/drm/i915/intel_lrc.h +0 -0

index 1593194..f6965ae 100644
@@ -104,4 +104,8 @@ struct i915_gem_context;
     void intel_lr_context_resume(struct drm_i915_private *dev_priv);

  +int +intel_lr_context_modify_data_port_coherency(struct i915_gem_context *ctx,
+                        bool enable);
+
  #endif /* _INTEL_LRC_H_ */

include/uapi/drm/i915_drm.h +2 -0

index 7f5634c..e677bea 100644
@@ -1456,6 +1456,12 @@ struct drm_i915_gem_context_param {
   #define   I915_CONTEXT_MAX_USER_PRIORITY    1023 /* inclusive */
   #define   I915_CONTEXT_DEFAULT_PRIORITY        0
   #define   I915_CONTEXT_MIN_USER_PRIORITY    -1023 /* inclusive */
+/*
+ * When data port level coherency is enabled, the GPU will update 

memory

Quoted 8 lines

+ * buffers shared with CPU, by forcing internal cache units to send memory
+ * writes to real RAM faster. Keeping such coherency has performance cost.
+ */
+#define I915_CONTEXT_PARAM_DATA_PORT_COHERENCY    0x7 __u64 value;
  };

Quoted 126 lines · Lionel Landwerlin

Quoted 125 lines · Tomasz Lis

The patch adds a parameter to control the data port coherency functionality on a per-context level. When the IOCTL is called, a command to switch data port coherency state is added to the ordered list. All prior requests are executed on old coherency settings, and all exec requests after the IOCTL will use new settings.

Rationale:

The OpenCL driver develpers requested a functionality to control cache coherency at data port level. Keeping the coherency at that level is disabled by default due to its performance costs. OpenCL driver is planning to enable it for a small subset of submissions, when such functionality is required. Below are answers to basic question explaining background of the functionality and reasoning for the proposed implementation:

1. Why do we need a coherency enable/disable switch for memory that is shared between CPU and GEN (GPU)?

Memory coherency between CPU and GEN, while being a great feature that enables CL_MEM_SVM_FINE_GRAIN_BUFFER OCL capability on Intel GEN architecture, adds overhead related to tracking (snooping) memory inside different cache units (L1$, L2$, L3$, LLC$, etc.). At the same time, minority of modern OCL applications actually use CL_MEM_SVM_FINE_GRAIN_BUFFER (and hence require memory coherency between CPU and GPU). The goal of coherency enable/disable switch is to remove overhead of memory coherency when memory coherency is not needed.

2. Why do we need a global coherency switch?

In order to support I/O commands from within EUs (Execution Units), Intel GEN ISA (GEN Instruction Set Assembly) contains dedicated "send" instructions. These send instructions provide several addressing models. One of these addressing models (named "stateless") provides most flexible I/O using plain virtual addresses (as opposed to buffer_handle+offset models). This "stateless" model is similar to regular memory load/store operations available on typical CPUs. Since this model provides I/O using arbitrary virtual addresses, it enables algorithmic designs that are based on pointer-to-pointer (e.g. buffer of pointers) concepts. For instance, it allows creating tree-like data structures such as:
                    ________________

                   |      NODE1     |
                   | uint64_t data  |
                   +----------------|
                   | NODE*  |  NODE*|
                   +--------+-------+
                     /              \
    ________________/                \________________
   |      NODE2     |                |      NODE3     |
   | uint64_t data  |                | uint64_t data  |
   +----------------|                +----------------|
   | NODE*  |  NODE*|                | NODE*  |  NODE*|
   +--------+-------+                +--------+-------+

Please note that pointers inside such structures can point to memory locations in different OCL allocations  - e.g. NODE1 and NODE2 can reside in one OCL allocation while NODE3 resides in a completely separate OCL allocation. Additionally, such pointers can be shared with CPU (i.e. using SVM - Shared Virtual Memory feature). Using pointers from different allocations doesn't affect the stateless addressing model which even allows scattered reading from different allocations at the same time (i.e. by utilizing SIMD-nature of send instructions).

When it comes to coherency programming, send instructions in stateless model can be encoded (at ISA level) to either use or disable coherency. However, for generic OCL applications (such as example with tree-like data structure), OCL compiler is not able to determine origin of memory pointed to by an arbitrary pointer - i.e. is not able to track given pointer back to a specific allocation. As such, it's not able to decide whether coherency is needed or not for specific pointer (or for specific I/O instruction). As a result, compiler encodes all stateless sends as coherent (doing otherwise would lead to functional issues resulting from data corruption). Please note that it would be possible to workaround this (e.g. based on allocations map and pointer bounds checking prior to each I/O instruction) but the performance cost of such workaround would be many times greater than the cost of keeping coherency always enabled. As such, enabling/disabling memory coherency at GEN ISA level is not feasible and alternative method is needed.

Such alternative solution is to have a global coherency switch that allows disabling coherency for single (though entire) GPU submission. This is beneficial because this way we:
* can enable (and pay for) coherency only in submissions that actually need coherency (submissions that use CL_MEM_SVM_FINE_GRAIN_BUFFER resources)
* don't care about coherency at GEN ISA granularity (no performance impact)

3. Will coherency switch be used frequently?

There are scenarios that will require frequent toggling of the coherency switch.
E.g. an application has two OCL compute kernels: kern_master and kern_worker. kern_master uses, concurrently with CPU, some fine grain SVM resources (CL_MEM_SVM_FINE_GRAIN_BUFFER). These resources contain descriptors of computational work that needs to be executed. kern_master analyzes incoming work descriptors and populates a plain OCL buffer (non-fine-grain) with payload for kern_worker. Once kern_master is done, kern_worker kicks-in and processes the payload that kern_master produced. These two kernels work in a loop, one after another. Since only kern_master requires coherency, kern_worker should not be forced to pay for it. This means that we need to have the ability to toggle coherency switch on or off per each GPU submission:
(ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> (ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> ...

v2: Fixed compilation warning.
v3: Refactored the patch to add IOCTL instead of exec flag.
v4: Renamed and documented the API flag. Used strict values. Removed redundant GEM_WARN_ON()s. Improved to coding standard. Introduced a macro for checking whether hardware supports the feature.

Cc Joonas Lahtinen <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Michal Winiarski <[email protected]>

Bspec: 11419

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h         |  1 + drivers/gpu/drm/i915/i915_gem_context.c | 41
+++++++++++++++++++++++++++
  drivers/gpu/drm/i915/i915_gem_context.h |  6 ++++ drivers/gpu/drm/i915/intel_lrc.c        | 49
+++++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_lrc.h        |  4 +++ include/uapi/drm/i915_drm.h             |  6 ++++

  6 files changed, 107 insertions(+)

Quoted 13 lines

drivers/gpu/drm/i915/i915_drv.h intel_info(const struct drm_i915_private · line 2528

Quoted 10 lines

drivers/gpu/drm/i915/i915_drv.h +1 -0

index 09ab124..7d4bbd5 100644
@@ -2524,6 +2524,7 @@ intel_info(const struct drm_i915_private 
 *dev_priv)
   #define HAS_EDRAM(dev_priv)    (!!((dev_priv)->edram_cap & EDRAM_ENABLED))
   #define HAS_WT(dev_priv)    ((IS_HASWELL(dev_priv) || \
                    IS_BROADWELL(dev_priv)) && HAS_EDRAM(dev_priv))
+#define HAS_DATA_PORT_COHERENCY(dev_priv) (INTEL_GEN(dev_priv) >= 9)

Reading the documentation it seems that the bit you want to set is gone in ICL/Gen11. Maybe limit this to >= 9 && < 11?

Icelake actually has the bit as well, just the address is different.
I will add its support as a separate patch as soon as the change which defines ICL_HDC_CHICKEN0 is accepted. But in the current form - you are right, ICL is not supported.
I will update the condition.
-Tomasz

Quoted 201 lines

Cheers,

-
Lionel

Quoted 195 lines

drivers/gpu/drm/i915/i915_drv.h +0 -0

     #define HWS_NEEDS_PHYSICAL(dev_priv) ((dev_priv)->info.hws_needs_physical)

  diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index b10770c..6db352e 100644

drivers/gpu/drm/i915/i915_gem_context.c +19 -0

@@ -711,6 +711,26 @@ static bool client_is_banned(struct 
 drm_i915_file_private *file_priv)
       return atomic_read(&file_priv->ban_score) >= I915_CLIENT_SCORE_BANNED;
   }
   +static int i915_gem_context_set_data_port_coherent(struct i915_gem_context *ctx)
+{
+    int ret;
+
+    ret = intel_lr_context_modify_data_port_coherency(ctx, true);
+    if (!ret)
+        __set_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+    return ret;
+}
+
+static int i915_gem_context_clear_data_port_coherent(struct 
 i915_gem_context *ctx)
+{
+    int ret;
+
+    ret = intel_lr_context_modify_data_port_coherency(ctx, false);
+    if (!ret)
+        __clear_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+    return ret;
+}
+
   int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,

                    struct drm_file *file)
  {

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

@@ -784,6 +804,7 @@ int i915_gem_context_destroy_ioctl(struct 
 drm_device *dev, void *data,
   int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
                       struct drm_file *file)
   {
+    struct drm_i915_private *dev_priv = to_i915(dev);
       struct drm_i915_file_private *file_priv = file->driver_priv;
       struct drm_i915_gem_context_param *args = data;

      struct i915_gem_context *ctx;

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

@@ -818,6 +839,12 @@ int i915_gem_context_getparam_ioctl(struct 
 drm_device *dev, void *data,

      case I915_CONTEXT_PARAM_PRIORITY:
          args->value = ctx->sched.priority;
          break;
+    case I915_CONTEXT_PARAM_DATA_PORT_COHERENCY:
+        if (!HAS_DATA_PORT_COHERENCY(dev_priv))
+            ret = -ENODEV;
+        else
+            args->value = i915_gem_context_is_data_port_coherent(ctx); +        break;
      default:
          ret = -EINVAL;
          break;

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

@@ -830,6 +857,7 @@ int i915_gem_context_getparam_ioctl(struct 
 drm_device *dev, void *data,
   int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
                       struct drm_file *file)
   {
+    struct drm_i915_private *dev_priv = to_i915(dev);
       struct drm_i915_file_private *file_priv = file->driver_priv;
       struct drm_i915_gem_context_param *args = data;

      struct i915_gem_context *ctx;

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

@@ -893,6 +921,19 @@ int i915_gem_context_setparam_ioctl(struct 
 drm_device *dev, void *data,
           }
           break;

  +    case I915_CONTEXT_PARAM_DATA_PORT_COHERENCY:
+        if (args->size)
+            ret = -EINVAL;
+        else if (!HAS_DATA_PORT_COHERENCY(dev_priv))
+            ret = -ENODEV;
+        else if (args->value == 1)
+            ret = i915_gem_context_set_data_port_coherent(ctx); +        else if (args->value == 0)
+            ret = i915_gem_context_clear_data_port_coherent(ctx); +        else
+            ret = -EINVAL;
+        break;
+
      default:
          ret = -EINVAL;
          break;

drivers/gpu/drm/i915/i915_gem_context.h +1 -0

index b116e49..e8ccb70 100644
@@ -126,6 +126,7 @@ struct i915_gem_context {
   #define CONTEXT_BANNABLE        3
   #define CONTEXT_BANNED            4
   #define CONTEXT_FORCE_SINGLE_SUBMISSION    5
+#define CONTEXT_DATA_PORT_COHERENT    6
         /**
        * @hw_id: - unique identifier for the context

@@ -257,6 +258,11 @@ static inline void 

i915_gem_context_set_force_single_submission(struct i915_gem_ __set_bit(CONTEXT_FORCE_SINGLE_SUBMISSION, &ctx->flags);
  }
  +static inline bool i915_gem_context_is_data_port_coherent(struct i915_gem_context *ctx)

drivers/gpu/drm/i915/i915_gem_context.h +4 -0

+{
+    return test_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+}
+

  static inline bool i915_gem_context_is_default(const struct i915_gem_context *c) {
      return c->user_handle == DEFAULT_CONTEXT_HANDLE;

drivers/gpu/drm/i915/intel_lrc.c +19 -0

index ab89dab..1f037e3 100644
@@ -259,6 +259,55 @@ intel_lr_context_descriptor_update(struct 
 i915_gem_context *ctx,
       ce->lrc_desc = desc;
   }
   +static int emit_set_data_port_coherency(struct i915_request *req, bool enable)
+{
+    u32 *cs;
+    i915_reg_t reg;
+
+    GEM_BUG_ON(req->engine->class != RENDER_CLASS);
+    GEM_BUG_ON(INTEL_GEN(req->i915) < 9);
+
+    cs = intel_ring_begin(req, 4);
+    if (IS_ERR(cs))
+        return PTR_ERR(cs);
+
+    if (INTEL_GEN(req->i915) >= 10)
+        reg = CNL_HDC_CHICKEN0;
+    else
+        reg = HDC_CHICKEN0;
+
+    *cs++ = MI_LOAD_REGISTER_IMM(1);
+    *cs++ = i915_mmio_reg_offset(reg);
+    /* Enabling coherency means disabling the bit which forces it 

off */
+    if (enable)
+        *cs++ = _MASKED_BIT_DISABLE(HDC_FORCE_NON_COHERENT); +    else
+        *cs++ = _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT); +    *cs++ = MI_NOOP;

drivers/gpu/drm/i915/intel_lrc.c +8 -0

+
+    intel_ring_advance(req, cs);
+
+    return 0;
+}
+
+int
+intel_lr_context_modify_data_port_coherency(struct i915_gem_context 

*ctx,
+                        bool enable)

drivers/gpu/drm/i915/intel_lrc.c +15 -0

+{
+    struct i915_request *req;
+    int ret;
+
+    req = i915_request_alloc(ctx->i915->engine[RCS], ctx);
+    if (IS_ERR(req))
+        return PTR_ERR(req);
+
+    ret = emit_set_data_port_coherency(req, enable);
+
+    i915_request_add(req);
+
+    return ret;
+}
+

  static struct i915_priolist * lookup_priolist(struct intel_engine_cs *engine, int prio) {

drivers/gpu/drm/i915/intel_lrc.h +0 -0

index 1593194..f6965ae 100644
@@ -104,4 +104,8 @@ struct i915_gem_context;
     void intel_lr_context_resume(struct drm_i915_private *dev_priv);

  +int +intel_lr_context_modify_data_port_coherency(struct i915_gem_context *ctx,
+                        bool enable);
+
  #endif /* _INTEL_LRC_H_ */

include/uapi/drm/i915_drm.h +2 -0

index 7f5634c..e677bea 100644
@@ -1456,6 +1456,12 @@ struct drm_i915_gem_context_param {
   #define   I915_CONTEXT_MAX_USER_PRIORITY    1023 /* inclusive */
   #define   I915_CONTEXT_DEFAULT_PRIORITY        0
   #define   I915_CONTEXT_MIN_USER_PRIORITY    -1023 /* inclusive */
+/*
+ * When data port level coherency is enabled, the GPU will update 

memory
+ * buffers shared with CPU, by forcing internal cache units to send memory

Quoted 8 lines

Quoted 6 lines

+ * writes to real RAM faster. Keeping such coherency has performance cost.
+ */
+#define I915_CONTEXT_PARAM_DATA_PORT_COHERENCY    0x7 __u64 value;
  };

Quoted 143 lines · Tomasz

Quoted 140 lines · Lionel Landwerlin

Quoted 131 lines · Tomasz Lis

The patch adds a parameter to control the data port coherency functionality on a per-context level. When the IOCTL is called, a command to switch data port coherency state is added to the ordered list. All prior requests are executed on old coherency settings, and all exec requests after the IOCTL will use new settings.

Rationale:

The OpenCL driver develpers requested a functionality to control cache coherency at data port level. Keeping the coherency at that level is disabled by default due to its performance costs. OpenCL driver is planning to enable it for a small subset of submissions, when such functionality is required. Below are answers to basic question explaining background of the functionality and reasoning for the proposed implementation:

1. Why do we need a coherency enable/disable switch for memory that is shared between CPU and GEN (GPU)?

Memory coherency between CPU and GEN, while being a great feature that enables CL_MEM_SVM_FINE_GRAIN_BUFFER OCL capability on Intel GEN architecture, adds overhead related to tracking (snooping) memory inside different cache units (L1$, L2$, L3$, LLC$, etc.). At the same time, minority of modern OCL applications actually use CL_MEM_SVM_FINE_GRAIN_BUFFER (and hence require memory coherency between CPU and GPU). The goal of coherency enable/disable switch is to remove overhead of memory coherency when memory coherency is not needed.

2. Why do we need a global coherency switch?

In order to support I/O commands from within EUs (Execution Units), Intel GEN ISA (GEN Instruction Set Assembly) contains dedicated "send" instructions. These send instructions provide several addressing models. One of these addressing models (named "stateless") provides most flexible I/O using plain virtual addresses (as opposed to buffer_handle+offset models). This "stateless" model is similar to regular memory load/store operations available on typical CPUs. Since this model provides I/O using arbitrary virtual addresses, it enables algorithmic designs that are based on pointer-to-pointer (e.g. buffer of pointers) concepts. For instance, it allows creating tree-like data structures such as:
                    ________________

                   |      NODE1     |
                   | uint64_t data  |
                   +----------------|
                   | NODE*  |  NODE*|
                   +--------+-------+
                     /              \
    ________________/                \________________
   |      NODE2     |                |      NODE3     |
   | uint64_t data  |                | uint64_t data  |
   +----------------|                +----------------|
   | NODE*  |  NODE*|                | NODE*  |  NODE*|
   +--------+-------+                +--------+-------+

Please note that pointers inside such structures can point to memory locations in different OCL allocations  - e.g. NODE1 and NODE2 can reside in one OCL allocation while NODE3 resides in a completely separate OCL allocation. Additionally, such pointers can be shared with CPU (i.e. using SVM
- Shared
Virtual Memory feature). Using pointers from different allocations doesn't affect the stateless addressing model which even allows scattered reading from different allocations at the same time (i.e. by utilizing SIMD-nature of send instructions).

When it comes to coherency programming, send instructions in stateless model can be encoded (at ISA level) to either use or disable coherency. However, for generic OCL applications (such as example with tree-like data structure), OCL compiler is not able to determine origin of memory pointed to by an arbitrary pointer - i.e. is not able to track given pointer back to a specific allocation. As such, it's not able to decide whether coherency is needed or not for specific pointer (or for specific I/O instruction). As a result, compiler encodes all stateless sends as coherent (doing otherwise would lead to functional issues resulting from data corruption). Please note that it would be possible to workaround this (e.g. based on allocations map and pointer bounds checking prior to each I/O instruction) but the performance cost of such workaround would be many times greater than the cost of keeping coherency always enabled. As such, enabling/disabling memory coherency at GEN ISA level is not feasible and alternative method is needed.

Such alternative solution is to have a global coherency switch that allows disabling coherency for single (though entire) GPU submission. This is beneficial because this way we:
* can enable (and pay for) coherency only in submissions that actually need coherency (submissions that use CL_MEM_SVM_FINE_GRAIN_BUFFER resources)
* don't care about coherency at GEN ISA granularity (no performance impact)

3. Will coherency switch be used frequently?

There are scenarios that will require frequent toggling of the coherency switch.
E.g. an application has two OCL compute kernels: kern_master and kern_worker. kern_master uses, concurrently with CPU, some fine grain SVM resources (CL_MEM_SVM_FINE_GRAIN_BUFFER). These resources contain descriptors of computational work that needs to be executed. kern_master analyzes incoming work descriptors and populates a plain OCL buffer (non-fine-grain) with payload for kern_worker. Once kern_master is done, kern_worker kicks-in and processes the payload that kern_master produced. These two kernels work in a loop, one after another. Since only kern_master requires coherency, kern_worker should not be forced to pay for it. This means that we need to have the ability to toggle coherency switch on or off per each GPU submission:
(ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> (ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> ...

v2: Fixed compilation warning.
v3: Refactored the patch to add IOCTL instead of exec flag.
v4: Renamed and documented the API flag. Used strict values. Removed redundant GEM_WARN_ON()s. Improved to coding standard. Introduced a macro for checking whether hardware supports the feature.

Cc Joonas Lahtinen <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Michal Winiarski <[email protected]>

Bspec: 11419

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h         |  1 + drivers/gpu/drm/i915/i915_gem_context.c | 41
+++++++++++++++++++++++++++
  drivers/gpu/drm/i915/i915_gem_context.h |  6 ++++ drivers/gpu/drm/i915/intel_lrc.c        | 49
+++++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_lrc.h        |  4 +++ include/uapi/drm/i915_drm.h             |  6 ++++

  6 files changed, 107 insertions(+)

drivers/gpu/drm/i915/i915_drv.h +0 -0

index 09ab124..7d4bbd5 100644

drivers/gpu/drm/i915/i915_drv.h intel_info(const struct drm_i915_private · line 2528

Quoted 5 lines

drivers/gpu/drm/i915/i915_drv.h +1 -0

@@ -2524,6 +2524,7 @@ intel_info(const struct drm_i915_private 
 *dev_priv)
   #define HAS_EDRAM(dev_priv) (!!((dev_priv)->edram_cap & EDRAM_ENABLED))
   #define HAS_WT(dev_priv)    ((IS_HASWELL(dev_priv) || \
                    IS_BROADWELL(dev_priv)) && HAS_EDRAM(dev_priv))
+#define HAS_DATA_PORT_COHERENCY(dev_priv) (INTEL_GEN(dev_priv) >= 9)

Reading the documentation it seems that the bit you want to set is gone in ICL/Gen11. Maybe limit this to >= 9 && < 11?

Quoted 5 lines

Icelake actually has the bit as well, just the address is different.
I will add its support as a separate patch as soon as the change which defines ICL_HDC_CHICKEN0 is accepted. But in the current form - you are right, ICL is not supported.
I will update the condition.
-Tomasz

Just out of curiosity, what address is ICL_HD_CHICKEN0 at?

Thanks,

-
Lionel

Quoted 212 lines

Quoted 210 lines

Cheers,

-
Lionel

Quoted 202 lines

drivers/gpu/drm/i915/i915_drv.h +0 -0

     #define HWS_NEEDS_PHYSICAL(dev_priv) ((dev_priv)->info.hws_needs_physical)

  diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index b10770c..6db352e 100644

drivers/gpu/drm/i915/i915_gem_context.c +19 -0

@@ -711,6 +711,26 @@ static bool client_is_banned(struct 
 drm_i915_file_private *file_priv)
       return atomic_read(&file_priv->ban_score) >= I915_CLIENT_SCORE_BANNED;
   }
   +static int i915_gem_context_set_data_port_coherent(struct i915_gem_context *ctx)
+{
+    int ret;
+
+    ret = intel_lr_context_modify_data_port_coherency(ctx, true);
+    if (!ret)
+        __set_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+    return ret;
+}
+
+static int i915_gem_context_clear_data_port_coherent(struct 
 i915_gem_context *ctx)
+{
+    int ret;
+
+    ret = intel_lr_context_modify_data_port_coherency(ctx, false);
+    if (!ret)
+        __clear_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+    return ret;
+}
+
   int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,

                    struct drm_file *file)
  {

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

@@ -784,6 +804,7 @@ int i915_gem_context_destroy_ioctl(struct 
 drm_device *dev, void *data,
   int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
                       struct drm_file *file)
   {
+    struct drm_i915_private *dev_priv = to_i915(dev);
       struct drm_i915_file_private *file_priv = file->driver_priv;
       struct drm_i915_gem_context_param *args = data;

      struct i915_gem_context *ctx;

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

@@ -818,6 +839,12 @@ int i915_gem_context_getparam_ioctl(struct 
 drm_device *dev, void *data,

      case I915_CONTEXT_PARAM_PRIORITY:
          args->value = ctx->sched.priority;
          break;
+    case I915_CONTEXT_PARAM_DATA_PORT_COHERENCY:
+        if (!HAS_DATA_PORT_COHERENCY(dev_priv))
+            ret = -ENODEV;
+        else
+            args->value =
i915_gem_context_is_data_port_coherent(ctx);
+        break;
      default:
          ret = -EINVAL;
          break;

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

@@ -830,6 +857,7 @@ int i915_gem_context_getparam_ioctl(struct 
 drm_device *dev, void *data,
   int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
                       struct drm_file *file)
   {
+    struct drm_i915_private *dev_priv = to_i915(dev);
       struct drm_i915_file_private *file_priv = file->driver_priv;
       struct drm_i915_gem_context_param *args = data;

      struct i915_gem_context *ctx;

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

@@ -893,6 +921,19 @@ int i915_gem_context_setparam_ioctl(struct 
 drm_device *dev, void *data,
           }
           break;

  +    case I915_CONTEXT_PARAM_DATA_PORT_COHERENCY:
+        if (args->size)
+            ret = -EINVAL;
+        else if (!HAS_DATA_PORT_COHERENCY(dev_priv))
+            ret = -ENODEV;
+        else if (args->value == 1)
+            ret = i915_gem_context_set_data_port_coherent(ctx); +        else if (args->value == 0)
+            ret = i915_gem_context_clear_data_port_coherent(ctx); +        else
+            ret = -EINVAL;
+        break;
+
      default:
          ret = -EINVAL;
          break;

drivers/gpu/drm/i915/i915_gem_context.h +1 -0

index b116e49..e8ccb70 100644
@@ -126,6 +126,7 @@ struct i915_gem_context {
   #define CONTEXT_BANNABLE        3
   #define CONTEXT_BANNED            4
   #define CONTEXT_FORCE_SINGLE_SUBMISSION    5
+#define CONTEXT_DATA_PORT_COHERENT    6
         /**
        * @hw_id: - unique identifier for the context

@@ -257,6 +258,11 @@ static inline void 

i915_gem_context_set_force_single_submission(struct i915_gem_ __set_bit(CONTEXT_FORCE_SINGLE_SUBMISSION, &ctx->flags);
  }
  +static inline bool i915_gem_context_is_data_port_coherent(struct i915_gem_context *ctx)

drivers/gpu/drm/i915/i915_gem_context.h +4 -0

+{
+    return test_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+}
+

  static inline bool i915_gem_context_is_default(const struct i915_gem_context *c) {
      return c->user_handle == DEFAULT_CONTEXT_HANDLE;

drivers/gpu/drm/i915/intel_lrc.c +19 -0

index ab89dab..1f037e3 100644
@@ -259,6 +259,55 @@ intel_lr_context_descriptor_update(struct 
 i915_gem_context *ctx,
       ce->lrc_desc = desc;
   }
   +static int emit_set_data_port_coherency(struct i915_request *req, bool enable)
+{
+    u32 *cs;
+    i915_reg_t reg;
+
+    GEM_BUG_ON(req->engine->class != RENDER_CLASS);
+    GEM_BUG_ON(INTEL_GEN(req->i915) < 9);
+
+    cs = intel_ring_begin(req, 4);
+    if (IS_ERR(cs))
+        return PTR_ERR(cs);
+
+    if (INTEL_GEN(req->i915) >= 10)
+        reg = CNL_HDC_CHICKEN0;
+    else
+        reg = HDC_CHICKEN0;
+
+    *cs++ = MI_LOAD_REGISTER_IMM(1);
+    *cs++ = i915_mmio_reg_offset(reg);
+    /* Enabling coherency means disabling the bit which forces it 

off */
+    if (enable)
+        *cs++ = _MASKED_BIT_DISABLE(HDC_FORCE_NON_COHERENT); +    else
+        *cs++ = _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT); +    *cs++ = MI_NOOP;

drivers/gpu/drm/i915/intel_lrc.c +8 -0

+
+    intel_ring_advance(req, cs);
+
+    return 0;
+}
+
+int
+intel_lr_context_modify_data_port_coherency(struct 

i915_gem_context *ctx,
+                        bool enable)

drivers/gpu/drm/i915/intel_lrc.c +15 -0

+{
+    struct i915_request *req;
+    int ret;
+
+    req = i915_request_alloc(ctx->i915->engine[RCS], ctx);
+    if (IS_ERR(req))
+        return PTR_ERR(req);
+
+    ret = emit_set_data_port_coherency(req, enable);
+
+    i915_request_add(req);
+
+    return ret;
+}
+

  static struct i915_priolist * lookup_priolist(struct intel_engine_cs *engine, int prio) {

drivers/gpu/drm/i915/intel_lrc.h +0 -0

index 1593194..f6965ae 100644
@@ -104,4 +104,8 @@ struct i915_gem_context;
     void intel_lr_context_resume(struct drm_i915_private *dev_priv);

  +int +intel_lr_context_modify_data_port_coherency(struct i915_gem_context *ctx,
+                        bool enable);
+
  #endif /* _INTEL_LRC_H_ */

include/uapi/drm/i915_drm.h +2 -0

index 7f5634c..e677bea 100644
@@ -1456,6 +1456,12 @@ struct drm_i915_gem_context_param {
   #define   I915_CONTEXT_MAX_USER_PRIORITY    1023 /* inclusive */
   #define   I915_CONTEXT_DEFAULT_PRIORITY        0
   #define   I915_CONTEXT_MIN_USER_PRIORITY    -1023 /* inclusive */
+/*
+ * When data port level coherency is enabled, the GPU will update 

memory
+ * buffers shared with CPU, by forcing internal cache units to send memory
+ * writes to real RAM faster. Keeping such coherency has performance cost.
+ */
+#define I915_CONTEXT_PARAM_DATA_PORT_COHERENCY    0x7 __u64 value;
  };

The patch adds a parameter to control the data port coherency functionality on a per-context level. When the IOCTL is called, a command to switch data port coherency state is added to the ordered list. All prior requests are executed on old coherency settings, and all exec requests after the IOCTL will use new settings.

Rationale:

The OpenCL driver develpers requested a functionality to control cache coherency at data port level. Keeping the coherency at that level is disabled by default due to its performance costs. OpenCL driver is planning to enable it for a small subset of submissions, when such functionality is required. Below are answers to basic question explaining background of the functionality and reasoning for the proposed implementation:

1. Why do we need a coherency enable/disable switch for memory that is shared between CPU and GEN (GPU)?

Memory coherency between CPU and GEN, while being a great feature that enables CL_MEM_SVM_FINE_GRAIN_BUFFER OCL capability on Intel GEN architecture, adds overhead related to tracking (snooping) memory inside different cache units (L1$, L2$, L3$, LLC$, etc.). At the same time, minority of modern OCL applications actually use CL_MEM_SVM_FINE_GRAIN_BUFFER (and hence require memory coherency between CPU and GPU). The goal of coherency enable/disable switch is to remove overhead of memory coherency when memory coherency is not needed.

2. Why do we need a global coherency switch?

In order to support I/O commands from within EUs (Execution Units), Intel GEN ISA (GEN Instruction Set Assembly) contains dedicated "send" instructions. These send instructions provide several addressing models. One of these addressing models (named "stateless") provides most flexible I/O using plain virtual addresses (as opposed to buffer_handle+offset models). This "stateless" model is similar to regular memory load/store operations available on typical CPUs. Since this model provides I/O using arbitrary virtual addresses, it enables algorithmic designs that are based on pointer-to-pointer (e.g. buffer of pointers) concepts. For instance, it allows creating tree-like data structures such as:

                    ________________
                   |      NODE1     |
                   | uint64_t data  |
                   +----------------|
                   | NODE*  |  NODE*|
                   +--------+-------+
                     /              \
    ________________/                \________________
   |      NODE2     |                |      NODE3     |
   | uint64_t data  |                | uint64_t data  |
   +----------------|                +----------------|
   | NODE*  |  NODE*|                | NODE*  |  NODE*|
   +--------+-------+                +--------+-------+

Please note that pointers inside such structures can point to memory locations in different OCL allocations - e.g. NODE1 and NODE2 can reside in one OCL allocation while NODE3 resides in a completely separate OCL allocation. Additionally, such pointers can be shared with CPU (i.e. using SVM - Shared Virtual Memory feature). Using pointers from different allocations doesn't affect the stateless addressing model which even allows scattered reading from different allocations at the same time (i.e. by utilizing SIMD-nature of send instructions).

When it comes to coherency programming, send instructions in stateless model can be encoded (at ISA level) to either use or disable coherency. However, for generic OCL applications (such as example with tree-like data structure), OCL compiler is not able to determine origin of memory pointed to by an arbitrary pointer - i.e. is not able to track given pointer back to a specific allocation. As such, it's not able to decide whether coherency is needed or not for specific pointer (or for specific I/O instruction). As a result, compiler encodes all stateless sends as coherent (doing otherwise would lead to functional issues resulting from data corruption). Please note that it would be possible to workaround this (e.g. based on allocations map and pointer bounds checking prior to each I/O instruction) but the performance cost of such workaround would be many times greater than the cost of keeping coherency always enabled. As such, enabling/disabling memory coherency at GEN ISA level is not feasible and alternative method is needed.

Such alternative solution is to have a global coherency switch that allows disabling coherency for single (though entire) GPU submission. This is beneficial because this way we:
* can enable (and pay for) coherency only in submissions that actually need coherency (submissions that use CL_MEM_SVM_FINE_GRAIN_BUFFER resources)
* don't care about coherency at GEN ISA granularity (no performance impact)

3. Will coherency switch be used frequently?

There are scenarios that will require frequent toggling of the coherency switch.
E.g. an application has two OCL compute kernels: kern_master and kern_worker. kern_master uses, concurrently with CPU, some fine grain SVM resources (CL_MEM_SVM_FINE_GRAIN_BUFFER). These resources contain descriptors of computational work that needs to be executed. kern_master analyzes incoming work descriptors and populates a plain OCL buffer (non-fine-grain) with payload for kern_worker. Once kern_master is done, kern_worker kicks-in and processes the payload that kern_master produced. These two kernels work in a loop, one after another. Since only kern_master requires coherency, kern_worker should not be forced to pay for it. This means that we need to have the ability to toggle coherency switch on or off per each GPU submission:
(ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> (ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> ...

v2: Fixed compilation warning.
v3: Refactored the patch to add IOCTL instead of exec flag.
v4: Renamed and documented the API flag. Used strict values.
Removed redundant GEM_WARN_ON()s. Improved to coding standard. Introduced a macro for checking whether hardware supports the feature.

Cc Joonas Lahtinen <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Michal Winiarski <[email protected]>

Bspec: 11419

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h         |  1 +
  drivers/gpu/drm/i915/i915_gem_context.c | 41 +++++++++++++++++++++++++++
  drivers/gpu/drm/i915/i915_gem_context.h |  6 ++++
  drivers/gpu/drm/i915/intel_lrc.c        | 49 +++++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_lrc.h        |  4 +++
  include/uapi/drm/i915_drm.h             |  6 ++++
  6 files changed, 107 insertions(+)

drivers/gpu/drm/i915/i915_drv.h +1 -0

index 09ab124..7d4bbd5 100644
@@ -2524,6 +2524,7 @@ intel_info(const struct drm_i915_private *dev_priv)
  #define HAS_EDRAM(dev_priv)	(!!((dev_priv)->edram_cap & EDRAM_ENABLED))
  #define HAS_WT(dev_priv)	((IS_HASWELL(dev_priv) || \
  				 IS_BROADWELL(dev_priv)) && HAS_EDRAM(dev_priv))
+#define HAS_DATA_PORT_COHERENCY(dev_priv)	(INTEL_GEN(dev_priv) >= 9)
 
  #define HWS_NEEDS_PHYSICAL(dev_priv)	((dev_priv)->info.hws_needs_physical)
 

drivers/gpu/drm/i915/i915_gem_context.c +8 -0

index b10770c..6db352e 100644
@@ -711,6 +711,26 @@ static bool client_is_banned(struct drm_i915_file_private *file_priv)
  	return atomic_read(&file_priv->ban_score) >= I915_CLIENT_SCORE_BANNED;
  }
 
+static int i915_gem_context_set_data_port_coherent(struct i915_gem_context *ctx)
+{
+	int ret;
+
+	ret = intel_lr_context_modify_data_port_coherency(ctx, true);
+	if (!ret)
+		__set_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+	return ret;

drivers/gpu/drm/i915/i915_gem_context.c +6 -0

  	struct drm_i915_file_private *file_priv = file->driver_priv;
  	struct drm_i915_gem_context_param *args = data;
  	struct i915_gem_context *ctx;

@@ -818,6 +839,12 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
  	case I915_CONTEXT_PARAM_PRIORITY:
  		args->value = ctx->sched.priority;
  		break;
+	case I915_CONTEXT_PARAM_DATA_PORT_COHERENCY:
+		if (!HAS_DATA_PORT_COHERENCY(dev_priv))
+			ret = -ENODEV;
+		else
+			args->value = i915_gem_context_is_data_port_coherent(ctx);
+		break;
  	default:
  		ret = -EINVAL;
  		break;

drivers/gpu/drm/i915/i915_gem_context.c +13 -0

  	struct drm_i915_file_private *file_priv = file->driver_priv;
  	struct drm_i915_gem_context_param *args = data;
  	struct i915_gem_context *ctx;

@@ -893,6 +921,19 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
  		}
  		break;
 
+	case I915_CONTEXT_PARAM_DATA_PORT_COHERENCY:
+		if (args->size)
+			ret = -EINVAL;
+		else if (!HAS_DATA_PORT_COHERENCY(dev_priv))
+			ret = -ENODEV;
+		else if (args->value == 1)
+			ret = i915_gem_context_set_data_port_coherent(ctx);
+		else if (args->value == 0)
+			ret = i915_gem_context_clear_data_port_coherent(ctx);
+		else
+			ret = -EINVAL;
+		break;
+
  	default:
  		ret = -EINVAL;
  		break;

drivers/gpu/drm/i915/i915_gem_context.h +6 -0

index b116e49..e8ccb70 100644
@@ -126,6 +126,7 @@ struct i915_gem_context {
  #define CONTEXT_BANNABLE		3
  #define CONTEXT_BANNED			4
  #define CONTEXT_FORCE_SINGLE_SUBMISSION	5
+#define CONTEXT_DATA_PORT_COHERENT	6
 
  	/**
  	 * @hw_id: - unique identifier for the context

@@ -257,6 +258,11 @@ static inline void i915_gem_context_set_force_single_submission(struct i915_gem_
  	__set_bit(CONTEXT_FORCE_SINGLE_SUBMISSION, &ctx->flags);
  }
 
+static inline bool i915_gem_context_is_data_port_coherent(struct i915_gem_context *ctx)
+{
+	return test_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+}
+
  static inline bool i915_gem_context_is_default(const struct i915_gem_context *c)
  {
  	return c->user_handle == DEFAULT_CONTEXT_HANDLE;

drivers/gpu/drm/i915/intel_lrc.c +0 -0

index ab89dab..1f037e3 100644

drivers/gpu/drm/i915/intel_lrc.c +25 -0

+{
+	u32 *cs;
+	i915_reg_t reg;
+
+	GEM_BUG_ON(req->engine->class != RENDER_CLASS);
+	GEM_BUG_ON(INTEL_GEN(req->i915) < 9);
+
+	cs = intel_ring_begin(req, 4);
+	if (IS_ERR(cs))
+		return PTR_ERR(cs);
+
+	if (INTEL_GEN(req->i915) >= 10)
+		reg = CNL_HDC_CHICKEN0;
+	else
+		reg = HDC_CHICKEN0;
+
+	*cs++ = MI_LOAD_REGISTER_IMM(1);
+	*cs++ = i915_mmio_reg_offset(reg);
+	/* Enabling coherency means disabling the bit which forces it off */
+	if (enable)
+		*cs++ = _MASKED_BIT_DISABLE(HDC_FORCE_NON_COHERENT);
+	else
+		*cs++ = _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT);
+	*cs++ = MI_NOOP;
+

drivers/gpu/drm/i915/intel_lrc.c +13 -0

+	int ret;
+
+	req = i915_request_alloc(ctx->i915->engine[RCS], ctx);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
+
+	ret = emit_set_data_port_coherency(req, enable);
+
+	i915_request_add(req);
+
+	return ret;
+}
+
  static struct i915_priolist *
  lookup_priolist(struct intel_engine_cs *engine, int prio)
  {

drivers/gpu/drm/i915/intel_lrc.h +4 -0

index 1593194..f6965ae 100644
@@ -104,4 +104,8 @@ struct i915_gem_context;
 
  void intel_lr_context_resume(struct drm_i915_private *dev_priv);
 
+int
+intel_lr_context_modify_data_port_coherency(struct i915_gem_context *ctx,
+					    bool enable);
+
  #endif /* _INTEL_LRC_H_ */

include/uapi/drm/i915_drm.h +0 -0

index 7f5634c..e677bea 100644

drivers/gpu/drm/i915/i915_gem_context.c static bool client_is_banned(struct drm_i915_file_private *file_priv) · line 731

Quoted 8 lines

drivers/gpu/drm/i915/i915_gem_context.c +8 -0

+static int i915_gem_context_clear_data_port_coherent(struct i915_gem_context *ctx)
+{
+     int ret;
+
+     ret = intel_lr_context_modify_data_port_coherency(ctx, false);
+     if (!ret)
+             __clear_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+     return ret;

Is there a good reason you allow userspace to keep emitting unlimited number of commands which actually do not change the status? If not please consider gating the command emission with test_and_set_bit/test_and_clear_bit. Hm.. apart even with that they could keep toggling ad infinitum with no real work in between. Has it been considered to only save the desired state in set param and then emit it, if needed, before next execbuf? Minor thing in any case, just curious since I wasn't following the threads.

Quoting Tvrtko Ursulin (2018-07-09 17:28:02)

Quoted 9 lines · Tomasz Lis

drivers/gpu/drm/i915/i915_gem_context.c static bool client_is_banned(struct drm_i915_file_private *file_priv) · line 731

Quoted 8 lines

drivers/gpu/drm/i915/i915_gem_context.c +8 -0

+static int i915_gem_context_clear_data_port_coherent(struct i915_gem_context *ctx)
+{
+     int ret;
+
+     ret = intel_lr_context_modify_data_port_coherency(ctx, false);
+     if (!ret)
+             __clear_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+     return ret;

Quoted 8 lines

Is there a good reason you allow userspace to keep emitting unlimited number of commands which actually do not change the status? If not please consider gating the command emission with test_and_set_bit/test_and_clear_bit. Hm.. apart even with that they could keep toggling ad infinitum with no real work in between. Has it been considered to only save the desired state in set param and then emit it, if needed, before next execbuf? Minor thing in any case, just curious since I wasn't following the threads.

The first patch tried to add a bit to execbuf, and having been mistakenly down that road before, we asked if there was any alternative. (Now if you've also been following execbuf3 conversations, having a packet for privileged LRI is definitely something we want.)

Setting the value in the context register is precisely what we want to do, and trivially serialised with execbuf since we have to serialise reservation of ring space, i.e. the normal rules of request generation. (execbuf is just a client and nothing special). From that point of view, we only care about frequency, if it is very frequent it should be controlled by userspace inside the batch (but it can't due to there being dangerous bits inside the reg aiui). At the other end of the scale, is context_setparam for set-once. And there should be no inbetween as that requires costly batch flushes.
-Chris

Quoted 6 lines · Joonas Lahtinen

I'm thinking we should set this value when it has changed, when we insert the requests into the command stream. So if you change back and forth, while not emitting any requests, nothing really happens. If you change the value and emit a request, we should emit a LRI before the jump to the commands. Similary if you keep setting the value to the value it already was in, nothing will happen, again.

When I considered that, my way of reasoning was:
If we execute the flag changing buffer right away, it may be sent to hardware faster if there is no job in progress.
If we use the lazy way, and trigger the change just before submission
-  there will be additional conditions in submission code, plus the
change will be made when there is another job pending (though it's not a considerable payload to just switch a flag).
If user space switches the flag back and forth without much sense, then there is something wrong with the user space driver, and it shouldn't be up to kernel to fix that.

This is why I chosen the current approach. But I can change it if you wish.

Quoted 23 lines · Chris Wilson

Quoting Tvrtko Ursulin (2018-07-09 17:28:02)

Quoted 9 lines · Tomasz Lis

drivers/gpu/drm/i915/i915_gem_context.c static bool client_is_banned(struct drm_i915_file_private *file_priv) · line 731

Quoted 8 lines

drivers/gpu/drm/i915/i915_gem_context.c +8 -0

+static int i915_gem_context_clear_data_port_coherent(struct i915_gem_context *ctx)
+{
+     int ret;
+
+     ret = intel_lr_context_modify_data_port_coherency(ctx, false);
+     if (!ret)
+             __clear_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+     return ret;

Quoted 8 lines

Is there a good reason you allow userspace to keep emitting unlimited number of commands which actually do not change the status? If not please consider gating the command emission with test_and_set_bit/test_and_clear_bit. Hm.. apart even with that they could keep toggling ad infinitum with no real work in between. Has it been considered to only save the desired state in set param and then emit it, if needed, before next execbuf? Minor thing in any case, just curious since I wasn't following the threads.

The first patch tried to add a bit to execbuf, and having been mistakenly down that road before, we asked if there was any alternative. (Now if you've also been following execbuf3 conversations, having a packet for privileged LRI is definitely something we want.)

Quoted 10 lines

Setting the value in the context register is precisely what we want to do, and trivially serialised with execbuf since we have to serialise reservation of ring space, i.e. the normal rules of request generation. (execbuf is just a client and nothing special). From that point of view, we only care about frequency, if it is very frequent it should be controlled by userspace inside the batch (but it can't due to there being dangerous bits inside the reg aiui). At the other end of the scale, is context_setparam for set-once. And there should be no inbetween as that requires costly batch flushes.
-Chris

Joonas did brought that concern in his review; here it is, with my response:

Quoted 14 lines · Tomasz

Quoted 6 lines · Joonas Lahtinen

I'm thinking we should set this value when it has changed, when we insert the requests into the command stream. So if you change back and forth, while not emitting any requests, nothing really happens. If you change the value and emit a request, we should emit a LRI before the jump to the commands. Similary if you keep setting the value to the value it already was in, nothing will happen, again.

When I considered that, my way of reasoning was:
If we execute the flag changing buffer right away, it may be sent to hardware faster if there is no job in progress.
If we use the lazy way, and trigger the change just before submission
-  there will be additional conditions in submission code, plus the
change will be made when there is another job pending (though it's not a considerable payload to just switch a flag).
If user space switches the flag back and forth without much sense, then there is something wrong with the user space driver, and it shouldn't be up to kernel to fix that.

Quoted 1 line

This is why I chosen the current approach. But I can change it if you wish.

So while I think the current solution is better from performance standpoint, but I will change it if you request that.

Quoted 157 lines · Tomasz Lis

The patch adds a parameter to control the data port coherency functionality on a per-context level. When the IOCTL is called, a command to switch data port coherency state is added to the ordered list. All prior requests are executed on old coherency settings, and all exec requests after the IOCTL will use new settings.

Rationale:

The OpenCL driver develpers requested a functionality to control cache coherency at data port level. Keeping the coherency at that level is disabled by default due to its performance costs. OpenCL driver is planning to enable it for a small subset of submissions, when such functionality is required. Below are answers to basic question explaining background of the functionality and reasoning for the proposed implementation:

1. Why do we need a coherency enable/disable switch for memory that is shared between CPU and GEN (GPU)?

Memory coherency between CPU and GEN, while being a great feature that enables CL_MEM_SVM_FINE_GRAIN_BUFFER OCL capability on Intel GEN architecture, adds overhead related to tracking (snooping) memory inside different cache units (L1$, L2$, L3$, LLC$, etc.). At the same time, minority of modern OCL applications actually use CL_MEM_SVM_FINE_GRAIN_BUFFER (and hence require memory coherency between CPU and GPU). The goal of coherency enable/disable switch is to remove overhead of memory coherency when memory coherency is not needed.

2. Why do we need a global coherency switch?

In order to support I/O commands from within EUs (Execution Units), Intel GEN ISA (GEN Instruction Set Assembly) contains dedicated "send" instructions. These send instructions provide several addressing models. One of these addressing models (named "stateless") provides most flexible I/O using plain virtual addresses (as opposed to buffer_handle+offset models). This "stateless" model is similar to regular memory load/store operations available on typical CPUs. Since this model provides I/O using arbitrary virtual addresses, it enables algorithmic designs that are based on pointer-to-pointer (e.g. buffer of pointers) concepts. For instance, it allows creating tree-like data structures such as:
                    ________________

                   |      NODE1     |
                   | uint64_t data  |
                   +----------------|
                   | NODE*  |  NODE*|
                   +--------+-------+
                     /              \
    ________________/                \________________
   |      NODE2     |                |      NODE3     |
   | uint64_t data  |                | uint64_t data  |
   +----------------|                +----------------|
   | NODE*  |  NODE*|                | NODE*  |  NODE*|
   +--------+-------+                +--------+-------+

Please note that pointers inside such structures can point to memory locations in different OCL allocations  - e.g. NODE1 and NODE2 can reside in one OCL allocation while NODE3 resides in a completely separate OCL allocation. Additionally, such pointers can be shared with CPU (i.e. using SVM - Shared Virtual Memory feature). Using pointers from different allocations doesn't affect the stateless addressing model which even allows scattered reading from different allocations at the same time (i.e. by utilizing SIMD-nature of send instructions).

When it comes to coherency programming, send instructions in stateless model can be encoded (at ISA level) to either use or disable coherency. However, for generic OCL applications (such as example with tree-like data structure), OCL compiler is not able to determine origin of memory pointed to by an arbitrary pointer - i.e. is not able to track given pointer back to a specific allocation. As such, it's not able to decide whether coherency is needed or not for specific pointer (or for specific I/O instruction). As a result, compiler encodes all stateless sends as coherent (doing otherwise would lead to functional issues resulting from data corruption). Please note that it would be possible to workaround this (e.g. based on allocations map and pointer bounds checking prior to each I/O instruction) but the performance cost of such workaround would be many times greater than the cost of keeping coherency always enabled. As such, enabling/disabling memory coherency at GEN ISA level is not feasible and alternative method is needed.

Such alternative solution is to have a global coherency switch that allows disabling coherency for single (though entire) GPU submission. This is beneficial because this way we:
* can enable (and pay for) coherency only in submissions that actually need coherency (submissions that use CL_MEM_SVM_FINE_GRAIN_BUFFER resources)
* don't care about coherency at GEN ISA granularity (no performance impact)

3. Will coherency switch be used frequently?

There are scenarios that will require frequent toggling of the coherency switch.
E.g. an application has two OCL compute kernels: kern_master and kern_worker. kern_master uses, concurrently with CPU, some fine grain SVM resources (CL_MEM_SVM_FINE_GRAIN_BUFFER). These resources contain descriptors of computational work that needs to be executed. kern_master analyzes incoming work descriptors and populates a plain OCL buffer (non-fine-grain) with payload for kern_worker. Once kern_master is done, kern_worker kicks-in and processes the payload that kern_master produced. These two kernels work in a loop, one after another. Since only kern_master requires coherency, kern_worker should not be forced to pay for it. This means that we need to have the ability to toggle coherency switch on or off per each GPU submission:
(ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> (ENABLE COHERENCY) kern_master -> (DISABLE COHERENCY)kern_worker -> ...

v2: Fixed compilation warning.
v3: Refactored the patch to add IOCTL instead of exec flag.
v4: Renamed and documented the API flag. Used strict values. Removed redundant GEM_WARN_ON()s. Improved to coding standard. Introduced a macro for checking whether hardware supports the feature.

Cc Joonas Lahtinen <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Michal Winiarski <[email protected]>

Bspec: 11419

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h         |  1 + drivers/gpu/drm/i915/i915_gem_context.c | 41
+++++++++++++++++++++++++++
  drivers/gpu/drm/i915/i915_gem_context.h |  6 ++++ drivers/gpu/drm/i915/intel_lrc.c        | 49
+++++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_lrc.h        |  4 +++ include/uapi/drm/i915_drm.h             |  6 ++++

  6 files changed, 107 insertions(+)

drivers/gpu/drm/i915/i915_drv.h +1 -0

index 09ab124..7d4bbd5 100644
@@ -2524,6 +2524,7 @@ intel_info(const struct drm_i915_private 
 *dev_priv)
   #define HAS_EDRAM(dev_priv)    (!!((dev_priv)->edram_cap & EDRAM_ENABLED))
   #define HAS_WT(dev_priv)    ((IS_HASWELL(dev_priv) || \
                    IS_BROADWELL(dev_priv)) && HAS_EDRAM(dev_priv))
+#define HAS_DATA_PORT_COHERENCY(dev_priv) (INTEL_GEN(dev_priv) >= 9)
     #define HWS_NEEDS_PHYSICAL(dev_priv) ((dev_priv)->info.hws_needs_physical)

  diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index b10770c..6db352e 100644

drivers/gpu/drm/i915/i915_gem_context.c +11 -0

@@ -711,6 +711,26 @@ static bool client_is_banned(struct 
 drm_i915_file_private *file_priv)
       return atomic_read(&file_priv->ban_score) >= I915_CLIENT_SCORE_BANNED;
   }
   +static int i915_gem_context_set_data_port_coherent(struct i915_gem_context *ctx)
+{
+    int ret;
+
+    ret = intel_lr_context_modify_data_port_coherency(ctx, true);
+    if (!ret)
+        __set_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+    return ret;
+}
+
+static int i915_gem_context_clear_data_port_coherent(struct 
 i915_gem_context *ctx)
+{

drivers/gpu/drm/i915/i915_gem_context.c static bool client_is_banned(struct drm_i915_file_private *file_priv) · line 731

Quoted 6 lines

drivers/gpu/drm/i915/i915_gem_context.c +6 -0

+    int ret;
+
+    ret = intel_lr_context_modify_data_port_coherency(ctx, false);
+    if (!ret)
+        __clear_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+    return ret;

Is there a good reason you allow userspace to keep emitting unlimited number of commands which actually do not change the status? If not please consider gating the command emission with test_and_set_bit/test_and_clear_bit. Hm.. apart even with that they could keep toggling ad infinitum with no real work in between. Has it been considered to only save the desired state in set param and then emit it, if needed, before next execbuf? Minor thing in any case, just curious since I wasn't following the threads.

drivers/gpu/drm/i915/i915_gem_context.c int i915_gem_context_destroy_ioctl(struct · line 808

Quoted 11 lines

drivers/gpu/drm/i915/i915_gem_context.c +2 -0

+}
+
   int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,

                    struct drm_file *file)
  {

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

@@ -784,6 +804,7 @@ int i915_gem_context_destroy_ioctl(struct 
 drm_device *dev, void *data,
   int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
                       struct drm_file *file)
   {
+    struct drm_i915_private *dev_priv = to_i915(dev);

Feel free to use the local for the other existing to_i915(dev) call sites in here.

Also use i915 for the local name. Unless I915_READ/WRITE is used i915 is preferred nowadays.

Quoted 18 lines

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

       struct drm_i915_file_private *file_priv = file->driver_priv;
       struct drm_i915_gem_context_param *args = data;

      struct i915_gem_context *ctx;

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

@@ -818,6 +839,12 @@ int i915_gem_context_getparam_ioctl(struct 
 drm_device *dev, void *data,

      case I915_CONTEXT_PARAM_PRIORITY:
          args->value = ctx->sched.priority;
          break;
+    case I915_CONTEXT_PARAM_DATA_PORT_COHERENCY:
+        if (!HAS_DATA_PORT_COHERENCY(dev_priv))
+            ret = -ENODEV;
+        else
+            args->value = i915_gem_context_is_data_port_coherent(ctx); +        break;
      default:
          ret = -EINVAL;
          break;

drivers/gpu/drm/i915/i915_gem_context.c int i915_gem_context_getparam_ioctl(struct · line 861

Quoted 6 lines

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

@@ -830,6 +857,7 @@ int i915_gem_context_getparam_ioctl(struct 
 drm_device *dev, void *data,
   int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
                       struct drm_file *file)
   {
+    struct drm_i915_private *dev_priv = to_i915(dev);

As with get_param.

Quoted 50 lines

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

       struct drm_i915_file_private *file_priv = file->driver_priv;
       struct drm_i915_gem_context_param *args = data;

      struct i915_gem_context *ctx;

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

@@ -893,6 +921,19 @@ int i915_gem_context_setparam_ioctl(struct 
 drm_device *dev, void *data,
           }
           break;

  +    case I915_CONTEXT_PARAM_DATA_PORT_COHERENCY:
+        if (args->size)
+            ret = -EINVAL;
+        else if (!HAS_DATA_PORT_COHERENCY(dev_priv))
+            ret = -ENODEV;
+        else if (args->value == 1)
+            ret = i915_gem_context_set_data_port_coherent(ctx); +        else if (args->value == 0)
+            ret = i915_gem_context_clear_data_port_coherent(ctx); +        else
+            ret = -EINVAL;
+        break;
+
      default:
          ret = -EINVAL;
          break;

drivers/gpu/drm/i915/i915_gem_context.h +1 -0

index b116e49..e8ccb70 100644
@@ -126,6 +126,7 @@ struct i915_gem_context {
   #define CONTEXT_BANNABLE        3
   #define CONTEXT_BANNED            4
   #define CONTEXT_FORCE_SINGLE_SUBMISSION    5
+#define CONTEXT_DATA_PORT_COHERENT    6
         /**
        * @hw_id: - unique identifier for the context

@@ -257,6 +258,11 @@ static inline void 

i915_gem_context_set_force_single_submission(struct i915_gem_ __set_bit(CONTEXT_FORCE_SINGLE_SUBMISSION, &ctx->flags);
  }
  +static inline bool i915_gem_context_is_data_port_coherent(struct i915_gem_context *ctx)

drivers/gpu/drm/i915/i915_gem_context.h +4 -0

+{
+    return test_bit(CONTEXT_DATA_PORT_COHERENT, &ctx->flags);
+}
+

  static inline bool i915_gem_context_is_default(const struct i915_gem_context *c) {
      return c->user_handle == DEFAULT_CONTEXT_HANDLE;

drivers/gpu/drm/i915/intel_lrc.c +0 -1

index ab89dab..1f037e3 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c

drivers/gpu/drm/i915/intel_lrc.c intel_lr_context_descriptor_update(struct · line 262

Quoted 6 lines

+++ b/drivers/gpu/drm/i915/intel_lrc.c

drivers/gpu/drm/i915/intel_lrc.c +0 -0

@@ -259,6 +259,55 @@ intel_lr_context_descriptor_update(struct 
 i915_gem_context *ctx,
       ce->lrc_desc = desc;
   }
   +static int emit_set_data_port_coherency(struct i915_request *req, bool enable)

After much disagreement we ended up with rq as the consistent naming for requests.

Quoted 32 lines

drivers/gpu/drm/i915/intel_lrc.c +19 -0

+{
+    u32 *cs;
+    i915_reg_t reg;
+
+    GEM_BUG_ON(req->engine->class != RENDER_CLASS);
+    GEM_BUG_ON(INTEL_GEN(req->i915) < 9);
+
+    cs = intel_ring_begin(req, 4);
+    if (IS_ERR(cs))
+        return PTR_ERR(cs);
+
+    if (INTEL_GEN(req->i915) >= 10)
+        reg = CNL_HDC_CHICKEN0;
+    else
+        reg = HDC_CHICKEN0;
+
+    *cs++ = MI_LOAD_REGISTER_IMM(1);
+    *cs++ = i915_mmio_reg_offset(reg);
+    /* Enabling coherency means disabling the bit which forces it 

off */
+    if (enable)
+        *cs++ = _MASKED_BIT_DISABLE(HDC_FORCE_NON_COHERENT); +    else
+        *cs++ = _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT); +    *cs++ = MI_NOOP;

drivers/gpu/drm/i915/intel_lrc.c +6 -0

+
+    intel_ring_advance(req, cs);
+
+    return 0;
+}
+