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;
Date:   Tue, 25 Oct 2022 15:26:54 +0200
From:   =?utf-8?Q?Micha=C5=82?= Winiarski <michal.winiarski@intel.com>
To:     Oded Gabbay <ogabbay@kernel.org>
CC:     Jeffrey Hugo <quic_jhugo@quicinc.com>,
        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>,
        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>
Subject: Re: [RFC PATCH 3/3] drm: add dedicated minor for accelerator devices
Message-ID: <20221025132654.fcbw7xvl7pu3yp6c@nostramo>
References: <20221022214622.18042-1-ogabbay@kernel.org>
 <20221022214622.18042-4-ogabbay@kernel.org>
 <bf8df463-3413-3027-0f4b-3977e6860404@quicinc.com>
 <CAFCwf12R1CWz8GdJ0sNsVL+_5b+G5Wqf5qwZ8ixtoXLOr2-obg@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAFCwf12R1CWz8GdJ0sNsVL+_5b+G5Wqf5qwZ8ixtoXLOr2-obg@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UlA3MTV3UUN0VEQxVk4wL2x0bUxFcm9rUERCc3FZVldRWHIwb3k4STdFNWJ4?=
 =?utf-8?B?RWhvVDF2OXladWN3R0dOeEQ0QmhMVFhndTNuZkpKOXNwdE9paXdWRlJHWkxN?=
 =?utf-8?B?QWpRUkd5R1ZidkFUUUlSNjdlSzNPY1J6OExZTkxKWGV0NDVwZXpjS0RZZjcv?=
 =?utf-8?B?Sk1ZTmZSMW9tenppaWtRbGVXSlZ3QUlJV21Kcm1MK0xuMFlpdElPWksyaTZC?=
 =?utf-8?B?TzNGdFdsTkJKdjE4TWh0NllxYWlXYVlTR1RtTlQraFNCSEwxWGRjaTBYdVRa?=
 =?utf-8?B?SmdCZnNDeDRSQUU0bWNzc1JHaDRvTGJhTVJvc2FPcEhQb1lsbzJVb2VlTTdo?=
 =?utf-8?B?b2c4Ri96bnNpZmxSSS9PVVAzTHNPTXJkWVRkakFYOVZjMlRZSHpaV01nYmxj?=
 =?utf-8?B?SVBCMndDM3Q2d2todUU5WDdTZXI1VWFaMTM1ZUZtUzBjN0JMWWRPZGdkdGxR?=
 =?utf-8?B?dWczclc1by9zWUJjRHZsK1VzOWY5U255bXdlUWdhUThSMlRZWnlqTkNQQ0Vs?=
 =?utf-8?B?UHlRTG43dW04OEY4WE5qOGRzVm9KbjJObXIzY2M2VVB3NUxsV3FhRVRJTk90?=
 =?utf-8?B?M1lqb0Y0K2VGYnBOS1RVS0lMbnJDOWJBRGJWREFHMmNqVnk2L2ZLeGs5dXRp?=
 =?utf-8?B?ZUx6d2JMY284MTVZYUMxK3Zqa2JQekJnV1RVYk50ZFVLYTV4YWF5ZUhCN3FZ?=
 =?utf-8?B?TnI5bCtwSFo4b2tNMGJweCtPb2ZWT0JIMzlsUjFuM3ZEbkl3R3dzZHdrWWlC?=
 =?utf-8?B?SzhMZ2h3R3c1TlZ6MVJ1aVhEUG5YbU9NbWlpSk5oWjRCTWEvM25wZ21VQ2VJ?=
 =?utf-8?B?RGcrQ0ZuWmg0RlJLZ2JjSHI5Mm02MnJONUtQZFg5UEJvaFhnM291VTZBU3pF?=
 =?utf-8?B?by9sblBCcU9PcXJkNUhwZmlka0ZYNWZPbk1ncDFDRm50SXZwMEkvNjJHbElZ?=
 =?utf-8?B?VFA5Tm9YbHlncHFwWEFnRkJySjBDT1lpNHB4dkpqUnJCcjZDMTB1NWNoZUVs?=
 =?utf-8?B?SUc3clltYzlrRXNTOWYwNVhkZEJsNjZlRVh0NmhNRlNlT0l2OEQwdVlOczdw?=
 =?utf-8?B?cTlINTdML0l6eWpzZzMxR0x4NXkxVjR3bDdZOXg5MGJIWU1xMmlDbFlRMnRS?=
 =?utf-8?B?UFZvUGlWWnVhVjEvZWJETDdPYUhDeCtBd25sd3YwMXJQU3liRUxMY3Y5RXB5?=
 =?utf-8?B?d0hrMUhqOUlJTlZ1WkpuOSsycEdGeGFuQUczVXJZNVJNaHNBdzMvQXBiYkJZ?=
 =?utf-8?B?WFlYR3JNTkNHTG85VFliWDJMRHJqY3NSMTRGaVZQT2ZGZXZHREFQMUc1SmRF?=
 =?utf-8?B?SUt1dTdHRkl0TUVuVGJrcVdHcnBPQk9vNUVoRlBhdUhUNVZSZ21makgxejI1?=
 =?utf-8?B?SDBURkVIMXBhT080QkNNaG1uM0VOa2J3czR4L2QzZzM1NlZlaU1IMWtqaXVQ?=
 =?utf-8?B?cERMS1VNNU5WNFpwMkxBUEd5NFNpMlUvOUkvOXRtZ0dUV3F2OFE0ZCtnUGtn?=
 =?utf-8?B?Z1lwNmR0d0R4WFVSTzVGWnp5OEZ0dHNQcDhMVFJIRVFaQ0JlQjN3eE53REJM?=
 =?utf-8?B?SE1uQ3ROVnJXelJtTndZR09qdVVYNVQ0ZjlrUWl3NVZVck5wM3I2bXB4NTBW?=
 =?utf-8?B?YWFmOXNNSVpsTGJTa0xqZVRhTUNyQkExeU9pVzNWNE10YVU4Mjk5VWdDQk15?=
 =?utf-8?B?NHlYcHROc2VMNlMzMWl5Wk9GWitIeFQyUGxSdk1lbnhPZlY5QllRWldjM0ZX?=
 =?utf-8?B?TUh6RFBsU3NBV0hKMGJsT1JMWDBpdVlEV3BMN0N0enlUY3FlUE83bVppNk1t?=
 =?utf-8?B?VzJ2TDQwZWxmQXdRamFKZHZTOVhnZjdlaFI4Y0JhMElqeHZ5VnkrbXNJbnFq?=
 =?utf-8?B?cmlxTFlMS1h2UEY5U1lpekdvWmpZYy9aeTRJWjBQRFh1U1F6YlI5MGQ2STNy?=
 =?utf-8?B?eElxNEdoQTN5cW8xRTNzL2tkUmRqWUpQeEc4QlAwOVgvWHV5ZjliR2YreW85?=
 =?utf-8?B?TzNYT2psWHNHdWptQmVwUmlKOE5rREphTFM5TkNFdmdlTEFmbExkMzVkTDlK?=
 =?utf-8?B?V25GVHVRUmtCMCtxb3VjVHNrZVhIZmFzTS9pSUxZK1FaZ01VTGx5dHBTZjdx?=
 =?utf-8?B?Mmdza2g5RnJRSzhteXFESC9PRHk1UFk2Nm5ML2kvVVJhaHQ2SDNYTDdVdENn?=
 =?utf-8?B?cXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9e2ac2c3-510b-46d3-27bc-08dab68c96ff
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB5373.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Oct 2022 13:26:58.0088
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: cZ3O/DLScKOQcN2L0TGjEXbMkT/clImYGi/gDt/atFXSg1ah/1yYoWp8q3/HguuPPnY66LTX+LAWyWqums55zAHdGDO/DzFlfJMd08qfM1Y=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB5004
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Oct 24, 2022 at 08:43:58PM +0300, Oded Gabbay wrote:
> On Mon, Oct 24, 2022 at 6:21 PM Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
> >
> > 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.
> Agreed. The only reason I haven't done it at this stage was because I
> wanted to get an initial reaction to the code itself, see if the
> direction is accepted.
> I didn't want to write documentation and then completely re-write it.
> So I will do it for the next patch-set, once I collect everyone's
> feedback and I see there is a majority agreement.
> >
> > >
> > > 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.
> ok, I wasn't aware. I don't have any problem replacing the idr to xarray.

The conversion is sitting on the mailinglist for a while now
(unfortunately, without much interest).
Perhaps you could help with reviewing it?
https://lore.kernel.org/dri-devel/20220911211443.581481-2-michal.winiarski@intel.com/

-Michał

> 
> Thanks,
> Oded
>