From patchwork Thu Mar 15 18:48:17 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Ville Syrjala X-Patchwork-Id: 10285567 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 168976061F for ; Thu, 15 Mar 2018 18:48:25 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0761528C12 for ; Thu, 15 Mar 2018 18:48:25 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id F031528C15; Thu, 15 Mar 2018 18:48:24 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=unavailable version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 7FBE128C12 for ; Thu, 15 Mar 2018 18:48:24 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 96F0A6E3AB; Thu, 15 Mar 2018 18:48:21 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 91E666E379; Thu, 15 Mar 2018 18:48:20 +0000 (UTC) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Mar 2018 11:48:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,311,1517904000"; d="scan'208";a="34205131" Received: from stinkbox.fi.intel.com (HELO stinkbox) ([10.237.72.174]) by FMSMGA003.fm.intel.com with SMTP; 15 Mar 2018 11:48:17 -0700 Received: by stinkbox (sSMTP sendmail emulation); Thu, 15 Mar 2018 20:48:17 +0200 Date: Thu, 15 Mar 2018 20:48:17 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Eric Anholt Subject: Re: [Intel-gfx] [PATCH 2/3] drm: Make sure at least one plane supports the fb format Message-ID: <20180315184817.GC5453@intel.com> References: <20180312204035.31387-1-ville.syrjala@linux.intel.com> <20180312204035.31387-2-ville.syrjala@linux.intel.com> <87woydgrra.fsf@anholt.net> <20180315174802.GZ5453@intel.com> <20180315180344.GA5453@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180315180344.GA5453@intel.com> User-Agent: Mutt/1.7.2 (2016-11-26) X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Mar 15, 2018 at 08:03:44PM +0200, Ville Syrjälä wrote: > On Thu, Mar 15, 2018 at 07:48:02PM +0200, Ville Syrjälä wrote: > > On Thu, Mar 15, 2018 at 10:42:17AM -0700, Eric Anholt wrote: > > > Ville Syrjala writes: > > > > > > > From: Ville Syrjälä > > > > > > > > To make life easier for drivers, let's have the core check that the > > > > requested pixel format is supported by at least one plane when creating > > > > a new framebuffer. > > > > > > > > This eases the burden on drivers by making sure they'll never get > > > > requests to create fbs with unsupported pixel formats. Thanks to the > > > > new .fb_modifier() hook this check can now be done whether the request > > > > specifies the modifier directly or driver has to deduce it from the > > > > gem bo tiling (or via any other method really). > > > > > > > > v0: Accept anyformat if the driver doesn't do planes (Eric) > > > > s/planes_have_format/any_plane_has_format/ (Eric) > > > > Check the modifier as well since we already have a function > > > > that does both > > > > v3: Don't do the check in the core since we may not know the > > > > modifier yet, instead export the function and let drivers > > > > call it themselves > > > > v4: Unexport the functiona and put the format_default check back > > > > since this will again be called by the core, ie. undo v3 ;) > > > > > > > > Cc: Eric Anholt > > > > Testcase: igt/kms_addfb_basic/expected-formats > > > > Signed-off-by: Ville Syrjälä > > > > --- > > > > drivers/gpu/drm/drm_framebuffer.c | 30 ++++++++++++++++++++++++++++++ > > > > 1 file changed, 30 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > > > > index 21d3d51eb261..e618a6b728d4 100644 > > > > --- a/drivers/gpu/drm/drm_framebuffer.c > > > > +++ b/drivers/gpu/drm/drm_framebuffer.c > > > > @@ -152,6 +152,26 @@ static int fb_plane_height(int height, > > > > return DIV_ROUND_UP(height, format->vsub); > > > > } > > > > > > > > +static bool any_plane_has_format(struct drm_device *dev, > > > > + u32 format, u64 modifier) > > > > +{ > > > > + struct drm_plane *plane; > > > > + > > > > + drm_for_each_plane(plane, dev) { > > > > + /* > > > > + * In case the driver doesn't really do > > > > + * planes we have to accept any format here. > > > > + */ > > > > + if (plane->format_default) > > > > + return true; > > > > + > > > > + if (drm_plane_check_pixel_format(plane, format, modifier) == 0) > > > > + return true; > > > > + } > > > > + > > > > + return false; > > > > +} > > > > > > This drm_plane_check_pixel_format() will always fail for VC4's SAND > > > modifiers or VC5's UIF modifiers, where we're using the middle 48 bits > > > as a bit of metadata (like how we have horizontal stride passed outside > > > of the modifier) and you can't list all of the possible values in an > > > array on the plane. > > > > Hmm. drm_atomic_plane_check() etc. call this thing as well. How does > > that manage to work currently? > > Maybe it doesn't. I added the modifier checks in > > commit 23163a7d4b032489d375099d56571371c0456980 > Author: Ville Syrjälä > AuthorDate: Fri Dec 22 21:22:30 2017 +0200 > Commit: Ville Syrjälä > CommitDate: Mon Feb 26 16:29:47 2018 +0200 > > drm: Check that the plane supports the request format+modifier combo > > Maybe that broke vc4? > > Hmm. So either we need to stop checking against the modifiers array and > rely purely or .format_mod_supported(), or we need to somehow get the > driver to reduce the modifier to its base form. I guess we could make > .fb_modifier() do that and call it also for addfb with modifiers. And > I'd need to undo some of the modifier[0] vs. deduced modifier changes > I did to framebuffer_check(), and we'd need to preserve the original > modifier in the request for .fb_create(). Oh, but that wouldn't allow > returning a non-base modifier from .fb_modifuer() for the !modifiers > case. This is turning slightly more tricky than I had hoped... > > I guess relying on .format_mod_supported() might be what we need to > do. Unfortunately it does mean that the .format_mod_supported() > implementations must be prepared for modifiers that were not > registered with the plane. Which does feel quite a bit more > fragile. And that last apporach would be annoying for i915. So I think the other apporach is better. Fortunately it seems your SAND modifiers aren't upstream yet so I didn't break them :) I had a quick look at your github and it looks like the first apporach would work. Something like this is what I had in mind. Loosk like you could plug in fourcc_mod_broadcom_mod() almost directly into .base_modifier(). diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index a5d1fc7e8a37..250f66d51c2c 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -552,6 +552,8 @@ int drm_mode_getplane(struct drm_device *dev, void *data, int drm_plane_check_pixel_format(struct drm_plane *plane, u32 format, u64 modifier) { + struct drm_device *dev = plane->dev; + u64 base_modifier; unsigned int i; for (i = 0; i < plane->format_count; i++) { @@ -564,8 +566,13 @@ int drm_plane_check_pixel_format(struct drm_plane *plane, if (!plane->modifier_count) return 0; + if (dev->mode_config.funcs->base_modifier) + base_modifier = dev->mode_config.funcs->base_modifier(dev, modifier); + else + base_modifier = modifier; + for (i = 0; i < plane->modifier_count; i++) { - if (modifier == plane->modifiers[i]) + if (base_modifier == plane->modifiers[i]) break; } if (i == plane->modifier_count) diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index c74aed292b58..2ff2438263ef 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -45,6 +45,19 @@ struct drm_display_mode; * involve drivers. */ struct drm_mode_config_funcs { + /** + * @base_modifier: + * + * Reduce the passed in modifier to its base form. This optional + * hook needs to be provided by any driver that embeds extra + * metadata within the format modifier. + * + * RETURNS: + * The base form of the modifier with any extra metadata + * cleared out. + */ + u64 (*base_modifier)(struct drm_device *dev, u64 modifier); + /** * @fb_modifier: * diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index f7bf4a48b1c3..5e26d2a5f6ab 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -489,8 +489,6 @@ enum drm_plane_type { * @format_types: array of formats supported by this plane * @format_count: number of formats supported * @format_default: driver hasn't supplied supported formats for the plane - * @modifiers: array of modifiers supported by this plane - * @modifier_count: number of modifiers supported * @old_fb: Temporary tracking of the old fb while a modeset is ongoing. Used by * drm_mode_set_config_internal() to implement correct refcounting. * @funcs: helper functions @@ -524,7 +522,15 @@ struct drm_plane { unsigned int format_count; bool format_default; + /** + * @modifiers: Array of modifiers supported by this plane. If + * the driver embeds any extra metadata within modifiers these + * modifiers must be in their base form. + */ uint64_t *modifiers; + /** + * @modifier_count: number of modifiers supported + */ unsigned int modifier_count; /**