From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D2FEDC433F5 for ; Fri, 8 Oct 2021 23:51:40 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 8BE3760F94 for ; Fri, 8 Oct 2021 23:51:40 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 8BE3760F94 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id A4A446E897; Fri, 8 Oct 2021 23:51:39 +0000 (UTC) Received: from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com [IPv6:2a00:1450:4864:20::12b]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9D1EF6E893; Fri, 8 Oct 2021 23:51:38 +0000 (UTC) Received: by mail-lf1-x12b.google.com with SMTP id x27so45322869lfa.9; Fri, 08 Oct 2021 16:51:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=uwgZMiF+2EZrddFYvj4zPW2uaBGbkJ1NsJjndwLFgmo=; b=QOKPxUmXBAKw6igDPT/zjPt5QMIFZE2KNEKOO/sVKw0K97S2Je2cABFymmu5qKAx4t iYMiWHnbtwY3LwV6wkpDvRGZ1RwVqJIxTlvl1E35YVrTFKAmmrmOCmaRrCZYSkr1ktvS H3haz772ryqXlSwRc957ycXHXmHvefPE5v9i6sCXDlVtMMKejk7Ocrk9JwR9lEdQzDX2 l0uzse9bTo9as0ZZ1N6eK/bxWW4xmwKTFsQG8vfWkccim6atXoPNvYYvhUVMDwA7Tfqi qPGoo1bIiKqbPcTo1LnnaPiNRXBXSWTqtYpev401iUR0auPdzGx1rag306LnXFCwoenu zo/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=uwgZMiF+2EZrddFYvj4zPW2uaBGbkJ1NsJjndwLFgmo=; b=y7osFFX8CBWtBrJx1YX53izlFkZPuhhxNEa5ucdHL0VzQ6zXLwYx9SSRQDpFwsiBOF C8QkYwDchOcMJLOibAsZcHKDiYeVBB6lpYnIad585lYKl05lM8BqOt+DT7owYVv12Pb/ 6rOmSH7CFWq1p3qUHbfGGeHe3Vmw98/qnCHCfugT+KL2NtnU2lRuR0dXfNg0jFvs/zvP EheBa1UD416EVTh8mk3rMTz4y0E1LHw34qfBOvC8O2WC8CVBvFNnpvpyYoI/9VKjqE5i Oi5ScPy/6D4UTk65XsSkLW4kzf3/iAPHwdk18fRk0Veo8SBEGgOlVIhXmiSDyt3iQ3PR Z93A== X-Gm-Message-State: AOAM531PI2qwPWFKbUnfqsq6AxL6vfBWNFw0nG2BOFkQ0G8RQ6R2rrWL jTtHa2z1Lit/YygGuSfY7SO/hG42ISEM3HPhIoM= X-Google-Smtp-Source: ABdhPJz7Ow+K8EIslKeXp+4vVFFt/sRu9Sx24RquRofTAvElLfDQdvi+dltK3N0wFBb4tXcgGAUE9bWtvMGRBVZNDYw= X-Received: by 2002:a05:6512:3d11:: with SMTP id d17mr11324880lfv.542.1633737096563; Fri, 08 Oct 2021 16:51:36 -0700 (PDT) MIME-Version: 1.0 References: <20211007230903.4084-1-andi@etezian.org> In-Reply-To: From: Lucas De Marchi Date: Fri, 8 Oct 2021 16:51:24 -0700 Message-ID: To: Andi Shyti Cc: Andi Shyti , Intel GFX , DRI Devel , Tvrtko Ursulin , Chris Wilson , Lucas De Marchi , Joonas Lahtinen Content-Type: text/plain; charset="UTF-8" Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/gt: move remaining debugfs interfaces into gt X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Fri, Oct 8, 2021 at 3:14 AM Andi Shyti wrote: > > Hi Lucas, > > > > I am reproposing this patch exactly as it was proposed initially > > > where the original interfaces are kept where they have been > > > originally placed. It might generate some duplicated code but, > > > well, it's debugfs and I don't see any issue. In the future we > > > can transform the upper interfaces to act upon all the GTs and > > > provide information from all the GTs. This is, for example, how > > > the sysfs interfaces will act. > > > > NACK. We've made this mistake in the past for other debugfs files. > > We don't want to do it again just to maintain 2 separate places for > > one year and then finally realize we want to merge them. > > In my opinion it's all about what mistake you like the most > because until we will have multi-gt support in upstream all the > patches come with the "promise" of a follow-up and maintenance > cost. no. If you put the implementation in a single place, later you only have the decision on what to do with the per-device entrypoint: - should we remove it once igt is converted? - should we make it iterate all gts? - should we make it mean root tile? Then you take the action that is needed and decide it per interface. Here you are leaving behind a lot of code that we will need to maintain until there is support for such a thing. It already happened once: we needed to maintain that duplicated code for over a year with multiple patches changing them (or failing to do so). > > > > The reason I removed them in V1 is because igt as only user is > > > not a strong reason to keep duplicated code, but as Chris > > > suggested offline: > > > > > > "It's debugfs, igt is the primary consumer. CI has to be bridged over > > > changes to the interfaces it is using in any case, as you want > > > comparable results before/after the patches land. > > > > That doesn't mean you have to copy and paste it. It may mean you > > do the implementation in one of them and the other calls that implementation. > > See how I did the deduplication in commit d0c560316d6f ("drm/i915: > > deduplicate frequency dump on debugfs") > > In this case, from a user perspective, which gt is the interface > affecting? is it affecting all the system? or gt 0, 1...? Does > the user know? The maintenance cost is that later you will need > to use for_each_gt and make all those interfaces multitile, and > this would be your "promise". multi-gt is irrelevant here. This patch without any "promise" should do what the commit message says: *move*. The only reason to keep the old entrypoint around is because it's missing the igt conversion. If you are going to support a per-device entrypoint and do for_each_gt(), or do a symlink to the root tile, or whatever, it isn't very relevant to this patch. Right now we have just a single directory, gt. Lucas De Marchi From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D363EC433F5 for ; Fri, 8 Oct 2021 23:51:43 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9F4CB60F94 for ; Fri, 8 Oct 2021 23:51:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 9F4CB60F94 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 946BD6E896; Fri, 8 Oct 2021 23:51:39 +0000 (UTC) Received: from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com [IPv6:2a00:1450:4864:20::12b]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9D1EF6E893; Fri, 8 Oct 2021 23:51:38 +0000 (UTC) Received: by mail-lf1-x12b.google.com with SMTP id x27so45322869lfa.9; Fri, 08 Oct 2021 16:51:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=uwgZMiF+2EZrddFYvj4zPW2uaBGbkJ1NsJjndwLFgmo=; b=QOKPxUmXBAKw6igDPT/zjPt5QMIFZE2KNEKOO/sVKw0K97S2Je2cABFymmu5qKAx4t iYMiWHnbtwY3LwV6wkpDvRGZ1RwVqJIxTlvl1E35YVrTFKAmmrmOCmaRrCZYSkr1ktvS H3haz772ryqXlSwRc957ycXHXmHvefPE5v9i6sCXDlVtMMKejk7Ocrk9JwR9lEdQzDX2 l0uzse9bTo9as0ZZ1N6eK/bxWW4xmwKTFsQG8vfWkccim6atXoPNvYYvhUVMDwA7Tfqi qPGoo1bIiKqbPcTo1LnnaPiNRXBXSWTqtYpev401iUR0auPdzGx1rag306LnXFCwoenu zo/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=uwgZMiF+2EZrddFYvj4zPW2uaBGbkJ1NsJjndwLFgmo=; b=y7osFFX8CBWtBrJx1YX53izlFkZPuhhxNEa5ucdHL0VzQ6zXLwYx9SSRQDpFwsiBOF C8QkYwDchOcMJLOibAsZcHKDiYeVBB6lpYnIad585lYKl05lM8BqOt+DT7owYVv12Pb/ 6rOmSH7CFWq1p3qUHbfGGeHe3Vmw98/qnCHCfugT+KL2NtnU2lRuR0dXfNg0jFvs/zvP EheBa1UD416EVTh8mk3rMTz4y0E1LHw34qfBOvC8O2WC8CVBvFNnpvpyYoI/9VKjqE5i Oi5ScPy/6D4UTk65XsSkLW4kzf3/iAPHwdk18fRk0Veo8SBEGgOlVIhXmiSDyt3iQ3PR Z93A== X-Gm-Message-State: AOAM531PI2qwPWFKbUnfqsq6AxL6vfBWNFw0nG2BOFkQ0G8RQ6R2rrWL jTtHa2z1Lit/YygGuSfY7SO/hG42ISEM3HPhIoM= X-Google-Smtp-Source: ABdhPJz7Ow+K8EIslKeXp+4vVFFt/sRu9Sx24RquRofTAvElLfDQdvi+dltK3N0wFBb4tXcgGAUE9bWtvMGRBVZNDYw= X-Received: by 2002:a05:6512:3d11:: with SMTP id d17mr11324880lfv.542.1633737096563; Fri, 08 Oct 2021 16:51:36 -0700 (PDT) MIME-Version: 1.0 References: <20211007230903.4084-1-andi@etezian.org> In-Reply-To: From: Lucas De Marchi Date: Fri, 8 Oct 2021 16:51:24 -0700 Message-ID: Subject: Re: [PATCH v2] drm/i915/gt: move remaining debugfs interfaces into gt To: Andi Shyti Cc: Andi Shyti , Intel GFX , DRI Devel , Tvrtko Ursulin , Chris Wilson , Lucas De Marchi , Joonas Lahtinen Content-Type: text/plain; charset="UTF-8" X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Fri, Oct 8, 2021 at 3:14 AM Andi Shyti wrote: > > Hi Lucas, > > > > I am reproposing this patch exactly as it was proposed initially > > > where the original interfaces are kept where they have been > > > originally placed. It might generate some duplicated code but, > > > well, it's debugfs and I don't see any issue. In the future we > > > can transform the upper interfaces to act upon all the GTs and > > > provide information from all the GTs. This is, for example, how > > > the sysfs interfaces will act. > > > > NACK. We've made this mistake in the past for other debugfs files. > > We don't want to do it again just to maintain 2 separate places for > > one year and then finally realize we want to merge them. > > In my opinion it's all about what mistake you like the most > because until we will have multi-gt support in upstream all the > patches come with the "promise" of a follow-up and maintenance > cost. no. If you put the implementation in a single place, later you only have the decision on what to do with the per-device entrypoint: - should we remove it once igt is converted? - should we make it iterate all gts? - should we make it mean root tile? Then you take the action that is needed and decide it per interface. Here you are leaving behind a lot of code that we will need to maintain until there is support for such a thing. It already happened once: we needed to maintain that duplicated code for over a year with multiple patches changing them (or failing to do so). > > > > The reason I removed them in V1 is because igt as only user is > > > not a strong reason to keep duplicated code, but as Chris > > > suggested offline: > > > > > > "It's debugfs, igt is the primary consumer. CI has to be bridged over > > > changes to the interfaces it is using in any case, as you want > > > comparable results before/after the patches land. > > > > That doesn't mean you have to copy and paste it. It may mean you > > do the implementation in one of them and the other calls that implementation. > > See how I did the deduplication in commit d0c560316d6f ("drm/i915: > > deduplicate frequency dump on debugfs") > > In this case, from a user perspective, which gt is the interface > affecting? is it affecting all the system? or gt 0, 1...? Does > the user know? The maintenance cost is that later you will need > to use for_each_gt and make all those interfaces multitile, and > this would be your "promise". multi-gt is irrelevant here. This patch without any "promise" should do what the commit message says: *move*. The only reason to keep the old entrypoint around is because it's missing the igt conversion. If you are going to support a per-device entrypoint and do for_each_gt(), or do a symlink to the root tile, or whatever, it isn't very relevant to this patch. Right now we have just a single directory, gt. Lucas De Marchi