Stop using PWRITE #1160

gfxstrand · 2021-03-11T20:03:02Z

We're attempting to remove PWRITE support from i915 for future platforms and this prepares the media driver for the move.

My attempt at media driver hacking looks correct to me but I've only compile-tested it.

This maps to the kernel's PWRITE ioctl but there's no real advantage in using it since the first thing the kernel does is i915_gem_object_wait(). We may as well do the wait in userspace and just write to it through a map. We're attempting to remove PWRITE support from i915 for future platforms and this prepares the media driver for the move.

Now that we've removed the two callers, this is all dead code.

The rationale for this change is roughly as follows: 1. The functionality can be done entirely in userspace with a combination of mmap + memcpy 2. The only reason anyone in userspace is still using it is because someone implemented bo_subdata that way in libdrm ages ago and they're all too lazy to write the 5 lines of code to do a map. 3. This falls cleanly into the category of things which will only get more painful with local memory support. These ioctls aren't used much anymore by "real" userspace drivers. Vulkan has never used them and neither has the iris GL driver. The old i965 GL driver does use PWRITE for glBufferSubData but it only supports up through Gen11; Gen12 was never enabled in i965. The compute driver has never used PREAD/PWRITE. The only remaining user is the media driver which uses it exactly twice and they're easily removed [1] so expecting them to drop it going forward is reasonable. IGT changes which handle this kernel change have also been submitted [2]. [1] intel/media-driver#1160 [2] https://patchwork.freedesktop.org/series/81384/ v2 (Jason Ekstrand): - Improved commit message with the status of all usermode drivers - A more future-proof platform check Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>

The rationale for this change is roughly as follows: 1. The functionality can be done entirely in userspace with a combination of mmap + memcpy 2. The only reason anyone in userspace is still using it is because someone implemented bo_subdata that way in libdrm ages ago and they're all too lazy to write the 5 lines of code to do a map. 3. This falls cleanly into the category of things which will only get more painful with local memory support. These ioctls aren't used much anymore by "real" userspace drivers. Vulkan has never used them and neither has the iris GL driver. The old i965 GL driver does use PWRITE for glBufferSubData but it only supports up through Gen11; Gen12 was never enabled in i965. The compute driver has never used PREAD/PWRITE. The only remaining user is the media driver which uses it exactly twice and they're easily removed [1] so expecting them to drop it going forward is reasonable. IGT changes which handle this kernel change have also been submitted [2]. [1] intel/media-driver#1160 [2] https://patchwork.freedesktop.org/series/81384/ v2 (Jason Ekstrand): - Improved commit message with the status of all usermode drivers - A more future-proof platform check v3 (Jason Ekstrand): - Drop the HAS_LMEM checks as they're already covered by the version checks Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>

XinfengZhang · 2021-03-15T15:27:49Z

media_driver/linux/common/codec/ddi/media_ddi_decode_vp8.cpp

-        {
-            return VA_STATUS_ERROR_INVALID_PARAMETER;
-        }
+        mos_bo_wait_rendering(vp8ProbDataBuff->bo);


if there are no a bo_map , vp8ProbDataBuff->pData should be NULL, should we add some protection or just add bo_map before memory copy?

This is already guarded by

if (vp8ProbDataBuff->pData && probInputBuf)

if you look a couple lines up in the file. It should be fine.

yes, it already been checked, thanks

XinfengZhang · 2021-03-15T15:28:36Z

media_driver/linux/common/codec/ddi/media_ddi_decode_base.cpp

    }
+    status = MOS_SecureMemcpy((void *)(buf->pData + buf->uiOffset), size * numElements, data, size * numElements);


a bo_map before memory copy?

I can't tell what the right thing is to do here. If it really is as simple as dropping in a mos_bo_map() right after the mos_bo_wait_rendering(), I can do that. That said, I think this one is actually ok too. It's guarded by if(true == buf->bCFlushReq) and the one place that ever sets bCFlushReq = true does so right after setting pData.

That said, I'm happy to do whatever you want. I'm coding entirely blind here; I've never touched the media driver before so I don't know any of your conventions.

CFlushReq should be used for the sku which has no LLC, but in this case, someone used it to distinguish whether the bistream buffer is swapped into system memory or gpu memory. and yes, it is already mapped.

looks good to me.

XinfengZhang

thanks , I have no other concern.

zxye

Looks fine.

