Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
On Thu, Jul 21, 2016 at 6:07 PM, Stéphane Marchesin wrote: > On Tue, Jul 19, 2016 at 6:36 AM, Rob Clark wrote: >> On Tue, Jul 19, 2016 at 6:54 AM, Emil Velikov >> wrote: >>> On 19 July 2016 at 04:21, Tomasz Figa wrote: On Tue, Jul 19, 2016 at 2:35 AM, Emil Velikov wrote: > On 18 July 2016 at 16:38, Tomasz Figa wrote: >> On Mon, Jul 18, 2016 at 11:58 PM, Emil Velikov >> wrote: >>> On 18 July 2016 at 13:02, Tomasz Figa wrote: On Mon, Jul 18, 2016 at 7:28 PM, Emil Velikov wrote: > Hi Tomasz, > > On 15 July 2016 at 08:53, Tomasz Figa wrote: > >> +#define DRM_RENDER_DEV_NAME "%s/renderD%d" >> + >> +static int >> +droid_open_device(_EGLDisplay *dpy) >> +{ >> + struct dri2_egl_display *dri2_dpy = dpy->DriverData; >> + const int limit = 64; >> + const int base = 128; >> + int fd; >> + int i; >> + >> + for (i = 0; i < limit; ++i) { >> + char *card_path; >> + if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, >> base + i) < 0) > Why do we need any of this ? What gralloc implementation are you guys > using ? We are using our heavily rewritten fork of some old drm_gralloc release. It supports only render nodes and PRIME FDs and doesn't export the DRI device FD outside of its internals (which isn't actually even fully correct, at least for PRIME and render nodes, see my reply to Rob's comments). >>> That explain it, since https://chromium.googlesource.com/ does not >>> have gralloc, and >>> https://android.googlesource.com/platform/external/drm_gralloc/ has >>> both the DRM_FD define and the gem/flink function(s)? >>> >>> Can I suggest porting the fd drm_gralloc/gbm_gralloc patches to your >>> private copy/repo. This way we'll have some consistency throughout >>> gralloc implementations >> >> I'd prefer if any code using flink names was not added back. On top of >> that, our drm_gralloc doesn't really have much in common with that >> from android-x86 anymore (as I said, it was heavily rewritten) and >> there is not even a chance that with its current design flink names >> could even work. >> >> Also I'm wondering why we want to consider current brokenness of >> drm_gralloc as something to be consistent with. It's supposed to be a >> HAL library providing an uniform abstraction, but it exports private >> APIs on the side instead. Moreover, as I mentioned before, flink names >> are considered insecure and it would be really much better if we could >> just forget about them. >> >>> and you can use gbm_gralloc directly in the >>> (hopefully) not too distant future. >> >> I agree with this part, though. gbm_gralloc is definitely something >> that we might want to migrate to in the future. Although it's a bit >> lacking at the moment, so it might need a bit more time to develop the >> missing bits. [I'm CCing Gurchetan, who was investigating GBM-backed >> gralloc usable for our purposes.] >> >> In any case, the missing flink API is quite easy to handle and can be >> just stubbed out in a local header as you suggested. I don't think it >> would hurt anyone and would definitely help us and anyone not willing >> to export any private APIs from their gralloc and rely only on the >> public HAL API. >> > Looks like I wasn't clear enough here, realyl sorry about that. No > objection on nuking _any_ of the gem/flink paths, but hoping to have > the behaviour consistent with the one described in > get_native_buffer_fd. Did you mean having the PRIME FD in native_handle_t::data[0]? If so, it's more or less guaranteed by the API, because all file descriptors in handle have to be stored in first N (equals to native_handle_t::numFds) ints of native_handle_t::data[] for respective general code to properly transfer the FDs through binder when sharing between processes. Our gralloc currently supports only one PRIME FD per buffer (no separate memory planes for planar YUV) and stores it exactly in native_handle_t::data[0]. >>> Wasn't sure if the PRIME FD is at idx 0. Glad to hear it's there, thanks. >>> > >>> > > Afaict the latter must provide reasonable result for > hw_get_module(GRALLOC_HARDWARE_MODULE_ID...) and as it's missing the > perform hook existing code should work just fine. Right ? Existing code would fail with -1 as file descriptor, wouldn't it? Or I'm failing to see something? >>> Nope you're spot on - I had a dull moment. May I suggest revering the >>> patch which removed the GRALLOC_MODULE_PERFORM_GET_DRM
Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
On Tue, Jul 19, 2016 at 6:36 AM, Rob Clark wrote: > On Tue, Jul 19, 2016 at 6:54 AM, Emil Velikov > wrote: >> On 19 July 2016 at 04:21, Tomasz Figa wrote: >>> On Tue, Jul 19, 2016 at 2:35 AM, Emil Velikov >>> wrote: On 18 July 2016 at 16:38, Tomasz Figa wrote: > On Mon, Jul 18, 2016 at 11:58 PM, Emil Velikov > wrote: >> On 18 July 2016 at 13:02, Tomasz Figa wrote: >>> On Mon, Jul 18, 2016 at 7:28 PM, Emil Velikov >>> wrote: Hi Tomasz, On 15 July 2016 at 08:53, Tomasz Figa wrote: > +#define DRM_RENDER_DEV_NAME "%s/renderD%d" > + > +static int > +droid_open_device(_EGLDisplay *dpy) > +{ > + struct dri2_egl_display *dri2_dpy = dpy->DriverData; > + const int limit = 64; > + const int base = 128; > + int fd; > + int i; > + > + for (i = 0; i < limit; ++i) { > + char *card_path; > + if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, > base + i) < 0) Why do we need any of this ? What gralloc implementation are you guys using ? >>> >>> We are using our heavily rewritten fork of some old drm_gralloc >>> release. It supports only render nodes and PRIME FDs and doesn't >>> export the DRI device FD outside of its internals (which isn't >>> actually even fully correct, at least for PRIME and render nodes, see >>> my reply to Rob's comments). >>> >> That explain it, since https://chromium.googlesource.com/ does not >> have gralloc, and >> https://android.googlesource.com/platform/external/drm_gralloc/ has >> both the DRM_FD define and the gem/flink function(s)? >> >> Can I suggest porting the fd drm_gralloc/gbm_gralloc patches to your >> private copy/repo. This way we'll have some consistency throughout >> gralloc implementations > > I'd prefer if any code using flink names was not added back. On top of > that, our drm_gralloc doesn't really have much in common with that > from android-x86 anymore (as I said, it was heavily rewritten) and > there is not even a chance that with its current design flink names > could even work. > > Also I'm wondering why we want to consider current brokenness of > drm_gralloc as something to be consistent with. It's supposed to be a > HAL library providing an uniform abstraction, but it exports private > APIs on the side instead. Moreover, as I mentioned before, flink names > are considered insecure and it would be really much better if we could > just forget about them. > >> and you can use gbm_gralloc directly in the >> (hopefully) not too distant future. > > I agree with this part, though. gbm_gralloc is definitely something > that we might want to migrate to in the future. Although it's a bit > lacking at the moment, so it might need a bit more time to develop the > missing bits. [I'm CCing Gurchetan, who was investigating GBM-backed > gralloc usable for our purposes.] > > In any case, the missing flink API is quite easy to handle and can be > just stubbed out in a local header as you suggested. I don't think it > would hurt anyone and would definitely help us and anyone not willing > to export any private APIs from their gralloc and rely only on the > public HAL API. > Looks like I wasn't clear enough here, realyl sorry about that. No objection on nuking _any_ of the gem/flink paths, but hoping to have the behaviour consistent with the one described in get_native_buffer_fd. >>> >>> Did you mean having the PRIME FD in native_handle_t::data[0]? >>> >>> If so, it's more or less guaranteed by the API, because all file >>> descriptors in handle have to be stored in first N (equals to >>> native_handle_t::numFds) ints of native_handle_t::data[] for >>> respective general code to properly transfer the FDs through binder >>> when sharing between processes. >>> >>> Our gralloc currently supports only one PRIME FD per buffer (no >>> separate memory planes for planar YUV) and stores it exactly in >>> native_handle_t::data[0]. >>> >> Wasn't sure if the PRIME FD is at idx 0. Glad to hear it's there, thanks. >> >> Afaict the latter must provide reasonable result for hw_get_module(GRALLOC_HARDWARE_MODULE_ID...) and as it's missing the perform hook existing code should work just fine. Right ? >>> >>> Existing code would fail with -1 as file descriptor, wouldn't it? Or >>> I'm failing to see something? >>> >> Nope you're spot on - I had a dull moment. May I suggest revering the >> patch which removed the GRALLOC_MODULE_PERFORM_GET_DRM_FD handling in >> your gralloc ? Reason being is that the proposed code is very 'flaky' >> and can open the wrong render node on systems which have more than >>
Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
On Tue, Jul 19, 2016 at 11:40 AM, Emil Velikov wrote: > On 19 July 2016 at 14:36, Rob Clark wrote: >> On Tue, Jul 19, 2016 at 6:54 AM, Emil Velikov >> wrote: >>> On 19 July 2016 at 04:21, Tomasz Figa wrote: On Tue, Jul 19, 2016 at 2:35 AM, Emil Velikov wrote: > On 18 July 2016 at 16:38, Tomasz Figa wrote: >> On Mon, Jul 18, 2016 at 11:58 PM, Emil Velikov >> wrote: >>> On 18 July 2016 at 13:02, Tomasz Figa wrote: On Mon, Jul 18, 2016 at 7:28 PM, Emil Velikov wrote: > Hi Tomasz, > > On 15 July 2016 at 08:53, Tomasz Figa wrote: > >> +#define DRM_RENDER_DEV_NAME "%s/renderD%d" >> + >> +static int >> +droid_open_device(_EGLDisplay *dpy) >> +{ >> + struct dri2_egl_display *dri2_dpy = dpy->DriverData; >> + const int limit = 64; >> + const int base = 128; >> + int fd; >> + int i; >> + >> + for (i = 0; i < limit; ++i) { >> + char *card_path; >> + if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, >> base + i) < 0) > Why do we need any of this ? What gralloc implementation are you guys > using ? We are using our heavily rewritten fork of some old drm_gralloc release. It supports only render nodes and PRIME FDs and doesn't export the DRI device FD outside of its internals (which isn't actually even fully correct, at least for PRIME and render nodes, see my reply to Rob's comments). >>> That explain it, since https://chromium.googlesource.com/ does not >>> have gralloc, and >>> https://android.googlesource.com/platform/external/drm_gralloc/ has >>> both the DRM_FD define and the gem/flink function(s)? >>> >>> Can I suggest porting the fd drm_gralloc/gbm_gralloc patches to your >>> private copy/repo. This way we'll have some consistency throughout >>> gralloc implementations >> >> I'd prefer if any code using flink names was not added back. On top of >> that, our drm_gralloc doesn't really have much in common with that >> from android-x86 anymore (as I said, it was heavily rewritten) and >> there is not even a chance that with its current design flink names >> could even work. >> >> Also I'm wondering why we want to consider current brokenness of >> drm_gralloc as something to be consistent with. It's supposed to be a >> HAL library providing an uniform abstraction, but it exports private >> APIs on the side instead. Moreover, as I mentioned before, flink names >> are considered insecure and it would be really much better if we could >> just forget about them. >> >>> and you can use gbm_gralloc directly in the >>> (hopefully) not too distant future. >> >> I agree with this part, though. gbm_gralloc is definitely something >> that we might want to migrate to in the future. Although it's a bit >> lacking at the moment, so it might need a bit more time to develop the >> missing bits. [I'm CCing Gurchetan, who was investigating GBM-backed >> gralloc usable for our purposes.] >> >> In any case, the missing flink API is quite easy to handle and can be >> just stubbed out in a local header as you suggested. I don't think it >> would hurt anyone and would definitely help us and anyone not willing >> to export any private APIs from their gralloc and rely only on the >> public HAL API. >> > Looks like I wasn't clear enough here, realyl sorry about that. No > objection on nuking _any_ of the gem/flink paths, but hoping to have > the behaviour consistent with the one described in > get_native_buffer_fd. Did you mean having the PRIME FD in native_handle_t::data[0]? If so, it's more or less guaranteed by the API, because all file descriptors in handle have to be stored in first N (equals to native_handle_t::numFds) ints of native_handle_t::data[] for respective general code to properly transfer the FDs through binder when sharing between processes. Our gralloc currently supports only one PRIME FD per buffer (no separate memory planes for planar YUV) and stores it exactly in native_handle_t::data[0]. >>> Wasn't sure if the PRIME FD is at idx 0. Glad to hear it's there, thanks. >>> > >>> > > Afaict the latter must provide reasonable result for > hw_get_module(GRALLOC_HARDWARE_MODULE_ID...) and as it's missing the > perform hook existing code should work just fine. Right ? Existing code would fail with -1 as file descriptor, wouldn't it? Or I'm failing to see something? >>> Nope you're spot on - I had a dull moment. May I suggest revering the >>> patch which removed the GRALLOC_MODULE_PERFORM_GET_DRM_FD handling
Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
On 19 July 2016 at 14:36, Rob Clark wrote: > On Tue, Jul 19, 2016 at 6:54 AM, Emil Velikov > wrote: >> On 19 July 2016 at 04:21, Tomasz Figa wrote: >>> On Tue, Jul 19, 2016 at 2:35 AM, Emil Velikov >>> wrote: On 18 July 2016 at 16:38, Tomasz Figa wrote: > On Mon, Jul 18, 2016 at 11:58 PM, Emil Velikov > wrote: >> On 18 July 2016 at 13:02, Tomasz Figa wrote: >>> On Mon, Jul 18, 2016 at 7:28 PM, Emil Velikov >>> wrote: Hi Tomasz, On 15 July 2016 at 08:53, Tomasz Figa wrote: > +#define DRM_RENDER_DEV_NAME "%s/renderD%d" > + > +static int > +droid_open_device(_EGLDisplay *dpy) > +{ > + struct dri2_egl_display *dri2_dpy = dpy->DriverData; > + const int limit = 64; > + const int base = 128; > + int fd; > + int i; > + > + for (i = 0; i < limit; ++i) { > + char *card_path; > + if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, > base + i) < 0) Why do we need any of this ? What gralloc implementation are you guys using ? >>> >>> We are using our heavily rewritten fork of some old drm_gralloc >>> release. It supports only render nodes and PRIME FDs and doesn't >>> export the DRI device FD outside of its internals (which isn't >>> actually even fully correct, at least for PRIME and render nodes, see >>> my reply to Rob's comments). >>> >> That explain it, since https://chromium.googlesource.com/ does not >> have gralloc, and >> https://android.googlesource.com/platform/external/drm_gralloc/ has >> both the DRM_FD define and the gem/flink function(s)? >> >> Can I suggest porting the fd drm_gralloc/gbm_gralloc patches to your >> private copy/repo. This way we'll have some consistency throughout >> gralloc implementations > > I'd prefer if any code using flink names was not added back. On top of > that, our drm_gralloc doesn't really have much in common with that > from android-x86 anymore (as I said, it was heavily rewritten) and > there is not even a chance that with its current design flink names > could even work. > > Also I'm wondering why we want to consider current brokenness of > drm_gralloc as something to be consistent with. It's supposed to be a > HAL library providing an uniform abstraction, but it exports private > APIs on the side instead. Moreover, as I mentioned before, flink names > are considered insecure and it would be really much better if we could > just forget about them. > >> and you can use gbm_gralloc directly in the >> (hopefully) not too distant future. > > I agree with this part, though. gbm_gralloc is definitely something > that we might want to migrate to in the future. Although it's a bit > lacking at the moment, so it might need a bit more time to develop the > missing bits. [I'm CCing Gurchetan, who was investigating GBM-backed > gralloc usable for our purposes.] > > In any case, the missing flink API is quite easy to handle and can be > just stubbed out in a local header as you suggested. I don't think it > would hurt anyone and would definitely help us and anyone not willing > to export any private APIs from their gralloc and rely only on the > public HAL API. > Looks like I wasn't clear enough here, realyl sorry about that. No objection on nuking _any_ of the gem/flink paths, but hoping to have the behaviour consistent with the one described in get_native_buffer_fd. >>> >>> Did you mean having the PRIME FD in native_handle_t::data[0]? >>> >>> If so, it's more or less guaranteed by the API, because all file >>> descriptors in handle have to be stored in first N (equals to >>> native_handle_t::numFds) ints of native_handle_t::data[] for >>> respective general code to properly transfer the FDs through binder >>> when sharing between processes. >>> >>> Our gralloc currently supports only one PRIME FD per buffer (no >>> separate memory planes for planar YUV) and stores it exactly in >>> native_handle_t::data[0]. >>> >> Wasn't sure if the PRIME FD is at idx 0. Glad to hear it's there, thanks. >> >> Afaict the latter must provide reasonable result for hw_get_module(GRALLOC_HARDWARE_MODULE_ID...) and as it's missing the perform hook existing code should work just fine. Right ? >>> >>> Existing code would fail with -1 as file descriptor, wouldn't it? Or >>> I'm failing to see something? >>> >> Nope you're spot on - I had a dull moment. May I suggest revering the >> patch which removed the GRALLOC_MODULE_PERFORM_GET_DRM_FD handling in >> your gralloc ? Reason being is that the proposed code is very 'flaky' >> and can open the wrong render node on systems which have more than >> on
Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
On Tue, Jul 19, 2016 at 6:54 AM, Emil Velikov wrote: > On 19 July 2016 at 04:21, Tomasz Figa wrote: >> On Tue, Jul 19, 2016 at 2:35 AM, Emil Velikov >> wrote: >>> On 18 July 2016 at 16:38, Tomasz Figa wrote: On Mon, Jul 18, 2016 at 11:58 PM, Emil Velikov wrote: > On 18 July 2016 at 13:02, Tomasz Figa wrote: >> On Mon, Jul 18, 2016 at 7:28 PM, Emil Velikov >> wrote: >>> Hi Tomasz, >>> >>> On 15 July 2016 at 08:53, Tomasz Figa wrote: >>> +#define DRM_RENDER_DEV_NAME "%s/renderD%d" + +static int +droid_open_device(_EGLDisplay *dpy) +{ + struct dri2_egl_display *dri2_dpy = dpy->DriverData; + const int limit = 64; + const int base = 128; + int fd; + int i; + + for (i = 0; i < limit; ++i) { + char *card_path; + if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, base + i) < 0) >>> Why do we need any of this ? What gralloc implementation are you guys >>> using ? >> >> We are using our heavily rewritten fork of some old drm_gralloc >> release. It supports only render nodes and PRIME FDs and doesn't >> export the DRI device FD outside of its internals (which isn't >> actually even fully correct, at least for PRIME and render nodes, see >> my reply to Rob's comments). >> > That explain it, since https://chromium.googlesource.com/ does not > have gralloc, and > https://android.googlesource.com/platform/external/drm_gralloc/ has > both the DRM_FD define and the gem/flink function(s)? > > Can I suggest porting the fd drm_gralloc/gbm_gralloc patches to your > private copy/repo. This way we'll have some consistency throughout > gralloc implementations I'd prefer if any code using flink names was not added back. On top of that, our drm_gralloc doesn't really have much in common with that from android-x86 anymore (as I said, it was heavily rewritten) and there is not even a chance that with its current design flink names could even work. Also I'm wondering why we want to consider current brokenness of drm_gralloc as something to be consistent with. It's supposed to be a HAL library providing an uniform abstraction, but it exports private APIs on the side instead. Moreover, as I mentioned before, flink names are considered insecure and it would be really much better if we could just forget about them. > and you can use gbm_gralloc directly in the > (hopefully) not too distant future. I agree with this part, though. gbm_gralloc is definitely something that we might want to migrate to in the future. Although it's a bit lacking at the moment, so it might need a bit more time to develop the missing bits. [I'm CCing Gurchetan, who was investigating GBM-backed gralloc usable for our purposes.] In any case, the missing flink API is quite easy to handle and can be just stubbed out in a local header as you suggested. I don't think it would hurt anyone and would definitely help us and anyone not willing to export any private APIs from their gralloc and rely only on the public HAL API. >>> Looks like I wasn't clear enough here, realyl sorry about that. No >>> objection on nuking _any_ of the gem/flink paths, but hoping to have >>> the behaviour consistent with the one described in >>> get_native_buffer_fd. >> >> Did you mean having the PRIME FD in native_handle_t::data[0]? >> >> If so, it's more or less guaranteed by the API, because all file >> descriptors in handle have to be stored in first N (equals to >> native_handle_t::numFds) ints of native_handle_t::data[] for >> respective general code to properly transfer the FDs through binder >> when sharing between processes. >> >> Our gralloc currently supports only one PRIME FD per buffer (no >> separate memory planes for planar YUV) and stores it exactly in >> native_handle_t::data[0]. >> > Wasn't sure if the PRIME FD is at idx 0. Glad to hear it's there, thanks. > >>> > >>> >>> Afaict the latter must provide reasonable result for >>> hw_get_module(GRALLOC_HARDWARE_MODULE_ID...) and as it's missing the >>> perform hook existing code should work just fine. Right ? >> >> Existing code would fail with -1 as file descriptor, wouldn't it? Or >> I'm failing to see something? >> > Nope you're spot on - I had a dull moment. May I suggest revering the > patch which removed the GRALLOC_MODULE_PERFORM_GET_DRM_FD handling in > your gralloc ? Reason being is that the proposed code is very 'flaky' > and can open the wrong render node on systems which have more than > one. I think the answer is a bit of yes and no at the same time. Starting with no, it's incorrect for gralloc to share the DRI device
Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
On 19 July 2016 at 04:21, Tomasz Figa wrote: > On Tue, Jul 19, 2016 at 2:35 AM, Emil Velikov > wrote: >> On 18 July 2016 at 16:38, Tomasz Figa wrote: >>> On Mon, Jul 18, 2016 at 11:58 PM, Emil Velikov >>> wrote: On 18 July 2016 at 13:02, Tomasz Figa wrote: > On Mon, Jul 18, 2016 at 7:28 PM, Emil Velikov > wrote: >> Hi Tomasz, >> >> On 15 July 2016 at 08:53, Tomasz Figa wrote: >> >>> +#define DRM_RENDER_DEV_NAME "%s/renderD%d" >>> + >>> +static int >>> +droid_open_device(_EGLDisplay *dpy) >>> +{ >>> + struct dri2_egl_display *dri2_dpy = dpy->DriverData; >>> + const int limit = 64; >>> + const int base = 128; >>> + int fd; >>> + int i; >>> + >>> + for (i = 0; i < limit; ++i) { >>> + char *card_path; >>> + if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, base >>> + i) < 0) >> Why do we need any of this ? What gralloc implementation are you guys >> using ? > > We are using our heavily rewritten fork of some old drm_gralloc > release. It supports only render nodes and PRIME FDs and doesn't > export the DRI device FD outside of its internals (which isn't > actually even fully correct, at least for PRIME and render nodes, see > my reply to Rob's comments). > That explain it, since https://chromium.googlesource.com/ does not have gralloc, and https://android.googlesource.com/platform/external/drm_gralloc/ has both the DRM_FD define and the gem/flink function(s)? Can I suggest porting the fd drm_gralloc/gbm_gralloc patches to your private copy/repo. This way we'll have some consistency throughout gralloc implementations >>> >>> I'd prefer if any code using flink names was not added back. On top of >>> that, our drm_gralloc doesn't really have much in common with that >>> from android-x86 anymore (as I said, it was heavily rewritten) and >>> there is not even a chance that with its current design flink names >>> could even work. >>> >>> Also I'm wondering why we want to consider current brokenness of >>> drm_gralloc as something to be consistent with. It's supposed to be a >>> HAL library providing an uniform abstraction, but it exports private >>> APIs on the side instead. Moreover, as I mentioned before, flink names >>> are considered insecure and it would be really much better if we could >>> just forget about them. >>> and you can use gbm_gralloc directly in the (hopefully) not too distant future. >>> >>> I agree with this part, though. gbm_gralloc is definitely something >>> that we might want to migrate to in the future. Although it's a bit >>> lacking at the moment, so it might need a bit more time to develop the >>> missing bits. [I'm CCing Gurchetan, who was investigating GBM-backed >>> gralloc usable for our purposes.] >>> >>> In any case, the missing flink API is quite easy to handle and can be >>> just stubbed out in a local header as you suggested. I don't think it >>> would hurt anyone and would definitely help us and anyone not willing >>> to export any private APIs from their gralloc and rely only on the >>> public HAL API. >>> >> Looks like I wasn't clear enough here, realyl sorry about that. No >> objection on nuking _any_ of the gem/flink paths, but hoping to have >> the behaviour consistent with the one described in >> get_native_buffer_fd. > > Did you mean having the PRIME FD in native_handle_t::data[0]? > > If so, it's more or less guaranteed by the API, because all file > descriptors in handle have to be stored in first N (equals to > native_handle_t::numFds) ints of native_handle_t::data[] for > respective general code to properly transfer the FDs through binder > when sharing between processes. > > Our gralloc currently supports only one PRIME FD per buffer (no > separate memory planes for planar YUV) and stores it exactly in > native_handle_t::data[0]. > Wasn't sure if the PRIME FD is at idx 0. Glad to hear it's there, thanks. >> >> >> Afaict the latter must provide reasonable result for >> hw_get_module(GRALLOC_HARDWARE_MODULE_ID...) and as it's missing the >> perform hook existing code should work just fine. Right ? > > Existing code would fail with -1 as file descriptor, wouldn't it? Or > I'm failing to see something? > Nope you're spot on - I had a dull moment. May I suggest revering the patch which removed the GRALLOC_MODULE_PERFORM_GET_DRM_FD handling in your gralloc ? Reason being is that the proposed code is very 'flaky' and can open the wrong render node on systems which have more than one. >>> >>> I think the answer is a bit of yes and no at the same time. >>> >>> Starting with no, it's incorrect for gralloc to share the DRI device >>> FD with Mesa for multiple reasons: >>> - there are cases when the allocator used is different that the render >>> node, >> Can you please provide an example how the cu
Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
On Tue, Jul 19, 2016 at 2:35 AM, Emil Velikov wrote: > On 18 July 2016 at 16:38, Tomasz Figa wrote: >> On Mon, Jul 18, 2016 at 11:58 PM, Emil Velikov >> wrote: >>> On 18 July 2016 at 13:02, Tomasz Figa wrote: On Mon, Jul 18, 2016 at 7:28 PM, Emil Velikov wrote: > Hi Tomasz, > > On 15 July 2016 at 08:53, Tomasz Figa wrote: > >> +#define DRM_RENDER_DEV_NAME "%s/renderD%d" >> + >> +static int >> +droid_open_device(_EGLDisplay *dpy) >> +{ >> + struct dri2_egl_display *dri2_dpy = dpy->DriverData; >> + const int limit = 64; >> + const int base = 128; >> + int fd; >> + int i; >> + >> + for (i = 0; i < limit; ++i) { >> + char *card_path; >> + if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, base >> + i) < 0) > Why do we need any of this ? What gralloc implementation are you guys > using ? We are using our heavily rewritten fork of some old drm_gralloc release. It supports only render nodes and PRIME FDs and doesn't export the DRI device FD outside of its internals (which isn't actually even fully correct, at least for PRIME and render nodes, see my reply to Rob's comments). >>> That explain it, since https://chromium.googlesource.com/ does not >>> have gralloc, and >>> https://android.googlesource.com/platform/external/drm_gralloc/ has >>> both the DRM_FD define and the gem/flink function(s)? >>> >>> Can I suggest porting the fd drm_gralloc/gbm_gralloc patches to your >>> private copy/repo. This way we'll have some consistency throughout >>> gralloc implementations >> >> I'd prefer if any code using flink names was not added back. On top of >> that, our drm_gralloc doesn't really have much in common with that >> from android-x86 anymore (as I said, it was heavily rewritten) and >> there is not even a chance that with its current design flink names >> could even work. >> >> Also I'm wondering why we want to consider current brokenness of >> drm_gralloc as something to be consistent with. It's supposed to be a >> HAL library providing an uniform abstraction, but it exports private >> APIs on the side instead. Moreover, as I mentioned before, flink names >> are considered insecure and it would be really much better if we could >> just forget about them. >> >>> and you can use gbm_gralloc directly in the >>> (hopefully) not too distant future. >> >> I agree with this part, though. gbm_gralloc is definitely something >> that we might want to migrate to in the future. Although it's a bit >> lacking at the moment, so it might need a bit more time to develop the >> missing bits. [I'm CCing Gurchetan, who was investigating GBM-backed >> gralloc usable for our purposes.] >> >> In any case, the missing flink API is quite easy to handle and can be >> just stubbed out in a local header as you suggested. I don't think it >> would hurt anyone and would definitely help us and anyone not willing >> to export any private APIs from their gralloc and rely only on the >> public HAL API. >> > Looks like I wasn't clear enough here, realyl sorry about that. No > objection on nuking _any_ of the gem/flink paths, but hoping to have > the behaviour consistent with the one described in > get_native_buffer_fd. Did you mean having the PRIME FD in native_handle_t::data[0]? If so, it's more or less guaranteed by the API, because all file descriptors in handle have to be stored in first N (equals to native_handle_t::numFds) ints of native_handle_t::data[] for respective general code to properly transfer the FDs through binder when sharing between processes. Our gralloc currently supports only one PRIME FD per buffer (no separate memory planes for planar YUV) and stores it exactly in native_handle_t::data[0]. > >>> > > Afaict the latter must provide reasonable result for > hw_get_module(GRALLOC_HARDWARE_MODULE_ID...) and as it's missing the > perform hook existing code should work just fine. Right ? Existing code would fail with -1 as file descriptor, wouldn't it? Or I'm failing to see something? >>> Nope you're spot on - I had a dull moment. May I suggest revering the >>> patch which removed the GRALLOC_MODULE_PERFORM_GET_DRM_FD handling in >>> your gralloc ? Reason being is that the proposed code is very 'flaky' >>> and can open the wrong render node on systems which have more than >>> one. >> >> I think the answer is a bit of yes and no at the same time. >> >> Starting with no, it's incorrect for gralloc to share the DRI device >> FD with Mesa for multiple reasons: >> - there are cases when the allocator used is different that the render node, > Can you please provide an example how the current open-source > gralloc/EGL stack might hit this ? Only a mix of closed and > open-source components comes to mind :-\ Well, yes. I don't think I can hide the fact that we have to use closed source components on some platforms. To put it simply,
Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
On 18 July 2016 at 16:38, Tomasz Figa wrote: > On Mon, Jul 18, 2016 at 11:58 PM, Emil Velikov > wrote: >> On 18 July 2016 at 13:02, Tomasz Figa wrote: >>> On Mon, Jul 18, 2016 at 7:28 PM, Emil Velikov >>> wrote: Hi Tomasz, On 15 July 2016 at 08:53, Tomasz Figa wrote: > We can support render nodes alone without any private headers, so let's > make support for control nodes depend on presence of private drm_gralloc > headers. > > Signed-off-by: Tomasz Figa > --- > src/egl/Android.mk | 1 + > src/egl/drivers/dri2/egl_dri2.h | 2 + > src/egl/drivers/dri2/platform_android.c | 194 > ++-- > 3 files changed, 138 insertions(+), 59 deletions(-) > > diff --git a/src/egl/Android.mk b/src/egl/Android.mk > index bfd56a7..72ec02a 100644 > --- a/src/egl/Android.mk > +++ b/src/egl/Android.mk > @@ -41,6 +41,7 @@ LOCAL_SRC_FILES := \ > LOCAL_CFLAGS := \ > -D_EGL_NATIVE_PLATFORM=_EGL_PLATFORM_ANDROID \ > -D_EGL_BUILT_IN_DRIVER_DRI2 \ > + -DHAS_GRALLOC_DRM_HEADERS \ > -DHAVE_ANDROID_PLATFORM > > LOCAL_C_INCLUDES := \ > diff --git a/src/egl/drivers/dri2/egl_dri2.h > b/src/egl/drivers/dri2/egl_dri2.h > index 3ffc177..6f9623b 100644 > --- a/src/egl/drivers/dri2/egl_dri2.h > +++ b/src/egl/drivers/dri2/egl_dri2.h > @@ -65,7 +65,9 @@ > #endif > > #include > +#ifdef HAS_GRALLOC_DRM_HEADERS > #include > +#endif All of this/these can be simplified, by using a local header which includes gralloc_drm_handle.h (if possible) and alternatively providing dummy defines and static inline function(s). >>> >>> Sounds good to me. I'll give it a try. >>> >> My grammar is a bit off so here and example of what I meant, just in case: >> >> cat local_header.h >> #ifdef HAS_GRALLOC_DRM_HEADERS >> #include >> #include >> #else >> #define FOO >> static inline bar(...) >> #endif >> > @@ -509,53 +516,43 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay > *disp, _EGLSurface *draw) > } > > static _EGLImage * > -dri2_create_image_android_native_buffer(_EGLDisplay *disp, > -_EGLContext *ctx, > -struct ANativeWindowBuffer *buf) > +droid_create_image_from_prime_fd(_EGLDisplay *disp, _EGLContext *ctx, > + struct ANativeWindowBuffer *buf) Please keep have this as a separate patch - "factorise dri2_create_image_android_native_buffer" >>> >>> Okay. >>> > + _eglError(EGL_BAD_PARAMETER, "eglCreateEGLImageKHR: Only PRIME > buffers are supported"); > + return NULL; This (s/NULL/0/) can live in as the static inline gralloc_drm_get_gem_handle() in our local header. >>> >>> Okay. >>> > +#define DRM_RENDER_DEV_NAME "%s/renderD%d" > + > +static int > +droid_open_device(_EGLDisplay *dpy) > +{ > + struct dri2_egl_display *dri2_dpy = dpy->DriverData; > + const int limit = 64; > + const int base = 128; > + int fd; > + int i; > + > + for (i = 0; i < limit; ++i) { > + char *card_path; > + if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, base + > i) < 0) Why do we need any of this ? What gralloc implementation are you guys using ? >>> >>> We are using our heavily rewritten fork of some old drm_gralloc >>> release. It supports only render nodes and PRIME FDs and doesn't >>> export the DRI device FD outside of its internals (which isn't >>> actually even fully correct, at least for PRIME and render nodes, see >>> my reply to Rob's comments). >>> >> That explain it, since https://chromium.googlesource.com/ does not >> have gralloc, and >> https://android.googlesource.com/platform/external/drm_gralloc/ has >> both the DRM_FD define and the gem/flink function(s)? >> >> Can I suggest porting the fd drm_gralloc/gbm_gralloc patches to your >> private copy/repo. This way we'll have some consistency throughout >> gralloc implementations > > I'd prefer if any code using flink names was not added back. On top of > that, our drm_gralloc doesn't really have much in common with that > from android-x86 anymore (as I said, it was heavily rewritten) and > there is not even a chance that with its current design flink names > could even work. > > Also I'm wondering why we want to consider current brokenness of > drm_gralloc as something to be consistent with. It's supposed to be a > HAL library providing an uniform abstraction, but it exports private > APIs on the side instead. Moreover, as I mentioned before, flink names > are considered insecure and it would be really much better if we could > just forget about them. > >> and you can use gbm_gralloc directly in the >> (hopefully) not too distant
Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
On Mon, Jul 18, 2016 at 11:58 PM, Emil Velikov wrote: > On 18 July 2016 at 13:02, Tomasz Figa wrote: >> On Mon, Jul 18, 2016 at 7:28 PM, Emil Velikov >> wrote: >>> Hi Tomasz, >>> >>> On 15 July 2016 at 08:53, Tomasz Figa wrote: We can support render nodes alone without any private headers, so let's make support for control nodes depend on presence of private drm_gralloc headers. Signed-off-by: Tomasz Figa --- src/egl/Android.mk | 1 + src/egl/drivers/dri2/egl_dri2.h | 2 + src/egl/drivers/dri2/platform_android.c | 194 ++-- 3 files changed, 138 insertions(+), 59 deletions(-) diff --git a/src/egl/Android.mk b/src/egl/Android.mk index bfd56a7..72ec02a 100644 --- a/src/egl/Android.mk +++ b/src/egl/Android.mk @@ -41,6 +41,7 @@ LOCAL_SRC_FILES := \ LOCAL_CFLAGS := \ -D_EGL_NATIVE_PLATFORM=_EGL_PLATFORM_ANDROID \ -D_EGL_BUILT_IN_DRIVER_DRI2 \ + -DHAS_GRALLOC_DRM_HEADERS \ -DHAVE_ANDROID_PLATFORM LOCAL_C_INCLUDES := \ diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h index 3ffc177..6f9623b 100644 --- a/src/egl/drivers/dri2/egl_dri2.h +++ b/src/egl/drivers/dri2/egl_dri2.h @@ -65,7 +65,9 @@ #endif #include +#ifdef HAS_GRALLOC_DRM_HEADERS #include +#endif >>> All of this/these can be simplified, by using a local header which >>> includes gralloc_drm_handle.h (if possible) and alternatively >>> providing dummy defines and static inline function(s). >> >> Sounds good to me. I'll give it a try. >> > My grammar is a bit off so here and example of what I meant, just in case: > > cat local_header.h > #ifdef HAS_GRALLOC_DRM_HEADERS > #include > #include > #else > #define FOO > static inline bar(...) > #endif > >>> >>> @@ -509,53 +516,43 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw) } static _EGLImage * -dri2_create_image_android_native_buffer(_EGLDisplay *disp, -_EGLContext *ctx, -struct ANativeWindowBuffer *buf) +droid_create_image_from_prime_fd(_EGLDisplay *disp, _EGLContext *ctx, + struct ANativeWindowBuffer *buf) >>> Please keep have this as a separate patch - "factorise >>> dri2_create_image_android_native_buffer" >> >> Okay. >> >>> >>> + _eglError(EGL_BAD_PARAMETER, "eglCreateEGLImageKHR: Only PRIME buffers are supported"); + return NULL; >>> This (s/NULL/0/) can live in as the static inline >>> gralloc_drm_get_gem_handle() in our local header. >> >> Okay. >> >>> >>> +#define DRM_RENDER_DEV_NAME "%s/renderD%d" + +static int +droid_open_device(_EGLDisplay *dpy) +{ + struct dri2_egl_display *dri2_dpy = dpy->DriverData; + const int limit = 64; + const int base = 128; + int fd; + int i; + + for (i = 0; i < limit; ++i) { + char *card_path; + if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, base + i) < 0) >>> Why do we need any of this ? What gralloc implementation are you guys using >>> ? >> >> We are using our heavily rewritten fork of some old drm_gralloc >> release. It supports only render nodes and PRIME FDs and doesn't >> export the DRI device FD outside of its internals (which isn't >> actually even fully correct, at least for PRIME and render nodes, see >> my reply to Rob's comments). >> > That explain it, since https://chromium.googlesource.com/ does not > have gralloc, and > https://android.googlesource.com/platform/external/drm_gralloc/ has > both the DRM_FD define and the gem/flink function(s)? > > Can I suggest porting the fd drm_gralloc/gbm_gralloc patches to your > private copy/repo. This way we'll have some consistency throughout > gralloc implementations I'd prefer if any code using flink names was not added back. On top of that, our drm_gralloc doesn't really have much in common with that from android-x86 anymore (as I said, it was heavily rewritten) and there is not even a chance that with its current design flink names could even work. Also I'm wondering why we want to consider current brokenness of drm_gralloc as something to be consistent with. It's supposed to be a HAL library providing an uniform abstraction, but it exports private APIs on the side instead. Moreover, as I mentioned before, flink names are considered insecure and it would be really much better if we could just forget about them. > and you can use gbm_gralloc directly in the > (hopefully) not too distant future. I agree with this part, though. gbm_gralloc is definitely something that we might want to migrate to in the future. Although it's a bit lacking at the moment, so it might need a bit more ti
Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
On 18 July 2016 at 13:02, Tomasz Figa wrote: > On Mon, Jul 18, 2016 at 7:28 PM, Emil Velikov > wrote: >> Hi Tomasz, >> >> On 15 July 2016 at 08:53, Tomasz Figa wrote: >>> We can support render nodes alone without any private headers, so let's >>> make support for control nodes depend on presence of private drm_gralloc >>> headers. >>> >>> Signed-off-by: Tomasz Figa >>> --- >>> src/egl/Android.mk | 1 + >>> src/egl/drivers/dri2/egl_dri2.h | 2 + >>> src/egl/drivers/dri2/platform_android.c | 194 >>> ++-- >>> 3 files changed, 138 insertions(+), 59 deletions(-) >>> >>> diff --git a/src/egl/Android.mk b/src/egl/Android.mk >>> index bfd56a7..72ec02a 100644 >>> --- a/src/egl/Android.mk >>> +++ b/src/egl/Android.mk >>> @@ -41,6 +41,7 @@ LOCAL_SRC_FILES := \ >>> LOCAL_CFLAGS := \ >>> -D_EGL_NATIVE_PLATFORM=_EGL_PLATFORM_ANDROID \ >>> -D_EGL_BUILT_IN_DRIVER_DRI2 \ >>> + -DHAS_GRALLOC_DRM_HEADERS \ >>> -DHAVE_ANDROID_PLATFORM >>> >>> LOCAL_C_INCLUDES := \ >>> diff --git a/src/egl/drivers/dri2/egl_dri2.h >>> b/src/egl/drivers/dri2/egl_dri2.h >>> index 3ffc177..6f9623b 100644 >>> --- a/src/egl/drivers/dri2/egl_dri2.h >>> +++ b/src/egl/drivers/dri2/egl_dri2.h >>> @@ -65,7 +65,9 @@ >>> #endif >>> >>> #include >>> +#ifdef HAS_GRALLOC_DRM_HEADERS >>> #include >>> +#endif >> All of this/these can be simplified, by using a local header which >> includes gralloc_drm_handle.h (if possible) and alternatively >> providing dummy defines and static inline function(s). > > Sounds good to me. I'll give it a try. > My grammar is a bit off so here and example of what I meant, just in case: cat local_header.h #ifdef HAS_GRALLOC_DRM_HEADERS #include #include #else #define FOO static inline bar(...) #endif >> >> >>> @@ -509,53 +516,43 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay >>> *disp, _EGLSurface *draw) >>> } >>> >>> static _EGLImage * >>> -dri2_create_image_android_native_buffer(_EGLDisplay *disp, >>> -_EGLContext *ctx, >>> -struct ANativeWindowBuffer *buf) >>> +droid_create_image_from_prime_fd(_EGLDisplay *disp, _EGLContext *ctx, >>> + struct ANativeWindowBuffer *buf) >> Please keep have this as a separate patch - "factorise >> dri2_create_image_android_native_buffer" > > Okay. > >> >> >>> + _eglError(EGL_BAD_PARAMETER, "eglCreateEGLImageKHR: Only PRIME buffers >>> are supported"); >>> + return NULL; >> This (s/NULL/0/) can live in as the static inline >> gralloc_drm_get_gem_handle() in our local header. > > Okay. > >> >> >>> +#define DRM_RENDER_DEV_NAME "%s/renderD%d" >>> + >>> +static int >>> +droid_open_device(_EGLDisplay *dpy) >>> +{ >>> + struct dri2_egl_display *dri2_dpy = dpy->DriverData; >>> + const int limit = 64; >>> + const int base = 128; >>> + int fd; >>> + int i; >>> + >>> + for (i = 0; i < limit; ++i) { >>> + char *card_path; >>> + if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, base + >>> i) < 0) >> Why do we need any of this ? What gralloc implementation are you guys using ? > > We are using our heavily rewritten fork of some old drm_gralloc > release. It supports only render nodes and PRIME FDs and doesn't > export the DRI device FD outside of its internals (which isn't > actually even fully correct, at least for PRIME and render nodes, see > my reply to Rob's comments). > That explain it, since https://chromium.googlesource.com/ does not have gralloc, and https://android.googlesource.com/platform/external/drm_gralloc/ has both the DRM_FD define and the gem/flink function(s)? Can I suggest porting the fd drm_gralloc/gbm_gralloc patches to your private copy/repo. This way we'll have some consistency throughout gralloc implementations and you can use gbm_gralloc directly in the (hopefully) not too distant future. >> >> Afaict the latter must provide reasonable result for >> hw_get_module(GRALLOC_HARDWARE_MODULE_ID...) and as it's missing the >> perform hook existing code should work just fine. Right ? > > Existing code would fail with -1 as file descriptor, wouldn't it? Or > I'm failing to see something? > Nope you're spot on - I had a dull moment. May I suggest revering the patch which removed the GRALLOC_MODULE_PERFORM_GET_DRM_FD handling in your gralloc ? Reason being is that the proposed code is very 'flaky' and can open the wrong render node on systems which have more than one. Regards, Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
On Mon, Jul 18, 2016 at 7:28 PM, Emil Velikov wrote: > Hi Tomasz, > > On 15 July 2016 at 08:53, Tomasz Figa wrote: >> We can support render nodes alone without any private headers, so let's >> make support for control nodes depend on presence of private drm_gralloc >> headers. >> >> Signed-off-by: Tomasz Figa >> --- >> src/egl/Android.mk | 1 + >> src/egl/drivers/dri2/egl_dri2.h | 2 + >> src/egl/drivers/dri2/platform_android.c | 194 >> ++-- >> 3 files changed, 138 insertions(+), 59 deletions(-) >> >> diff --git a/src/egl/Android.mk b/src/egl/Android.mk >> index bfd56a7..72ec02a 100644 >> --- a/src/egl/Android.mk >> +++ b/src/egl/Android.mk >> @@ -41,6 +41,7 @@ LOCAL_SRC_FILES := \ >> LOCAL_CFLAGS := \ >> -D_EGL_NATIVE_PLATFORM=_EGL_PLATFORM_ANDROID \ >> -D_EGL_BUILT_IN_DRIVER_DRI2 \ >> + -DHAS_GRALLOC_DRM_HEADERS \ >> -DHAVE_ANDROID_PLATFORM >> >> LOCAL_C_INCLUDES := \ >> diff --git a/src/egl/drivers/dri2/egl_dri2.h >> b/src/egl/drivers/dri2/egl_dri2.h >> index 3ffc177..6f9623b 100644 >> --- a/src/egl/drivers/dri2/egl_dri2.h >> +++ b/src/egl/drivers/dri2/egl_dri2.h >> @@ -65,7 +65,9 @@ >> #endif >> >> #include >> +#ifdef HAS_GRALLOC_DRM_HEADERS >> #include >> +#endif > All of this/these can be simplified, by using a local header which > includes gralloc_drm_handle.h (if possible) and alternatively > providing dummy defines and static inline function(s). Sounds good to me. I'll give it a try. > > >> @@ -509,53 +516,43 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, >> _EGLSurface *draw) >> } >> >> static _EGLImage * >> -dri2_create_image_android_native_buffer(_EGLDisplay *disp, >> -_EGLContext *ctx, >> -struct ANativeWindowBuffer *buf) >> +droid_create_image_from_prime_fd(_EGLDisplay *disp, _EGLContext *ctx, >> + struct ANativeWindowBuffer *buf) > Please keep have this as a separate patch - "factorise > dri2_create_image_android_native_buffer" Okay. > > >> + _eglError(EGL_BAD_PARAMETER, "eglCreateEGLImageKHR: Only PRIME buffers >> are supported"); >> + return NULL; > This (s/NULL/0/) can live in as the static inline > gralloc_drm_get_gem_handle() in our local header. Okay. > > >> +#define DRM_RENDER_DEV_NAME "%s/renderD%d" >> + >> +static int >> +droid_open_device(_EGLDisplay *dpy) >> +{ >> + struct dri2_egl_display *dri2_dpy = dpy->DriverData; >> + const int limit = 64; >> + const int base = 128; >> + int fd; >> + int i; >> + >> + for (i = 0; i < limit; ++i) { >> + char *card_path; >> + if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, base + i) >> < 0) > Why do we need any of this ? What gralloc implementation are you guys using ? We are using our heavily rewritten fork of some old drm_gralloc release. It supports only render nodes and PRIME FDs and doesn't export the DRI device FD outside of its internals (which isn't actually even fully correct, at least for PRIME and render nodes, see my reply to Rob's comments). > > Afaict the latter must provide reasonable result for > hw_get_module(GRALLOC_HARDWARE_MODULE_ID...) and as it's missing the > perform hook existing code should work just fine. Right ? Existing code would fail with -1 as file descriptor, wouldn't it? Or I'm failing to see something? Best regards, Tomasz ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
Hi Tomasz, On 15 July 2016 at 08:53, Tomasz Figa wrote: > We can support render nodes alone without any private headers, so let's > make support for control nodes depend on presence of private drm_gralloc > headers. > > Signed-off-by: Tomasz Figa > --- > src/egl/Android.mk | 1 + > src/egl/drivers/dri2/egl_dri2.h | 2 + > src/egl/drivers/dri2/platform_android.c | 194 > ++-- > 3 files changed, 138 insertions(+), 59 deletions(-) > > diff --git a/src/egl/Android.mk b/src/egl/Android.mk > index bfd56a7..72ec02a 100644 > --- a/src/egl/Android.mk > +++ b/src/egl/Android.mk > @@ -41,6 +41,7 @@ LOCAL_SRC_FILES := \ > LOCAL_CFLAGS := \ > -D_EGL_NATIVE_PLATFORM=_EGL_PLATFORM_ANDROID \ > -D_EGL_BUILT_IN_DRIVER_DRI2 \ > + -DHAS_GRALLOC_DRM_HEADERS \ > -DHAVE_ANDROID_PLATFORM > > LOCAL_C_INCLUDES := \ > diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h > index 3ffc177..6f9623b 100644 > --- a/src/egl/drivers/dri2/egl_dri2.h > +++ b/src/egl/drivers/dri2/egl_dri2.h > @@ -65,7 +65,9 @@ > #endif > > #include > +#ifdef HAS_GRALLOC_DRM_HEADERS > #include > +#endif All of this/these can be simplified, by using a local header which includes gralloc_drm_handle.h (if possible) and alternatively providing dummy defines and static inline function(s). > @@ -509,53 +516,43 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, > _EGLSurface *draw) > } > > static _EGLImage * > -dri2_create_image_android_native_buffer(_EGLDisplay *disp, > -_EGLContext *ctx, > -struct ANativeWindowBuffer *buf) > +droid_create_image_from_prime_fd(_EGLDisplay *disp, _EGLContext *ctx, > + struct ANativeWindowBuffer *buf) Please keep have this as a separate patch - "factorise dri2_create_image_android_native_buffer" > + _eglError(EGL_BAD_PARAMETER, "eglCreateEGLImageKHR: Only PRIME buffers > are supported"); > + return NULL; This (s/NULL/0/) can live in as the static inline gralloc_drm_get_gem_handle() in our local header. > +#define DRM_RENDER_DEV_NAME "%s/renderD%d" > + > +static int > +droid_open_device(_EGLDisplay *dpy) > +{ > + struct dri2_egl_display *dri2_dpy = dpy->DriverData; > + const int limit = 64; > + const int base = 128; > + int fd; > + int i; > + > + for (i = 0; i < limit; ++i) { > + char *card_path; > + if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, base + i) > < 0) Why do we need any of this ? What gralloc implementation are you guys using ? Afaict the latter must provide reasonable result for hw_get_module(GRALLOC_HARDWARE_MODULE_ID...) and as it's missing the perform hook existing code should work just fine. Right ? Regards, Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
On Sat, Jul 16, 2016 at 11:17 PM, Rob Herring wrote: > On Fri, Jul 15, 2016 at 2:53 AM, Tomasz Figa wrote: >> We can support render nodes alone without any private headers, so let's >> make support for control nodes depend on presence of private drm_gralloc >> headers. > > This is a complicated change for what amounts to just needing the > flink name out of the gralloc handle. You don't have the dependency > for prime fds because I assumed it is the first fd in the gralloc > handle (as I was trying to avoid the header dependency). We could do > the same for flink names. > > Whether we want to make support from control nodes ifdef'ed should be > its own option not implied from having headers or not. Except that flink name is just an integer and it doesn't even have to be present in the handle, so the assumption is much less likely to hold. Prime FD belongs to a separate class and it's very unlikely that there is another file descriptor there, except maybe other Prime FDs when the buffer consist of multiple planes. We don't have flink names in our handles. I was trying to make the change in a manner that doesn't break anything for existing users, while letting new users with gralloc that doesn't provide any custom interface (doesn't even provide access to the handle struct, which should be opaque as the name suggests). Also flink names are not the only custom API that requires those headers. There is also the GET_DRM_FD perform call, which we don't support, because it generally isn't a good idea (gralloc can be stepping over Mesa's feet, because, for example, GEM handles are not reference counted...). Possibly just the macro should be split into two, one GRALLOC_USES_FLINK_NAMES and GRALLOC_HAS_PERFORM_GET_DRM_FD. However, to be honest, I'd be for dropping support for both features and making the gralloc you use saner. Flink names pose a security risk and I don't think there is any reason why we should continue using them, while GET_DRM_FD, even if potentially not unsafe, has its own set of problems. Best regards, Tomasz ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
On Fri, Jul 15, 2016 at 2:53 AM, Tomasz Figa wrote: > We can support render nodes alone without any private headers, so let's > make support for control nodes depend on presence of private drm_gralloc > headers. This is a complicated change for what amounts to just needing the flink name out of the gralloc handle. You don't have the dependency for prime fds because I assumed it is the first fd in the gralloc handle (as I was trying to avoid the header dependency). We could do the same for flink names. Whether we want to make support from control nodes ifdef'ed should be its own option not implied from having headers or not. Rob ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
We can support render nodes alone without any private headers, so let's make support for control nodes depend on presence of private drm_gralloc headers. Signed-off-by: Tomasz Figa --- src/egl/Android.mk | 1 + src/egl/drivers/dri2/egl_dri2.h | 2 + src/egl/drivers/dri2/platform_android.c | 194 ++-- 3 files changed, 138 insertions(+), 59 deletions(-) diff --git a/src/egl/Android.mk b/src/egl/Android.mk index bfd56a7..72ec02a 100644 --- a/src/egl/Android.mk +++ b/src/egl/Android.mk @@ -41,6 +41,7 @@ LOCAL_SRC_FILES := \ LOCAL_CFLAGS := \ -D_EGL_NATIVE_PLATFORM=_EGL_PLATFORM_ANDROID \ -D_EGL_BUILT_IN_DRIVER_DRI2 \ + -DHAS_GRALLOC_DRM_HEADERS \ -DHAVE_ANDROID_PLATFORM LOCAL_C_INCLUDES := \ diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h index 3ffc177..6f9623b 100644 --- a/src/egl/drivers/dri2/egl_dri2.h +++ b/src/egl/drivers/dri2/egl_dri2.h @@ -65,7 +65,9 @@ #endif #include +#ifdef HAS_GRALLOC_DRM_HEADERS #include +#endif #include #endif /* HAVE_ANDROID_PLATFORM */ diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c index 0f707dd..4473400 100644 --- a/src/egl/drivers/dri2/platform_android.c +++ b/src/egl/drivers/dri2/platform_android.c @@ -38,7 +38,10 @@ #include "loader.h" #include "egl_dri2.h" #include "egl_dri2_fallbacks.h" + +#ifdef HAS_GRALLOC_DRM_HEADERS #include "gralloc_drm.h" +#endif static int get_format_bpp(int native) @@ -104,11 +107,13 @@ get_native_buffer_fd(struct ANativeWindowBuffer *buf) return (handle && handle->numFds) ? handle->data[0] : -1; } +#ifdef HAS_GRALLOC_DRM_HEADERS static int get_native_buffer_name(struct ANativeWindowBuffer *buf) { return gralloc_drm_get_gem_handle(buf->handle); } +#endif static EGLBoolean droid_window_dequeue_buffer(struct dri2_egl_surface *dri2_surf) @@ -210,6 +215,7 @@ droid_window_cancel_buffer(_EGLDisplay *disp, struct dri2_egl_surface *dri2_surf droid_window_enqueue_buffer(disp, dri2_surf); } +#ifdef HAS_GRALLOC_DRM_HEADERS static __DRIbuffer * droid_alloc_local_buffer(struct dri2_egl_surface *dri2_surf, unsigned int att, unsigned int format) @@ -228,6 +234,7 @@ droid_alloc_local_buffer(struct dri2_egl_surface *dri2_surf, return dri2_surf->local_buffers[att]; } +#endif static void droid_free_local_buffers(struct dri2_egl_surface *dri2_surf) @@ -509,53 +516,43 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw) } static _EGLImage * -dri2_create_image_android_native_buffer(_EGLDisplay *disp, -_EGLContext *ctx, -struct ANativeWindowBuffer *buf) +droid_create_image_from_prime_fd(_EGLDisplay *disp, _EGLContext *ctx, + struct ANativeWindowBuffer *buf) { - struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp); - struct dri2_egl_image *dri2_img; - int name, fd; - int format; - - if (ctx != NULL) { - /* From the EGL_ANDROID_image_native_buffer spec: - * - * * If is EGL_NATIVE_BUFFER_ANDROID and is not - * EGL_NO_CONTEXT, the error EGL_BAD_CONTEXT is generated. - */ - _eglError(EGL_BAD_CONTEXT, "eglCreateEGLImageKHR: for " -"EGL_NATIVE_BUFFER_ANDROID, the context must be " -"EGL_NO_CONTEXT"); - return NULL; - } - - if (!buf || buf->common.magic != ANDROID_NATIVE_BUFFER_MAGIC || - buf->common.version != sizeof(*buf)) { - _eglError(EGL_BAD_PARAMETER, "eglCreateEGLImageKHR"); - return NULL; - } + int fd; fd = get_native_buffer_fd(buf); - if (fd >= 0) { - const int fourcc = get_fourcc(get_format(buf->format)); - const int pitch = buf->stride * get_format_bpp(buf->format); + if (fd < 0) + return NULL; + + const int fourcc = get_fourcc(get_format(buf->format)); + const int pitch = buf->stride * get_format_bpp(buf->format); + + const EGLint attr_list[14] = { + EGL_WIDTH, buf->width, + EGL_HEIGHT, buf->height, + EGL_LINUX_DRM_FOURCC_EXT, fourcc, + EGL_DMA_BUF_PLANE0_FD_EXT, fd, + EGL_DMA_BUF_PLANE0_PITCH_EXT, pitch, + EGL_DMA_BUF_PLANE0_OFFSET_EXT, 0, + EGL_NONE, 0 + }; - const EGLint attr_list[14] = { - EGL_WIDTH, buf->width, - EGL_HEIGHT, buf->height, - EGL_LINUX_DRM_FOURCC_EXT, fourcc, - EGL_DMA_BUF_PLANE0_FD_EXT, fd, - EGL_DMA_BUF_PLANE0_PITCH_EXT, pitch, - EGL_DMA_BUF_PLANE0_OFFSET_EXT, 0, - EGL_NONE, 0 - }; + if (fourcc == -1 || pitch == 0) + return NULL; - if (fourcc == -1 || pitch == 0) - return NULL; + return dri2_create_image_dma_buf(disp, ctx, NULL, attr_list); +} - return dri2_create_image_dma_buf(disp, ctx, NULL, attr_list); - } +static _EGLImage * +droid_create_image_from_na