Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bf8df463-3413-3027-0f4b-3977e6860404@quicinc.com>
Date:   Mon, 24 Oct 2022 09:21:20 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.0
Subject: Re: [RFC PATCH 3/3] drm: add dedicated minor for accelerator devices
Content-Language: en-US
To:     Oded Gabbay <ogabbay@kernel.org>, David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>, Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        <linux-kernel@vger.kernel.org>, <dri-devel@lists.freedesktop.org>,
        Jason Gunthorpe <jgg@nvidia.com>,
        John Hubbard <jhubbard@nvidia.com>,
        Alex Deucher <alexander.deucher@amd.com>
CC:     Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>,
        Jiho Chu <jiho.chu@samsung.com>,
        Daniel Stone <daniel@fooishbar.org>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        Christoph Hellwig <hch@infradead.org>,
        Kevin Hilman <khilman@baylibre.com>,
        Jagan Teki <jagan@amarulasolutions.com>,
        Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>,
        Maciej Kwapulinski <maciej.kwapulinski@linux.intel.com>
References: <20221022214622.18042-1-ogabbay@kernel.org>
 <20221022214622.18042-4-ogabbay@kernel.org>
From:   Jeffrey Hugo <quic_jhugo@quicinc.com>
In-Reply-To: <20221022214622.18042-4-ogabbay@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/22/2022 3:46 PM, Oded Gabbay wrote:
> The accelerator devices are exposed to user-space using a dedicated
> major. In addition, they are represented in /dev with new, dedicated
> device char names: /dev/accel/accel*. This is done to make sure any
> user-space software that tries to open a graphic card won't open
> the accelerator device by mistake.
> 
> The above implies that the minor numbering should be separated from
> the rest of the drm devices. However, to avoid code duplication, we
> want the drm_minor structure to be able to represent the accelerator
> device.
> 
> To achieve this, we add a new drm_minor* to drm_device that represents
> the accelerator device. This pointer is initialized for drivers that
> declare they handle compute accelerator, using a new driver feature
> flag called DRIVER_COMPUTE_ACCEL. It is important to note that this
> driver feature is mutually exclusive with DRIVER_RENDER. Devices that
> want to expose both graphics and compute device char files should be
> handled by two drivers that are connected using the auxiliary bus
> framework.
> 
> In addition, we define a different idr to handle the accelerators
> minors. This is done to make the minor's index be identical to the
> device index in /dev/. In most places, this is hidden inside the drm
> core functions except when calling drm_minor_acquire(), where I had to
> add an extra parameter to specify the idr to use (because the
> accelerators minors index and the drm primary minor index both begin
> at 0).
> 
> Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
> ---
>   drivers/gpu/drm/drm_drv.c      | 171 +++++++++++++++++++++++++--------
>   drivers/gpu/drm/drm_file.c     |  69 +++++++++----
>   drivers/gpu/drm/drm_internal.h |   2 +-
>   drivers/gpu/drm/drm_sysfs.c    |  29 ++++--
>   include/drm/drm_device.h       |   3 +
>   include/drm/drm_drv.h          |   8 ++
>   include/drm/drm_file.h         |  21 +++-
>   7 files changed, 235 insertions(+), 68 deletions(-)

Can we please add something to Documentation?  I know this leverages DRM 
a lot, but I believe that a new subsystem should not be introduced 
without documentation.  A lot of the info in the commit message is very 
good, but should not be buried in the git log.

Besides, imagine this has been in mainline for N years, and someone 
completely new to the kernel wants to write an accel driver.  They 
should be able to get started with something from Documentation that 
at-least gives that person some insight into what to grep the code for.

> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index b58ffb1433d6..c13701a8d4be 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -56,6 +56,9 @@ MODULE_LICENSE("GPL and additional rights");
>   static DEFINE_SPINLOCK(drm_minor_lock);
>   static struct idr drm_minors_idr;
>   
> +static DEFINE_SPINLOCK(accel_minor_lock);
> +static struct idr accel_minors_idr;

IDR is deprecated.  XArray is the preferred mechanism.
Yes, there already is IDR here, but I believe we should not be adding 
new uses.  Maybe at some point, the current IDR will be converted.  Also 
with XArray, I think you don't need the spinlock since XArray has 
internal locking already.