From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1AC3720B803 for ; Thu, 23 Jan 2025 08:11:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737619864; cv=none; b=dB+78rWEldTKmI4aXDdwX/Qy4MRWFojvVliwatCR5zMToEWJAue4JTSmceplOCt7NEM7l2laaUIUtaM//4WLKFFKVBom0hV9MSBB2uVqKq/kByfGUhM4ElJPDOFSJsrcvs0sMxUSQS8PkOXPWBuPZempx212DfqTXfMOmPjdHUw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737619864; c=relaxed/simple; bh=6zyWtjSXOFF/dI58ZsFxdcivwfL8mhY//HqPx9F3Pho=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=iKVxd68waV8aMXmGAR0bxiDItd13rj0RJIATpdDZt+LKOkuNivXO2fksJwNUdEeHLou4EfvWOOz5Y0tISFfzWKm93e+mtFDbhQmN6S2cNq4Aic0QNNwe1K5h3E2M2/+PWB4HEQi8k2xs031vnIhtf1It0FpNqClOa9Sh0fzdliI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=T5eLXgAx; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="T5eLXgAx" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1737619861; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=LDEdIsdcqGWdcyBaLw2Kh9VKml8ZAgVTT2SKLvAoRWM=; b=T5eLXgAxiTyeL0v1qpEvxRLWrn/GKAzKJV8Z6t7WQ/UrDdTpctAOByXwp1T3YzDnlP5bhs LIQcXHxcSPq9gbaDlV/ibYjYkSutcEaBvyJEe5aRVgcBZNH3ztts+7Q/+W/XaYk7k0x/hH HrsMOZ+2RrHXJnVM9SwCHPHIj9XyDrs= Received: from mail-pl1-f200.google.com (mail-pl1-f200.google.com [209.85.214.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-617-vklys6A4Mlu_-oyrqehiBQ-1; Thu, 23 Jan 2025 03:10:59 -0500 X-MC-Unique: vklys6A4Mlu_-oyrqehiBQ-1 X-Mimecast-MFC-AGG-ID: vklys6A4Mlu_-oyrqehiBQ Received: by mail-pl1-f200.google.com with SMTP id d9443c01a7336-21661949f23so19101275ad.3 for ; Thu, 23 Jan 2025 00:10:59 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737619858; x=1738224658; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=LDEdIsdcqGWdcyBaLw2Kh9VKml8ZAgVTT2SKLvAoRWM=; b=VYY/Vkx6YkzSnA51dzXboxj3qvZqrPQuIVMh4srx1TjNQVPZOf5f/Z72G0mz5mhyhd b5tkTIOg8tFfx9fZvAbkEhAza7nRTbZ1jW73U2veszuGqDKJqn4vNQSuHFdTJVvGb1gS ttreP1HY6b0ZQ+fTMlkMBVUunsFAy372tCrR4xqmTNoXJ+7yx2SotbHt+34QBlm/QGZl OPEOPfEMXGZj5EXj2A+Y1FxIsZ+X2yZ5T/KdZVkp3WyAjKfOXWVesckKSaXUGhDB+ACd 8HPIANuCTJ9wY/fqPNnhobYB35Wl2AstvnzicgxF920wZ/krjaxmR+q6IlMjEA4NF4YG Ey5A== X-Forwarded-Encrypted: i=1; AJvYcCU48JCzFvbAMVWYiyzFW+JI1BK9g/AfxfHdwXV+vYs78zmiP5Y3clHBBF12CeUJzMIvBAtYkyiMATa1HmU=@vger.kernel.org X-Gm-Message-State: AOJu0YwIWahXnf66I8MTD9OnnhyDRP0PYWYHRw+4sAGda+4Ut5uhFJvT B0+FTf7lB+Z02XMsaxp8QSiE/F7hRBYDfrCaindl37p4OFprHlrArw3w1/CauyHL1xPYo3+XZyD JMFp3aVZJC5UZJmAp2iadBcPgU1H25CRhX5ABCJcW7+/kPq0RqK5y/l1STkV+Lg== X-Gm-Gg: ASbGncsPJgg7xR5KPJwbkW17M2OJRDCs0yK5Y0bCKHW5jvCIHDSdydzMzWw5iSVfn8e B+VxHEbViltu0yzGHtummuhMIWaVdgqoLMPu+rildu1Gqp3JQCibZ3q05dNmdNdGWUPfuQ5Fly0 YO79Pyfn9WiWSxwqTcaWJ+Lzm8rmLQXw9UAEPrKv1EfcuSy9oBDfkEYojs4K1JAdasgvhHDRypG Qifu6ru0uBynjO/aN9YgNY1vjr3KU2g1LI1mvJt7nmqw7w2WiqPRdn2w1SPDiIMypu698U5ppf/ R3VioaLqeWeDBvu8xnLm X-Received: by 2002:a05:6a21:999e:b0:1db:e0d7:675c with SMTP id adf61e73a8af0-1eb2148cc78mr41049400637.13.1737619858094; Thu, 23 Jan 2025 00:10:58 -0800 (PST) X-Google-Smtp-Source: AGHT+IH+36dB3XE2+vMLbg7FcqxiNuXmVO5+XixmEOWE9lkbQuRZwM0dXXk81m7Fw5CzxcBMx0XwwA== X-Received: by 2002:a05:6a21:999e:b0:1db:e0d7:675c with SMTP id adf61e73a8af0-1eb2148cc78mr41049361637.13.1737619857677; Thu, 23 Jan 2025 00:10:57 -0800 (PST) Received: from [10.200.68.91] (nat-pool-muc-u.redhat.com. [149.14.88.27]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-72dab9c8de3sm12422995b3a.89.2025.01.23.00.10.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Jan 2025 00:10:57 -0800 (PST) Message-ID: <9713798aa175aef2041e6d688ac4814006f789bc.camel@redhat.com> Subject: Re: [PATCH] drm/sched: Use struct for drm_sched_init() params From: Philipp Stanner To: =?ISO-8859-1?Q?Ma=EDra?= Canal , Philipp Stanner , Alex Deucher , Christian =?ISO-8859-1?Q?K=F6nig?= , Xinhui Pan , David Airlie , Simona Vetter , Lucas Stach , Russell King , Christian Gmeiner , Frank Binns , Matt Coster , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , Qiang Yu , Rob Clark , Sean Paul , Konrad Dybcio , Abhinav Kumar , Dmitry Baryshkov , Marijn Suijten , Karol Herbst , Lyude Paul , Danilo Krummrich , Boris Brezillon , Rob Herring , Steven Price , Liviu Dudau , Luben Tuikov , Matthew Brost , Melissa Wen , Lucas De Marchi , Thomas =?ISO-8859-1?Q?Hellstr=F6m?= , Rodrigo Vivi , Sunil Khatri , Lijo Lazar , Mario Limonciello , Ma Jun , Yunxiang Li Cc: amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, etnaviv@lists.freedesktop.org, lima@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org, nouveau@lists.freedesktop.org, intel-xe@lists.freedesktop.org Date: Thu, 23 Jan 2025 09:10:24 +0100 In-Reply-To: <24f1c52f-1768-47de-88e3-d4104969d0a9@igalia.com> References: <20250122140818.45172-3-phasta@kernel.org> <24f1c52f-1768-47de-88e3-d4104969d0a9@igalia.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.52.4 (3.52.4-2.fc40) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 On Wed, 2025-01-22 at 19:07 -0300, Ma=C3=ADra Canal wrote: > Hi Philipp, >=20 > On 22/01/25 11:08, Philipp Stanner wrote: > > drm_sched_init() has a great many parameters and upcoming new > > functionality for the scheduler might add even more. Generally, the > > great number of parameters reduces readability and has already > > caused > > one missnaming in: > >=20 > > commit 6f1cacf4eba7 ("drm/nouveau: Improve variable name in > > nouveau_sched_init()"). > >=20 > > Introduce a new struct for the scheduler init parameters and port > > all > > users. > >=20 > > Signed-off-by: Philipp Stanner > > --- > > Howdy, > >=20 > > I have a patch-series in the pipe that will add a `flags` argument > > to > > drm_sched_init(). I thought it would be wise to first rework the > > API as > > detailed in this patch. It's really a lot of parameters by now, and > > I > > would expect that it might get more and more over the years for > > special > > use cases etc. > >=20 > > Regards, > > P. > > --- > > =C2=A0 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |=C2=A0 21 +++- > > =C2=A0 drivers/gpu/drm/etnaviv/etnaviv_sched.c=C2=A0=C2=A0=C2=A0 |=C2= =A0 20 ++- > > =C2=A0 drivers/gpu/drm/imagination/pvr_queue.c=C2=A0=C2=A0=C2=A0 |=C2= =A0 21 +++- > > =C2=A0 drivers/gpu/drm/lima/lima_sched.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 21 +++- > > =C2=A0 drivers/gpu/drm/msm/msm_ringbuffer.c=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 |=C2=A0 22 ++-- > > =C2=A0 drivers/gpu/drm/nouveau/nouveau_sched.c=C2=A0=C2=A0=C2=A0 |=C2= =A0 20 ++- > > =C2=A0 drivers/gpu/drm/panfrost/panfrost_job.c=C2=A0=C2=A0=C2=A0 |=C2= =A0 22 ++-- > > =C2=A0 drivers/gpu/drm/panthor/panthor_mmu.c=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 |=C2=A0 18 ++- > > =C2=A0 drivers/gpu/drm/panthor/panthor_sched.c=C2=A0=C2=A0=C2=A0 |=C2= =A0 23 ++-- > > =C2=A0 drivers/gpu/drm/scheduler/sched_main.c=C2=A0=C2=A0=C2=A0=C2=A0 |= =C2=A0 53 +++----- > > =C2=A0 drivers/gpu/drm/v3d/v3d_sched.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | 135 +++++++++++++++- > > ----- > > =C2=A0 drivers/gpu/drm/xe/xe_execlist.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 20 ++- > > =C2=A0 drivers/gpu/drm/xe/xe_gpu_scheduler.c=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 |=C2=A0 19 ++- > > =C2=A0 include/drm/gpu_scheduler.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 35 +++++- > > =C2=A0 14 files changed, 311 insertions(+), 139 deletions(-) > >=20 >=20 > [...] >=20 > > diff --git a/drivers/gpu/drm/v3d/v3d_sched.c > > b/drivers/gpu/drm/v3d/v3d_sched.c > > index 99ac4995b5a1..716e6d074d87 100644 > > --- a/drivers/gpu/drm/v3d/v3d_sched.c > > +++ b/drivers/gpu/drm/v3d/v3d_sched.c > > @@ -814,67 +814,124 @@ static const struct drm_sched_backend_ops > > v3d_cpu_sched_ops =3D { > > =C2=A0=C2=A0 .free_job =3D v3d_cpu_job_free > > =C2=A0 }; > > =C2=A0=20 > > +/* > > + * v3d's scheduler instances are all identical, except for ops and > > name. > > + */ > > +static void > > +v3d_common_sched_init(struct drm_sched_init_params *params, struct > > device *dev) > > +{ > > + memset(params, 0, sizeof(struct drm_sched_init_params)); > > + > > + params->submit_wq =3D NULL; /* Use the system_wq. */ > > + params->num_rqs =3D DRM_SCHED_PRIORITY_COUNT; > > + params->credit_limit =3D 1; > > + params->hang_limit =3D 0; > > + params->timeout =3D msecs_to_jiffies(500); > > + params->timeout_wq =3D NULL; /* Use the system_wq. */ > > + params->score =3D NULL; > > + params->dev =3D dev; > > +} >=20 > Could we use only one function that takes struct v3d_dev *v3d, enum > v3d_queue, and sched_ops as arguments (instead of one function per > queue)? You can get the name of the scheduler by concatenating "v3d_" > to > the return of v3d_queue_to_string(). >=20 > I believe it would make the code much simpler. Hello, so just to get that right: You'd like to have one universal function that switch-cases over an enum, sets the ops and creates the name with string concatenation? I'm not convinced that this is simpler than a few small functions, but it's not my component, so=E2=80=A6 Whatever we'll do will be simpler than the existing code, though. Right now no reader can see at first glance whether all those schedulers are identically parametrized or not. P. >=20 > Best Regards, > - Ma=C3=ADra >=20 > > + > > +static int > > +v3d_bin_sched_init(struct v3d_dev *v3d) > > +{ > > + struct drm_sched_init_params params; > > + > > + v3d_common_sched_init(¶ms, v3d->drm.dev); > > + params.ops =3D &v3d_bin_sched_ops; > > + params.name =3D "v3d_bin"; > > + > > + return drm_sched_init(&v3d->queue[V3D_BIN].sched, > > ¶ms); > > +} > > + > > +static int > > +v3d_render_sched_init(struct v3d_dev *v3d) > > +{ > > + struct drm_sched_init_params params; > > + > > + v3d_common_sched_init(¶ms, v3d->drm.dev); > > + params.ops =3D &v3d_render_sched_ops; > > + params.name =3D "v3d_render"; > > + > > + return drm_sched_init(&v3d->queue[V3D_RENDER].sched, > > ¶ms); > > +} > > + > > +static int > > +v3d_tfu_sched_init(struct v3d_dev *v3d) > > +{ > > + struct drm_sched_init_params params; > > + > > + v3d_common_sched_init(¶ms, v3d->drm.dev); > > + params.ops =3D &v3d_tfu_sched_ops; > > + params.name =3D "v3d_tfu"; > > + > > + return drm_sched_init(&v3d->queue[V3D_TFU].sched, > > ¶ms); > > +} > > + > > +static int > > +v3d_csd_sched_init(struct v3d_dev *v3d) > > +{ > > + struct drm_sched_init_params params; > > + > > + v3d_common_sched_init(¶ms, v3d->drm.dev); > > + params.ops =3D &v3d_csd_sched_ops; > > + params.name =3D "v3d_csd"; > > + > > + return drm_sched_init(&v3d->queue[V3D_CSD].sched, > > ¶ms); > > +} > > + > > +static int > > +v3d_cache_sched_init(struct v3d_dev *v3d) > > +{ > > + struct drm_sched_init_params params; > > + > > + v3d_common_sched_init(¶ms, v3d->drm.dev); > > + params.ops =3D &v3d_cache_clean_sched_ops; > > + params.name =3D "v3d_cache_clean"; > > + > > + return drm_sched_init(&v3d->queue[V3D_CACHE_CLEAN].sched, > > ¶ms); > > +} > > + > > +static int > > +v3d_cpu_sched_init(struct v3d_dev *v3d) > > +{ > > + struct drm_sched_init_params params; > > + > > + v3d_common_sched_init(¶ms, v3d->drm.dev); > > + params.ops =3D &v3d_cpu_sched_ops; > > + params.name =3D "v3d_cpu"; > > + > > + return drm_sched_init(&v3d->queue[V3D_CPU].sched, > > ¶ms); > > +} > > + > > =C2=A0 int > > =C2=A0 v3d_sched_init(struct v3d_dev *v3d) > > =C2=A0 { > > - int hw_jobs_limit =3D 1; > > - int job_hang_limit =3D 0; > > - int hang_limit_ms =3D 500; > > =C2=A0=C2=A0 int ret; > > =C2=A0=20 > > - ret =3D drm_sched_init(&v3d->queue[V3D_BIN].sched, > > - =C2=A0=C2=A0=C2=A0=C2=A0 &v3d_bin_sched_ops, NULL, > > - =C2=A0=C2=A0=C2=A0=C2=A0 DRM_SCHED_PRIORITY_COUNT, > > - =C2=A0=C2=A0=C2=A0=C2=A0 hw_jobs_limit, job_hang_limit, > > - =C2=A0=C2=A0=C2=A0=C2=A0 msecs_to_jiffies(hang_limit_ms), > > NULL, > > - =C2=A0=C2=A0=C2=A0=C2=A0 NULL, "v3d_bin", v3d->drm.dev); > > + ret =3D v3d_bin_sched_init(v3d); > > =C2=A0=C2=A0 if (ret) > > =C2=A0=C2=A0 return ret; > > =C2=A0=20 > > - ret =3D drm_sched_init(&v3d->queue[V3D_RENDER].sched, > > - =C2=A0=C2=A0=C2=A0=C2=A0 &v3d_render_sched_ops, NULL, > > - =C2=A0=C2=A0=C2=A0=C2=A0 DRM_SCHED_PRIORITY_COUNT, > > - =C2=A0=C2=A0=C2=A0=C2=A0 hw_jobs_limit, job_hang_limit, > > - =C2=A0=C2=A0=C2=A0=C2=A0 msecs_to_jiffies(hang_limit_ms), > > NULL, > > - =C2=A0=C2=A0=C2=A0=C2=A0 NULL, "v3d_render", v3d->drm.dev); > > + ret =3D v3d_render_sched_init(v3d); > > =C2=A0=C2=A0 if (ret) > > =C2=A0=C2=A0 goto fail; > > =C2=A0=20 > > - ret =3D drm_sched_init(&v3d->queue[V3D_TFU].sched, > > - =C2=A0=C2=A0=C2=A0=C2=A0 &v3d_tfu_sched_ops, NULL, > > - =C2=A0=C2=A0=C2=A0=C2=A0 DRM_SCHED_PRIORITY_COUNT, > > - =C2=A0=C2=A0=C2=A0=C2=A0 hw_jobs_limit, job_hang_limit, > > - =C2=A0=C2=A0=C2=A0=C2=A0 msecs_to_jiffies(hang_limit_ms), > > NULL, > > - =C2=A0=C2=A0=C2=A0=C2=A0 NULL, "v3d_tfu", v3d->drm.dev); > > + ret =3D v3d_tfu_sched_init(v3d); > > =C2=A0=C2=A0 if (ret) > > =C2=A0=C2=A0 goto fail; > > =C2=A0=20 > > =C2=A0=C2=A0 if (v3d_has_csd(v3d)) { > > - ret =3D drm_sched_init(&v3d->queue[V3D_CSD].sched, > > - =C2=A0=C2=A0=C2=A0=C2=A0 &v3d_csd_sched_ops, NULL, > > - =C2=A0=C2=A0=C2=A0=C2=A0 DRM_SCHED_PRIORITY_COUNT, > > - =C2=A0=C2=A0=C2=A0=C2=A0 hw_jobs_limit, > > job_hang_limit, > > - =C2=A0=C2=A0=C2=A0=C2=A0 > > msecs_to_jiffies(hang_limit_ms), NULL, > > - =C2=A0=C2=A0=C2=A0=C2=A0 NULL, "v3d_csd", v3d- > > >drm.dev); > > + ret =3D v3d_csd_sched_init(v3d); > > =C2=A0=C2=A0 if (ret) > > =C2=A0=C2=A0 goto fail; > > =C2=A0=20 > > - ret =3D drm_sched_init(&v3d- > > >queue[V3D_CACHE_CLEAN].sched, > > - =C2=A0=C2=A0=C2=A0=C2=A0 &v3d_cache_clean_sched_ops, > > NULL, > > - =C2=A0=C2=A0=C2=A0=C2=A0 DRM_SCHED_PRIORITY_COUNT, > > - =C2=A0=C2=A0=C2=A0=C2=A0 hw_jobs_limit, > > job_hang_limit, > > - =C2=A0=C2=A0=C2=A0=C2=A0 > > msecs_to_jiffies(hang_limit_ms), NULL, > > - =C2=A0=C2=A0=C2=A0=C2=A0 NULL, "v3d_cache_clean", v3d- > > >drm.dev); > > + ret =3D v3d_cache_sched_init(v3d); > > =C2=A0=C2=A0 if (ret) > > =C2=A0=C2=A0 goto fail; > > =C2=A0=C2=A0 } > > =C2=A0=20 > > - ret =3D drm_sched_init(&v3d->queue[V3D_CPU].sched, > > - =C2=A0=C2=A0=C2=A0=C2=A0 &v3d_cpu_sched_ops, NULL, > > - =C2=A0=C2=A0=C2=A0=C2=A0 DRM_SCHED_PRIORITY_COUNT, > > - =C2=A0=C2=A0=C2=A0=C2=A0 1, job_hang_limit, > > - =C2=A0=C2=A0=C2=A0=C2=A0 msecs_to_jiffies(hang_limit_ms), > > NULL, > > - =C2=A0=C2=A0=C2=A0=C2=A0 NULL, "v3d_cpu", v3d->drm.dev); > > + ret =3D v3d_cpu_sched_init(v3d); > > =C2=A0=C2=A0 if (ret) > > =C2=A0=C2=A0 goto fail; > > =C2=A0=20 >=20