[Intel-xe] [PATCH] drm/xe/reg_sr: Apply limit to register whitelisting

[Gustavo Sousa]

Quoting Lucas De Marchi (2023-05-30 01:01:34-03:00)
>On Mon, May 29, 2023 at 06:48:17PM -0300, Gustavo Sousa wrote:
>>Quoting Lucas De Marchi (2023-05-26 13:58:22-03:00)
>>>On Thu, May 25, 2023 at 10:14:21PM -0300, Gustavo Sousa wrote:
>>>>Quoting Matt Roper (2023-05-25 21:10:38-03:00)
>>>>>On Thu, May 25, 2023 at 08:24:06PM -0300, Gustavo Sousa wrote:
>>>>>> If RING_MAX_NONPRIV_SLOTS denotes the maximum number of whitelisting
>>>>>> slots, then it makes sense to refuse going above it.
>>>>>>
>>>>>> Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/xe/xe_reg_sr.c | 7 +++++++
>>>>>>  1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/xe/xe_reg_sr.c b/drivers/gpu/drm/xe/xe_reg_sr.c
>>>>>> index 0312823101ad..b82f704179a0 100644
>>>>>> --- a/drivers/gpu/drm/xe/xe_reg_sr.c
>>>>>> +++ b/drivers/gpu/drm/xe/xe_reg_sr.c
>>>>>> @@ -231,6 +231,13 @@ void xe_reg_sr_apply_whitelist(struct xe_reg_sr *sr, u32 mmio_base,
>>>>>>
>>>>>>          p = drm_debug_printer(KBUILD_MODNAME);
>>>>>>          xa_for_each(&sr->xa, reg, entry) {
>>>>>> +                if (slot == RING_MAX_NONPRIV_SLOTS) {
>>>>>> +                        drm_err(&xe->drm,
>>>>>
>>>>>We probably want xe_gt_err() here so that we'll know which GT we hit
>>>>>this error on.  Also, it wouldn't hurt to pass hwe as a parameter to
>>>>>this function (instead of mmio_base) and include hwe->name in the error
>>>>>message here so that we also know which engine ran out of slots.
>>>>
>>>>Yeah, makes sense. Arguably, we could simply pass hwe as a single
>>>>parameter and figure out the rest. Is there any reason not to do that?
>>>>
>>>>If the above is something reasonable, I wonder if xe_reg_sr.c is really
>>>>the correct place to have this function. Maybe it should be something
>>>>like xe_hw_engine_apply_whilelist()?
>>>
>>>As long as xe_reg_sr_apply_mmio() is there, I think we could leave this
>>
>>By "there" do you mean "inside xe_reg_sr_apply_whitelist()"? I'm asking
>>because xe_reg_sr_apply_mmio() is not called by
>>xe_reg_sr_apply_whitelist().
>>
>>Or is "there" the whole file?
>
>yes

Ah, well... It looks to me that xe_reg_sr_apply_mmio() indeed belongs
here. Now, for the register whitelisting, I wonder if using the
xe_reg_sr infrastructure is the more correct thing. It works, but
it feels more like a special case... Maybe we could in the future
extract a platform/IP matching mechanism out of xe_rtp that can be used
for different types of actions, with general register programming (i.e.
rtp) and register whitelisting as two examples.

I think I'm going a bit off-topic now... So, for this patch, I guess we
can just keep the function here and make it either (i) receive only the
hwe or (ii) both the sr reference and hwe. Option (i) looks more correct
as it avoids redundancy in the parameter list, but at the same time
feels a bit weird because all exported xe_reg_sr_*() functions receive
the sr reference.

>
>>
>>>one in xe_reg_sr.c. If we are going to migrate it somewhere, then
>>>probably xe_reg_whitelist.c would be a better home so the RING_FORCE_*
>>
>>Yeah, that makes sense.
>>
>>>are all contained in that unit.... We could then add a func() action and
>>>remove XE_RTP_ACTION_WHITELIST() too. Not sure sure it's worth though.
>>
>>Well, we could at least do the move to xe_reg_whitelist.c for now, and
>>think about XE_RTP_ACTION_WHITELIST() in another patch eventually.
>
>I was meaning more a XE_RTP_ACTION_FUNC() and then it's the caller
>resposibility to do whatever they want. Similarly to how we have a func
>for the condition (XE_RTP_RULE_FUNC())

Yeah, or maybe do (in the future) something like what I'm suggesting
above: to extract a matching mechanism that can be used for multiple
purposes.

--
Gustavo Sousa