[PATCH 06/13] tests/exynos: add fimg2d event test

[Emil Velikov]

On 30 October 2015 at 11:28, Tobias Jakobi
<tjakobi at math.uni-bielefeld.de> wrote:
> Hello Emil,
>
>
> Emil Velikov wrote:
>> On 30 October 2015 at 11:16, Tobias Jakobi
>> <tjakobi at math.uni-bielefeld.de> wrote:
>>> Hello Hyungwon,
>>>
>>> first of all thanks for reviewing the series!
>>>
>>>
>>>
>>> Hyungwon Hwang wrote:
>>>> On Tue, 22 Sep 2015 17:54:55 +0200
>>>> Tobias Jakobi <tjakobi at math.uni-bielefeld.de> wrote:
>>>>
>>
>>>>> +    evhandler->evctx.base.version = DRM_EVENT_CONTEXT_VERSION;
>>>>> +    evhandler->evctx.version = EXYNOS_EVENT_CONTEXT_VERSION;
>>>>
>>>> The versions must be set not using XXX_EVENT_CONTEXT_VERSION. After the
>>>> versions are bumped, the event will contains wrong version info.
>>> Hmm, I don't see how this is true. Both DRM_EVENT_CONTEXT_VERSION and
>>> EXYNOS_EVENT_CONTEXT_VERSION come from the public libdrm header. If the
>>> version in the public header is bumped, then it's also bumped here. So I
>>> don't see the issue.
>>>
>> The issue is that the public header defines the interface available,
>> while you set the version that you implement. Currently those match,
>> but if/when we expand the API (bumping the version in the header) and
>> rebuild your program we will crash.
> Hmm, I'm still not sure I understand this. Do you mean rebuilding the
> tests out-of-tree and then running them against a newer/older libdrm
> version?
>
> Because from my understanding the tests are always build together with
> libdrm anyway. Or am I misunderstanding here something?
>
We're not talking about building out of tree or anything like that
here. The following example should illustrate what I have in mind.
Greatly appreciated if you can explain it in your own words.


Currently

* xf86drm.h
#define DRM_EVENT_CONTEXT_VERSION 2
struct drmEventContext {
   int version;
   ...
   void (*page_flip_handler)(...);
}

* user
struct foo {
   .version = ...VERSION // 2
   ...
}


API bump

* xf86drm.h
#define DRM_EVENT_CONTEXT_VERSION 3
struct drmEventContext {
   int version;
   ...
   void (*page_flip_handler)(...);
   void (*page_flip_handler2)(...);
}

* user (hasn't been updated, only rebuilt)
struct foo {
   .version = ...VERSION // 3
   ...
   .page_flip_handler2 = 0 // ... or garbage.
}


* xf86drmMode.c
int drmHandleEvent(...)
{
...
if (foo.version >= 3)
   foo.page_flip_handler2(); // boom !
else
   foo.page_flip_handler();
...
}


Also worth mentioning is that the issue is rather wide spread rather
than limited to libdrm :'(

>
>> Before you ask - yes current libdrm users are not doing the right
>> thing and should be updated one of these days.
> Maybe a dumb question, but what would be the right thing to do in may case.
>
> Define my own MY_XZY_EVENT_CONTEXT_VERSION and use these?
>
No need for extra defines imho. Just set the versions to 2 and 1 for
evctx.base.version and evctx.version respectively.


Glad to see some of the Samsung/Exynos people looking this way :-)

Regards,
Emil