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: <509d8bb0-f030-f8ca-a610-da5faaa6396b@amd.com>
Date:   Wed, 10 Aug 2022 11:16:04 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [Linaro-mm-sig] Re: [PATCH v2] drm/gem: Fix GEM handle release
 errors
Content-Language: en-US
To:     Chen Jeffy <jeffy.chen@rock-chips.com>,
        Daniel Vetter <daniel.vetter@ffwll.ch>
Cc:     Andy Yan <andy.yan@rock-chips.com>,
        Jianqun Xu <jay.xu@rock-chips.com>,
        Maxime Ripard <mripard@kernel.org>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        linaro-mm-sig@lists.linaro.org, David Airlie <airlied@linux.ie>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        linux-media@vger.kernel.org, Daniel Vetter <daniel@ffwll.ch>
References: <20220803083237.3701-1-jeffy.chen@rock-chips.com>
 <c7cb225b-7f21-8d9a-773b-efc655e6332c@amd.com>
 <7cd16264-fa84-7b50-f3ed-64f7f22dcef2@rock-chips.com>
 <64bf4e4b-4e22-0ff0-5f92-76f603c04ec0@amd.com>
 <cd806954-e94e-aec8-2b0c-4047da9a92ec@rock-chips.com>
 <0e284f57-e03c-f128-f6e7-52a58edbcd54@amd.com>
 <71e47fe6-440b-e9ea-cd66-8362c41428ca@amd.com>
 <6b3e82f9-6902-fd5c-c67d-e2c42c995133@rock-chips.com>
 <cb478ae3-a6c6-ea90-a47c-13d52b606942@amd.com>
 <05488346-ebbd-b1d9-4094-a83daf65f6db@rock-chips.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <05488346-ebbd-b1d9-4094-a83daf65f6db@rock-chips.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Q1haa25DNFVHK05EZGtkL1pUcHBvc3hRVWY5QjM4SjFKbzJ5OXg2aTlUcFFh?=
 =?utf-8?B?OGhSVHpOdk93L1hHbE92NHdvQjdrRXBxckQrUVEwTG9ZQUloTXlhYWNCdVgw?=
 =?utf-8?B?Yjg3K1NOb29JOWVQdUNWMzRET0NNZE5iQlE5d1FES0JSb3NwM0dYVEFrNjZS?=
 =?utf-8?B?OGZsaHIvRW9XTDdYZFExT05TUGJ2anZpM3VkZEtYUDZCT0hKV2NseHBxb3Vq?=
 =?utf-8?B?TkI1dU1nRkpzSmdOV2pmQlYxc3lYbTF6TEhoZTNIUWljSGwxK2E3YmYxTno3?=
 =?utf-8?B?aGxrSlZEcDVZMUlDeDl2L0xQaTA1U3NXVDNIUHR6aE9PZlU1S0l5ZjhYZ1k1?=
 =?utf-8?B?L0xLYzBUTm00ajM1OGNUaFNJSTZqajhpRCtEL05TTjk1RVp1cDZ4SDdmakFp?=
 =?utf-8?B?VkFGcVpOelp1eGM3b0JlekpSUFd2czJITThjbDk0Qyt4VW9Nd2tpSmUxZmZz?=
 =?utf-8?B?dzJ6enJGZWl1eVNCMDUzcUdLQUZ2eTBpU0d2WFYxbFNWSDVCaXZ1QnFmc2F5?=
 =?utf-8?B?RUxZcmRpRUk0ZU9zeXByYUtjSUZXNlhKZUMxUHpvQjUxMm9BeHVTRjgrMkF1?=
 =?utf-8?B?cVFWRWM4U1lHMklDOTBvS05BU3dRRlQwckwxaVpEWHJYRWtUc3V3OURVQjk2?=
 =?utf-8?B?ZzBSNW5DbDROQ0F6MnJ4YTNERlgxUG5lVmRaamhpVFo4YmlpUWZDK2VtbE15?=
 =?utf-8?B?MTR2ZGRwbGhpNFJJRmxoZjJXUkhrK3NRWHZEd1JPUEZFMC91VmhoaXF6Z1Zj?=
 =?utf-8?B?alplOHJLcnNVbTR0ZG42OS9Zc1FxTHY1MUtWUEhVUDdhY09JT080bTdZc2E1?=
 =?utf-8?B?YVZLaENlWjRNdEJqZmlTK3IzOEVEMDFxOEU1TG9WYXJ4QVcrRFBSa3VMZktK?=
 =?utf-8?B?OE1nZHorL0s3Y09sZkhGZ2h0VXFCbTN6MFVWREtTeVRBbThBMnoza05EUUo3?=
 =?utf-8?B?ME9GcC9uUTNtNm9WNkQrWVRZWU9GeVQzeVZhTnoyR2ZtTks0aVprT2trOEhD?=
 =?utf-8?B?R2VMM3NERXBLUEdPRHMycXRLRjdkUEF5U1BDdGVRSVFGZVJ6RGhRQit5cHcw?=
 =?utf-8?B?VXVrRVdmZlpGWFZRYkYzQVowa2x3ZkFiMlNFbDk4MlU0ZVQ0WGQxTHNDbWI5?=
 =?utf-8?B?ZmQ2cENQVWwwSXNpNXNLZ2VVNXg5dElxN0FrbFBFcXFFODdPUlQrZE5adFdj?=
 =?utf-8?B?VzFtcGhSZ2ppcEVRdHc1OWVFR1BvWXB2bzBhNDJlaFJobEpBMXl5ODN0VXY2?=
 =?utf-8?B?WlNaQ0pkTUtSR3NWSkhKeUk4a2k1NWlBaFJ2azhZa3ZMejIwN04vZGRJR0tH?=
 =?utf-8?B?UzFkblhMU3dOUTdUaTU4NnlldmlvRW9HK0FrVkNiOWp0U1Q0Rkk4QlFYazVC?=
 =?utf-8?B?MWE1UmFWWnFCYXdxNG5WcW8rTEFMeGdZN2FOZWF2MFJabnBXNWF6N1Juc1Ew?=
 =?utf-8?B?R0tOY055YUl2WmUrVk9Ibm5ERldDK2xtaFJrdzFDMm5tRVpjeDRFaEtOdXdo?=
 =?utf-8?B?c0hWWTFQUWsxaE1VYllmL1prZ0djM2FxalBxVUNRUkE4NWFnUjlmV1RRemo3?=
 =?utf-8?B?VW54WEFONDhlbWd2a1M4NGFDMzkzcXJBSzIxQ0sydUREc29hdWVVTmI0RCtP?=
 =?utf-8?B?Ukp1TnhNRVNOaEZvVk5hOU1OUlcyWnR3dzJUL1U3Nm82ZjlnUC9rbUFRWDRr?=
 =?utf-8?B?d283RHlNUDFTaFROR0ZBNG80TkZPVi9QOHRlOHlqMTAzT3IraEwycmNCT3lw?=
 =?utf-8?B?MVhQNU5KamxKNzJhNlFkNFBUZTZEM0lDRHI5NWduRngzR0tjS1Avb29saGQ5?=
 =?utf-8?B?TkQrNGFjRDd3ZHpSV2lOMHpRay8veTY3MG9jbzN3RFI1eG5HTVp3ZG51V3lG?=
 =?utf-8?B?WUxzRHFwRFhOaUZ4Q0tQbm9Od0MwMXhwc1lraGVhdVBRWmlEMytySGVjcys2?=
 =?utf-8?B?UWR0QWtVdkhHMlQxcjJaTW1FNVV3MDI1dS85MjhXRytRdVYvU05sRnRRZVRm?=
 =?utf-8?B?Y2p1bXNVMVIyU2ZaRjJxSHBTcXpvSFZIWFhLT2RWWGk0RE5HMGRKR0VKemh6?=
 =?utf-8?B?OXF1U2VFQVpOLys2V1ozSExRbndEblR0RGFFTlRsQkhIckQ3NnVOT0Y2MWU1?=
 =?utf-8?B?TDdPUCtINDVBcWZrMUs1TkFpRkM3N3ZIaEpoK2hBRDdvbXluUlR3RWtQYnE5?=
 =?utf-8?Q?J0K1BZ1Db2BHRWUXzut83Em4a2CnIQtKerFZl/TcGhHg?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 52030c85-59b8-44cc-87d8-08da7ab0f811
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Aug 2022 09:16:12.1702
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 5Rs0hKe8XEnGKBG6umyP4/JRcI65vnFRBycj4btpG2R0AunI8LSGuZ1nAy8Z91ry
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR12MB4384
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        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

