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

[Gustavo Sousa]

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?

>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.

Let me know if that would be acceptable.

--
Gustavo Sousa