From mboxrd@z Thu Jan 1 00:00:00 1970 From: Emil Velikov Subject: Re: [RFC PATCH 01/20] drm: Remove users of drm_format_num_planes Date: Thu, 4 Apr 2019 17:24:03 +0100 Message-ID: References: <20190402145114.bpt5atxxooufgzz5@flea> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20190402145114.bpt5atxxooufgzz5@flea> Sender: linux-kernel-owner@vger.kernel.org To: Maxime Ripard Cc: Daniel Vetter , David Airlie , Maarten Lankhorst , Sean Paul , Mauro Carvalho Chehab , Sakari Ailus , "Linux-Kernel@Vger. Kernel. Org" , ML dri-devel , Paul Kocialkowski , Hans Verkuil , Laurent Pinchart , Thomas Petazzoni , linux-media@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org On Tue, 2 Apr 2019 at 15:51, Maxime Ripard wrote: > > Hi Emil, > > On Tue, Apr 02, 2019 at 10:43:31AM +0100, Emil Velikov wrote: > > On Tue, 19 Mar 2019 at 21:57, Maxime Ripard wrote: > > > drm_format_num_planes() is basically a lookup in the drm_format_info table > > > plus an access to the num_planes field of the appropriate entry. > > > > > > Most drivers are using this function while having access to the entry > > > already, which means that we will perform an unnecessary lookup. Removing > > > the call to drm_format_num_planes is therefore more efficient. > > > > > > Some drivers will not have access to that entry in the function, but in > > > this case the overhead is minimal (we just have to call drm_format_info() > > > to perform the lookup) and we can even avoid multiple, inefficient lookups > > > in some places that need multiple fields from the drm_format_info > > > structure. > > > > > > > I'm not fan of the duplicated loop-ups either. > > > > > -int drm_format_num_planes(uint32_t format) > > > -{ > > > - const struct drm_format_info *info; > > > - > > > - info = drm_format_info(format); > > > - return info ? info->num_planes : 1; > > > -} > > > -EXPORT_SYMBOL(drm_format_num_planes); > > > - > > > > The existing users are not updated to cater for the num_planes != 0 > > case... Which seems non-existent scenario since all the current format > > descriptions have 1+ planes. > > Should we add a test (alike the ones in 6/20) to ensure, that no entry > > has 0 planes? Is it even worth it or I'm a bit too paranoid? > > > > The above comments apply to 2/20. > > I'm not entirely sure what you mean. num_planes is returned as is in > the drm_format_num_planes function and it doesn't check for the > num_planes value itself. > > That being said, we could definitely add some more tests to check that > we haven't falling into the situation you describe, since most of the > drivers indeed don't check for that value themselves. But that seems > pretty othorgonal to me? > Hmm I misread the old function as "return info->num_planes ? info->num_planes : 1;" Pardon for the noise. -Emil