[Mesa-dev] [PATCH] egl/dri2: implement platform_null.

[Emil Velikov]

On 22/01/15 22:23, Haixia Shi wrote:
> Hi Emil
> 
> On Thu, Jan 22, 2015 at 1:36 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> Hi Haixia Shi,
>>
>> On 22/01/15 17:35, Haixia Shi wrote:
>>> Try the render node first and use it if available. Otherwise fall back to
>>> normal nodes.
>>>
>> What is the use-case for such a platform - I assume it's worth
>> mentioning in the commit message ?
>>
>> No other platform picks the device at random as seen below. Why did you
>> choose such an approach ? It seems like one can easily shoot themselves
>> by using it.
> 
> CC Stephane. The goal here is just to pick the first available node
> for off-screen rendering only.
> 
Hmm I'm guessing that using the drm/gbm platform is out of the question
? Iirc there has been a bit of love on the gbm topic, and afaiu this
solution is to be used with minigbm ?

What I'm thinking here is:
If you're testing a device with provides two or more nodes (be that the
classic card or the render ones), one cannot guarantee that the kernel
module for hw#1 will be loaded first. Thus even if one presumes that
they are working on (testing) hw#1 that may or may not be the case.

Not 100% sure on the module order part, so I could be wrong.

>>
>> ...
>>> diff --git a/src/egl/drivers/dri2/platform_null.c b/src/egl/drivers/dri2/platform_null.c
>>> new file mode 100644
>>> index 0000000..4f0b18f
>>> --- /dev/null
>>> +++ b/src/egl/drivers/dri2/platform_null.c
>> ...
>>> +static const char* node_path_fmt_card = "/dev/dri/card%d";
>> You can reuse the DRM_DIR_NAME + DRM_DEV_NAME macros (from xf86drm.h)
>> for this.
>>
>>> +static const char* node_path_fmt_render = "/dev/dri/renderD%d";
>> There is no macro for the renderD%d, although you can still use
>> DRM_DIR_NAME for the path.
> 
> Will update this part shortly.
> 
I would personally wait for more feedback, rather than going through xx
revisions. But that's just me being lazy :P

>>
>>> +
>>> +EGLBoolean
>>> +dri2_initialize_null(_EGLDriver *drv, _EGLDisplay *disp)
>>> +{
>>> +   struct dri2_egl_display *dri2_dpy;
>>> +   const char* err;
>>> +   int i, render_node;
>>> +   int driver_loaded = 0;
>>> +
>>> +   loader_set_logger(_eglLog);
>>> +
>>> +   dri2_dpy = calloc(1, sizeof *dri2_dpy);
>>> +   if (!dri2_dpy)
>>> +      return _eglError(EGL_BAD_ALLOC, "eglInitialize");
>>> +
>>> +   disp->DriverData = (void *) dri2_dpy;
>>> +
>>> +   for (render_node = 1; render_node >= 0; --render_node) {
>>> +      const char* node_path_fmt =
>>> +            render_node ? node_path_fmt_render : node_path_fmt_card;
>>> +      const int base = render_node ? 128 : 0;
>>> +      for (i = 0; i < 16; ++i) {
>> What was the reason behind choosing 16 here ?
> 
> It's an arbitrary number.
My bad it's DRM_MAX_MINOR, as defined in xf86drm.h. Please use the
symbolic name.

> How about choosing 64 here as it is the
> limit chosen by drm_stub.c: drm_minor_get_id()?
> 
We nuked the file with kernel 3.17 :P

>>
>>> +         char *card_path;
>>> +         if (asprintf(&card_path, node_path_fmt, base + i) < 0)
>>> +            continue;
>>> +
>>> +         dri2_dpy->fd = open(card_path, O_RDWR);
>> If you open a normal node (card%d) I believe that you'll need an
>> authenticate hook in dri2_egl_display_vtbl. Does things work without it
>> on your system/platform ?
> 
> You're correct; normal node would require the legacy auth hook, and it
> would only work without auth if the process is run as root, which is
> why we're trying render nodes first.
> 
So you're saying that people without render nodes should run egl(mesa)
as root ? That does not sound like a wise suggestion imho.

Basically what I'm trying to say is - if you have a fall-back to normal
nodes, some form of auth ought to be in place.

Not an expert on the topic
-Emil