Hi Jeffy,

Am 10.08.22 um 06:16 schrieb Chen Jeffy:
> Hi Christian,
>
> On 8/9 星期二 18:18, Christian König wrote:
>> Hi Jeffy,
>> [SNIP]
>>> Maybe cache the latest returned handle in the obj(after 
>>> drm_gem_prime_fd_to_handle), and clear it when that handle been 
>>> deleted in drm_gem_handle_delete()?
>>
>> That won't work. The handle is per fpriv, but the same object is used 
>> by multiple fpriv instances. >
>> What we could maybe do is to prevent adding multiple lockup 
>> structures when there is already one, but that's not something I can 
>> easily judge.
>
> So maybe we need to protect that unique lookup structure been deleted 
> before deleting the last handle, and make the handle unique for GEM 
> obj, in case of that unique lookup's handle been deleted earlier that 
> others?
>
> How about adding a GEM obj rbtree too, and make drm_prime_member kref-ed?
>
> So the 
> drm_prime_add_buf_handle/drm_gem_handle_create_tail/drm_gem_handle_delete 
> would be looking up drm_prime_member by GEM obj, then update dmabuf rb 
> and inc/dec drm_prime_member's kref, 
> drm_gem_prime_fd_to_handle/drm_gem_prime_handle_to_fd remain unchanged.

I think we should probably come up with an idea what the UAPI should 
look like before we try to implement this in the kernel, but in general 
I think we should make the solution simpler and not even more complex.

Recording multiple handles for the same DMA-buf/fpriv combination 
doesn't seem to make sense, so the duplicated tracking of 
handle->dma_buf mapping just seems to be overkill.

So my proposal would be this:
1. Only the first used GEM handle is tracker for each DMA-buf/fpriv 
combination.
2. Imports either return this first used handle or allocate a new one if 
there isn't any.
3. If the first used handle is closed we allocate a new one on re-import 
even when there duplicate handles.

The alternative as we have it now is to just return a more or less 
random handle if there are duplicates which doesn't sound like something 
we would want.

Daniel, can we agree on that?

Regards,
Christian.