RE: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation
It is impossible to pass the PAT setting when sharing the fd. 1. All api , include vaapi, vulkan, gl... need such extension , the impact is huge. Even these API level accept , some middle ware , such as ffmpeg... also will reject it. 2. PAT is intel specific implementation , and MTL , LNL has different value and meanings, you could not expect application developer understand the details, it should be transparent to application developer. Thanks Carl From: Yang, Fei Sent: Friday, June 9, 2023 11:13 PM To: Andi Shyti ; Zhang, Carl Cc: Joonas Lahtinen ; Tvrtko Ursulin ; Chris Wilson ; Roper, Matthew D ; Justen, Jordan L ; Gu, Lihao ; Intel GFX ; DRI Devel Subject: Re: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation > Hi Carl, > >>>> besides this, ask a dumb question. >>>> How we retrieve the pat_index from a shared resource though dma_buf fd? >>>> maybe we need to know whether it could be CPU cached if we want map it. >>>> Of course, looks there are no real usage to access it though CPU. >>>> Just use it directly without any pat related options ? >>> >>> I am not understanding. Do you want to ask the PAT table to the driver? Are >>> you referring to the CPU PAT index? >>> >>> In any case, if I understood correctly, you don't necessarily always need to >>> set the PAT options and the cache options will fall into the default values. >>> >>> Please let me know if I haven't answered the question. >>> >> >> If mesa create a resource , then use DRM_IOCTL_PRIME_HANDLE_TO_FD convert it >> to a dma fd. >> Then share it to media, media use DRM_IOCTL_PRIME_FD_TO_HANDLE convert it to >> a gem bo. >> But media does not know the PAT index , because mesa create it and set it. >> So, if media want to call DRM_IOCTL_I915_GEM_MMAP_OFFSET, media does not >> know whether it could be WB. > > That's a good point. To be honest I am not really sure how this > is handled. > > Fei, Jordan? Do you have suggestion here? Is it possible to pass the PAT setting when sharing the fd? Or perhaps we should have kept the get_caching ioctl functional? Joonas, could you chime in here? > Andi
RE: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation
> -Original Message-
> From: Andi Shyti
> Sent: Wednesday, June 7, 2023 1:11 PM
> To: Zhang, Carl
> Cc: Andi Shyti ; Joonas Lahtinen
> ; Tvrtko Ursulin
> ; Yang, Fei ; Chris
> Wilson ; Roper, Matthew D
> ; Justen, Jordan L ;
> Gu, Lihao ; Intel GFX ;
> DRI Devel
> Subject: Re: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation
>
> Hi Carl,
>
> On Wed, Jun 07, 2023 at 03:40:20AM +, Zhang, Carl wrote:
> > Media driver reverted previous patches, and file a new PR
> > https://github.com/intel/media-driver/pull/1680
> > will hold this PR until the uapi changes appear in drm_next.
>
> That's great, thanks a lot for the quick actions here.
>
> Before pushing I am going to replace the Media part in the commit log with
> the following sentence:
>
> "
> The media driver supprt has bin submitted in this merge request:
> https://github.com/intel/media-driver/pull/1680
> "
>
> > besides this, ask a dumb question.
> > How we retrieve the pat_index from a shared resource though dma_buf fd?
> > maybe we need to know whether it could be CPU cached if we want map it.
> > Of course, looks there are no real usage to access it though CPU.
> > Just use it directly without any pat related options ?
>
> I am not understanding. Do you want to ask the PAT table to the driver? Are
> you referring to the CPU PAT index?
>
> In any case, if I understood correctly, you don't necessarily always need to
> set the PAT options and the cache options will fall into the default values.
>
> Please let me know if I haven't answered the question.
>
If mesa create a resource , then use DRM_IOCTL_PRIME_HANDLE_TO_FD convert it to
a dma fd.
Then share it to media, media use DRM_IOCTL_PRIME_FD_TO_HANDLE convert it to a
gem bo.
But media does not know the PAT index , because mesa create it and set it.
So, if media want to call DRM_IOCTL_I915_GEM_MMAP_OFFSET, media does not know
whether it could be WB.
> Andi
>
> > Thanks
> > Carl
> >
> > > -Original Message-
> > > From: Andi Shyti
> > > Sent: Tuesday, June 6, 2023 7:15 PM
> > > To: Joonas Lahtinen
> > > Cc: Andi Shyti ; Tvrtko Ursulin
> > > ; Yang, Fei ;
> > > Chris Wilson ; Roper, Matthew D
> > > ; Justen, Jordan L
> > > ; Zhang, Carl ; Gu,
> > > Lihao ; Intel GFX
> > > ; DRI Devel > > de...@lists.freedesktop.org>
> > > Subject: Re: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO
> > > creation
> > >
> > > > > > > To comply with the design that buffer objects shall have
> > > > > > > immutable cache setting through out their life cycle, {set,
> > > > > > > get}_caching ioctl's are no longer supported from MTL onward.
> > > > > > > With that change caching policy can only be set at object
> > > > > > > creation time. The current code applies a default (platform
> > > > > > > dependent)
> > > cache setting for all objects.
> > > > > > > However this is not optimal for performance tuning. The
> > > > > > > patch extends the existing gem_create uAPI to let user set
> > > > > > > PAT index for the object at creation time.
> > > > > > > The new extension is platform independent, so UMD's can
> > > > > > > switch to using this extension for older platforms as well,
> > > > > > > while {set, get}_caching are still supported on these legacy
> > > > > > > paltforms for
> > > compatibility reason.
> > > > > > > However, since PAT index was not clearly defined for
> > > > > > > platforms prior to
> > > > > > > GEN12 (TGL), so we are limiting this externsion to GEN12+
> > > > > > > platforms only. See ext_set_pat() in for the implementation
> > > > > > > details.
> > > > > > >
> > > > > > > Note: The documentation related to the PAT/MOCS tables is
> > > > > > > currently available for Tiger Lake here:
> > > > > > > https://www.intel.com/content/www/us/en/docs/graphics-for-li
> > > > > > > nux/ developer-reference/1-0/tiger-lake.html
> > > > > > >
> > > > > > > BSpec: 45101
> > > > > > >
> > > > > > > Mesa support has been submitted in this merge request:
> > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22
> > > > > > > 878
> > > > > > >
> > > > > > > The media driver is supported by the following commits:
> > > > > > > https://github.com/intel/media-
> > > driver/commit/92c00a857433ebb34ec
> > > > > > > 575e9834f473c6fcb6341
> > > > > > > https://github.com/intel/media-driver/commit/fd375cf2c5e1f6b
> > > > > > > f6b4
> > > > > > > 3258ff797b3134aadc9fd
> > > > > > > https://github.com/intel/media-
> > > driver/commit/08dd244b22484770a33
> > > > > > > 464c2c8ae85430e548000
> > > >
> > > > Andi, let's still get these corrected before merging once the
> > > > media-driver revert is completed.
> > >
> > > Sure!
> > >
> > > At least this doesn't need a new version to be respinned.
> > >
> > > Please, Carl, link the new pull request and I will update the commit log.
> > >
> > > Andi
RE: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation
Media driver reverted previous patches, and file a new PR
https://github.com/intel/media-driver/pull/1680
will hold this PR until the uapi changes appear in drm_next.
besides this, ask a dumb question.
How we retrieve the pat_index from a shared resource though dma_buf fd?
maybe we need to know whether it could be CPU cached if we want map it.
Of course, looks there are no real usage to access it though CPU.
Just use it directly without any pat related options ?
Thanks
Carl
> -Original Message-
> From: Andi Shyti
> Sent: Tuesday, June 6, 2023 7:15 PM
> To: Joonas Lahtinen
> Cc: Andi Shyti ; Tvrtko Ursulin
> ; Yang, Fei ; Chris
> Wilson ; Roper, Matthew D
> ; Justen, Jordan L ;
> Zhang, Carl ; Gu, Lihao ; Intel
> GFX ; DRI Devel de...@lists.freedesktop.org>
> Subject: Re: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation
>
> > > > > To comply with the design that buffer objects shall have
> > > > > immutable cache setting through out their life cycle, {set,
> > > > > get}_caching ioctl's are no longer supported from MTL onward.
> > > > > With that change caching policy can only be set at object
> > > > > creation time. The current code applies a default (platform dependent)
> cache setting for all objects.
> > > > > However this is not optimal for performance tuning. The patch
> > > > > extends the existing gem_create uAPI to let user set PAT index
> > > > > for the object at creation time.
> > > > > The new extension is platform independent, so UMD's can switch
> > > > > to using this extension for older platforms as well, while {set,
> > > > > get}_caching are still supported on these legacy paltforms for
> compatibility reason.
> > > > > However, since PAT index was not clearly defined for platforms
> > > > > prior to
> > > > > GEN12 (TGL), so we are limiting this externsion to GEN12+
> > > > > platforms only. See ext_set_pat() in for the implementation details.
> > > > >
> > > > > Note: The documentation related to the PAT/MOCS tables is
> > > > > currently available for Tiger Lake here:
> > > > > https://www.intel.com/content/www/us/en/docs/graphics-for-linux/
> > > > > developer-reference/1-0/tiger-lake.html
> > > > >
> > > > > BSpec: 45101
> > > > >
> > > > > Mesa support has been submitted in this merge request:
> > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
> > > > >
> > > > > The media driver is supported by the following commits:
> > > > > https://github.com/intel/media-
> driver/commit/92c00a857433ebb34ec
> > > > > 575e9834f473c6fcb6341
> > > > > https://github.com/intel/media-driver/commit/fd375cf2c5e1f6bf6b4
> > > > > 3258ff797b3134aadc9fd
> > > > > https://github.com/intel/media-
> driver/commit/08dd244b22484770a33
> > > > > 464c2c8ae85430e548000
> >
> > Andi, let's still get these corrected before merging once the
> > media-driver revert is completed.
>
> Sure!
>
> At least this doesn't need a new version to be respinned.
>
> Please, Carl, link the new pull request and I will update the commit log.
>
> Andi
RE: [PATCH v12 1/1] drm/i915: Allow user to set cache at BO creation
Hi Andi & Fei,
We verified your change by UMD change:
1. implement the uAPI by
https://github.com/intel/media-driver/commit/92c00a857433ebb34ec575e9834f473c6fcb6341
2. old kernel may not support new uAPI, so UMD try the interface firstly, if it
failed, will fallback to older interfaces
https://github.com/intel/media-driver/commit/fd375cf2c5e1f6bf6b43258ff797b3134aadc9fd
3. removed some check for CPU cacheable and PAT conflict
https://github.com/intel/media-driver/commit/08dd244b22484770a33464c2c8ae85430e548000
after that, UMD works with your patches serious on MTL.
Acked-by: Carl Zhang
Tested-by: Lihao Gu
Thanks
Carl
> -Original Message-
> From: Andi Shyti
> Sent: Wednesday, May 31, 2023 6:49 PM
> To: Yang, Fei ; Zhang, Carl
> Cc: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vivi,
> Rodrigo ; andi.sh...@linux.intel.com; Chris Wilson
> ; Roper, Matthew D
> ; Justen, Jordan L
> Subject: Re: [PATCH v12 1/1] drm/i915: Allow user to set cache at BO creation
>
> Hi Carl,
>
> On Wed, May 24, 2023 at 01:02:55PM -0700, fei.y...@intel.com wrote:
> > From: Fei Yang
> >
> > To comply with the design that buffer objects shall have immutable
> > cache setting through out their life cycle, {set, get}_caching ioctl's
> > are no longer supported from MTL onward. With that change caching
> > policy can only be set at object creation time. The current code
> > applies a default (platform dependent) cache setting for all objects.
> > However this is not optimal for performance tuning. The patch extends
> > the existing gem_create uAPI to let user set PAT index for the object
> > at creation time.
> > The new extension is platform independent, so UMD's can switch to
> > using this extension for older platforms as well, while {set,
> > get}_caching are still supported on these legacy paltforms for compatibility
> reason.
> >
> > BSpec: 45101
> >
> > Test igt@gem_create@create_ext_set_pat posted at
> > https://patchwork.freedesktop.org/series/118314/
> >
> > Tested with
> > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
> >
> > Signed-off-by: Fei Yang
> > Cc: Chris Wilson
> > Cc: Matt Roper
> > Cc: Andi Shyti
> > Reviewed-by: Andi Shyti
> > Acked-by: Jordan Justen
> > Tested-by: Jordan Justen
>
> was it your intention to ack this patch?
>
> Thanks,
> Andi
