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

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

On 03/04/2023 21:22, Kalvala, Haridhar wrote:
>
> On 3/31/2023 12:35 PM, Kalvala, Haridhar wrote:
>>
>> 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" ?
>
>
> I checked with Matt Ropper as well, looks like this implementation 
> should be part of "general_render_compute_wa_init".


I did send a v3 of the patch last Thursday to address this.

Let me know if that's good.


Thanks,


-Lionel


>
>>
>>>>>> +        /* 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
>>>
>>>