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

[Lucas De Marchi]

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

>
>>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())

Lucas De Marchi

>
>Let me know if that would be acceptable.
>
>--
>Gustavo Sousa