Re: [Intel-gfx] [PATCH] drm/i915: disable sampler indirect state in bindless heap

["Kalvala, Haridhar" <haridhar.kalvala@intel.com>]

On 3/30/2023 10:49 PM, Lionel Landwerlin wrote:
> On 29/03/2023 01:49, Matt Atwood wrote:
>> On Tue, Mar 28, 2023 at 04:14:33PM +0530, Kalvala, Haridhar wrote:
>>> On 3/9/2023 8:56 PM, Lionel Landwerlin wrote:
>>>> By default the indirect state sampler data (border colors) are stored
>>>> in the same heap as the SAMPLER_STATE structure. For userspace drivers
>>>> that can be 2 different heaps (dynamic state heap & bindless sampler
>>>> state heap). This means that border colors have to copied in 2
>>>> different places so that the same SAMPLER_STATE structure find the
>>>> right data.
>>>>
>>>> This change is forcing the indirect state sampler data to only be in
>>>> the dynamic state pool (more convinient for userspace drivers, they
>>>> only have to have one copy of the border colors). This is reproducing
>>>> the behavior of the Windows drivers.
>>>>
>> Bspec:46052
>
>
> Sorry, missed your answer.
>
>
> Should I just add the Bspec number to the commit message ?
>
>
> Thanks,
>
>
> -Lionel
>
>
>>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>>> Cc: stable@vger.kernel.org
>>>> ---
>>>>    drivers/gpu/drm/i915/gt/intel_gt_regs.h     |  1 +
>>>>    drivers/gpu/drm/i915/gt/intel_workarounds.c | 17 +++++++++++++++++
>>>>    2 files changed, 18 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h 
>>>> b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>>>> index 08d76aa06974c..1aaa471d08c56 100644
>>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>>>> @@ -1141,6 +1141,7 @@
>>>>    #define   ENABLE_SMALLPL            REG_BIT(15)
>>>>    #define   SC_DISABLE_POWER_OPTIMIZATION_EBB    REG_BIT(9)
>>>>    #define   GEN11_SAMPLER_ENABLE_HEADLESS_MSG    REG_BIT(5)
>>>> +#define   GEN11_INDIRECT_STATE_BASE_ADDR_OVERRIDE REG_BIT(0)
>>>>    #define GEN9_HALF_SLICE_CHICKEN7        MCR_REG(0xe194)
>>>>    #define   DG2_DISABLE_ROUND_ENABLE_ALLOW_FOR_SSLA REG_BIT(15)
>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
>>>> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>>> index 32aa1647721ae..734b64e714647 100644
>>>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>>> @@ -2542,6 +2542,23 @@ rcs_engine_wa_init(struct intel_engine_cs 
>>>> *engine, struct i915_wa_list *wal)
>>>>                     ENABLE_SMALLPL);
>>>>        }
>>>> +    if (GRAPHICS_VER(i915) >= 11) {
>>> Hi Lionel,
>>>
>>> Not sure should this implementation be part of "rcs_engine_wa_init" or
>>> "general_render_compute_wa_init" ?

>>>> +        /* This is not a Wa (although referred to as
>>>> +         * WaSetInidrectStateOverride in places), this allows
>>>> +         * applications that reference sampler states through
>>>> +         * the BindlessSamplerStateBaseAddress to have their
>>>> +         * border color relative to DynamicStateBaseAddress
>>>> +         * rather than BindlessSamplerStateBaseAddress.
>>>> +         *
>>>> +         * Otherwise SAMPLER_STATE border colors have to be
>>>> +         * copied in multiple heaps (DynamicStateBaseAddress &
>>>> +         * BindlessSamplerStateBaseAddress)
>>>> +         */
>>>> +        wa_mcr_masked_en(wal,
>>>> +                 GEN10_SAMPLER_MODE,
>>>   since we checking the condition for GEN11 or above, can this 
>>> register be
>>> defined as GEN11_SAMPLER_MODE
>> We use the name of the first time the register was introduced, gen 10 is
>> fine here.
ok
>>>> + GEN11_INDIRECT_STATE_BASE_ADDR_OVERRIDE);
>>>> +    }
>>>> +
>>>>        if (GRAPHICS_VER(i915) == 11) {
>>>>            /* This is not an Wa. Enable for better image quality */
>>>>            wa_masked_en(wal,
>>> -- 
>>> Regards,
>>> Haridhar Kalvala
>>>
>> Regards,
>> MattA
>
>
-- 
Regards,
Haridhar Kalvala