[Intel-gfx] [PATCH v5] drm/i915/bxt: Broxton decoupled MMIO

Wed Nov 16 09:08:30 UTC 2016

On 16/11/2016 09:03, Praveen Paneri wrote:
> Hi Tvrtko,
>
> On Wednesday 16 November 2016 01:55 PM, Tvrtko Ursulin wrote:
>>
>> On 15/11/2016 17:19, Praveen Paneri wrote:
>>> Decoupled MMIO is an alternative way to access forcewake domain
>>> registers, which requires less cycles for a single read/write and
>>> avoids frequent software forcewake.
>>> This certainly gives advantage over the forcewake as this new
>>> mechanism “decouples” CPU cycles and allow them to complete even
>>> when GT is in a CPD (frequency change) or C6 state.
>>>
>>> This can co-exist with forcewake and we will continue to use forcewake
>>> as appropriate. E.g. 64-bit register writes to avoid writing 2 dwords
>>> separately and land into funny situations.
>>>
>>> v2:
>>> - Moved platform check out of the function and got rid of duplicate
>>>  functions to find out decoupled power domain (Chris)
>>> - Added a check for forcewake already held and skipped decoupled
>>>  access (Chris)
>>> - Skipped writing 64 bit registers through decoupled MMIO (Chris)
>>>
>>> v3:
>>> - Improved commit message with more info on decoupled mmio (Tvrtko)
>>> - Changed decoupled operation to enum and used u32 instead of
>>>  uint_32 data type for register offset (Tvrtko)
>>> - Moved HAS_DECOUPLED_MMIO to device info (Tvrtko)
>>> - Added lookup table for converting fw_engine to pd_engine (Tvrtko)
>>> - Improved __gen9_decoupled_read and __gen9_decoupled_write
>>>  routines (Tvrtko)
>>>
>>> v4:
>>> - Fixed alignment and variable names (Chris)
>>> - Write GEN9_DECOUPLED_REG0_DW1 register in just one go (Zhe Wang)
>>>
>>> v5:
>>> - Changed HAS_DECOUPLED_MMIO() argument name to dev_priv (Tvrtko)
>>> - Sanitize info->had_decoupled_mmio at init (Chris)
>>>
>>> Signed-off-by: Zhe Wang <zhe1.wang at intel.com>
>>> Signed-off-by: Praveen Paneri <praveen.paneri at intel.com>
>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> Please put "(v4)" when you carry over the r-b and it wasn't explicitly
>> said you are OK to just keep it.
> Sure will take care going fwd.
>>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.h     |  17 +++++-
>>>  drivers/gpu/drm/i915/i915_pci.c     |   1 +
>>>  drivers/gpu/drm/i915/i915_reg.h     |   7 +++
>>>  drivers/gpu/drm/i915/intel_uncore.c | 115
>>> ++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 139 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 4e7148a..c1eec04 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -549,6 +549,18 @@ enum forcewake_domains {
>>>  #define FW_REG_READ  (1)
>>>  #define FW_REG_WRITE (2)
>>>
>>> +enum decoupled_power_domain {
>>> +    GEN9_DECOUPLED_PD_BLITTER = 0,
>>> +    GEN9_DECOUPLED_PD_RENDER,
>>> +    GEN9_DECOUPLED_PD_MEDIA,
>>> +    GEN9_DECOUPLED_PD_ALL
>>> +};
>>> +
>>> +enum decoupled_ops {
>>> +    GEN9_DECOUPLED_OP_WRITE = 0,
>>> +    GEN9_DECOUPLED_OP_READ
>>> +};
>>> +
>>>  enum forcewake_domains
>>>  intel_uncore_forcewake_for_reg(struct drm_i915_private *dev_priv,
>>>                     i915_reg_t reg, unsigned int op);
>>> @@ -683,7 +695,8 @@ struct intel_csr {
>>>      func(cursor_needs_physical); \
>>>      func(hws_needs_physical); \
>>>      func(overlay_needs_physical); \
>>> -    func(supports_tv)
>>> +    func(supports_tv); \
>>> +    func(has_decoupled_mmio)
>>>
>>>  struct sseu_dev_info {
>>>      u8 slice_mask;
>>> @@ -2652,6 +2665,8 @@ struct drm_i915_cmd_table {
>>>  #define GT_FREQUENCY_MULTIPLIER 50
>>>  #define GEN9_FREQ_SCALER 3
>>>
>>> +#define HAS_DECOUPLED_MMIO(dev_priv)
>>> (INTEL_INFO(dev_priv)->has_decoupled_mmio)
>>> +
>>>  #include "i915_trace.h"
>>>
>>>  static inline bool intel_scanout_needs_vtd_wa(struct drm_i915_private
>>> *dev_priv)
>>> diff --git a/drivers/gpu/drm/i915/i915_pci.c
>>> b/drivers/gpu/drm/i915/i915_pci.c
>>> index 70a99ce..fce8e19 100644
>>> --- a/drivers/gpu/drm/i915/i915_pci.c
>>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>>> @@ -363,6 +363,7 @@
>>>      .has_hw_contexts = 1,
>>>      .has_logical_ring_contexts = 1,
>>>      .has_guc = 1,
>>> +    .has_decoupled_mmio = 1,
>>>      .ddb_size = 512,
>>>      GEN_DEFAULT_PIPEOFFSETS,
>>>      IVB_CURSOR_OFFSETS,
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>> b/drivers/gpu/drm/i915/i915_reg.h
>>> index 3361d7f..c70c07a 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -7342,6 +7342,13 @@ enum {
>>>  #define  SKL_FUSE_PG1_DIST_STATUS              (1<<26)
>>>  #define  SKL_FUSE_PG2_DIST_STATUS              (1<<25)
>>>
>>> +/* Decoupled MMIO register pair for kernel driver */
>>> +#define GEN9_DECOUPLED_REG0_DW0            _MMIO(0xF00)
>>> +#define GEN9_DECOUPLED_REG0_DW1            _MMIO(0xF04)
>>> +#define GEN9_DECOUPLED_DW1_GO            (1<<31)
>>> +#define GEN9_DECOUPLED_PD_SHIFT            28
>>> +#define GEN9_DECOUPLED_OP_SHIFT            24
>>> +
>>>  /* Per-pipe DDI Function Control */
>>>  #define _TRANS_DDI_FUNC_CTL_A        0x60400
>>>  #define _TRANS_DDI_FUNC_CTL_B        0x61400
>>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
>>> b/drivers/gpu/drm/i915/intel_uncore.c
>>> index e2b188d..e953303 100644
>>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>>> @@ -402,6 +402,8 @@ static void intel_uncore_edram_detect(struct
>>> drm_i915_private *dev_priv)
>>>  static void __intel_uncore_early_sanitize(struct drm_i915_private
>>> *dev_priv,
>>>                        bool restore_forcewake)
>>>  {
>>> +    struct intel_device_info *info = mkwrite_device_info(dev_priv);
>>> +
>>>      /* clear out unclaimed reg detection bit */
>>>      if (check_for_unclaimed_mmio(dev_priv))
>>>          DRM_DEBUG("unclaimed mmio detected on uncore init,
>>> clearing\n");
>>> @@ -419,6 +421,10 @@ static void __intel_uncore_early_sanitize(struct
>>> drm_i915_private *dev_priv,
>>>                     GT_FIFO_CTL_RC6_POLICY_STALL);
>>>      }
>>>
>>> +    /* Enable Decoupled MMIO only on BXT C stepping onwards */
>>> +    if (!IS_BXT_REVID(dev_priv, BXT_REVID_C0, REVID_FOREVER))
>>> +        info->has_decoupled_mmio = false;
>>
>> Why have you decided to put it in here? It doesn't make much difference
>> except conceptually. This runs every time on resume while I thought
>> intel_device_info_runtime_init would have been more appropriate.
> Sorry about picking the bad place for this but intel_uncore_init() which
> uses this info gets executed before intel_device_info_runtime_init() in
> driver_load sequence. If I am not wrong, we need to find an appropriate
> place for this.

You are correct, very illogical. Someone else can fix that so:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko