[Mesa-dev] [PATCH v2 1/3] Add support for swrast to the DRM EGL platform
Emil Velikov
emil.l.velikov at gmail.com
Sun May 18 10:37:32 PDT 2014
On 18/05/14 13:50, Giovanni Campagna wrote:
> 2014-05-15 2:47 GMT+02:00 Emil Velikov <emil.l.velikov at gmail.com>:
>> On 05/05/14 17:07, Giovanni Campagna wrote:
>>> From: Giovanni Campagna <gcampagna at src.gnome.org>
>>>
>>> Turn GBM into a swrast loader (providing putimage/getimage backed
>>> by a dumb KMS buffer). This allows to run KMS+DRM GL applications
>>> (such as weston or mutter-wayland) unmodified on cards that don't
>>> have any client side HW acceleration component but that can do
>>> modeset (examples include simpledrm and qxl)
>>> ---
>>> src/egl/drivers/dri2/platform_drm.c | 186 ++++++++++++++++++++++++++++----
>>> src/gbm/backends/dri/gbm_dri.c | 210 +++++++++++++++++++++++++++++-------
>>> src/gbm/backends/dri/gbm_driint.h | 19 +++-
>>> src/gbm/main/gbm.h | 3 +
>>> src/loader/loader.c | 6 ++
>>> 5 files changed, 363 insertions(+), 61 deletions(-)
>>>
[snip]
>>> +static void *
>>> +gbm_dri_bo_map(struct gbm_dri_bo *bo)
>>> +{
>>> + struct drm_mode_map_dumb map_arg;
>> + struct drm_mode_destroy_dumb destroy_arg;
>>
>>> + int ret;
>>> +
>>> + if (bo->image != NULL)
>>> + return NULL;
>>> +
>>> + if (bo->map != NULL)
>>> + return bo->map;
>>> +
>>> + memset(&map_arg, 0, sizeof(map_arg));
>>> + map_arg.handle = bo->handle;
>>> +
>>> + ret = drmIoctl(bo->base.base.gbm->fd, DRM_IOCTL_MODE_MAP_DUMB, &map_arg);
>>> + if (ret)
>>> + return NULL;
>>> +
>>> + bo->map = mmap(0, bo->size, PROT_WRITE,
>>> + MAP_SHARED, bo->base.base.gbm->fd, map_arg.offset);
>>> + if (bo->map == MAP_FAILED)
>>> + return NULL;
>>
>> You might want to cleanup the ioct on failure (and drop it from the caller).
>> Might be worth adding clearing up map, to prevent nasty things happening up
>> the call chain. Something like the code below:
>>
>> + if (bo->map == MAP_FAILED) {
>> + memset(&destroy_arg, 0, sizeof destroy_arg);
>> + destroy_arg.handle = map_arg.handle;
>> + drmIoctl(bo->base.base.gbm->fd, DRM_IOCTL_MODE_DESTROY_DUMB,
>> &destroy_arg);
>> + bo->map = NULL;
>> + }
>
> Except that I don't want to destroy the buffer on failure, it might be
> a transient issue like out of address space, and the code is not
> prepared to reallocate the buffer.
> I could "clean up" the map_dumb ioctl, except that it is a getter
> ioctl really, there is nothing to cleanup.
>
Ouch, that was rather silly of me.
>>
>>> +
>>> + return bo->map;
>>> +
>>> +}
>>> +
>>> +static void
>>> +gbm_dri_bo_unmap(struct gbm_dri_bo *bo)
>>> +{
>>> + munmap(bo->map, bo->size);
>>> + bo->map = NULL;
>> Can you move the ioctl inside the function to keep things symmetric/prevent
>> leaks ?
>
> Which ioctl? You don't want to destroy on unmap, it would make the
> whole drawing useless.
>
Ditto. Ignore my rambling here.
[snip]
>>> diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_dri.c
>>> index cec12d1..53d0bf3 100644
>>> --- a/src/gbm/backends/dri/gbm_dri.c
>>> +++ b/src/gbm/backends/dri/gbm_dri.c
[snip]
>>> @@ -163,18 +241,24 @@ struct dri_extension_match {
>>> int offset;
>>> };
>>>
>>> -static struct dri_extension_match dri_core_extensions[] = {
>>> +static struct dri_extension_match dri2_core_extensions[] = {
>>> { __DRI2_FLUSH, 1, offsetof(struct gbm_dri_device, flush) },
>>> { __DRI_IMAGE, 1, offsetof(struct gbm_dri_device, image) },
>>> { NULL, 0, 0 }
>>> };
>>>
>>> -static struct dri_extension_match gbm_dri_device_extensions[] = {
>>> +static struct dri_extension_match gbm_dri2_device_extensions[] = {
>>> { __DRI_CORE, 1, offsetof(struct gbm_dri_device, core) },
>>> { __DRI_DRI2, 1, offsetof(struct gbm_dri_device, dri2) },
>>> { NULL, 0, 0 }
>>> };
>>>
>> Please keep the above two names as is.
>
> Well, to me it's more clear to rename gbm_dri_device_extensions to
> gbm_dri2_device_extensions, because they are the marker for a dri2
> driver.
> And the dri_core_extensions are only used with dri2 drivers, so they
> make sense as dri2_core_extensions.
>
IIRC the dri_core_extensions are used by dri3 drivers as well :)
>>> +static struct dri_extension_match gbm_swrast_device_extensions[] = {
>>> + { __DRI_CORE, 1, offsetof(struct gbm_dri_device, core), },
>>> + { __DRI_SWRAST, 1, offsetof(struct gbm_dri_device, swrast) },
>>> + { NULL, 0, 0 }
>>> +};
>>> +
>>> static int
>>> dri_bind_extensions(struct gbm_dri_device *dri,
>>> struct dri_extension_match *matches,
>>> @@ -268,10 +352,12 @@ dri_load_driver(struct gbm_dri_device *dri)
>>> }
>>> dri->driver_extensions = extensions;
>>>
>>> - if (dri_bind_extensions(dri, gbm_dri_device_extensions, extensions) < 0) {
>>> - dlclose(dri->driver);
>>> - fprintf(stderr, "failed to bind extensions\n");
>>> - return -1;
>>> + if (dri_bind_extensions(dri, gbm_dri2_device_extensions, extensions) < 0) {
>>> + if (dri_bind_extensions(dri, gbm_swrast_device_extensions, extensions) < 0) {
>>> + dlclose(dri->driver);
>>> + fprintf(stderr, "failed to bind extensions\n");
>>> + return -1;
>>> + }
>> Shouldn't one be checking the driver_name which extensions we need to bind ?
>
> No other loader does that.
>
egl_dri2 does something similar to what I have in mind. Considering that gbm
is just gaining swrast support you might want to do something similar
gbm_dri.c:
dri_device_create()
{
...
int force_sw = getenv("GBM_SOFTWARE?")...
if (!force_sw) {
ret = dri_screen_create....
if (!ret)
ret = dri_screen_create_swrast..
} else {
ret = dri_screen_create_swrast...
}
...
}
It will get rid of most of the code movement and will make things a bit
clearer higher up the call stack. It will also drop the hack in src/loader you
have below.
[snip]
>>> diff --git a/src/loader/loader.c b/src/loader/loader.c
>>> index 666d015..8361cf4 100644
>>> --- a/src/loader/loader.c
>>> +++ b/src/loader/loader.c
>>> @@ -67,6 +67,7 @@
>>> #include <stdarg.h>
>>> #include <stdio.h>
>>> #include <string.h>
>>> +#include <stdlib.h>
>>> #ifdef HAVE_LIBUDEV
>>> #include <assert.h>
>>> #include <dlfcn.h>
>>> @@ -325,6 +326,11 @@ loader_get_driver_for_fd(int fd, unsigned driver_types)
>>> if (!driver_types)
>>> driver_types = _LOADER_GALLIUM | _LOADER_DRI;
>>>
>>> + if (getenv("LIBGL_ALWAYS_SOFTWARE") != NULL) {
>>> + log_(_LOADER_INFO, "using software rendering (forced by environment)\n");
>>> + return "swrast";
>>> + }
>>> +
>> Not a fan of this. What exatly are you trying to achieve here ?
>
> Debugging, mostly. It allows testing this code on a real drm driver.
>
IMHO this is not the most appropriate place and/or way of doing this.
Cheers,
Emil
> Giovanni
>
More information about the mesa-dev
mailing list