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

[Matt Roper]

On Tue, May 30, 2023 at 09:54:55AM -0300, Gustavo Sousa wrote:
> 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.

Option (i) sounds fine to me.  No real need to pass redundant
parameters as long as we can extract the sr from what does get passed.

Thanks.


Matt

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

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation