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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 10E76C433EF for ; Thu, 25 Nov 2021 10:57:38 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3899C6E930; Thu, 25 Nov 2021 10:57:38 +0000 (UTC) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id 041776E928 for ; Thu, 25 Nov 2021 10:57:35 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10178"; a="296300488" X-IronPort-AV: E=Sophos;i="5.87,263,1631602800"; d="scan'208";a="296300488" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Nov 2021 02:57:35 -0800 X-IronPort-AV: E=Sophos;i="5.87,263,1631602800"; d="scan'208";a="538948454" Received: from dshe-mobl1.ger.corp.intel.com (HELO localhost) ([10.252.10.64]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Nov 2021 02:57:32 -0800 From: Jani Nikula To: Ville =?utf-8?B?U3lyasOkbMOk?= In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20211124113652.22090-1-ville.syrjala@linux.intel.com> <20211124113652.22090-12-ville.syrjala@linux.intel.com> <87pmqplft3.fsf@intel.com> Date: Thu, 25 Nov 2021 12:57:27 +0200 Message-ID: <87bl28lcyw.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Intel-gfx] [PATCH 11/20] drm/i915/fbc: Move FBC debugfs stuff into intel_fbc.c 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: , Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Thu, 25 Nov 2021, Ville Syrj=C3=A4l=C3=A4 wrote: > On Wed, Nov 24, 2021 at 05:43:52PM +0200, Jani Nikula wrote: >> On Wed, 24 Nov 2021, Ville Syrjala wrote: >> > From: Ville Syrj=C3=A4l=C3=A4 >> > >> > In order to encapsulate FBC harder let's just move the debugfs >> > stuff into intel_fbc.c. >>=20 >> Mmmh, I've kind of moved towards a split where i915_debugfs.c and >> intel_display_debugfs.c have all the debugfs boilerplate, while the >> implementation files have the guts with struct drm_i915_private *i915 >> (or something more specific) and struct seq_file *m passed in. >>=20 >> In some ways the split is arbitrary, but I kind of find the debugfs >> boilerplate a distraction in the implementation files, and we also skip >> building the debugfs files completely for CONFIG_DEBUG_FS=3Dn. I don't >> think I'd want to add #ifdefs on that spread around either. > > If we want to keep the debugfs in a separate file then we'll have to > expose the guts of the FBC implementation in intel_fbc.h (or some other > header) just for that, or we add a whole bunch of otherwise useless > functions that pretend to provide some higher level of abstraction. > > Not really a fan of either of those options. Obviously I'm in favour of hiding the guts, no question about it. I'm also very much in favour of moving the details out of our *debugfs.c files. It's just a question of where to draw the line, and which side of the line the debugfs boilerplate lands. Which leaves us either your approach in the patch at hand, or adding the fbc helper functions for debugfs, which would be something like: intel_fbc_get_status intel_fbc_get_false_color intel_fbc_set_false_color >From a technical standpoint, I can appreciate the pros and cons of both approaches. And I could be persuaded either way. With the maintainer hat on, I'm considering the precedent this sets, and where things start flowing after that. Do we accept all options for other files, case by case, making this a continuous bikeshedding topic, or do we want to choose an approach and start driving it everywhere? Frankly I'm not all that fond of giving people a lot of options on stuff like this, I prefer saying this is how we roll, let's stick to it. (Until we decide otherwise, and stick to something else! ;) Sooo... what would it be like if every file had their own intel_foo_debugfs_register()? Including connector specific stuff such as "i915_psr_status". Cc'd other maintainers for good measure. Let's have one debugfs bikeshedding now, and stick to it, instead of every time this pops up? BR, Jani. --=20 Jani Nikula, Intel Open Source Graphics Center