WIP: minigbm: using dri extensions [chromiumos/platform/minigbm : master]
Satyajit Sahu (Gerrit)
Satyajit Sahu has uploaded this change for review.
WIP: minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
This patch is depedent on mesa CL:863268
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M helpers.c
5 files changed, 419 insertions(+), 278 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Is this ready to be reviewed?
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #2 to this change.
WIP: minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and verified basic use-cases
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
4 files changed, 492 insertions(+), 273 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
9 comments:
Please remove this if this is ready for review.
Patch Set #2, Line 14:
Chrome booted to UI and verified basic use-casesPlease pass graphics_Gbm with this backend too. You can copy the test directly onto the DUT as well:
scp /build/kahlee/usr/local/build/autotest/client/site_tests/graphics_Gbm/src/gbmtest root@IP1:/tmp/
(on dut)
/tmp/gbmtest
nit: extraneous whitespace:
*bo_size = info.alloc_size;
*base_align = info.phys_alignment;
*tile_flags = info.metadata.tiling_info;The reason for this entire function seems to be to get the *bo_size. Can't you just do stride (returned from __DRI_IMAGE_ATTRIB_STRIDE) * height?
malloc(sizeof(struct dri_driver_priv));
memset(drv->priv, 0, sizeof(struct dri_driver_priv));calloc(1, sizeof(struct dri_driver_priv));
ret = dri_bo_create(bo, width, height, format, use_flags, &flink_name);
if (flink_name) {
amdgpu_get_gpu_info(drv_get_fd(bo->drv), flink_name, bo->sizes,
&bo->tiling, &base_align);
bo->total_size = bo->sizes[0];
}
}
gem_create.in.bo_size = bo->total_size;
gem_create.in.alignment = base_align;
gem_create.in.domain_flags = 0;
if (use_flags & (BO_USE_LINEAR | BO_USE_SW)) {
gem_create.in.domain_flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
bo->tiling = 1;
}
if (use_flags & (BO_USE_SCANOUT | BO_USE_CURSOR)) {
/* TODO(dbehr) do not use VRAM after we enable display VM */
gem_create.in.domains = AMDGPU_GEM_DOMAIN_VRAM;
} else {
gem_create.in.domains = AMDGPU_GEM_DOMAIN_GTT;
if (!(use_flags & BO_USE_SW_READ_OFTEN))
gem_create.in.domain_flags |= AMDGPU_GEM_CREATE_CPU_GTT_USWC;
}
/* Allocate the buffer with the preferred heap. */
ret = drmCommandWriteRead(drv_get_fd(bo->drv), DRM_AMDGPU_GEM_CREATE,
&gem_create, sizeof(gem_create));
if (ret < 0)
return ret;
metadata.tiling_info = bo->tiling;
for (size_t plane = 0; plane < bo->num_planes; plane++)
bo->handles[plane].u32 = gem_create.out.handle;
ret = amdgpu_set_metadata(drv_get_fd(bo->drv), bo->handles[0].u32, &metadata);I'm confused at what you're doing here. We call dri_bo_create, and then also call DRM_AMDGPU_GEM_CREATE. The dri backend should just call dri_bo_create, as it originally was in Nicolai's branch (https://cgit.freedesktop.org/~nh/minigbm/).
If you want to use the GBM helpers for YUV buffers, and the DRI backend for normal buffers, that's fine too. But's not what you're doing here.
Patch Set #2, Line 248:
if (bo->tiling == 1) {What does a tiling of 1 indicate? Can you use TILE_TYPE_NON_DISPLAYABLE in this case?
memset(&gem_map, 0, sizeof(gem_map));
gem_map.in.handle = bo->handles[plane].u32;
ret = drmIoctl(bo->drv->fd, DRM_IOCTL_AMDGPU_GEM_MMAP, &gem_map);
if (ret) {
fprintf(stderr, "drv: DRM_IOCTL_AMDGPU_GEM_MMAP failed\n");
return MAP_FAILED;
}
vma->length = bo->total_size;
vma->addr = mmap(0, bo->total_size, map_flags, MAP_SHARED,
bo->drv->fd, gem_map.out.addr_ptr);
return vma->addr;Why wouldn't the dri backend be able to handle this as well?
Patch Set #2, Line 298:
.bo_map = amdgpu_map_bo,what about .bo_invalidate and .bo_flush? Nicolai seemed to indicate that was possible in his email. Will the __DRI2flushExtensionRec work in this case?
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Dominik Behr (Gerrit)
3 comments:
Patch Set #2, Line 248:
if (bo->tiling == 1) {
What does a tiling of 1 indicate? Can you use TILE_TYPE_NON_DISPLAYABLE in this case?
I think it may be linear general or linear aligned and in this case simple mmap should give linear view. We should use enums here instead of number.
Patch Set #2, Line 117:
/* Version 13 for __DRI_IMAGE_ATTRIB_OFFSET */13 or 12?
how is it possible that we do not have context here? it should be created in dri_init. And if dri_init has not been called how is it safe to call createNewContext()?
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
11 comments:
Please remove this if this is ready for review.
Done
Patch Set #2, Line 14:
Chrome booted to UI and verified basic use-cases
Please pass graphics_Gbm with this backend too. […]
graphics_Gbm passes after a small change in the autotest. Here is the patch
https://chromium-review.googlesource.com/c/chromiumos/third_party/autotest/+/910810
nit: extraneous whitespace:
Done
*bo_size = info.alloc_size;
*base_align = info.phys_alignment;
*tile_flags = info.metadata.tiling_info;
The reason for this entire function seems to be to get the *bo_size. […]
This function also sets the tiling_flags and base_align value.
malloc(sizeof(struct dri_driver_priv));
memset(drv->priv, 0, sizeof(struct dri_driver_priv));calloc(1, sizeof(struct dri_driver_priv));
Done
I'm confused at what you're doing here. […]
DRM_AMDGPU_GEM_CREATE is needed if flink_name is set. Next patch I'll take care of cleaning up of this part of the code.
Patch Set #2, Line 248:
if (bo->tiling == 1) {
I think it may be linear general or linear aligned and in this case simple mmap should give linear v […]
Yes that's correct. Basically the tiling flag is overwritten to linear i.e. 1. enum might not help here as what should be the value of non linear?
memset(&gem_map, 0, sizeof(gem_map));
gem_map.in.handle = bo->handles[plane].u32;
ret = drmIoctl(bo->drv->fd, DRM_IOCTL_AMDGPU_GEM_MMAP, &gem_map);
if (ret) {
fprintf(stderr, "drv: DRM_IOCTL_AMDGPU_GEM_MMAP failed\n");
return MAP_FAILED;
}
vma->length = bo->total_size;
vma->addr = mmap(0, bo->total_size, map_flags, MAP_SHARED,
bo->drv->fd, gem_map.out.addr_ptr);
return vma->addr;Why wouldn't the dri backend be able to handle this as well?
for linear data only mmap should work instead of mesa's mapImage.
Patch Set #2, Line 298:
.bo_map = amdgpu_map_bo,
what about .bo_invalidate and .bo_flush? Nicolai seemed to indicate that was possible in his email. […]
It should work. Not tried yet.
Patch Set #2, Line 117:
/* Version 13 for __DRI_IMAGE_ATTRIB_OFFSET */13 or 12?
Changing to 13
how is it possible that we do not have context here? it should be created in dri_init. […]
this if case wont be hit at all. removing it.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #3 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
4 files changed, 501 insertions(+), 288 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #4 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
4 files changed, 501 insertions(+), 288 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
3 comments:
ret = dri_bo_create(bo, width, height, format, use_flags, &flink_name);
if (flink_name) {
ret = amdgpu_get_gpu_info(drv_get_fd(bo->drv), flink_name,
&bo->tiling, &base_align);
if (ret)
return ret;
bo->strides[0] = ALIGN(bo->strides[0], 256);
bo->total_size = bo->sizes[0] = bo->strides[0] * height;
}
}
if (!bo->handles[0].u32) {
gem_create.in.bo_size = bo->total_size;
gem_create.in.alignment = base_align;
gem_create.in.domain_flags = 0;
if (use_flags & (BO_USE_LINEAR | BO_USE_SW)) {
gem_create.in.domain_flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
//set it to linear
bo->tiling = 1;
}
if (use_flags & (BO_USE_SCANOUT | BO_USE_CURSOR)) {
/* TODO(dbehr) do not use VRAM after we enable display VM */
gem_create.in.domains = AMDGPU_GEM_DOMAIN_VRAM;
} else {
gem_create.in.domains = AMDGPU_GEM_DOMAIN_GTT;
if (!(use_flags & BO_USE_SW_READ_OFTEN))
gem_create.in.domain_flags |= AMDGPU_GEM_CREATE_CPU_GTT_USWC;
}
/* Allocate the buffer with the preferred heap. */
ret = drmCommandWriteRead(drv_get_fd(bo->drv), DRM_AMDGPU_GEM_CREATE,
&gem_create, sizeof(gem_create));
if (ret < 0)
return ret;
metadata.tiling_info = bo->tiling;
DRM_AMDGPU_GEM_CREATE is needed if flink_name is set. […]
So all of this code is an error case if dri_bo_create fails?
In my opinion, that's too complicated. We should expect drivers using the DRI backend to use the DRI backend successfully. If not, we can use return NULL, but having a complicated fallback path is unnecessary.
Also IIRC, flink is considered insecure and that's why GEM is preferred. Also render node won't be able to use it (cros_gralloc). So just try get the handle from __DRI_IMAGE_ATTRIB_HANDLE and forget about flink.
static void *amdgpu_map_bo(struct bo *bo, struct vma *vma, size_t plane, uint32_t map_flags) {
union drm_amdgpu_gem_mmap gem_map;
int ret = 0;
if (bo->tiling == 1) {
memset(&gem_map, 0, sizeof(gem_map));
gem_map.in.handle = bo->handles[plane].u32;
ret = drmIoctl(bo->drv->fd, DRM_IOCTL_AMDGPU_GEM_MMAP, &gem_map);
if (ret) {
fprintf(stderr, "drv: DRM_IOCTL_AMDGPU_GEM_MMAP failed\n");
return MAP_FAILE
for linear data only mmap should work instead of mesa's mapImage.
If Mesa's mapImage can handle linear data, why not use that instead? The performance differences will be negligible.
Patch Set #2, Line 298:
.init = amdgpu_init,
It should work. Not tried yet.
Well, now's a good time as ever to add it. The gbmtest should exercise that path.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
3 comments:
So all of this code is an error case if dri_bo_create fails? […]
dri_init-->createNewScreen-->amdgpu_device_initialize.
amdgpu_device_initialize, if called with a fd pointing to a primary device (it is /dev/dri/card0 for us) which has been initialized earlier, then the previous amdgpu_device handle is returned and the new fd is stored in a variable named flink_fd.
Here is the link to the code.
https://cs.chromium.org/chromium/src/third_party/libdrm/src/amdgpu/amdgpu_device.c?l=209In our case createNewScreen seems to be called (may be from broswer) before minigbm init. So the amdgpu_device_handle we get, is not corresponding to drv->fd of minigbm.
The gem handle returned by queryImage (__DRI_IMAGE_ATTRIB_HANDLE) is also not corresponding to the drv->fd. So a new gem_handle needs to be created. For that tiling info is needed. queryImage does not provide this data.To get the tiling info and base_alignment flink handle is used here. queryImage for __DRI_IMAGE_ATTRIB_NAME calles amdgpu_bo_export_flink of libdrm. Here is the link
https://cs.chromium.org/chromium/src/third_party/libdrm/src/amdgpu/amdgpu_bo.c?l=183So flink is not an error case here. We are using gem_handle only, flink is used just to get tiling info.
static void *amdgpu_map_bo(struct bo *bo, struct vma *vma, size_t plane, uint32_t map_flags) {
union drm_amdgpu_gem_mmap gem_map;
int ret = 0;
if (bo->tiling == 1) {
memset(&gem_map, 0, sizeof(gem_map));
gem_map.in.handle = bo->handles[plane].u32;
ret = drmIoctl(bo->drv->fd, DRM_IOCTL_AMDGPU_GEM_MMAP, &gem_map);
if (ret) {
fprintf(stderr, "drv: DRM_IOCTL_AMDGPU_GEM_MMAP failed\n");
return MAP_FAILE
If Mesa's mapImage can handle linear data, why not use that instead? The performance differences wi […]
I did a quick try for the same. When ran mmap_test -g the output is garbled. I need to look into the issue. We will open a separate bug for the same.
Patch Set #2, Line 298:
.init = amdgpu_init,
Well, now's a good time as ever to add it. The gbmtest should exercise that path.
Can this be taken as a separate feature.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
4 comments:
Patch Set #2, Line 14:
Chrome booted to UI and passed graphics_Gbm autographics_Gbm passes after a small change in the autotest. Here is the patch […]
You also need to verify the Android container still boots with this patch. Also, I recommend you start the youtube app to verify video playback. Here are the instructions:
cros_workon --board=kahlee start arc-cros-gralloc
emerge-kahlee arc-cros-gralloc
sh ~/trunk/src/private-overlays/project-cheets-private/scripts/deploy_vendor_image.sh $IP
dri_init-->createNewScreen-->amdgpu_device_initialize. […]
You use .base_align and bo->tiling as inputs to DRM_AMDGPU_GEM_CREATE and DRM_AMDGPU_GEM_METADATA, respectively.
Why are you calling DRM_AMDGPU_GEM_CREATE when dri_bo_create (specifically, dri->image_extension->createImage) is already supposed to create the buffer?
You seem to only use the tiling in amdgpu_map_bo. Why don't you just say it's TILE_MODE_2D_TILED when we call dri_bo_create? Why are you fishing for the exact tiling via flink if we don't really need to know (since the dri extension knows)?
static void *amdgpu_map_bo(struct bo *bo, struct vma *vma, size_t plane, uint32_t map_flags) {
union drm_amdgpu_gem_mmap gem_map;
int ret = 0;
if (bo->tiling == 1) {
memset(&gem_map, 0, sizeof(gem_map));
gem_map.in.handle = bo->handles[plane].u32;
ret = drmIoctl(bo->drv->fd, DRM_IOCTL_AMDGPU_GEM_MMAP, &gem_map);
if (ret) {
fprintf(stderr, "drv: DRM_IOCTL_AMDGPU_GEM_MMAP failed\n");
return MAP_FAILE
I did a quick try for the same. When ran mmap_test -g the output is garbled. […]
The easiest case for mapImage is linear data. I'd bet the bug is probably in this patch rather than Mesa, since it's the newest and least tested.
Patch Set #2, Line 298:
.init = amdgpu_init,
Can this be taken as a separate feature.
No. Without invalidate/flush, the tiling will still be incorrect for certain tests.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Dominik Behr (Gerrit)
1 comment:
Patch Set #2, Line 248:
if (bo->tiling == 1) {Yes that's correct. Basically the tiling flag is overwritten to linear i.e. 1. […]
You already define some tile modes
#define TILE_MODE_LINEAR 0
#define TILE_MODE_2D_TILED 4
shouldnt we have TILE_MODE_LINEAR_ALIGNED 1 and use it?
Also, doesnt this case also apply to bo->tiling == 0?
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
5 comments:
Patch Set #2, Line 14:
Chrome booted to UI and passed graphics_Gbm auto
You also need to verify the Android container still boots with this patch. […]
I have verified youtube. I'll test the android part
You use . […]
The gem_handle obtained from queryImage is not corresponding to the drv->fd. If the bo->handle is directly set to the gem_handle, UI fails with below error.
ERROR:gbm_buffer.cc(206)] Failed to export buffer to dma_buf: No such file or directory (2)After new gem_handle is created and the proper tiling flag are set then UI is proper.
Tiling flags are needed to be set on gem handle to get the proper UI.
You already define some tile modes […]
Yes, this can be done. Currently mesa's mapImage for linear data is having some issue. Working on that. Once that is fixed this part of the code can be removed.
static void *amdgpu_map_bo(struct bo *bo, struct vma *vma, size_t plane, uint32_t map_flags) {
union drm_amdgpu_gem_mmap gem_map;
int ret = 0;
if (bo->tiling == 1) {
memset(&gem_map, 0, sizeof(gem_map));
gem_map.in.handle = bo->handles[plane].u32;
ret = drmIoctl(bo->drv->fd, DRM_IOCTL_AMDGPU_GEM_MMAP, &gem_map);
if (ret) {
fprintf(stderr, "drv: DRM_IOCTL_AMDGPU_GEM_MMAP failed\n");
return MAP_FAILE
The easiest case for mapImage is linear data. […]
I dumped the mapped data in mmap_test.c. Only the first two frames are corrupted the other frames are proper. I am looking into this issue.
Patch Set #2, Line 298:
.init = amdgpu_init,
No. Without invalidate/flush, the tiling will still be incorrect for certain tests.
Ok. I'll add invalidate and flush.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
2 comments:
Patch Set #2, Line 14:
Chrome booted to UI and passed graphics_Gbm auto
I have verified youtube. […]
Test Android sooner rather than later. I'm pretty sure the DRI_PATH ("/usr/lib64/dri/radeonsi_dri.so") is not present in the container (it'll be "/vendor/lib64/radeon_si.so"). You'll have to figure how to make it work for both cases.
But you're allocating two GEM buffers -- one from createImage and one from DRM_AMDGPU_GEM_CREATE. That's not desirable.
The gem_handle obtained from queryImage is not corresponding to the drv->fd.
You can query __DRI_IMAGE_ATTRIB_FD and then do drmPrimeHandleToFd into drv->fd when you're creating.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Also,
1 comment:
Patch Set #4, Line 124:
drv_get_fd(drv),Also, why is the gem namespace of the DRI screen/context different from the minigbm one? We call createNewScreen2 with the minigbm fd (drv_get_fd(drv)).
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
1 comment:
But you're allocating two GEM buffers -- one from createImage and one from DRM_AMDGPU_GEM_CREATE. […]
After modify gbm_bo_get_fd to return prime fd handle return by queryImage __DRI_IMAGE_ATTRIB_FD, the browser (i.e. Failed to export buffer to dma_buf) is fixed. Now I dont see any error in /var/log/ui/ui.LATEST.
But still UI crashes. Debugging further.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
1 comment:
After modify gbm_bo_get_fd to return prime fd handle return by queryImage __DRI_IMAGE_ATTRIB_FD, the […]
The browser failure was because of mmap failure. mmap was failing with error 13 (permission denied).
queryImage for __DRI_IMAGE_ATTRIB_FD calls amdgpu_bo_export of libdrm. In amdgpu_bo_export drmPrimeHandleToFD DRM_RDWR flag is not there. Because of this mmap fails for the returned prime_fd.
https://cs.chromium.org/chromium/src/third_party/libdrm/src/amdgpu/amdgpu_bo.c?l=253After adding DRM_RDWR flag, UI comes up.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
3 comments:
Patch Set #2, Line 14:
Chrome booted to UI and passed graphics_Gbm auto
Test Android sooner rather than later. I'm pretty sure the DRI_PATH ("/usr/lib64/dri/radeonsi_dri. […]
arc-cros-gralloc build is failing due to missing GL headers in android container. arc-mesa is not exporting the GL headers to android container. I have raised a separate bug for this.
The browser failure was because of mmap failure. mmap was failing with error 13 (permission denied). […]
libdrm modification has been pushed to upstream.
https://patchwork.freedesktop.org/patch/206802/
static void *amdgpu_map_bo(struct bo *bo, struct vma *vma, size_t plane, uint32_t map_flags) {
union drm_amdgpu_gem_mmap gem_map;
int ret = 0;
if (bo->tiling == 1) {
memset(&gem_map, 0, sizeof(gem_map));
gem_map.in.handle = bo->handles[plane].u32;
ret = drmIoctl(bo->drv->fd, DRM_IOCTL_AMDGPU_GEM_MMAP, &gem_map);
if (ret) {
fprintf(stderr, "drv: DRM_IOCTL_AMDGPU_GEM_MMAP failed\n");
return MAP_FAILE
I dumped the mapped data in mmap_test.c. […]
gbm_bo_unmap was calling drv_bo_flush instead of drv_bo_unmap. After changing it to call unmap and adding flush after unmap, now mmap_test -g is proper.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #5 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M gbm.c
5 files changed, 495 insertions(+), 302 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
5 comments:
Patch Set #2, Line 14:
Chrome booted to UI and passed graphics_Gbm autoarc-cros-gralloc build is failing due to missing GL headers in android container. […]
This needs to be fixed before this patch can land. We don't want to break the Kahlee build.
stWhy do you need this? The alignment should be set at allocation time.
case DRM_FORMAT_FLEX_YCbCr_420_888:
return DRM_FORMAT_NV12;
default:
return format;
}
}
const struct backend backend_amdgpu = {
.name = "amdgpu",
.init = amdgpu_init,
.close = amdgpu_close,
.bo_create = amdgpgbm_bo_unmap was calling drv_bo_flush instead of drv_bo_unmap. […]
That's the desired behavior. Doing flush instead of unmap works on all other platforms. You need to add a flush implementation.
Please use enum.
nit: extraneous line
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
5 comments:
Patch Set #2, Line 14:
Chrome booted to UI and passed graphics_Gbm auto
This needs to be fixed before this patch can land. We don't want to break the Kahlee build.
I have opened a new bug for this. I am working on it.
stWhy do you need this? The alignment should be set at allocation time.
Removed in Patch #5
case DRM_FORMAT_FLEX_YCbCr_420_888:
return DRM_FORMAT_NV12;
default:
return format;
}
}
const struct backend backend_amdgpu = {
.name = "amdgpu",
.init = amdgpu_init,
.close = amdgpu_close,
.bo_create = amdgp
That's the desired behavior. Doing flush instead of unmap works on all other platforms. […]
I'll implement flush and check.
Please use enum.
This part of the code has been removed in Patch #5
nit: extraneous line
Done
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
6 comments:
int prime_fd;
we already have a field in the struct bo for the prime_fd. No need to duplicate.
}
ret = drmPrimeFDToHandle(bo->drv->fd, bo_dri->prime_fd, (uint32_t*)&bo_handle);
if (ret)
fprintf(stderr, "drmPrimeFDToHandle in %s failed ret %d\n",
__func__, ret);close the prime_fd after using it.
&bo_dri->prime_fd))
close prime_fd after using it.
if (bo_dri->prime_fd >= 0) {
close(bo_dri->prime_fd);
}not needed here.
Patch Set #5, Line 333:
dri->image_extension->unmapImage(dri->context, bo_dri->image, vma->priv);Check return value and propagate.
dri->flush_extension->flush_with_flags(dri->context, NULL, __DRI2_FLUSH_CONTEXT, 0);
you need to implement bo_flush callback (amdgpu_bo_flush) and make that call dri->flush_extension->flush_with_flags(dri->context, NULL, __DRI2_FLUSH_CONTEXT, 0). Dont unmap the image before flushing.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Also can you run ./presubmit.sh. That'll make this patch follow our clang-format rules.
Satyajit Sahu (Gerrit)
6 comments:
int prime_fd;
we already have a field in the struct bo for the prime_fd. No need to duplicate.
struct bo doesnt have a field for prime_fd.
}
ret = drmPrimeFDToHandle(bo->drv->fd, bo_dri->prime_fd, (uint32_t*)&bo_handle);
if (ret)
fprintf(stderr, "drmPrimeFDToHandle in %s failed ret %d\n",
__func__, ret);close the prime_fd after using it.
I'll modify it to a local variable and close it after using.
&bo_dri->prime_fd))
close prime_fd after using it.
I'll modify it to a local variable and close it after using.
if (bo_dri->prime_fd >= 0) {
close(bo_dri->prime_fd);
}not needed here.
Done
Patch Set #5, Line 333:
dri->image_extension->unmapImage(dri->context, bo_dri->image, vma->priv);Check return value and propagate.
Done
dri->flush_extension->flush_with_flags(dri->context, NULL, __DRI2_FLUSH_CONTEXT, 0);
you need to implement bo_flush callback (amdgpu_bo_flush) and make that call dri->flush_extension->f […]
I did try that, but with only flush (without unmap) still see the garbled output for mmap_test -g.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Looking better and better. Here's what remains, for those keeping track:
1) arc-cros-gralloc needs to build with this patch, Android needs to boot.
2) flush_with_flags needs to be implemented.
3) various coding nits.
1 comment:
dri->flush_extension->flush_with_flags(dri->context, NULL, __DRI2_FLUSH_CONTEXT, 0);
I did try that, but with only flush (without unmap) still see the garbled output for mmap_test -g.
It seems amdgpu doesn't implement the flush_with_flags callback. To verify this, run:
grep -r "flush_with_flags" .
in a Mesa source tree. Can you communicate to AMD Mesa engineers the desire to implement this? According to my prior conversation with Nicolai, this should be possible and not too difficult.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #6 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
4 files changed, 512 insertions(+), 298 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Patch Set 5:
(1 comment)
Looking better and better. Here's what remains, for those keeping track:
1) arc-cros-gralloc needs to build with this patch, Android needs to boot.
Verified android (play store) with new patch.
2) flush_with_flags needs to be implemented.
We are implementing the missing features in mesa.
3) various coding nits.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #7 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
4 files changed, 519 insertions(+), 302 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
We are implementing the missing features in mesa.
Implement those first, and then we can land this.
12 comments:
Patch Set #7, Line 25:
ARC_GRALLOCifdef __ANDROID__ should work fine (the Android cross-compiler seems to define it):
$ prebuilts/clang/host/linux-x86/clang-stable/bin/clang -dM -E - </dev/null | grep ANDROID
$$ prebuilts/clang/host/linux-x86/clang-stable/bin/clang -target x86_64-linux-android -dM -E - </dev/null | grep ANDROID
#define __ANDROID__ 1
struct bo;
struct driver;
struct mapping;
struct vma;remove this and #include "drv.h"
Patch Set #7, Line 25:
dri_driver_privnit: can you change this to dri_driver
Patch Set #7, Line 16:
#include "dri.h"move with the other local includes
Patch Set #7, Line 20:
#include <fcntl.h>not needed
Patch Set #7, Line 23:
#include <string.h>not needed
Patch Set #7, Line 24:
#include <sys/mman.h>not needed
Patch Set #7, Line 33:
struct dri_bo_priv {you don't need this. You just set:
bo->priv = image;Patch Set #7, Line 187:
bo_dri = (struct dri_bo_priv *)calloc(1, sizeof(*bo_dri));you shouldn't need this anymore
Patch Set #7, Line 229:
fail_freefree_image:
Patch Set #7, Line 230:
for(size_t plane = 0; plane < bo->num_planes; plane++) {add newline before this.
vma->addr = dri->image_extension->mapImage(
dri->context, bo_dri->image,
0, 0, bo->width, bo->height, /* rectangle to map */
map_flags, /* GBM flags and DRI flags are the same */
&stride, &vma->priv);the formatting looks weird. Make sure to ./presubmit.sh to get the proper formating.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Patch Set 7:
(12 comments)
We are implementing the missing features in mesa.
Implement those first, and then we can land this.
This is work in progress.
12 comments:
Patch Set #7, Line 25:
ARC_GRALLOC
ifdef __ANDROID__ should work fine (the Android cross-compiler seems to define it): […]
Done
struct bo;
struct driver;
struct mapping;
struct vma;
remove this and #include "drv. […]
Done
Patch Set #7, Line 25:
dri_driver_privnit: can you change this to dri_driver
Done
Patch Set #7, Line 16:
#include "dri.h"move with the other local includes
Done
Patch Set #7, Line 20:
#include <fcntl.h>not needed
Done
Patch Set #7, Line 23:
#include <string.h>not needed
strcmp is used inside lookup_extension
Patch Set #7, Line 24:
#include <sys/mman.h>not needed
required for MAP_FAILED
Patch Set #7, Line 33:
struct dri_bo_priv {
you don't need this. You just set: […]
Done
Patch Set #7, Line 187:
bo_dri = (struct dri_bo_priv *)calloc(1, sizeof(*bo_dri));you shouldn't need this anymore
Done
Patch Set #7, Line 229:
fail_freefree_image:
Done
Patch Set #7, Line 230:
for(size_t plane = 0; plane < bo->num_planes; plane++) {add newline before this.
Done
vma->addr = dri->image_extension->mapImage(
dri->context, bo_dri->image,
0, 0, bo->width, bo->height, /* rectangle to map */
map_flags, /* GBM flags and DRI flags are the same */
&stride, &vma->priv);the formatting looks weird. Make sure to ./presubmit.sh to get the proper formating.
Done
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #8 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
4 files changed, 489 insertions(+), 303 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Gurchetan Singh uploaded patch set #9 to the change originally created by Satyajit Sahu.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
4 files changed, 426 insertions(+), 326 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Gurchetan Singh uploaded patch set #10 to the change originally created by Satyajit Sahu.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
4 files changed, 423 insertions(+), 326 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
I fixed some issues with this patch. Satyajit, can you test on Chrome and Android? Also, we still need to have a proper flush implementation before we can land this. What's the plan to do this? Is to expand the DRI api?
1 comment:
if (!dri->image_extension)
if (!lookup_extension(dri->core_extension->getExtensions(dri->device), __DRI_IMAGE,
12, (const __DRIextension **)&dri->image_extension))
goto free_context;
if (!dri->flush_extension)
if (!lookup_extension(dri->core_extension->getExtensions(dri->device), __DRI2_FLUSH,
4, (const __DRIextension **)&dri->flush_extension))
goto free_contextWhy do you attempt to lookup the image and flush extensions twice (see line 127 and line 129)??
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Patch Set 10:
(1 comment)
I fixed some issues with this patch. Satyajit, can you test on Chrome and Android? Also, we still need to have a proper flush implementation before we can land this. What's the plan to do this? Is to expand the DRI api?
I verified Chrome and Android works. But graphics_Gbm autotest is failing because of the num_planes assert dri.c line 185.
Satyajit Sahu (Gerrit)
1 comment:
if (!dri->image_extension)
if (!lookup_extension(dri->core_extension->getExtensions(dri->device), __DRI_IMAGE,
12, (const __DRIextension **)&dri->image_extension))
goto free_context;
if (!dri->flush_extension)
if (!lookup_extension(dri->core_extension->getExtensions(dri->device), __DRI2_FLUSH,
4, (const __DRIextension **)&dri->flush_extension))
goto free_contextWhy do you attempt to lookup the image and flush extensions twice (see line 127 and line 129)??
lookup_extension for __DRI_IMAGE fails for the first time (line 127) as gallium_driver_extensions does not have __DRI_IMAGE extension. After createNewScreen2 __DRI_IMAGE extension is available as part of the getExtensions.
The first call for __DRI_IMAGE as of now always fails can be removed.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Gurchetan Singh uploaded patch set #11 to the change originally created by Satyajit Sahu.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
4 files changed, 417 insertions(+), 325 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Gurchetan Singh uploaded patch set #12 to the change originally created by Satyajit Sahu.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
5 files changed, 418 insertions(+), 326 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Patch Set 10:
Patch Set 10:
(1 comment)
I fixed some issues with this patch. Satyajit, can you test on Chrome and Android? Also, we still need to have a proper flush implementation before we can land this. What's the plan to do this? Is to expand the DRI api?
I verified Chrome and Android works. But graphics_Gbm autotest is failing because of the num_planes assert dri.c line 185.
Okay, the assert should be fixed. Can you test again? If it still has the issue, feel free to upload the fix ;-)
The only thing I'm not certain about is the flushing / invalidating mechanism.
The graphics_Gbm autotest does exercise the flush path. The commit message states graphics_Gbm passed with a prior version of this patchset. Currently, dri_bo_flush should be a no-op, so I'm surprised the test passes. Two questions:
1) Do you still see garbled output for mmap_test -g with the newest version of the patch?
2) If not, can you try removing dri_bo_flush, the __DRI_FLUSH extension etc., and see if graphics_Gbm and mmap_test -g still succeed?
Stéphane Marchesin (Gerrit)
1 comment:
Patch Set #12, Line 295:
/* TODO: Does this work?? */It will do something, but it doesn't do what you need for gbm. For minigbm, ->flush() basically needs to flush the mapping out of the CPU caches so that the memory is visible to the GPU. Dri's ->flush_with_flags() will flush the GPU pipeline so that the CPU gets the latest data. So this is the opposite of what you want.
Btw I don't see an invalidate callback...
How are the mappings done on AMD? Are they cached or uncached? Are they tiled and do they need detiling on CPU?
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Marek Olšák (Gerrit)
The CPU access is cached for mappings with the read flag. This has better CPU read performance but worse GPU read/write performance.
Write-only mappings are uncached and use write combining. This has worse CPU read performance but better GPU read/write performance.
Images are always tiled. (exceptions are narrow images like Nx1 pixels that are linear) Detiling is never done on the CPU. The driver uses a copy engine on the GPU for detiling (called SDMA). It can do arbitrary subimage copies and fills. SDMA runs in parallel with graphics.
Marek Olšák (Gerrit)
What is the invalidate callback? (this page doesn't seem to have a link to browse the source code)
Stéphane Marchesin (Gerrit)
What is the invalidate callback? (this page doesn't seem to have a
link to browse the source code)
Here is how things work:
- minigbm will try to keep mappings persistent for performance reasons. This means that the driver will not see the map/unmap() callbacks after the first mapping, but instead will get invalidate() and flush() callbacks
- the fact that the mappings are cached for your case means that we need something in the invalidate and flush callbacks, to ensure coherency. Obviously if you have a coherent CPU/GPU architecture, invalidate() and flush() are noops, but it doesn't seem like that's the case here.
- the invalidate callback invalidates the CPU cache's view of a surface, and obtains latest GPU data
- the flush callback flushes the current CPU cache's view of a surface, so that the GPU can see it
Gurchetan Singh (Gerrit)
Patch Set 12:
What is the invalidate callback? (this page doesn't seem to have a link to browse the source code)
The invalidate callback is drv_bo_invalidate, and only called when the driver implements. You can browse the repo here:
https://chromium.googlesource.com/chromiumos/platform/minigbm/+/master/drv.c
or
git clone https://chromium.googlesource.com/chromiumos/platform/minigbm
Marek Olšák (Gerrit)
Patch Set 12:
What is the invalidate callback? (this page doesn't seem to have a
link to browse the source code)
Here is how things work:
- minigbm will try to keep mappings persistent for performance reasons. This means that the driver will not see the map/unmap() callbacks after the first mapping, but instead will get invalidate() and flush() callbacks
- the fact that the mappings are cached for your case means that we need something in the invalidate and flush callbacks, to ensure coherency. Obviously if you have a coherent CPU/GPU architecture, invalidate() and flush() are noops, but it doesn't seem like that's the case here.
- the invalidate callback invalidates the CPU cache's view of a surface, and obtains latest GPU data
- the flush callback flushes the current CPU cache's view of a surface, so that the GPU can see it
Because of delta color compression (DCC) on GCN 1.2 and later, persistent image mappings are unsupported. In order to support invalidate & flush, we have to keep a linear copy of the image in memory. Invalidate would be a blit from tiled+compressed to linear, while flush would be a blit from linear to tiled+compressed.
My concern is that invalidate & flush give the misleading impression that they are only cheap cache flushes and not potentially expensive blits.
The good news is that the driver doesn't need (almost?) any changes. minigbm can allocate the linear images manually and do the blits for flush & invalidate also manually.
Gurchetan Singh (Gerrit)
Gurchetan Singh uploaded patch set #13 to the change originally created by Satyajit Sahu.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
5 files changed, 390 insertions(+), 329 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Patch Set 12:
Patch Set 12:
What is the invalidate callback? (this page doesn't seem to have a
link to browse the source code)Here is how things work:
- minigbm will try to keep mappings persistent for performance reasons. This means that the driver will not see the map/unmap() callbacks after the first mapping, but instead will get invalidate() and flush() callbacks
- the fact that the mappings are cached for your case means that we need something in the invalidate and flush callbacks, to ensure coherency. Obviously if you have a coherent CPU/GPU architecture, invalidate() and flush() are noops, but it doesn't seem like that's the case here.
- the invalidate callback invalidates the CPU cache's view of a surface, and obtains latest GPU data
- the flush callback flushes the current CPU cache's view of a surface, so that the GPU can see it
Because of delta color compression (DCC) on GCN 1.2 and later, persistent image mappings are unsupported. In order to support invalidate & flush, we have to keep a linear copy of the image in memory. Invalidate would be a blit from tiled+compressed to linear, while flush would be a blit from linear to tiled+compressed.
My concern is that invalidate & flush give the misleading impression that they are only cheap cache flushes and not potentially expensive blits.
The good news is that the driver doesn't need (almost?) any changes. minigbm can allocate the linear images manually and do the blits for flush & invalidate also manually.
We decided to modify minigbm (crrev.com/c/990892) to not always keep persistent mappings, since blits can be expensive. Shirish, can you test both patches (kahlee devices are in limited supply here)?
Stéphane Marchesin (Gerrit)
Patch Set 12:
> > Patch Set 12:
>
> > What is the invalidate callback? (this page doesn't seem to
have a
> > link to browse the source code)
>
> Here is how things work:
> - minigbm will try to keep mappings persistent for performance
reasons. This means that the driver will not see the map/unmap()
callbacks after the first mapping, but instead will get
invalidate() and flush() callbacks
> - the fact that the mappings are cached for your case means
that we need something in the invalidate and flush callbacks, to
ensure coherency. Obviously if you have a coherent CPU/GPU
architecture, invalidate() and flush() are noops, but it doesn't
seem like that's the case here.
> - the invalidate callback invalidates the CPU cache's view of a
surface, and obtains latest GPU data
> - the flush callback flushes the current CPU cache's view of a
surface, so that the GPU can see it
> Because of delta color compression (DCC) on GCN 1.2 and later,
persistent image mappings are unsupported. In order to support
invalidate & flush, we have to keep a linear copy of the image in
memory. Invalidate would be a blit from tiled+compressed to linear,
while flush would be a blit from linear to tiled+compressed.
> My concern is that invalidate & flush give the misleading
impression that they are only cheap cache flushes and not
potentially expensive blits.
> The good news is that the driver doesn't need (almost?) any
changes. minigbm can allocate the linear images manually and do the
blits for flush & invalidate also manually.
We decided to modify minigbm (crrev.com/c/990892) to not always
keep persistent mappings, since blits can be expensive. Shirish,
can you test both patches (kahlee devices are in limited supply
here)?
Note that if we find ways to implement flush/invalidate later, we can always add them back.
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #14 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
5 files changed, 414 insertions(+), 350 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Fixed some minor issues. Verified graphics_Gbm autotest, mmap_test -g and Android playstore.
Satyajit Sahu (Gerrit)
libdrm change https://cgit.freedesktop.org/mesa/drm/commit/?id=b81d44d587d1706d5c7568e539340632a748782b is a dependency for this patch.
Gurchetan Singh (Gerrit)
Gurchetan Singh uploaded patch set #15 to the change originally created by Satyajit Sahu.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
5 files changed, 394 insertions(+), 330 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Patch Set 14:
Fixed some minor issues. Verified graphics_Gbm autotest, mmap_test -g and Android playstore.
Good catch! I believe the original intention was subclass to the dri driver when needed, which the newest patch does.
Gurchetan Singh (Gerrit)
Patch Set 14:
libdrm change https://cgit.freedesktop.org/mesa/drm/commit/?id=b81d44d587d1706d5c7568e539340632a748782b is a dependency for this patch.
Can you backport this patch to ~/trunk/src/third_party/libdrm/?
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #16 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
5 files changed, 398 insertions(+), 333 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Patch Set 14:
Patch Set 14:
Fixed some minor issues. Verified graphics_Gbm autotest, mmap_test -g and Android playstore.
Good catch! I believe the original intention was subclass to the dri driver when needed, which the newest patch does.
The patch worked after a small modification.
Satyajit Sahu (Gerrit)
Patch Set 15:
Patch Set 14:
libdrm change https://cgit.freedesktop.org/mesa/drm/commit/?id=b81d44d587d1706d5c7568e539340632a748782b is a dependency for this patch.
Can you backport this patch to ~/trunk/src/third_party/libdrm/?
Done. crrev.com/c/994934
Gurchetan Singh (Gerrit)
lgtm, though I'll let someone else +2 since I made modifications to this patchset aswell
Patch set 16:Code-Review +1
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #17 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222 CL:955552
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
5 files changed, 398 insertions(+), 333 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #18 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:955552
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
5 files changed, 398 insertions(+), 333 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Drew Davenport (Gerrit)
Who would be a good person to review and +2 this? Dominik or Stephane?
Drew Davenport (Gerrit)
With this patch, there may be an issue with mapping a second time.
When testing https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/987531, I cherry-picked this CL to fix the garbled image on the display, but found that the loop to draw the transparent stripes was having no effect.
For a simpler repro case, call draw_to_plane(...., DRAW_ELLIPSE) on the primary plane right after DRAW_STRIPE in test_underlay(). An ellipse should be drawn to completely overwrite the stripes, but the stripes are still there unaffected.
Gurchetan Singh (Gerrit)
Patch Set 18:
With this patch, there may be an issue with mapping a second time.
When testing https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/987531, I cherry-picked this CL to fix the garbled image on the display, but found that the loop to draw the transparent stripes was having no effect.
For a simpler repro case, call draw_to_plane(...., DRAW_ELLIPSE) on the primary plane right after DRAW_STRIPE in test_underlay(). An ellipse should be drawn to completely overwrite the stripes, but the stripes are still there unaffected.
Did you have crrev.com/c/990892 also applied?
Drew Davenport (Gerrit)
Patch Set 18:
Patch Set 18:
With this patch, there may be an issue with mapping a second time.
When testing https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/987531, I cherry-picked this CL to fix the garbled image on the display, but found that the loop to draw the transparent stripes was having no effect.
For a simpler repro case, call draw_to_plane(...., DRAW_ELLIPSE) on the primary plane right after DRAW_STRIPE in test_underlay(). An ellipse should be drawn to completely overwrite the stripes, but the stripes are still there unaffected.
Did you have crrev.com/c/990892 also applied?
Yes, I had synced up to ToT earlier today, which had that CL.
Gurchetan Singh (Gerrit)
Patch Set 18:
Patch Set 18:
Patch Set 18:
With this patch, there may be an issue with mapping a second time.
When testing https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/987531, I cherry-picked this CL to fix the garbled image on the display, but found that the loop to draw the transparent stripes was having no effect.
For a simpler repro case, call draw_to_plane(...., DRAW_ELLIPSE) on the primary plane right after DRAW_STRIPE in test_underlay(). An ellipse should be drawn to completely overwrite the stripes, but the stripes are still there unaffected.
Did you have crrev.com/c/990892 also applied?
Yes, I had synced up to ToT earlier today, which had that CL.
Does graphics_Gbm pass for you with this patch applied? test_gem_map_format does try to verify writes are successful:
Drew Davenport (Gerrit)
Patch Set 18:
Patch Set 18:
Patch Set 18:
Patch Set 18:
With this patch, there may be an issue with mapping a second time.
When testing https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/987531, I cherry-picked this CL to fix the garbled image on the display, but found that the loop to draw the transparent stripes was having no effect.
For a simpler repro case, call draw_to_plane(...., DRAW_ELLIPSE) on the primary plane right after DRAW_STRIPE in test_underlay(). An ellipse should be drawn to completely overwrite the stripes, but the stripes are still there unaffected.
Did you have crrev.com/c/990892 also applied?
Yes, I had synced up to ToT earlier today, which had that CL.
Does graphics_Gbm pass for you with this patch applied? test_gem_map_format does try to verify writes are successful:
That test succeeded for me with this patch applied.
Satyajit Sahu (Gerrit)
Patch Set 18:
Patch Set 18:
Patch Set 18:
Patch Set 18:
Patch Set 18:
With this patch, there may be an issue with mapping a second time.
When testing https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/987531, I cherry-picked this CL to fix the garbled image on the display, but found that the loop to draw the transparent stripes was having no effect.
For a simpler repro case, call draw_to_plane(...., DRAW_ELLIPSE) on the primary plane right after DRAW_STRIPE in test_underlay(). An ellipse should be drawn to completely overwrite the stripes, but the stripes are still there unaffected.
Did you have crrev.com/c/990892 also applied?
Yes, I had synced up to ToT earlier today, which had that CL.
Does graphics_Gbm pass for you with this patch applied? test_gem_map_format does try to verify writes are successful:
That test succeeded for me with this patch applied.
There are two issues.
It looks like the dri context needs to be flushed after unmap.
And map returns a bigger stride. Which causes some corruption.
With below patch test_underlay is working fine.
diff --git a/dri.c b/dri.c
index 776658d..06fa2e7 100644
--- a/dri.c
+++ b/dri.c
@@ -261,6 +261,7 @@ void *dri_bo_map(struct bo *bo, struct vma *vma, size_t plane, uint32_t map_flag
/* GBM flags and DRI flags are the same. */
vma->addr = dri->image_extension->mapImage(dri->context, bo->priv, 0, 0, bo->width,
bo->height, map_flags, &stride, &vma->priv);
+ bo->strides[plane] = stride;
if (!vma->addr)
return MAP_FAILED;
@@ -270,9 +271,13 @@ void *dri_bo_map(struct bo *bo, struct vma *vma, size_t plane, uint32_t map_flag
int dri_bo_unmap(struct bo *bo, struct vma *vma)
{
struct dri_driver *dri = bo->drv->priv;
+ const __DRI2flushExtension *flush_extension;
assert(vma->priv);
dri->image_extension->unmapImage(dri->context, bo->priv, vma->priv);
+ if (lookup_extension(dri->core_extension->getExtensions(dri->device), __DRI2_FLUSH, 4,
+ (const __DRIextension **)&flush_extension))
+ flush_extension->flush_with_flags(dri->context, NULL, __DRI2_FLUSH_CONTEXT, 0);
return 0;
}
Kristian H. Kristensen (Gerrit)
1 comment:
Patch Set #18, Line 262:
mapImageI'm not sure about this... we have to allocate a DRI context for this (more or less a full GL context) and the map/unmap operations now involve running the dma/blit engine or the GPU to copy in and out of staging buffers. Is it possible to mix the DRI allocation path with the DRM_IOCTL_AMDGPU_GEM_MMAP ioctl for mmap that we used before?
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Patch Set 18:
(1 comment)
1 comment:
Patch Set #18, Line 262:
mapImage
I'm not sure about this... […]
I dont think that is possible as DRM_IOCTL_GEM_MMAP will return the tiled data.
Flushing the dri context after unmap solves the repeated map/unmap issue.
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #19 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
5 files changed, 402 insertions(+), 333 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Gurchetan Singh uploaded patch set #20 to the change originally created by Satyajit Sahu.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
5 files changed, 431 insertions(+), 332 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Yes, unfortunately, DRM_IOCTL_AMDGPU_GEM_MMAP doesn't detile at kernel level like other drivers do. We can avoid some of the cost associated with this interface by making sure all linear formats skip the dri path, as patch set 20 does.
Satyajit@, you also mentioned to get atomictest -t test_underlay to work, you needed to change the stride:
"And map returns a bigger stride. Which causes some corruption."
However, patchset 19 didn't include any changes to the stride. Do you still need this to get atomictest -t test_underlay to work?
Gurchetan Singh (Gerrit)
Gurchetan Singh uploaded patch set #21 to the change originally created by Satyajit Sahu.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
5 files changed, 432 insertions(+), 332 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Kristian H. Kristensen (Gerrit)
I think this looks mostly reasonable now. I'd still prefer if we could avoid using the DRI context for mapping, but we can make that change later if it's an issue.
2 comments:
Why drop this check?
#define TILE_TYPE_LINEAR 0
and use that instead?
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Gurchetan Singh uploaded patch set #22 to the change originally created by Satyajit Sahu.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
5 files changed, 434 insertions(+), 330 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
2 comments:
Why drop this check?
Done
#define TILE_TYPE_LINEAR 0 […]
Done
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Kristian H. Kristensen (Gerrit)
Cool, looks good. Let me give it a spin on my grunt here.
Gurchetan Singh (Gerrit)
Patch Set 22:
Cool, looks good. Let me give it a spin on my grunt here.
Here's hoegsberg@'s test results (from another thread)
"Testing on R67-10564.0.0 on grunt, UI fails to start:
Crash dump received by crash_reporter
[19889:19902:0411/122126.869046:ERROR:drm_device.cc(577)] Cannot prepare dumb buffer for mapping: Operation not permitted (1)
[19889:19902:0411/122126.907403:ERROR:drm_device.cc(577)] Cannot prepare dumb buffer for mapping: Operation not permitted (1)
[19755:19755:0411/122127.267100:ERROR:object_proxy.cc(617)] Failed to call method: org.chromium.SessionManagerInterface.StartArcMiniContainer: object_path= /org/chromium/SessionManager: org.chromium.SessionManagerInterface.ContainerStartupFail: Starting Android container failed.
Crash dump received by crash_reporter
amdgpu_parse_asic_ids: Cannot parse ASIC IDs: Resource temporarily unavailable
[20103:20103:0411/122128.054912:ERROR:sandbox_linux.cc(374)] InitializeSandbox() called with multiple threads in process gpu-process. This error can be safely ignored in VMTests. Try waiting for /proc to be updated.
amdgpu_parse_asic_ids: Cannot parse ASIC IDs: Resource temporarily unavailable
[20065:20082:0411/122131.100816:ERROR:object_proxy.cc(617)] Failed to call method: org.chromium.SessionManagerInterface.RetrievePolicyEx: object_path= /org/chromium/SessionManager: org.freedesktop.DBus.Error.NoReply: Message recipient disconnected from message bus without replying"
Satyajit, can you determine why the UI is not working on grunt? CL:997192 landed in 10554.0.0, so I'm unsure what else could be failing ...
Kristian H. Kristensen (Gerrit)
Patch Set 22:
Patch Set 22:
Cool, looks good. Let me give it a spin on my grunt here.
Here's hoegsberg@'s test results (from another thread)
Yikes, I posted that on the wrong CL. Thanks Gurchetan.
Kristian H. Kristensen (Gerrit)
Drew said that there's a libdrm CL that I need for this to work - I'll give it a try again in a bit.
Deepak Sharma (Gerrit)
Patch Set 22:
Patch Set 22:
Patch Set 22:
Cool, looks good. Let me give it a spin on my grunt here.
Here's hoegsberg@'s test results (from another thread)
Yikes, I posted that on the wrong CL. Thanks Gurchetan.
I did test this recently , UI was working fine.
Gurchetan Singh (Gerrit)
Patch Set 22:
Patch Set 22:
Patch Set 22:
Patch Set 22:
Cool, looks good. Let me give it a spin on my grunt here.
Here's hoegsberg@'s test results (from another thread)
Yikes, I posted that on the wrong CL. Thanks Gurchetan.
I did test this recently , UI was working fine.
Cool, if crrev.com/c/981872 (atomictest -t video_underlay) works on Kahlee/Grunt without having to change the stride at mmap time, we should be good. Let me know.
Satyajit Sahu (Gerrit)
Patch Set 22:
Patch Set 22:
Patch Set 22:
Patch Set 22:
Patch Set 22:
Cool, looks good. Let me give it a spin on my grunt here.
Here's hoegsberg@'s test results (from another thread)
Yikes, I posted that on the wrong CL. Thanks Gurchetan.
I did test this recently , UI was working fine.
Cool, if crrev.com/c/981872 (atomictest -t video_underlay) works on Kahlee/Grunt without having to change the stride at mmap time, we should be good. Let me know.
UI fails to come up when I tried this patch set.
The below change in amdgpu_init seems to be causing the UI failure.
use_flags &= ~BO_USE_LINEAR;
Also mmap_test -g fails because of below
use_flags &= ~BO_USE_SW_WRITE_OFTEN;
use_flags &= ~BO_USE_SW_READ_OFTEN;
Satyajit Sahu (Gerrit)
Patch Set 20:
Yes, unfortunately, DRM_IOCTL_AMDGPU_GEM_MMAP doesn't detile at kernel level like other drivers do. We can avoid some of the cost associated with this interface by making sure all linear formats skip the dri path, as patch set 20 does.
Satyajit@, you also mentioned to get atomictest -t test_underlay to work, you needed to change the stride:
"And map returns a bigger stride. Which causes some corruption."
However, patchset 19 didn't include any changes to the stride. Do you still need this to get atomictest -t test_underlay to work?
The corruption was not related to the stride changes. The corruption is a random. I have attached the snapshot in the issue tracker.
Gurchetan Singh (Gerrit)
Patch Set 22:
Patch Set 22:
Patch Set 22:
Patch Set 22:
Patch Set 22:
Patch Set 22:
Cool, looks good. Let me give it a spin on my grunt here.
Here's hoegsberg@'s test results (from another thread)
Yikes, I posted that on the wrong CL. Thanks Gurchetan.
I did test this recently , UI was working fine.
Cool, if crrev.com/c/981872 (atomictest -t video_underlay) works on Kahlee/Grunt without having to change the stride at mmap time, we should be good. Let me know.
UI fails to come up when I tried this patch set.
The below change in amdgpu_init seems to be causing the UI failure.
use_flags &= ~BO_USE_LINEAR;Also mmap_test -g fails because of below
use_flags &= ~BO_USE_SW_WRITE_OFTEN;
use_flags &= ~BO_USE_SW_READ_OFTEN;
What error are you getting when you run the test? Just corruption on screen?
This implies allocating linear buffers with the non-dri path is broken. For example, mmap_test -g uses these use flags:
case 'g':
ctx.mapper = bs_mapper_gem_new();
flags |= GBM_BO_USE_SW_READ_OFTEN | GBM_BO_USE_SW_WRITE_OFTEN;
Can you debug? We essentially want to allocate linear buffers on AMDGPU, without using addrlib or dri.
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #23 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
M helpers.c
6 files changed, 449 insertions(+), 345 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Patch Set 22:
Patch Set 22:
Patch Set 22:
Patch Set 22:
Patch Set 22:
Patch Set 22:
Patch Set 22:
Cool, looks good. Let me give it a spin on my grunt here.
Here's hoegsberg@'s test results (from another thread)
Yikes, I posted that on the wrong CL. Thanks Gurchetan.
I did test this recently , UI was working fine.
Cool, if crrev.com/c/981872 (atomictest -t video_underlay) works on Kahlee/Grunt without having to change the stride at mmap time, we should be good. Let me know.
UI fails to come up when I tried this patch set.
The below change in amdgpu_init seems to be causing the UI failure.
use_flags &= ~BO_USE_LINEAR;Also mmap_test -g fails because of below
use_flags &= ~BO_USE_SW_WRITE_OFTEN;
use_flags &= ~BO_USE_SW_READ_OFTEN;What error are you getting when you run the test? Just corruption on screen?
This implies allocating linear buffers with the non-dri path is broken. For example, mmap_test -g uses these use flags:
case 'g':
ctx.mapper = bs_mapper_gem_new();
flags |= GBM_BO_USE_SW_READ_OFTEN | GBM_BO_USE_SW_WRITE_OFTEN;Can you debug? We essentially want to allocate linear buffers on AMDGPU, without using addrlib or dri.
subsample_stride calculation was not proper for RGB buffers. Modified and uploaded patchset 3. With this modification UI comes up and mmap_test -g also proper.
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #24 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
M helpers.c
6 files changed, 452 insertions(+), 345 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #25 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
M helpers.c
6 files changed, 449 insertions(+), 345 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
Gurchetan Singh uploaded patch set #26 to the change originally created by Satyajit Sahu.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
CQ-DEPEND=CL:970222
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
5 files changed, 437 insertions(+), 329 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Gurchetan Singh (Gerrit)
The problem is drv_bo_from_format expects the stride, not the width. Makes sense why it wasn't working for 4bpp formats. It should work now. Satyajit, test once more and if everything passes please land this.
Patch set 26:Code-Review +2
Satyajit Sahu (Gerrit)
Patch set 26:Verified +1
Satyajit Sahu (Gerrit)
Patch set 26:Commit-Queue +1
Satyajit Sahu (Gerrit)
Satyajit Sahu uploaded patch set #27 to this change.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
5 files changed, 437 insertions(+), 329 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
Satyajit Sahu (Gerrit)
Patch set 27:Verified +1
Satyajit Sahu (Gerrit)
Patch set 27:Commit-Queue +1
ChromeOS bot (Gerrit)
ChromeOS bot uploaded patch set #28 to the change originally created by Satyajit Sahu.
minigbm: using dri extensions
addrlib dependency removed. dri extensions are called instead.
dri.c/dri.h implements the generic dri extensions call.
amdgpu.c implements amdgpu specific.
BUG=b:72972511
TEST=Chrome booted to UI and passed graphics_Gbm autotest
Change-Id: Ia0edec7752a258fe2f70bc4838dac6398d46def2
Signed-off-by: Satyajit <satyaj...@amd.com>
Reviewed-on: https://chromium-review.googlesource.com/863723
Commit-Ready: Satyajit Sahu <satyaj...@amd.com>
Tested-by: Satyajit Sahu <satyaj...@amd.com>
Reviewed-by: Gurchetan Singh <gurchet...@chromium.org>
---
M Makefile
M amdgpu.c
A dri.c
A dri.h
M drv_priv.h
5 files changed, 437 insertions(+), 329 deletions(-)
To view, visit change 863723. To unsubscribe, or for help writing mail filters, visit settings.
ChromeOS bot (Gerrit)
ChromeOS bot merged this change.