From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?B?TWFyZWsgT2zFocOhaw==?= Subject: Re: [PATCH libdrm] amdgpu: add amdgpu_bo_handle_type_kms_noimport Date: Thu, 12 Jul 2018 13:03:24 -0400 Message-ID: References: <20180712004750.2024-1-maraeo@gmail.com> <9a8d8f7f-a468-fd5b-dec5-472ce9c88483@daenzer.net> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0276532107==" Return-path: In-Reply-To: <9a8d8f7f-a468-fd5b-dec5-472ce9c88483-otUistvHUpPR7s880joybQ@public.gmane.org> List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: =?UTF-8?Q?Michel_D=C3=A4nzer?= Cc: amd-gfx mailing list --===============0276532107== Content-Type: multipart/alternative; boundary="0000000000000473110570d0581a" --0000000000000473110570d0581a Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Jul 12, 2018, 3:31 AM Michel D=C3=A4nzer wrote= : > On 2018-07-12 02:47 AM, Marek Ol=C5=A1=C3=A1k wrote: > > From: Marek Ol=C5=A1=C3=A1k > > > > --- > > amdgpu/amdgpu.h | 7 ++++++- > > amdgpu/amdgpu_bo.c | 4 ++++ > > 2 files changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h > > index 36f91058..be83b457 100644 > > --- a/amdgpu/amdgpu.h > > +++ b/amdgpu/amdgpu.h > > @@ -77,21 +77,26 @@ struct drm_amdgpu_info_hw_ip; > > * > > */ > > enum amdgpu_bo_handle_type { > > /** GEM flink name (needs DRM authentication, used by DRI2) */ > > amdgpu_bo_handle_type_gem_flink_name =3D 0, > > > > /** KMS handle which is used by all driver ioctls */ > > amdgpu_bo_handle_type_kms =3D 1, > > > > /** DMA-buf fd handle */ > > - amdgpu_bo_handle_type_dma_buf_fd =3D 2 > > + amdgpu_bo_handle_type_dma_buf_fd =3D 2, > > + > > + /** KMS handle, but re-importing as a DMABUF handle through > > + * drmPrimeHandleToFD is forbidden. (Glamor does that) > > + */ > > + amdgpu_bo_handle_type_kms_noimport =3D 3, > > }; > > > > /** Define known types of GPU VM VA ranges */ > > enum amdgpu_gpu_va_range > > { > > /** Allocate from "normal"/general range */ > > amdgpu_gpu_va_range_general =3D 0 > > }; > > > > enum amdgpu_sw_info { > > diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c > > index 9e37b149..d29be244 100644 > > --- a/amdgpu/amdgpu_bo.c > > +++ b/amdgpu/amdgpu_bo.c > > @@ -234,20 +234,22 @@ int amdgpu_bo_export(amdgpu_bo_handle bo, > > case amdgpu_bo_handle_type_gem_flink_name: > > r =3D amdgpu_bo_export_flink(bo); > > if (r) > > return r; > > > > *shared_handle =3D bo->flink_name; > > return 0; > > > > case amdgpu_bo_handle_type_kms: > > amdgpu_add_handle_to_table(bo); > > + /* fall through */ > > + case amdgpu_bo_handle_type_kms_noimport: > > *shared_handle =3D bo->handle; > > return 0; > > What is the rationale for this? I.e. why do you want to not store some > handles in the hash table? Because I have the option. And how can code using > amdgpu_bo_handle_type_kms_noimport be sure that the BO will never be > re-imported via dma-buf? > That's for the user to decide and prove when it's safe. > The experience with the previous patch has shown that it's hard to keep > track of all possible ways in which BOs are imported, and that if we > miss one, this breaks pretty spectacularly. > You are assuming that it will be used incorrectly based on your previous bad experience. All I need to do is not to hand the handle to components that would misuse it. Marek > > -- > Earthling Michel D=C3=A4nzer | http://www.amd= .com > Libre software enthusiast | Mesa and X developer > --0000000000000473110570d0581a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Thu, = Jul 12, 2018, 3:31 AM Michel D=C3=A4nzer <michel-otUistvHUpPR7s880joybQ@public.gmane.org> wrote:
On 2018-07-12 02:47 AM, Marek Ol=C5=A1=C3=A1k wrote:
> From: Marek Ol=C5=A1=C3=A1k <marek.olsak-5C7GfCeVMHo@public.gmane.org>
>
> ---
>=C2=A0 amdgpu/amdgpu.h=C2=A0 =C2=A0 | 7 ++++++-
>=C2=A0 amdgpu/amdgpu_bo.c | 4 ++++
>=C2=A0 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
> index 36f91058..be83b457 100644
> --- a/amdgpu/amdgpu.h
> +++ b/amdgpu/amdgpu.h
> @@ -77,21 +77,26 @@ struct drm_amdgpu_info_hw_ip;
>=C2=A0 =C2=A0*
>=C2=A0 */
>=C2=A0 enum amdgpu_bo_handle_type {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/** GEM flink name (needs DRM authentication= , used by DRI2) */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0amdgpu_bo_handle_type_gem_flink_name =3D 0,<= br> >=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/** KMS handle which is used by all driver i= octls */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0amdgpu_bo_handle_type_kms =3D 1,
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/** DMA-buf fd handle */
> -=C2=A0 =C2=A0 =C2=A0amdgpu_bo_handle_type_dma_buf_fd =3D 2
> +=C2=A0 =C2=A0 =C2=A0amdgpu_bo_handle_type_dma_buf_fd =3D 2,
> +
> +=C2=A0 =C2=A0 =C2=A0/** KMS handle, but re-importing as a DMABUF hand= le through
> +=C2=A0 =C2=A0 =C2=A0 *=C2=A0 drmPrimeHandleToFD is forbidden. (Glamor= does that)
> +=C2=A0 =C2=A0 =C2=A0 */
> +=C2=A0 =C2=A0 =C2=A0amdgpu_bo_handle_type_kms_noimport =3D 3,
>=C2=A0 };
>=C2=A0
>=C2=A0 /** Define known types of GPU VM VA ranges */
>=C2=A0 enum amdgpu_gpu_va_range
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/** Allocate from "normal"/general= range */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0amdgpu_gpu_va_range_general =3D 0
>=C2=A0 };
>=C2=A0
>=C2=A0 enum amdgpu_sw_info {
> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
> index 9e37b149..d29be244 100644
> --- a/amdgpu/amdgpu_bo.c
> +++ b/amdgpu/amdgpu_bo.c
> @@ -234,20 +234,22 @@ int amdgpu_bo_export(amdgpu_bo_handle bo,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0case amdgpu_bo_handle_type_gem_flink_name: >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r =3D amdgpu_bo_= export_flink(bo);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (r)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0return r;
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*shared_handle = =3D bo->flink_name;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0case amdgpu_bo_handle_type_kms:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0amdgpu_add_handl= e_to_table(bo);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* fall through */ > +=C2=A0 =C2=A0 =C2=A0case amdgpu_bo_handle_type_kms_noimport:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*shared_handle = =3D bo->handle;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;

What is the rationale for this? I.e. why do you want to not store some
handles in the hash table?

Because I have the option.
And how can code using
amdgpu_bo_handle_type_kms_noimport be sure that the BO will never be
re-imported via dma-buf?

=
That's for the user to decide and prove when it= 's safe.


The experience with the previous patch has shown that it's hard to keep=
track of all possible ways in which BOs are imported, and that if we
miss one, this breaks pretty spectacularly.

You are assuming that it will be= used incorrectly based on your previous bad experience. All I need to do i= s not to hand the handle to components that would misuse it.

Marek



--
Earthling Michel D=C3=A4nzer=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=A0htt= p://www.amd.com
Libre software enthusiast=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=A0Mesa and X developer
--0000000000000473110570d0581a-- --===============0276532107== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4Cg== --===============0276532107==--