[Mesa-dev] [PATCH 4/4] egl/dri2: add image extension to swrast_core_extensions
From: Gurchetan Singh Otherwise, this extension is not visible to the EGL user --- src/egl/drivers/dri2/egl_dri2.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c index 7175e827c9..9e845e99e3 100644 --- a/src/egl/drivers/dri2/egl_dri2.c +++ b/src/egl/drivers/dri2/egl_dri2.c @@ -429,6 +429,7 @@ static const struct dri2_extension_match swrast_driver_extensions[] = { static const struct dri2_extension_match swrast_core_extensions[] = { { __DRI_TEX_BUFFER, 2, offsetof(struct dri2_egl_display, tex_buffer) }, + { __DRI_IMAGE, 1, offsetof(struct dri2_egl_display, image) }, { NULL, 0, 0 } }; -- 2.12.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] egl/dri2: add image extension to swrast_core_extensions
Ping... On Wed, Jun 21, 2017 at 4:40 PM, Gurchetan Singh < gurchetansi...@chromium.org> wrote: > Emil, > > If I understand you correctly, you're proposing to add the ability to use > the kms_swrast driver in platform_x11.c (the host is a standard Ubuntu box > for the emulator use case, not CrOS) alongside swrast. > > In that case, we would need to: > > 1) Have a dri2_initialize_x11_kms_swrast function that's called when some > environment variable is set instead of dri2_initialize_x11_swrast. > 2) dri2_initialize_x11_kms_swrast would need access to the host card fd > (dri_kms_init_screen requires this) and call dri2_load_driver instead of > dri2_load_driver_swrast . > 3) Use dri2_loader_extensions instead of swrast_loader_extensions, > dri2_x11_display_vtbl instead dri2_x11_swrast_display_vtbl etc. > > I'm having trouble getting this to work, and I was wondering if what I'm > trying to do is what you want. Attached is the patch I'm trying (it > compiles, but will crash your display). > > Regarding the issues with the emulator, I filed a bug based your comments > and the emulator team has started looking at it (see > https://android-review.googlesource.com/#/c/418541/). > > > On Tue, Jun 20, 2017 at 1:19 AM, Emil Velikov > wrote: > >> On 19 June 2017 at 20:46, Chad Versace wrote: >> > On Thu 15 Jun 2017, Gurchetan Singh wrote: >> >> Emil, would you be fine with leaving the image extension in dri2.c but >> still >> >> adding it as a drisw extension? That solution would look like: >> >> >> >> [1]https://patchwork.freedesktop.org/patch/154807/ >> > >> > Observations: >> > - src/gallium/state_trackers/dri/dri2.c:dri2ImageExtension >> advertises v15 of __DRI_IMAGE. >> > - egl_dri2.c requires only v1 of __DRI_IMAGE. Maybe a higher version >> > is required in practive, but the egl_dri2.c code checks only for >> v1. >> > >> > Questions: >> > 1. All functions implemented in dri2.c:dri2ImageExtensions, do they >> >under swrast? Honest question, because I'm no expert on >> >gallium. >> > >> > If question #1 is true, then I see no problem with your latest plan. But >> > maybe Emil does. >> > >> > If question #1 is false, it should be straightforward to implement in >> > drisw.c the small subset of __DRI_IMAGE functions required for v1. >> >> While I haven't checked how much [or well] DRI_IMAGE works with >> swrast, there's no need to actually add it there. >> An alternative is to add kms_swrast support for EGL like we already do >> for GBM, as mentioned earlier [1]. >> >> Gents, keep in mind that: >> - one cannot pull DRM specifics (dri2.c) code within drisw.c, and >> - DRI_IMAGE pulls DRM specifics, hence adding it into drisw.c is >> again a no-go :-\ >> >> FWIW the above architectural split applies for classic drivers as >> well. swrast_dri.so simply cannot depend on anything DRM related. >> >> -Emil >> >> [1] https://lists.freedesktop.org/archives/mesa-dev/2017-June/159519.html >> > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] egl/dri2: add image extension to swrast_core_extensions
Hi Gurchetan, Pardon for the delay. On 22 June 2017 at 00:40, Gurchetan Singh wrote: > Emil, > > If I understand you correctly, you're proposing to add the ability to use > the kms_swrast driver in platform_x11.c (the host is a standard Ubuntu box > for the emulator use case, not CrOS) alongside swrast. > > In that case, we would need to: > > 1) Have a dri2_initialize_x11_kms_swrast function that's called when some > environment variable is set instead of dri2_initialize_x11_swrast. There is the obvious caveat that LIBGL_ALWAYS_SOFTWARE should be set. Other than that - we can try kms_swrast and fallback to swrast. If the former is very incomplete relative to the latter then we might consider adding another envvar. > 2) dri2_initialize_x11_kms_swrast would need access to the host card fd > (dri_kms_init_screen requires this) and call dri2_load_driver instead of > dri2_load_driver_swrast . Indeed - in the GBM case, we get that from the user. In here, we _should_ be able to use the existing one passed from X. See below for more. > 3) Use dri2_loader_extensions instead of swrast_loader_extensions, > dri2_x11_display_vtbl instead dri2_x11_swrast_display_vtbl etc. > Correct again. > I'm having trouble getting this to work, and I was wondering if what I'm > trying to do is what you want. Attached is the patch I'm trying (it > compiles, but will crash your display). > Please keep patches a) inline and b) functional changes separate from code movement. Fishing for ~5 diff in a 100 line patch isn't fun :-\ Re the issue: - opening card0 without authenticating is a serious no-go - at the same time we shouldn't need the open - reuse the fd, as mentioned above. - if above does not work use the renderD for the given fd -> open(drmGetRenderDeviceNameFromFd(fd)...) > Regarding the issues with the emulator, I filed a bug based your comments > and the emulator team has started looking at it (see > https://android-review.googlesource.com/#/c/418541/). > Great, glad to hear that it is getting resolved. Thanks Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] egl/dri2: add image extension to swrast_core_extensions
On 28 June 2017 at 11:17, Emil Velikov wrote: > Hi Gurchetan, > > Pardon for the delay. > > On 22 June 2017 at 00:40, Gurchetan Singh wrote: >> Emil, >> >> If I understand you correctly, you're proposing to add the ability to use >> the kms_swrast driver in platform_x11.c (the host is a standard Ubuntu box >> for the emulator use case, not CrOS) alongside swrast. >> >> In that case, we would need to: >> >> 1) Have a dri2_initialize_x11_kms_swrast function that's called when some >> environment variable is set instead of dri2_initialize_x11_swrast. > There is the obvious caveat that LIBGL_ALWAYS_SOFTWARE should be set. > > Other than that - we can try kms_swrast and fallback to swrast. > If the former is very incomplete relative to the latter then we might > consider adding another envvar. > >> 2) dri2_initialize_x11_kms_swrast would need access to the host card fd >> (dri_kms_init_screen requires this) and call dri2_load_driver instead of >> dri2_load_driver_swrast . > Indeed - in the GBM case, we get that from the user. In here, we > _should_ be able to use the existing one passed from X. See below for > more. > >> 3) Use dri2_loader_extensions instead of swrast_loader_extensions, >> dri2_x11_display_vtbl instead dri2_x11_swrast_display_vtbl etc. >> > Correct again. > >> I'm having trouble getting this to work, and I was wondering if what I'm >> trying to do is what you want. Attached is the patch I'm trying (it >> compiles, but will crash your display). >> > Please keep patches a) inline and b) functional changes separate from > code movement. > Fishing for ~5 diff in a 100 line patch isn't fun :-\ > > Re the issue: > - opening card0 without authenticating is a serious no-go > - at the same time we shouldn't need the open - reuse the fd, as > mentioned above. > - if above does not work use the renderD for the given fd -> > open(drmGetRenderDeviceNameFromFd(fd)...) > Upon second thought - I'm not too sure if my original suggestion may not work without some serious work. Which seemingly is not required for EGL_KHR_gl_image - lucky me got mislead by more missing checks out our codebase :-\ Some background: The __DRI_IMAGE extension adds the following .createImageFromName .createImageFromRenderbuffer .destroyImage .createImage .queryImage .dupImage .validateUsage .createImageFromNames .fromPlanar .createImageFromTexture .createImageFromFds .createImageFromDmaBufs .blitImage .getCapabilities .mapImage .unmapImage Of which createImageFromTexture and createImageFromRenderbuffer seems to be required for KHR_gl_texture_*_image and KHR_gl_renderbuffer_image respectively (see dri2_create_image_khr). Although overall implementation-wise _DRI_IMAGE is as follows: For software gallium drivers: - MESA_drm_image, MESA_image_dma_buf_export are not possible since they require a) fd set to -1, and required for the drmGetCap() ioctl b) .queryImage is not implemented resource_get_handle -> {softpipe,llvmpipe} displaytarget_get_handle -> sw/dri - NOOP, sw/kms-dri fully implemented c) .createImageFrom{Name,Fds,DmaBuf,DmaBufs2} resource_from_handle -> {softpipe,llvmpipe} displaytarget_from_handle -> sw/dri - NOOP, sw/kms-dri fully implemented Overall st/dri: - KHR_gl_renderbuffer_image although advertised does not work a) .createImageFromRenderbuffer is a stub (see dri2_create_image_from_renderbuffer) :-\ Tl;Dr; your initial approach may work with some minor changes - comments coming shortly. There's handful of bugs in Mesa we need to address as well. Thanks Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] egl/dri2: add image extension to swrast_core_extensions
On 9 June 2017 at 01:28, gurchetansi...@chromium.org wrote: > From: Gurchetan Singh > > Otherwise, this extension is not visible to the EGL user > --- > src/egl/drivers/dri2/egl_dri2.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c > index 7175e827c9..9e845e99e3 100644 > --- a/src/egl/drivers/dri2/egl_dri2.c > +++ b/src/egl/drivers/dri2/egl_dri2.c > @@ -429,6 +429,7 @@ static const struct dri2_extension_match > swrast_driver_extensions[] = { > > static const struct dri2_extension_match swrast_core_extensions[] = { > { __DRI_TEX_BUFFER, 2, offsetof(struct dri2_egl_display, tex_buffer) }, > + { __DRI_IMAGE, 1, offsetof(struct dri2_egl_display, image) }, Considering the entry points used, check the exact _DRI_IMAGE version needed (see dri_interface.h) and update accordingly. Also move the line to optional_core_extensions, since by using swrast_core_extensions you will fail to load older swrast_dri.so which are otherwise perfectly capable of working (albeit w/o said EGL extensions). Before you ask - yes, extension is present in dri2_core_extensions, so doing a second check is sub-optimal/strange. Do add a comment clearing any ambiguities. With that said - I'm back to addressing all the issues I've saw in libEGL ;-) Thanks Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] egl/dri2: add image extension to swrast_core_extensions
Thanks for the review, Emil. If we want to use swrast we'll probably need to implement a new swrast version of the image extension. We can't reuse the functions from dri2.c, even for version 1 of the extension (createImageFromName, createImageFromRenderbuffer etc), since they require definitions from dri_driver.h (like struct winsys_handle). However, we would like to get around the the constant re-implementation and code movement, and use kms_swrast if possible. Can you outline some the challenges and possible solutions to using kms_swrast with platform_x11? We could invest the time in making it work if it is achievable ... On Wed, Jun 28, 2017 at 8:24 AM, Emil Velikov wrote: > On 9 June 2017 at 01:28, gurchetansi...@chromium.org > wrote: > > From: Gurchetan Singh > > > > Otherwise, this extension is not visible to the EGL user > > --- > > src/egl/drivers/dri2/egl_dri2.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_ > dri2.c > > index 7175e827c9..9e845e99e3 100644 > > --- a/src/egl/drivers/dri2/egl_dri2.c > > +++ b/src/egl/drivers/dri2/egl_dri2.c > > @@ -429,6 +429,7 @@ static const struct dri2_extension_match > swrast_driver_extensions[] = { > > > > static const struct dri2_extension_match swrast_core_extensions[] = { > > { __DRI_TEX_BUFFER, 2, offsetof(struct dri2_egl_display, tex_buffer) > }, > > + { __DRI_IMAGE, 1, offsetof(struct dri2_egl_display, image) }, > Considering the entry points used, check the exact _DRI_IMAGE version > needed (see dri_interface.h) and update accordingly. > > Also move the line to optional_core_extensions, since by using > swrast_core_extensions you will fail to load older swrast_dri.so which > are otherwise perfectly capable of working (albeit w/o said EGL > extensions). > > Before you ask - yes, extension is present in dri2_core_extensions, so > doing a second check is sub-optimal/strange. > Do add a comment clearing any ambiguities. > > With that said - I'm back to addressing all the issues I've saw in libEGL > ;-) > > Thanks > Emil > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] egl/dri2: add image extension to swrast_core_extensions
Hi Gurchetan, On 9 June 2017 at 01:28, gurchetansi...@chromium.org wrote: > From: Gurchetan Singh > > Otherwise, this extension is not visible to the EGL user > --- > src/egl/drivers/dri2/egl_dri2.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c > index 7175e827c9..9e845e99e3 100644 > --- a/src/egl/drivers/dri2/egl_dri2.c > +++ b/src/egl/drivers/dri2/egl_dri2.c > @@ -429,6 +429,7 @@ static const struct dri2_extension_match > swrast_driver_extensions[] = { > > static const struct dri2_extension_match swrast_core_extensions[] = { > { __DRI_TEX_BUFFER, 2, offsetof(struct dri2_egl_display, tex_buffer) }, > + { __DRI_IMAGE, 1, offsetof(struct dri2_egl_display, image) }, IIRC the current codebase will not use it even, we expose it. Correct? Wild guess here is that you guys have some extra patches around, like say using vgem? Is there a public repo with the lot? On the st/dri side (earlier patches) - one should be able to build st/dri without any hardware specific knowledge and/or files. Currently that's done by isolating all the DRM specifics in dri2.c. Not sure if breaking that, architectural split imho, is a good idea. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] egl/dri2: add image extension to swrast_core_extensions
Actually, these are the only patches that are required. We're trying to run the Android Studio emulator using the host's GLES implementation. The emulator uses the image extension in that case: https://android.googlesource.com/platform/sdk/+/emu-2.4- release/emulator/opengl/host/libs/libOpenglRender/FrameBuffer.cpp https://android.googlesource.com/platform/sdk/+/emu-2.4- release/emulator/opengl/host/libs/libOpenglRender/ColorBuffer.cpp It does only use a subset of the extension, but nothing hardware specific. On Fri, Jun 9, 2017 at 4:17 AM, Emil Velikov wrote: > Hi Gurchetan, > > On 9 June 2017 at 01:28, gurchetansi...@chromium.org > wrote: > > From: Gurchetan Singh > > > > Otherwise, this extension is not visible to the EGL user > > --- > > src/egl/drivers/dri2/egl_dri2.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_ > dri2.c > > index 7175e827c9..9e845e99e3 100644 > > --- a/src/egl/drivers/dri2/egl_dri2.c > > +++ b/src/egl/drivers/dri2/egl_dri2.c > > @@ -429,6 +429,7 @@ static const struct dri2_extension_match > swrast_driver_extensions[] = { > > > > static const struct dri2_extension_match swrast_core_extensions[] = { > > { __DRI_TEX_BUFFER, 2, offsetof(struct dri2_egl_display, tex_buffer) > }, > > + { __DRI_IMAGE, 1, offsetof(struct dri2_egl_display, image) }, > IIRC the current codebase will not use it even, we expose it. Correct? > Wild guess here is that you guys have some extra patches around, like > say using vgem? Is there a public repo with the lot? > > On the st/dri side (earlier patches) - one should be able to build > st/dri without any hardware specific knowledge and/or files. > Currently that's done by isolating all the DRM specifics in dri2.c. > Not sure if breaking that, architectural split imho, is a good idea. > > -Emil > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] egl/dri2: add image extension to swrast_core_extensions
Emil, would you be fine with leaving the image extension in dri2.c but still adding it as a drisw extension? That solution would look like: https://patchwork.freedesktop.org/patch/154807/ On Fri, Jun 9, 2017 at 8:40 AM, Gurchetan Singh wrote: > Actually, these are the only patches that are required. We're trying to > run the Android Studio emulator using the host's GLES implementation. The > emulator uses the image extension in that case: > > https://android.googlesource.com/platform/sdk/+/emu-2.4-rele > ase/emulator/opengl/host/libs/libOpenglRender/FrameBuffer.cpp > https://android.googlesource.com/platform/sdk/+/emu-2.4-rele > ase/emulator/opengl/host/libs/libOpenglRender/ColorBuffer.cpp > > It does only use a subset of the extension, but nothing hardware specific. > > > On Fri, Jun 9, 2017 at 4:17 AM, Emil Velikov > wrote: > >> Hi Gurchetan, >> >> On 9 June 2017 at 01:28, gurchetansi...@chromium.org >> wrote: >> > From: Gurchetan Singh >> > >> > Otherwise, this extension is not visible to the EGL user >> > --- >> > src/egl/drivers/dri2/egl_dri2.c | 1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/src/egl/drivers/dri2/egl_dri2.c >> b/src/egl/drivers/dri2/egl_dri2.c >> > index 7175e827c9..9e845e99e3 100644 >> > --- a/src/egl/drivers/dri2/egl_dri2.c >> > +++ b/src/egl/drivers/dri2/egl_dri2.c >> > @@ -429,6 +429,7 @@ static const struct dri2_extension_match >> swrast_driver_extensions[] = { >> > >> > static const struct dri2_extension_match swrast_core_extensions[] = { >> > { __DRI_TEX_BUFFER, 2, offsetof(struct dri2_egl_display, >> tex_buffer) }, >> > + { __DRI_IMAGE, 1, offsetof(struct dri2_egl_display, image) }, >> IIRC the current codebase will not use it even, we expose it. Correct? >> Wild guess here is that you guys have some extra patches around, like >> say using vgem? Is there a public repo with the lot? >> >> On the st/dri side (earlier patches) - one should be able to build >> st/dri without any hardware specific knowledge and/or files. >> Currently that's done by isolating all the DRM specifics in dri2.c. >> Not sure if breaking that, architectural split imho, is a good idea. >> >> -Emil >> > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] egl/dri2: add image extension to swrast_core_extensions
On 9 June 2017 at 16:40, Gurchetan Singh wrote: > Actually, these are the only patches that are required. We're trying to run > the Android Studio emulator using the host's GLES implementation. The > emulator uses the image extension in that case: > > https://android.googlesource.com/platform/sdk/+/emu-2.4-release/emulator/opengl/host/libs/libOpenglRender/FrameBuffer.cpp > https://android.googlesource.com/platform/sdk/+/emu-2.4-release/emulator/opengl/host/libs/libOpenglRender/ColorBuffer.cpp > > It does only use a subset of the extension, but nothing hardware specific. > True, there's nothing HW specific, yet it does DRM specifics ;-) From a quick look, all one needs to do is add kms_swrast support in EGL analogous to GBM [1]. On the cool side, I've spotted a few (too many) bugs that we want to address. I can tackle the Mesa ones [2], but please give the emulator/qemu ones a try [3]. Thanks Emil [1] git show 3b176c441b7ddc5f7d2f891da3f76cf3c1814ce1 -- src/gbm/ [2] Mesa side - dozen or so missed checks for EGL_KHR_gl_texture_*_image extension caps [3] The emulator code - seems to be moved to qemu/distrib/android, so I'll omit the ones in the original sdk codebase ;-) - query for ES1 extensions and assume that the ES2 context will have the same ones - error out if no ES1 configs are found - we don't care about those since only ES2 are used In general it seems like someone started ripping out ES1 support, but did not quite complete it, pulling some ES2 bits by mistake. Having a grep for s_gles1 and working up might be a good idea. - has_eglimage_texture_2d can never be false, yet ColorBuffer::create() takes it as an argument. In the unlikely case that the extension is missing, function does a some setup, only to _not_ create any images as needed later on. - using strstr for checking extension presence is flawed. For example: It will give false positives for OES_EGL_image_external and OES_EGL_image_external_essl3 when one is interested in OES_EGL_image. Disclaimer: I haven't checked if the alternative extensions provide the required functionality. - EGL_KHR_gl_renderbuffer_image is unused, yet checked ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] egl/dri2: add image extension to swrast_core_extensions
On Thu 15 Jun 2017, Gurchetan Singh wrote: > Emil, would you be fine with leaving the image extension in dri2.c but still > adding it as a drisw extension? That solution would look like: > > [1]https://patchwork.freedesktop.org/patch/154807/ Observations: - src/gallium/state_trackers/dri/dri2.c:dri2ImageExtension advertises v15 of __DRI_IMAGE. - egl_dri2.c requires only v1 of __DRI_IMAGE. Maybe a higher version is required in practive, but the egl_dri2.c code checks only for v1. Questions: 1. All functions implemented in dri2.c:dri2ImageExtensions, do they under swrast? Honest question, because I'm no expert on gallium. If question #1 is true, then I see no problem with your latest plan. But maybe Emil does. If question #1 is false, it should be straightforward to implement in drisw.c the small subset of __DRI_IMAGE functions required for v1. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] egl/dri2: add image extension to swrast_core_extensions
On 19 June 2017 at 20:46, Chad Versace wrote: > On Thu 15 Jun 2017, Gurchetan Singh wrote: >> Emil, would you be fine with leaving the image extension in dri2.c but still >> adding it as a drisw extension? That solution would look like: >> >> [1]https://patchwork.freedesktop.org/patch/154807/ > > Observations: > - src/gallium/state_trackers/dri/dri2.c:dri2ImageExtension advertises v15 > of __DRI_IMAGE. > - egl_dri2.c requires only v1 of __DRI_IMAGE. Maybe a higher version > is required in practive, but the egl_dri2.c code checks only for v1. > > Questions: > 1. All functions implemented in dri2.c:dri2ImageExtensions, do they >under swrast? Honest question, because I'm no expert on >gallium. > > If question #1 is true, then I see no problem with your latest plan. But > maybe Emil does. > > If question #1 is false, it should be straightforward to implement in > drisw.c the small subset of __DRI_IMAGE functions required for v1. While I haven't checked how much [or well] DRI_IMAGE works with swrast, there's no need to actually add it there. An alternative is to add kms_swrast support for EGL like we already do for GBM, as mentioned earlier [1]. Gents, keep in mind that: - one cannot pull DRM specifics (dri2.c) code within drisw.c, and - DRI_IMAGE pulls DRM specifics, hence adding it into drisw.c is again a no-go :-\ FWIW the above architectural split applies for classic drivers as well. swrast_dri.so simply cannot depend on anything DRM related. -Emil [1] https://lists.freedesktop.org/archives/mesa-dev/2017-June/159519.html ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] egl/dri2: add image extension to swrast_core_extensions
Emil, If I understand you correctly, you're proposing to add the ability to use the kms_swrast driver in platform_x11.c (the host is a standard Ubuntu box for the emulator use case, not CrOS) alongside swrast. In that case, we would need to: 1) Have a dri2_initialize_x11_kms_swrast function that's called when some environment variable is set instead of dri2_initialize_x11_swrast. 2) dri2_initialize_x11_kms_swrast would need access to the host card fd (dri_kms_init_screen requires this) and call dri2_load_driver instead of dri2_load_driver_swrast . 3) Use dri2_loader_extensions instead of swrast_loader_extensions, dri2_x11_display_vtbl instead dri2_x11_swrast_display_vtbl etc. I'm having trouble getting this to work, and I was wondering if what I'm trying to do is what you want. Attached is the patch I'm trying (it compiles, but will crash your display). Regarding the issues with the emulator, I filed a bug based your comments and the emulator team has started looking at it (see https://android-review.googlesource.com/#/c/418541/). On Tue, Jun 20, 2017 at 1:19 AM, Emil Velikov wrote: > On 19 June 2017 at 20:46, Chad Versace wrote: > > On Thu 15 Jun 2017, Gurchetan Singh wrote: > >> Emil, would you be fine with leaving the image extension in dri2.c but > still > >> adding it as a drisw extension? That solution would look like: > >> > >> [1]https://patchwork.freedesktop.org/patch/154807/ > > > > Observations: > > - src/gallium/state_trackers/dri/dri2.c:dri2ImageExtension > advertises v15 of __DRI_IMAGE. > > - egl_dri2.c requires only v1 of __DRI_IMAGE. Maybe a higher version > > is required in practive, but the egl_dri2.c code checks only for > v1. > > > > Questions: > > 1. All functions implemented in dri2.c:dri2ImageExtensions, do they > >under swrast? Honest question, because I'm no expert on > >gallium. > > > > If question #1 is true, then I see no problem with your latest plan. But > > maybe Emil does. > > > > If question #1 is false, it should be straightforward to implement in > > drisw.c the small subset of __DRI_IMAGE functions required for v1. > > While I haven't checked how much [or well] DRI_IMAGE works with > swrast, there's no need to actually add it there. > An alternative is to add kms_swrast support for EGL like we already do > for GBM, as mentioned earlier [1]. > > Gents, keep in mind that: > - one cannot pull DRM specifics (dri2.c) code within drisw.c, and > - DRI_IMAGE pulls DRM specifics, hence adding it into drisw.c is > again a no-go :-\ > > FWIW the above architectural split applies for classic drivers as > well. swrast_dri.so simply cannot depend on anything DRM related. > > -Emil > > [1] https://lists.freedesktop.org/archives/mesa-dev/2017-June/159519.html > From cf984192dba114a91630f5d9fb6cf46061e64d68 Mon Sep 17 00:00:00 2001 From: Gurchetan Singh Date: Mon, 19 Jun 2017 16:09:09 -0700 Subject: [PATCH] egl/dri2: Add kms_swrast in platform_x11 --- src/egl/drivers/dri2/platform_x11.c | 100 +++- 1 file changed, 53 insertions(+), 47 deletions(-) diff --git a/src/egl/drivers/dri2/platform_x11.c b/src/egl/drivers/dri2/platform_x11.c index 95e560a32a..19e4b0c5ca 100644 --- a/src/egl/drivers/dri2/platform_x11.c +++ b/src/egl/drivers/dri2/platform_x11.c @@ -1224,52 +1224,6 @@ disconnect: return _eglError(EGL_BAD_ALLOC, msg); } -static EGLBoolean -dri2_initialize_x11_swrast(_EGLDriver *drv, _EGLDisplay *disp) -{ - struct dri2_egl_display *dri2_dpy; - - dri2_dpy = calloc(1, sizeof *dri2_dpy); - if (!dri2_dpy) - return _eglError(EGL_BAD_ALLOC, "eglInitialize"); - - dri2_dpy->fd = -1; - if (!dri2_get_xcb_connection(drv, disp, dri2_dpy)) - goto cleanup; - - /* -* Every hardware driver_name is set using strdup. Doing the same in -* here will allow is to simply free the memory at dri2_terminate(). -*/ - dri2_dpy->driver_name = strdup("swrast"); - if (!dri2_load_driver_swrast(disp)) - goto cleanup; - - dri2_dpy->loader_extensions = swrast_loader_extensions; - - if (!dri2_create_screen(disp)) - goto cleanup; - - if (!dri2_setup_extensions(disp)) - goto cleanup; - - dri2_setup_screen(disp); - - if (!dri2_x11_add_configs_for_visuals(dri2_dpy, disp, true)) - goto cleanup; - - /* Fill vtbl last to prevent accidentally calling virtual function during -* initialization. -*/ - dri2_dpy->vtbl = &dri2_x11_swrast_display_vtbl; - - return EGL_TRUE; - - cleanup: - dri2_display_destroy(disp); - return EGL_FALSE; -} - static void dri2_x11_setup_swap_interval(struct dri2_egl_display *dri2_dpy) { @@ -1422,6 +1376,58 @@ static const __DRIextension *dri2_loader_extensions[] = { NULL, }; +static EGLBoolean +dri2_initialize_x11_kms_swrast(_EGLDriver *drv, _EGLDisplay *disp) +{ + struct dri2_egl_display *dri2_dpy; + + dri2_dpy = calloc(1, sizeof *dri2_dpy); + if (!dri2_dpy) + return _eglError(EGL_BAD_ALLOC, "eglInitialize");