The rationale for this change is roughly as follows: 1. The functionality can be done entirely in userspace with a combination of mmap + memcpy 2. The only reason anyone in userspace is still using it is because someone implemented bo_subdata that way in libdrm ages ago and they're all too lazy to write the 5 lines of code to do a map. 3. This falls cleanly into the category of things which will only get more painful with local memory support. These ioctls aren't used much anymore by "real" userspace drivers. Vulkan has never used them and neither has the iris GL driver. The old i965 GL driver does use PWRITE for glBufferSubData but it only supports up through Gen11; Gen12 was never enabled in i965. The compute driver has never used PREAD/PWRITE. The only remaining user is the media driver which uses it exactly twice and they're easily removed [1] so expecting them to drop it going forward is reasonable. IGT changes which handle this kernel change have also been submitted [2]. [1] intel/media-driver#1160 [2] https://patchwork.freedesktop.org/series/81384/ v2 (Jason Ekstrand): - Improved commit message with the status of all usermode drivers - A more future-proof platform check v3 (Jason Ekstrand): - Drop the HAS_LMEM checks as they're already covered by the version checks Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>

The rationale for this change is roughly as follows: 1. The functionality can be done entirely in userspace with a combination of mmap + memcpy 2. The only reason anyone in userspace is still using it is because someone implemented bo_subdata that way in libdrm ages ago and they're all too lazy to write the 5 lines of code to do a map. 3. This falls cleanly into the category of things which will only get more painful with local memory support. These ioctls aren't used much anymore by "real" userspace drivers. Vulkan has never used them and neither has the iris GL driver. The old i965 GL driver does use PWRITE for glBufferSubData but it only supports up through Gen11; Gen12 was never enabled in i965. The compute driver has never used PREAD/PWRITE. The only remaining user is the media driver which uses it exactly twice and they're easily removed [1] so expecting them to drop it going forward is reasonable. IGT changes which handle this kernel change have also been submitted [2]. [1] intel/media-driver#1160 [2] https://patchwork.freedesktop.org/series/81384/ v2 (Jason Ekstrand): - Improved commit message with the status of all usermode drivers - A more future-proof platform check v3 (Jason Ekstrand): - Drop the HAS_LMEM checks as they're already covered by the version checks Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210317234014.2271006-4-jason@jlekstrand.net

gfxstrand added 2 commits March 11, 2021 13:59

Drop mos_bo_[get_]subdata

a8d010a

Now that we've removed the two callers, this is all dead code.

XinfengZhang requested review from Jister, Xiaogangli-intel, zxye and XinfengZhang March 15, 2021 01:28

Jister approved these changes Mar 15, 2021

View reviewed changes

XinfengZhang requested changes Mar 15, 2021

View reviewed changes

XinfengZhang approved these changes Mar 16, 2021

View reviewed changes

zxye approved these changes Mar 16, 2021

View reviewed changes

XinfengZhang added the verifying PR: fix ready and verifying with build/test label Mar 16, 2021

intel-mediadev merged commit e015df1 into intel:master Mar 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop using PWRITE #1160

Stop using PWRITE #1160

gfxstrand commented Mar 11, 2021 •

edited

Loading

XinfengZhang Mar 15, 2021

gfxstrand Mar 15, 2021 •

edited

Loading

XinfengZhang Mar 16, 2021

XinfengZhang Mar 15, 2021

gfxstrand Mar 15, 2021

XinfengZhang Mar 16, 2021 •

edited

Loading

zxye Mar 16, 2021

XinfengZhang left a comment

zxye left a comment

		}
		status = MOS_SecureMemcpy((void )(buf->pData + buf->uiOffset), size numElements, data, size * numElements);

Stop using PWRITE #1160

Stop using PWRITE #1160

Conversation

gfxstrand commented Mar 11, 2021 • edited Loading

XinfengZhang Mar 15, 2021

Choose a reason for hiding this comment

gfxstrand Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

XinfengZhang Mar 16, 2021

Choose a reason for hiding this comment

XinfengZhang Mar 15, 2021

Choose a reason for hiding this comment

gfxstrand Mar 15, 2021

Choose a reason for hiding this comment

XinfengZhang Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

zxye Mar 16, 2021

Choose a reason for hiding this comment

XinfengZhang left a comment

Choose a reason for hiding this comment

zxye left a comment

Choose a reason for hiding this comment

gfxstrand commented Mar 11, 2021 •

edited

Loading

gfxstrand Mar 15, 2021 •

edited

Loading

XinfengZhang Mar 16, 2021 •

edited

Loading