Re: [Mesa-dev] [RFC 3/5] platform/android: Enable kms_swrast fallback

2018-07-06 Thread Emil Velikov 
Hi Rob,

On 6 July 2018 at 09:58, Robert Foss  wrote:

>>
>> You can trivially eliminate the goto statement here.
>>
>
> As Tomasz noted this fallback is already implemented in egldriver.c,
> so it can be removed entirely.
>
Right forgot about that one.

Currently we have the fallback in egldriver.c and another one in the
platforms themselves - wayland, x11...
Gut suggests that we want to have only one, although we can do that as
follow-up.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC 3/5] platform/android: Enable kms_swrast fallback

2018-07-06 Thread Robert Foss 

Hey Emil,

On 05/07/18 15:04, Emil Velikov wrote:

Hi Rob,

On 5 July 2018 at 11:07, Robert Foss  wrote:

Add support for the ForceSoftware option, which is togglable
on the Android platform through setting the "drm.gpu.force_software"
property to a non-zero value.

kms_swrast is also enabled as a fallback for when a driver is not
able to be loaded for for a drm node that was opened.

Signed-off-by: Robert Foss 
---
  src/egl/drivers/dri2/platform_android.c | 26 +
  1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index 92b2d2b343e..bc644c25bf9 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1193,17 +1193,21 @@ static const __DRIextension 
*droid_image_loader_extensions[] = {
  };

  EGLBoolean
