[Intel-xe] [PATCH] drm/xe/xe_exec_queue: Add check for access counter granularity

[Zeng, Oak]

Thanks,
Oak

> -----Original Message-----
> From: Welty, Brian <brian.welty at intel.com>
> Sent: Friday, October 20, 2023 4:32 PM
> To: Dandamudi, Priyanka <priyanka.dandamudi at intel.com>; Zeng, Oak
> <oak.zeng at intel.com>; intel-xe at lists.freedesktop.org; Kumar, Janga Rahul
> <janga.rahul.kumar at intel.com>
> Subject: Re: [Intel-xe] [PATCH] drm/xe/xe_exec_queue: Add check for access
> counter granularity
> 
> 
> On 10/19/2023 9:50 PM, priyanka.dandamudi at intel.com wrote:
> > From: Priyanka Dandamudi <priyanka.dandamudi at intel.com>
> >
> > Add conditional check for access counter granularity.
> > This check will return -EINVAL if granularity is beyond 64M
> > which is a hardware limitation.
> >
> > v2: Defined
> > XE_ACC_GRANULARITY_128K 0
> > XE_ACC_GRANULARITY_2M 1
> > XE_ACC_GRANULARITY_16M 2
> > XE_ACC_GRANULARITY_64M 3
> > as part of uAPI.
> > So, that user can also use it.(Oak)
> >
> > Cc: Oak Zeng <oak.zeng at intel.com>
> > Cc: Janga Rahul Kumar <janga.rahul.kumar at intel.com>
> > Signed-off-by: Priyanka Dandamudi <priyanka.dandamudi at intel.com>
> > Reviewed-by: Oak Zeng <oak.zeng at intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_exec_queue.c | 3 +++
> >   include/uapi/drm/xe_drm.h          | 5 +++++
> >   2 files changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c
> b/drivers/gpu/drm/xe/xe_exec_queue.c
> > index dd61c4267e24..4fd44a9203e4 100644
> > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > @@ -393,6 +393,9 @@ static int exec_queue_set_acc_granularity(struct
> xe_device *xe, struct xe_exec_q
> >   	if (XE_IOCTL_DBG(xe, !xe->info.supports_usm))
> >   		return -EINVAL;
> >
> > +	if (value > XE_ACC_GRANULARITY_64M)
> > +		return -EINVAL;
> > +
> >   	q->usm.acc_granularity = value;
> >
> >   	return 0;
> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index 24bf8f0f52e8..8b5e7e212f61 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -126,6 +126,11 @@ struct xe_user_extension {
> >   #define DRM_IOCTL_XE_WAIT_USER_FENCE
> 	DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE,
> struct drm_xe_wait_user_fence)
> >   #define DRM_IOCTL_XE_VM_MADVISE
> DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_MADVISE, struct
> drm_xe_vm_madvise)
> >
> 
> Maybe it's obvious, but I think nice to add a comment here before the
> defines,

Agreed, calling it "granularity" is a little confusing, i.e., _128K means monitor 32x4k regions, so total of 128k region monitored. Maybe say something like below, see inline

> /* For use with XE_EXEC_QUEUE_SET_PROPERTY_ACC_GRANULARITY */
> 
/*Monitor 128KB contiguous region with 4K sub-granularity*/
> > +#define XE_ACC_GRANULARITY_128K 0
Monitor 2MB contiguous region with 64KB sub-granularity
> > +#define XE_ACC_GRANULARITY_2M 1
Monitor 16MB contiguous region with 512KB sub-granularity
> > +#define XE_ACC_GRANULARITY_16M 2
Monitor 64MB contiguous region with 2M sub-granularity

Oak
> > +#define XE_ACC_GRANULARITY_64M 3
> 
> And a bit better to place these further down below, before the
> drm_xe_exec_queue_set_property structure is defined.
> 
> -Brian
> 
> 
> > +
> >   /** struct drm_xe_engine_class_instance - instance of an engine class */
> >   struct drm_xe_engine_class_instance {
> >   #define DRM_XE_ENGINE_CLASS_RENDER		0