Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
To:     robert.foss@collabora.com
Cc:     airlied@linux.ie, brianp@vmware.com,
        dri-devel@lists.freedesktop.org, emil.l.velikov@gmail.com,
        eric.engestrom@intel.com, gustavo@padovan.org,
        linux-kernel@vger.kernel.org, maarten.lankhorst@linux.intel.com,
        norvez@chromium.org, robh@kernel.org, seanpaul@chromium.org,
        tfiga@chromium.org, tomeu.vizoso@collabora.com
References: <20180724082213.25677-1-robert.foss@collabora.com>
Subject: Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes
From:   Martin Fuzzey <martin.fuzzey@flowbird.group>
Message-ID: <f718f3ec-3726-3f51-6756-80b357fc23c9@flowbird.group>
Date:   Wed, 1 Aug 2018 16:24:55 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20180724082213.25677-1-robert.foss@collabora.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: fr
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

I am running into the same problem using etnaviv on i.MX6 under Android 8.1

The mesa etnaviv code uses CREATE_DUMB / MAP_DUMB for the scanout buffers
and is called from the surfaceflinger process.

But, under Android 8.1 drm_hwcomposer runs in a seperate process to surfaceflinger.
Since drm_hwcomposer needs to use the KMS API it must be the DRM master,
that means that surface flinger cannot be DRM master too.

Adding a render node to the imx drm device and configuring mesa to user fixes
the problem but requires the render node to have access rights to use CREATE_DUMB
/ MAP_DUMB.


Here is my full patch:

Make imx-drm export a render node so that mesa can use it to allocate

From: Martin Fuzzey <martin.fuzzey@flowbird.group>
Date: 2018-07-26 15:37:28 +0200

dumb memory rather than requiring the master node.

The problem with using the master node is that, on android 8.1, drm_hwcomposer
is a seperate process and must be the master node (to use the KMS API),
since only a single process may be master surfaceflinger cannot be master too.

With this change surfaceflinger can use just a rendernode.

Note that we also have to modify the permissions table to allow render nodes
to use the dumb allocation functions. This is a hack and unlikely to be accepted
upstream but it's better than the huge security hole of allowing everything we were
using before.

Need to discuss an upstream acceptable way for this...
---
  drivers/gpu/drm/drm_ioctl.c        |    6 +++---
  drivers/gpu/drm/imx/imx-drm-core.c |    2 +-
  2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index a9ae6dd..31c4c86 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -639,9 +639,9 @@ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_RMFB, drm_mode_rmfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_PAGE_FLIP, drm_mode_page_flip_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DIRTYFB, drm_mode_dirtyfb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
-	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_DUMB, drm_mode_create_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
-	DRM_IOCTL_DEF(DRM_IOCTL_MODE_MAP_DUMB, drm_mode_mmap_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
-	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_DUMB, drm_mode_create_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_MAP_DUMB, drm_mode_mmap_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED|DRM_RENDER_ALLOW),
  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_GETPROPERTIES, drm_mode_obj_get_properties_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_SETPROPERTY, drm_mode_obj_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CURSOR2, drm_mode_cursor2_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index f91cb72..0c34306 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -177,7 +177,7 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
  
  static struct drm_driver imx_drm_driver = {
  	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME |
-				  DRIVER_ATOMIC,
+				  DRIVER_ATOMIC | DRIVER_RENDER,
  	.lastclose		= imx_drm_driver_lastclose,
  	.gem_free_object_unlocked = drm_gem_cma_free_object,
  	.gem_vm_ops		= &drm_gem_cma_vm_ops,

If it is not acceptable to modify the access rights for the DUMB allocation functions how should this be done?

Best regards,

Martin