"Siluvery, Arun" <[email protected]> writes:

> On 07/07/2015 20:13, Francisco Jerez wrote:
>> From: Peter Antoine <[email protected]>
>>
>> This change adds the programming of the MOCS registers to the gen 9+
>> platforms. This change set programs the MOCS register values to a set
>> of values that are defined to be optimal.
>>
>> It creates a fixed register set that is programmed across the different
>> engines so that all engines have the same table. This is done as the
>> main RCS context only holds the registers for itself and the shared
>> L3 values. By trying to keep the registers consistent across the
>> different engines it should make the programming for the registers
>> consistent.
>>
>> v2:
>> -'static const' for private data structures and style changes.(Matt Turner)
>> v3:
>> - Make the tables "slightly" more readable. (Damien Lespiau)
>> - Updated tables fix performance regression.
>> v4:
>> - Code formatting. (Chris Wilson)
>> - re-privatised mocs code. (Daniel Vetter)
>> v5:
>> - Changed the name of a function. (Chris Wilson)
>> v6:
>> - re-based
>> - Added Mesa table entry (skylake & broxton) (Francisco Jerez)
>> - Tidied up the readability defines (Francisco Jerez)
>> - NUMBER of entries defines wrong. (Jim Bish)
>> - Added comments to clear up the meaning of the tables (Jim Bish)
>>
>> Signed-off-by: Peter Antoine <[email protected]>
>>
>> v7 (Francisco Jerez):
>> - Don't write L3-specific MOCS_ESC/SCC values into the e/LLC control
>>    tables.  Prefix L3-specific defines consistently with L3_ and
>>    e/LLC-specific defines with LE_ to avoid this kind of confusion in
>>    the future.
>> - Change L3CC WT define back to RESERVED (matches my hardware
>>    documentation and the original patch, probably a misunderstanding
>>    of my own previous comment).
>> - Drop Android tables, define new minimal tables more suitable for the
>>    open source stack.
>> - Add comment that the MOCS tables are part of the kernel ABI.
>> - Move intel_logical_ring_begin() and _advance() calls one level down
>>    (Chris Wilson).
>> - Minor formatting and style fixes.
>>
>> Signed-off-by: Francisco Jerez <[email protected]>
>> ---
>>   drivers/gpu/drm/i915/Makefile     |   1 +
>>   drivers/gpu/drm/i915/i915_reg.h   |   9 ++
>>   drivers/gpu/drm/i915/intel_lrc.c  |  11 +-
>>   drivers/gpu/drm/i915/intel_lrc.h  |   1 +
>>   drivers/gpu/drm/i915/intel_mocs.c | 324 
>> ++++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_mocs.h |  57 +++++++
>>   6 files changed, 401 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/gpu/drm/i915/intel_mocs.c
>>   create mode 100644 drivers/gpu/drm/i915/intel_mocs.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index de21965..e52e012 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -36,6 +36,7 @@ i915-y += i915_cmd_parser.o \
>>        i915_trace_points.o \
>>        intel_hotplug.o \
>>        intel_lrc.o \
>> +      intel_mocs.o \
>>        intel_ringbuffer.o \
>>        intel_uncore.o
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 2a29bcc..9b17260 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7906,4 +7906,13 @@ enum skl_disp_power_wells {
>>   #define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000)
>>   #define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800)
>>
>> +/* MOCS (Memory Object Control State) registers */
>> +#define GEN9_LNCFCMOCS0             (0xB020)        /* L3 Cache Control 
>> base */
>> +
>> +#define GEN9_GFX_MOCS_0             (0xc800)        /* Graphics MOCS base 
>> register*/
>> +#define GEN9_MFX0_MOCS_0    (0xc900)        /* Media 0 MOCS base register*/
>> +#define GEN9_MFX1_MOCS_0    (0xcA00)        /* Media 1 MOCS base register*/
>> +#define GEN9_VEBOX_MOCS_0   (0xcB00)        /* Video MOCS base register*/
>> +#define GEN9_BLT_MOCS_0             (0xcc00)        /* Blitter MOCS base 
>> register*/
>> +
>>   #endif /* _I915_REG_H_ */
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
>> b/drivers/gpu/drm/i915/intel_lrc.c
>> index d4f8b43..466d17c 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -135,6 +135,7 @@
>>   #include <drm/drmP.h>
>>   #include <drm/i915_drm.h>
>>   #include "i915_drv.h"
>> +#include "intel_mocs.h"
>>
>>   #define GEN9_LR_CONTEXT_RENDER_SIZE (22 * PAGE_SIZE)
>>   #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
>> @@ -772,8 +773,7 @@ static int logical_ring_prepare(struct 
>> drm_i915_gem_request *req, int bytes)
>>    *
>>    * Return: non-zero if the ringbuffer is not ready to be written to.
>>    */
>> -static int intel_logical_ring_begin(struct drm_i915_gem_request *req,
>> -                                int num_dwords)
>> +int intel_logical_ring_begin(struct drm_i915_gem_request *req, int 
>> num_dwords)
>>   {
>>      struct drm_i915_private *dev_priv;
>>      int ret;
>> @@ -1675,6 +1675,13 @@ static int gen8_init_rcs_context(struct 
>> drm_i915_gem_request *req)
>>      if (ret)
>>              return ret;
>>
>> +    /*
>> +     * Failing to program the MOCS is non-fatal.The system will not
>> +     * run at peak performance. So generate a warning and carry on.
>> +     */
>> +    if (intel_rcs_context_init_mocs(req) != 0)
>> +            DRM_ERROR("MOCS failed to program: expect performance issues.");
>
> we don't see this type of constructs, to be consistent why not,
> ret = intel_rcs_context_init_mocs(req);
> if (ret)
>       DRM_ERROR();
>
> nitpick, comment says warning but DRM_ERROR is used.
> '\n' missing in DRM_ERROR
>
Thanks, fixed.

>> +
>>      return intel_lr_context_render_state_init(req);
>>   }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.h 
>> b/drivers/gpu/drm/i915/intel_lrc.h
>> index e0299fb..64f89f99 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.h
>> +++ b/drivers/gpu/drm/i915/intel_lrc.h
>> @@ -42,6 +42,7 @@ int intel_logical_ring_reserve_space(struct 
>> drm_i915_gem_request *request);
>>   void intel_logical_ring_stop(struct intel_engine_cs *ring);
>>   void intel_logical_ring_cleanup(struct intel_engine_cs *ring);
>>   int intel_logical_rings_init(struct drm_device *dev);
>> +int intel_logical_ring_begin(struct drm_i915_gem_request *req, int 
>> num_dwords);
>>
>>   int logical_ring_flush_all_caches(struct drm_i915_gem_request *req);
>>   /**
>> diff --git a/drivers/gpu/drm/i915/intel_mocs.c 
>> b/drivers/gpu/drm/i915/intel_mocs.c
>> new file mode 100644
>> index 0000000..f7b93e9
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_mocs.c
>> @@ -0,0 +1,324 @@
>> +/*
>> + * Copyright (c) 2015 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions: *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
>> OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
>> FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS 
>> IN THE
>> + * SOFTWARE.
>> + */
>> +
>> +#include "intel_mocs.h"
>> +#include "intel_lrc.h"
>> +#include "intel_ringbuffer.h"
>> +
>> +/* structures required */
>> +struct drm_i915_mocs_entry {
>> +    u32     control_value;
>> +    u16     l3cc_value;
>> +};
>> +
>> +struct drm_i915_mocs_table {
>> +    u32                                     size;
>> +    const struct drm_i915_mocs_entry        *table;
>> +};
>> +
> too much spacing.
>
>> +/* Defines for the tables (XXX_MOCS_0 - XXX_MOCS_63) */
>> +#define LE_CACHEABILITY(value)      (value << 0)
>> +#define LE_TGT_CACHE(value) (value << 2)
>> +#define LE_LRUM(value)              (value << 4)
>> +#define LE_AOM(value)               (value << 6)
>> +#define LE_RSC(value)               (value << 7)
>> +#define LE_SCC(value)               (value << 8)
>> +#define LE_PFM(value)               (value << 11)
>> +#define LE_SCF(value)               (value << 14)
>> +
>> +/* Defines for the tables (LNCFMOCS0 - LNCFMOCS31) - two entries per word */
>> +#define L3_ESC(value)               (value << 0)
>> +#define L3_SCC(value)               (value << 1)
>> +#define L3_CACHEABILITY(value)      (value << 4)
>> +
>> +/* Helper defines */
>> +#define GEN9_NUM_MOCS_ENTRIES       (62)  /* 62 out of 64 - 63 & 64 are 
>> reserved. */
>> +
>> +/* (e)LLC caching options */
>> +#define LE_PAGETABLE                (0)
>> +#define LE_UC                       (1)
>> +#define LE_WT                       (2)
>> +#define LE_WB                       (3)
>> +
>> +/* L3 caching options */
>> +#define L3_DIRECT           (0)
>> +#define L3_UC                       (1)
>> +#define L3_RESERVED         (2)
>> +#define L3_WB                       (3)
>> +
>> +/* Target cache */
>> +#define ELLC                        (0)
>> +#define LLC                 (1)
>> +#define LLC_ELLC            (2)
>> +
>> +/*
>> + * MOCS tables
>> + *
>> + * These are the MOCS tables that are programmed across all the rings.
>> + * The control value is programmed to all the rings that support the
>> + * MOCS registers. While the l3cc_values are only programmed to the
>> + * LNCFCMOCS0 - LNCFCMOCS32 registers.
>> + *
>> + * These tables are intended to be kept reasonably consistent across
>> + * platforms. However some of the fields are not applicable to all of
>> + * them.
>> + *
>> + * NOTE: These tables MUST start with being uncached and the length
>> + *       MUST be less than 63 as the last two registers are reserved
>> + *       by the hardware.  These tables are part of the kernel ABI and
>> + *       may only be updated incrementally by adding entries at the
>> + *       end.
>> + */
>> +static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
>> +    /* { 0x00000009, 0x0010 } */
>> +    { (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
>> +       LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
>> +      (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
>> +    /* { 0x00000038, 0x0030 } */
>> +    { (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
>> +       LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
>> +      (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
>> +    /* { 0x0000003b, 0x0030 } */
>> +    { (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
>> +       LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
>> +      (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
>> +};
>> +
>> +/* NOTE: the LE_TGT_CACHE is not used on Broxton */
>> +static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
>> +    /* { 0x00000009, 0x0010 } */
>> +    { (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
>> +       LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
>> +      (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
>> +    /* { 0x00000038, 0x0030 } */
>> +    { (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
>> +       LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
>> +      (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
>> +    /* { 0x0000003b, 0x0030 } */
>> +    { (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
>> +       LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
>> +      (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
>> +};
>> +
>> +/**
>> + * get_mocs_settings
>> + *
>> + * This function will return the values of the MOCS table that needs to
>> + * be programmed for the platform. It will return the values that need
>> + * to be programmed and if they need to be programmed.
>> + *
>> + * If the return values is false then the registers do not need programming.
>> + */
>> +static bool get_mocs_settings(struct drm_device *dev,
>> +                          struct drm_i915_mocs_table *table)
>> +{
> you will get a warning from kernel 0-day that description is missing for 
> arguments and return value, I think it is better to add now or new 
> warning gets added to the current list of warnings.
>

Fixed.

>> +    bool result = false;
>> +
>> +    if (IS_SKYLAKE(dev)) {
>> +            table->size  = ARRAY_SIZE(skylake_mocs_table);
>> +            table->table = skylake_mocs_table;
>> +            result = true;
>> +    } else if (IS_BROXTON(dev)) {
>> +            table->size  = ARRAY_SIZE(broxton_mocs_table);
>> +            table->table = broxton_mocs_table;
>> +            result = true;
>> +    } else {
>> +            /* Platform that should have a MOCS table does not */
>> +            WARN_ON(INTEL_INFO(dev)->gen >= 9);
> if we use this kernel on future platforms before MOCS support is added, 
> this prints WARNING every time context is initialized, I faced similar 
> issue with WA batch and the comment that I received was that WARNING is 
> not very useful here hence changed it to DRM_ERROR but no strong 
> opinion, perhaps WARN_ONCE if you want to retain the warning.
>
Right, I've changed it locally to use WARN_ONCE() instead.

>> +    }
>> +
>> +    return result;
>> +}
>> +
>> +/**
>> + * emit_mocs_control_table() - emit the mocs control table
>> + * @req:    Request to set up the MOCS table for.
>> + * @table:  The values to program into the control regs.
>> + * @reg_base:       The base for the engine that needs to be programmed.
>> + *
>> + * This function simply emits a MI_LOAD_REGISTER_IMM command for the
>> + * given table starting at the given address.
>> + *
>> + * @return 0 on success, otherwise the error status.
>> + */
>> +static int emit_mocs_control_table(struct drm_i915_gem_request *req,
>> +                               const struct drm_i915_mocs_table *table,
>> +                               u32 reg_base)
>> +{
>> +    struct intel_ringbuffer *ringbuf = req->ringbuf;
>> +    unsigned int index;
>> +    int ret;
>> +
>> +    ret = intel_logical_ring_begin(req, 2 + 2 * GEN9_NUM_MOCS_ENTRIES);
>> +    if (ret) {
>> +            DRM_DEBUG("intel_logical_ring_begin failed %d\n", ret);
>> +            return ret;
>> +    }
>> +
>> +    intel_logical_ring_emit(ringbuf,
>> +                            MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES));
>> +
>> +    for (index = 0; index < table->size; index++) {
>> +            intel_logical_ring_emit(ringbuf, reg_base + index * 4);
>> +            intel_logical_ring_emit(ringbuf,
>> +                                    table->table[index].control_value);
>> +    }
>
> intel_logical_ring_emit(ringbuf, MI_NOOP);
> after the loop to make number of commands even.
> Count also need to be updated.
>

It would be illegal to emit a MI_NOOP here in the middle of another
command.  It's already being done a few lines later BTW.

>> +
>> +    /*
>> +     * Ok, now set the unused entries to uncached. These entries
>> +     * are officially undefined and no contract for the contents
>> +     * and settings is given for these entries.
>> +     *
>> +     * Entry 0 in the table is uncached - so we are just writing
>> +     * that value to all the used entries.
>> +     */
>> +    for (; index < GEN9_NUM_MOCS_ENTRIES; index++) {
>> +            intel_logical_ring_emit(ringbuf, reg_base + index * 4);
>> +            intel_logical_ring_emit(ringbuf, table->table[0].control_value);
>> +    }
>> +
>> +    intel_logical_ring_emit(ringbuf, MI_NOOP);
>> +    intel_logical_ring_advance(ringbuf);
>> +
>> +    return 0;
>> +}
>> +
>> +/**
>> + * emit_mocs_l3cc_table() - emit the mocs control table
>> + * @req:    Request to set up the MOCS table for.
>> + * @table:  The values to program into the control regs.
>> + *
>> + * This function simply emits a MI_LOAD_REGISTER_IMM command for the
>> + * given table starting at the given address. This register set is
>> + * programmed in pairs.
>> + *
>> + * @return 0 on success, otherwise the error status.
>> + */
>> +static int emit_mocs_l3cc_table(struct drm_i915_gem_request *req,
>> +                            const struct drm_i915_mocs_table *table)
>> +{
>> +    struct intel_ringbuffer *ringbuf = req->ringbuf;
>> +    unsigned int count;
>> +    unsigned int i;
>> +    u32 value;
>> +    u32 filler = (table->table[0].l3cc_value & 0xffff) |
>> +                    ((table->table[0].l3cc_value & 0xffff) << 16);
>> +    int ret;
>> +
>> +    ret = intel_logical_ring_begin(req, 2 + GEN9_NUM_MOCS_ENTRIES);
>> +    if (ret) {
>> +            DRM_DEBUG("intel_logical_ring_begin failed %d\n", ret);
>> +            return ret;
>> +    }
>> +
>> +    intel_logical_ring_emit(ringbuf,
>> +                    MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES / 2));
>> +
>> +    for (i = 0, count = 0; i < table->size / 2; i++, count += 2) {
>> +            value = (table->table[count].l3cc_value & 0xffff) |
>> +                    ((table->table[count + 1].l3cc_value & 0xffff) << 16);
>> +
>> +            intel_logical_ring_emit(ringbuf, GEN9_LNCFCMOCS0 + i * 4);
>> +            intel_logical_ring_emit(ringbuf, value);
>> +    }
>> +
> same as above.
>
>> +    if (table->size & 0x01) {
>> +            /* Odd table size - 1 left over */
>> +            value = (table->table[count].l3cc_value & 0xffff) |
>> +                    ((table->table[0].l3cc_value & 0xffff) << 16);
>> +    } else
>> +            value = filler;
>> +
>> +    /*
>> +     * Now set the rest of the table to uncached - use entry 0 as
>> +     * this will be uncached. Leave the last pair uninitialised as
>> +     * they are reserved by the hardware.
>> +     */
>> +    for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
>> +            intel_logical_ring_emit(ringbuf, GEN9_LNCFCMOCS0 + i * 4);
>> +            intel_logical_ring_emit(ringbuf, value);
>> +
>> +            value = filler;
>> +    }
>> +
>> +    intel_logical_ring_emit(ringbuf, MI_NOOP);
>> +    intel_logical_ring_advance(ringbuf);
>> +
>> +    return 0;
>> +}
>> +
>> +/**
>> + * intel_rcs_context_init_mocs() - program the MOCS register.
>> + *
>> + * @req:    Request to set up the MOCS tables for.
>> + *
>> + * This function will emit a batch buffer with the values required for
>> + * programming the MOCS register values for all the currently supported
>> + * rings.
>> + *
>> + * These registers are partially stored in the RCS context, so they are
>> + * emitted at the same time so that when a context is created these 
>> registers
>> + * are set up. These registers have to be emitted into the start of the
>> + * context as setting the ELSP will re-init some of these registers back
>> + * to the hw values.
>> + *
>> + * @return 0 on success, otherwise the error status.
>> + */
>> +int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req)
>> +{
>> +    struct drm_i915_mocs_table t;
>> +    int ret;
>> +
>> +    if (get_mocs_settings(req->ring->dev, &t)) {
> can be changed to "ret = get_mocs_settings();" to be consistent and 
> handle error case first; otherwise in this case non-zero is treated as 
> success case and zero as failure.
>

Maybe.  The thing is there seems to be no error case here AFAICT.  It's
not an error for a pre-Gen9 platform not to have MOCS tables, and it's
also not an error for other hardware to have MOCS tables to program.
Apparently it was Peter's intention to make get_mocs_settings() return a
boolean with no failure implications reporting whether the platform has
applicable MOCS settings or not, which seems to make sense to me.

> any sanity check on the received table, if at all required?
>
>> +            /* Program the control registers */
>> +            ret = emit_mocs_control_table(req, &t, GEN9_GFX_MOCS_0);
>> +            if (ret)
>> +                    return ret;
>> +
>> +            ret = emit_mocs_control_table(req, &t, GEN9_MFX0_MOCS_0);
>> +            if (ret)
>> +                    return ret;
>> +
>> +            ret = emit_mocs_control_table(req, &t, GEN9_MFX1_MOCS_0);
>> +            if (ret)
>> +                    return ret;
>> +
>> +            ret = emit_mocs_control_table(req, &t, GEN9_VEBOX_MOCS_0);
>> +            if (ret)
>> +                    return ret;
>> +
>> +            ret = emit_mocs_control_table(req, &t, GEN9_BLT_MOCS_0);
>> +            if (ret)
>> +                    return ret;
>> +
>> +            /* Now program the l3cc registers */
>> +            ret = emit_mocs_l3cc_table(req, &t);
>> +            if (ret)
>> +                    return ret;
>
> In case if we fail before setting tables for all rings, is that a valid 
> setup? I mean do we need to revert the entries in those rings that are 
> successfully setup?
>
Strictly speaking no, it wouldn't be a valid setup, but reverting back
to the original entries wouldn't give a valid setup either, and most
likely it would fail too because it would need to use basically the same
path that just failed to re-program the original entries.

>> +
>> +            DRM_DEBUG("MOCS: Table set in context.\n");
>> +    } else {
>> +            DRM_DEBUG("MOCS: Table not supported on platform.\n");
> DRM_ERROR may be?
> and it returns 0 in this case also as success; perhaps we can remove 
> printing any message in this as we already WARN/DRM_ERROR in 
> get_mocs_settings() or move it here and remove it that function.

As explained earlier neither of these two conditions is an error, but
the debug messages do seem a bit redundant, I've removed them locally.

>
> regards
> Arun
>
Thanks.

>> +    }
>> +
>> +    return 0;
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_mocs.h 
>> b/drivers/gpu/drm/i915/intel_mocs.h
>> new file mode 100644
>> index 0000000..76e45b1
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_mocs.h
>> @@ -0,0 +1,57 @@
>> +/*
>> + * Copyright (c) 2015 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
>> OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
>> FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS 
>> IN THE
>> + * SOFTWARE.
>> + */
>> +
>> +#ifndef INTEL_MOCS_H
>> +#define INTEL_MOCS_H
>> +
>> +/**
>> + * DOC: Memory Objects Control State (MOCS)
>> + *
>> + * Motivation:
>> + * In previous Gens the MOCS settings was a value that was set by user land 
>> as
>> + * part of the batch. In Gen9 this has changed to be a single table (per 
>> ring)
>> + * that all batches now reference by index instead of programming the MOCS
>> + * directly.
>> + *
>> + * The one wrinkle in this is that only PART of the MOCS tables are included
>> + * in context (The GFX_MOCS_0 - GFX_MOCS_64 and the LNCFCMOCS0 - LNCFCMOCS32
>> + * registers). The rest are not (the settings for the other rings).
>> + *
>> + * This table needs to be set at system start-up because the way the table
>> + * interacts with the contexts and the GmmLib interface.
>> + *
>> + *
>> + * Implementation:
>> + *
>> + * The tables (one per supported platform) are defined in intel_mocs.c
>> + * and are programmed in the first batch after the context is loaded
>> + * (with the hardware workarounds). This will then let the usual
>> + * context handling keep the MOCS in step.
>> + */
>> +
>> +#include <drm/drmP.h>
>> +#include "i915_drv.h"
>> +
>> +int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req);
>> +
>> +#endif
>>

Attachment: signature.asc
Description: PGP signature

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

Reply via email to