Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional

[Rob Clark]

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(_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 

Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional

[Stéphane Marchesin]

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(_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 

Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional

[Rob Clark]

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(_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 

Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional

[Emil Velikov]

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(_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 

Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional

[Rob Clark]

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(_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 

Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional

[Emil Velikov]

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(_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 

Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional

[Tomasz Figa]

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(_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 

Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional

[Emil Velikov]

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(_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 

Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional

[Tomasz Figa]

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(_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 

Re: [Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional

[Emil Velikov]

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(_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

[Tomasz Figa]

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(_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

[Emil Velikov]

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(_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

[Tomasz Figa]

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

[Rob Herring]

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