-droid_load_driver(_EGLDisplay *disp)
+droid_load_driver(_EGLDisplay *disp, EGLBoolean force_software)
  {
 struct dri2_egl_display *dri2_dpy = disp->DriverData;
 const char *err;

-   dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
+   if (force_software) {
+  dri2_dpy->driver_name = strdup("kms_swrast");
+   } else {
+  dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
+   }
+

Nit: no brackets are needed here.


Ack.




 if (dri2_dpy->driver_name == NULL)
return false;

 dri2_dpy->is_render_node = drmGetNodeTypeFromFd(dri2_dpy->fd) == 
DRM_NODE_RENDER;
-
 if (!dri2_dpy->is_render_node) {
 #ifdef HAVE_DRM_GRALLOC
 /* Handle control nodes using __DRI_DRI2_LOADER extension and GEM names
@@ -1359,6 +1363,7 @@ EGLBoolean
  dri2_initialize_android(_EGLDriver *drv, _EGLDisplay *disp)
  {
 struct dri2_egl_display *dri2_dpy;
+   int force_software = disp->Options.ForceSoftware;

ForceSoftware is EGLBoolean and droid_load_driver() uses the same.
Yet "int" is used here and "= true" below.



Ack.


 const char *err;
 int ret;

@@ -1384,13 +1389,18 @@ dri2_initialize_android(_EGLDriver *drv, _EGLDisplay 
*disp)

 dri2_dpy->fd = droid_open_device(disp);
 if (dri2_dpy->fd < 0) {
-  err = "DRI2: failed to open device";
-  goto cleanup;
+  err = "DRI2: failed to open device, trying software device";

Ignoring the first patch for a second - the goto cleanup should stay.




Ack.


 }

-   if (!droid_load_driver(disp)) {
-  err = "DRI2: failed to load driver";
-  goto cleanup;
+load_driver:
+   if (!droid_load_driver(disp, force_software)) {
+  if (force_software) {
+ err = "DRI2: failed to load driver";
+ goto cleanup;
+  } else {
+ force_software = true;
+ goto load_driver;
+  }

You can trivially eliminate the goto statement here.



As Tomasz noted this fallback is already implemented in egldriver.c,
so it can be removed entirely.


Thanks
Emil


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC 3/5] platform/android: Enable kms_swrast fallback

2018-07-06 Thread Robert Foss 

Hey Tomasz,

On 05/07/18 16:26, Tomasz Figa wrote:

Hi Rob,

On Thu, Jul 5, 2018 at 7:07 PM Robert Foss  wrote:


Add support for the ForceSoftware option, which is togglable
on the Android platform through setting the "drm.gpu.force_software"
property to a non-zero value.

kms_swrast is also enabled as a fallback for when a driver is not
able to be loaded for for a drm node that was opened.

Signed-off-by: Robert Foss 
---
  src/egl/drivers/dri2/platform_android.c | 26 +
  1 file changed, 18 insertions(+), 8 deletions(-)


Thanks for the patch. Please see my comments inline.



diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index 92b2d2b343e..bc644c25bf9 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1193,17 +1193,21 @@ static const __DRIextension 
*droid_image_loader_extensions[] = {
  };

  EGLBoolean
-droid_load_driver(_EGLDisplay *disp)
+droid_load_driver(_EGLDisplay *disp, EGLBoolean force_software)
  {
 struct dri2_egl_display *dri2_dpy = disp->DriverData;
 const char *err;

-   dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
+   if (force_software) {
+  dri2_dpy->driver_name = strdup("kms_swrast");
+   } else {
+  dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
+   }
+
 if (dri2_dpy->driver_name == NULL)
return false;

 dri2_dpy->is_render_node = drmGetNodeTypeFromFd(dri2_dpy->fd) == 
DRM_NODE_RENDER;
-


Unrelated whitespace change.


Ack.




 if (!dri2_dpy->is_render_node) {
 #ifdef HAVE_DRM_GRALLOC
 /* Handle control nodes using __DRI_DRI2_LOADER extension and GEM names
@@ -1359,6 +1363,7 @@ EGLBoolean
  dri2_initialize_android(_EGLDriver *drv, _EGLDisplay *disp)
  {
 struct dri2_egl_display *dri2_dpy;
+   int force_software = disp->Options.ForceSoftware;


EGLBoolean


Ack.




 const char *err;
 int ret;

@@ -1384,13 +1389,18 @@ dri2_initialize_android(_EGLDriver *drv, _EGLDisplay 
*disp)

 dri2_dpy->fd = droid_open_device(disp);
 if (dri2_dpy->fd < 0) {
-  err = "DRI2: failed to open device";
-  goto cleanup;
+  err = "DRI2: failed to open device, trying software device";
 }

-   if (!droid_load_driver(disp)) {
-  err = "DRI2: failed to load driver";
-  goto cleanup;
+load_driver:
+   if (!droid_load_driver(disp, force_software)) {
+  if (force_software) {
+ err = "DRI2: failed to load driver";
+ goto cleanup;
+  } else {
+ force_software = true;
+ goto load_driver;
+  }


I believe we don't need this retry here, because if we fail here once,
_eglMatchDriver() would retry us with ForceSoftware = EGL_TRUE [1].

[1] https://cgit.freedesktop.org/mesa/mesa/tree/src/egl/main/egldriver.c#n80


Oh nice. I didn't realize that the fallback was implemented somewhere else 
already.



Best regards,
Tomasz


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC 3/5] platform/android: Enable kms_swrast fallback

2018-07-05 Thread Tomasz Figa 
Hi Rob,

On Thu, Jul 5, 2018 at 7:07 PM Robert Foss  wrote:
>
> Add support for the ForceSoftware option, which is togglable
> on the Android platform through setting the "drm.gpu.force_software"
> property to a non-zero value.
>
> kms_swrast is also enabled as a fallback for when a driver is not
> able to be loaded for for a drm node that was opened.
>
> Signed-off-by: Robert Foss 
> ---
>  src/egl/drivers/dri2/platform_android.c | 26 +
>  1 file changed, 18 insertions(+), 8 deletions(-)

Thanks for the patch. Please see my comments inline.

>
> diff --git a/src/egl/drivers/dri2/platform_android.c 
> b/src/egl/drivers/dri2/platform_android.c
> index 92b2d2b343e..bc644c25bf9 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -1193,17 +1193,21 @@ static const __DRIextension 
> *droid_image_loader_extensions[] = {
>  };
>
>  EGLBoolean
> -droid_load_driver(_EGLDisplay *disp)
> +droid_load_driver(_EGLDisplay *disp, EGLBoolean force_software)
>  {
> struct dri2_egl_display *dri2_dpy = disp->DriverData;
> const char *err;
>
> -   dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
> +   if (force_software) {
> +  dri2_dpy->driver_name = strdup("kms_swrast");
> +   } else {
> +  dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
> +   }
> +
> if (dri2_dpy->driver_name == NULL)
>return false;
>
> dri2_dpy->is_render_node = drmGetNodeTypeFromFd(dri2_dpy->fd) == 
> DRM_NODE_RENDER;
> -

Unrelated whitespace change.

> if (!dri2_dpy->is_render_node) {
> #ifdef HAVE_DRM_GRALLOC
> /* Handle control nodes using __DRI_DRI2_LOADER extension and GEM 
> names
> @@ -1359,6 +1363,7 @@ EGLBoolean
>  dri2_initialize_android(_EGLDriver *drv, _EGLDisplay *disp)
>  {
> struct dri2_egl_display *dri2_dpy;
> +   int force_software = disp->Options.ForceSoftware;

EGLBoolean

> const char *err;
> int ret;
>
> @@ -1384,13 +1389,18 @@ dri2_initialize_android(_EGLDriver *drv, _EGLDisplay 
> *disp)
>
> dri2_dpy->fd = droid_open_device(disp);
> if (dri2_dpy->fd < 0) {
> -  err = "DRI2: failed to open device";
> -  goto cleanup;
> +  err = "DRI2: failed to open device, trying software device";
> }
>
> -   if (!droid_load_driver(disp)) {
> -  err = "DRI2: failed to load driver";
> -  goto cleanup;
> +load_driver:
> +   if (!droid_load_driver(disp, force_software)) {
> +  if (force_software) {
> + err = "DRI2: failed to load driver";
> + goto cleanup;
> +  } else {
> + force_software = true;
> + goto load_driver;
> +  }

I believe we don't need this retry here, because if we fail here once,
_eglMatchDriver() would retry us with ForceSoftware = EGL_TRUE [1].

[1] https://cgit.freedesktop.org/mesa/mesa/tree/src/egl/main/egldriver.c#n80

Best regards,
Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC 3/5] platform/android: Enable kms_swrast fallback

2018-07-05 Thread Emil Velikov 
Hi Rob,

On 5 July 2018 at 11:07, Robert Foss  wrote:
> Add support for the ForceSoftware option, which is togglable
> on the Android platform through setting the "drm.gpu.force_software"
> property to a non-zero value.
>
> kms_swrast is also enabled as a fallback for when a driver is not
> able to be loaded for for a drm node that was opened.
>
> Signed-off-by: Robert Foss 
> ---
>  src/egl/drivers/dri2/platform_android.c | 26 +
>  1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/platform_android.c 
> b/src/egl/drivers/dri2/platform_android.c
> index 92b2d2b343e..bc644c25bf9 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -1193,17 +1193,21 @@ static const __DRIextension 
> *droid_image_loader_extensions[] = {
>  };
>
>  EGLBoolean
> -droid_load_driver(_EGLDisplay *disp)
> +droid_load_driver(_EGLDisplay *disp, EGLBoolean force_software)
>  {
> struct dri2_egl_display *dri2_dpy = disp->DriverData;
> const char *err;
>
> -   dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
> +   if (force_software) {
> +  dri2_dpy->driver_name = strdup("kms_swrast");
> +   } else {
> +  dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
> +   }
> +
Nit: no brackets are needed here.

> if (dri2_dpy->driver_name == NULL)
>return false;
>
> dri2_dpy->is_render_node = drmGetNodeTypeFromFd(dri2_dpy->fd) == 
> DRM_NODE_RENDER;
> -
> if (!dri2_dpy->is_render_node) {
> #ifdef HAVE_DRM_GRALLOC
> /* Handle control nodes using __DRI_DRI2_LOADER extension and GEM 
> names
> @@ -1359,6 +1363,7 @@ EGLBoolean
>  dri2_initialize_android(_EGLDriver *drv, _EGLDisplay *disp)
>  {
> struct dri2_egl_display *dri2_dpy;
> +   int force_software = disp->Options.ForceSoftware;
ForceSoftware is EGLBoolean and droid_load_driver() uses the same.
Yet "int" is used here and "= true" below.

> const char *err;
> int ret;
>
> @@ -1384,13 +1389,18 @@ dri2_initialize_android(_EGLDriver *drv, _EGLDisplay 
> *disp)
>
> dri2_dpy->fd = droid_open_device(disp);
> if (dri2_dpy->fd < 0) {
> -  err = "DRI2: failed to open device";
> -  goto cleanup;
> +  err = "DRI2: failed to open device, trying software device";
Ignoring the first patch for a second - the goto cleanup should stay.

> }
>
> -   if (!droid_load_driver(disp)) {
> -  err = "DRI2: failed to load driver";
> -  goto cleanup;
> +load_driver:
> +   if (!droid_load_driver(disp, force_software)) {
> +  if (force_software) {
> + err = "DRI2: failed to load driver";
> + goto cleanup;
> +  } else {
> + force_software = true;
> + goto load_driver;
> +  }
You can trivially eliminate the goto statement here.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [RFC 3/5] platform/android: Enable kms_swrast fallback

2018-07-05 Thread Robert Foss 
Add support for the ForceSoftware option, which is togglable
on the Android platform through setting the "drm.gpu.force_software"
property to a non-zero value.

kms_swrast is also enabled as a fallback for when a driver is not
able to be loaded for for a drm node that was opened.

Signed-off-by: Robert Foss 
---
 src/egl/drivers/dri2/platform_android.c | 26 +
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index 92b2d2b343e..bc644c25bf9 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1193,17 +1193,21 @@ static const __DRIextension 
*droid_image_loader_extensions[] = {
 };
 
 EGLBoolean
-droid_load_driver(_EGLDisplay *disp)
+droid_load_driver(_EGLDisplay *disp, EGLBoolean force_software)
 {
struct dri2_egl_display *dri2_dpy = disp->DriverData;
const char *err;
 
-   dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
+   if (force_software) {
+  dri2_dpy->driver_name = strdup("kms_swrast");
+   } else {
+  dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
+   }
+
if (dri2_dpy->driver_name == NULL)
   return false;
 
dri2_dpy->is_render_node = drmGetNodeTypeFromFd(dri2_dpy->fd) == 
DRM_NODE_RENDER;
-
if (!dri2_dpy->is_render_node) {
#ifdef HAVE_DRM_GRALLOC
/* Handle control nodes using __DRI_DRI2_LOADER extension and GEM names
@@ -1359,6 +1363,7 @@ EGLBoolean
 dri2_initialize_android(_EGLDriver *drv, _EGLDisplay *disp)
 {
struct dri2_egl_display *dri2_dpy;
+   int force_software = disp->Options.ForceSoftware;
const char *err;
int ret;
 
@@ -1384,13 +1389,18 @@ dri2_initialize_android(_EGLDriver *drv, _EGLDisplay 
*disp)
 
dri2_dpy->fd = droid_open_device(disp);
if (dri2_dpy->fd < 0) {
-  err = "DRI2: failed to open device";
-  goto cleanup;
+  err = "DRI2: failed to open device, trying software device";
}
 
-   if (!droid_load_driver(disp)) {
-  err = "DRI2: failed to load driver";
-  goto cleanup;
+load_driver:
+   if (!droid_load_driver(disp, force_software)) {
+  if (force_software) {
+ err = "DRI2: failed to load driver";
+ goto cleanup;
+  } else {
+ force_software = true;
+ goto load_driver;
+  }
}
 
if (!dri2_create_screen(disp)) {
-- 
2.17.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev