From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 1/3] drm: Trust format_mod_supported() when it OKs a plane modifier. Date: Mon, 19 Mar 2018 18:33:33 +0200 Message-ID: <20180319163333.GO5453@intel.com> References: <20180316220435.31416-1-eric@anholt.net> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id F032C6E5EA for ; Mon, 19 Mar 2018 16:33:37 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20180316220435.31416-1-eric@anholt.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Eric Anholt Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org T24gRnJpLCBNYXIgMTYsIDIwMTggYXQgMDM6MDQ6MzNQTSAtMDcwMCwgRXJpYyBBbmhvbHQgd3Jv dGU6Cj4gRm9yIHBhcmFtZXRlcml6ZWQgbW9kaWZpZXJzIChCcm9hZGNvbSdzIFNBTkQgYW5kIFVJ RiksIHdlIG5lZWQgdG8KPiBhbGxvdyB0aGUgcGFyYW1ldGVyIGZpZWxkcyB0byBiZSBmaWxsZWQg aW4sIHdoaWxlIGV4cG9zaW5nIG9ubHkgdGhlCj4gdmFyaWFudCBvZiB0aGUgbW9kaWZpZXIgd2l0 aCB0aGUgcGFyYW1ldGVyIHVuZmlsbGVkIGluIHRoZSBpbnRlcm5hbAo+IGFycmF5cyBhbmQgdGhl IGZvcm1hdCBibG9iLgo+IAo+IFNpZ25lZC1vZmYtYnk6IEVyaWMgQW5ob2x0IDxlcmljQGFuaG9s dC5uZXQ+Cj4gQ2M6IFZpbGxlIFN5cmrDpGzDpCA8dmlsbGUuc3lyamFsYUBsaW51eC5pbnRlbC5j b20+Cj4gLS0tCj4gIGRyaXZlcnMvZ3B1L2RybS9kcm1fcGxhbmUuYyB8IDIzICsrKysrKysrKysr Ky0tLS0tLS0tLS0tCj4gIGluY2x1ZGUvZHJtL2RybV9wbGFuZS5oICAgICB8ICA1ICsrKystCj4g IDIgZmlsZXMgY2hhbmdlZCwgMTYgaW5zZXJ0aW9ucygrKSwgMTIgZGVsZXRpb25zKC0pCj4gCj4g ZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9kcm1fcGxhbmUuYyBiL2RyaXZlcnMvZ3B1L2Ry bS9kcm1fcGxhbmUuYwo+IGluZGV4IDQ2ZmJkMDE5YTMzNy4uNWJiNTAxZjFhYWU4IDEwMDY0NAo+ IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9kcm1fcGxhbmUuYwo+ICsrKyBiL2RyaXZlcnMvZ3B1L2Ry bS9kcm1fcGxhbmUuYwo+IEBAIC01NjEsMTkgKzU2MSwyMCBAQCBpbnQgZHJtX3BsYW5lX2NoZWNr X3BpeGVsX2Zvcm1hdChzdHJ1Y3QgZHJtX3BsYW5lICpwbGFuZSwKPiAgCWlmIChpID09IHBsYW5l LT5mb3JtYXRfY291bnQpCj4gIAkJcmV0dXJuIC1FSU5WQUw7Cj4gIAo+IC0JaWYgKCFwbGFuZS0+ bW9kaWZpZXJfY291bnQpCj4gLQkJcmV0dXJuIDA7Cj4gKwlpZiAocGxhbmUtPmZ1bmNzLT5mb3Jt YXRfbW9kX3N1cHBvcnRlZCkgewo+ICsJCWlmICghcGxhbmUtPmZ1bmNzLT5mb3JtYXRfbW9kX3N1 cHBvcnRlZChwbGFuZSwgZm9ybWF0LCBtb2RpZmllcikpCj4gKwkJCXJldHVybiAtRUlOVkFMOwo+ ICsJfSBlbHNlIHsKPiArCQlpZiAoIXBsYW5lLT5tb2RpZmllcl9jb3VudCkKPiArCQkJcmV0dXJu IDA7Cj4gIAo+IC0JZm9yIChpID0gMDsgaSA8IHBsYW5lLT5tb2RpZmllcl9jb3VudDsgaSsrKSB7 Cj4gLQkJaWYgKG1vZGlmaWVyID09IHBsYW5lLT5tb2RpZmllcnNbaV0pCj4gLQkJCWJyZWFrOwo+ ICsJCWZvciAoaSA9IDA7IGkgPCBwbGFuZS0+bW9kaWZpZXJfY291bnQ7IGkrKykgewo+ICsJCQlp ZiAobW9kaWZpZXIgPT0gcGxhbmUtPm1vZGlmaWVyc1tpXSkKPiArCQkJCWJyZWFrOwo+ICsJCX0K PiArCQlpZiAoaSA9PSBwbGFuZS0+bW9kaWZpZXJfY291bnQpCj4gKwkJCXJldHVybiAtRUlOVkFM Owo+ICAJfQo+IC0JaWYgKGkgPT0gcGxhbmUtPm1vZGlmaWVyX2NvdW50KQo+IC0JCXJldHVybiAt RUlOVkFMOwo+IC0KPiAtCWlmIChwbGFuZS0+ZnVuY3MtPmZvcm1hdF9tb2Rfc3VwcG9ydGVkICYm Cj4gLQkgICAgIXBsYW5lLT5mdW5jcy0+Zm9ybWF0X21vZF9zdXBwb3J0ZWQocGxhbmUsIGZvcm1h dCwgbW9kaWZpZXIpKQo+IC0JCXJldHVybiAtRUlOVkFMOwo+ICAKPiAgCXJldHVybiAwOwo+ICB9 Cj4gZGlmZiAtLWdpdCBhL2luY2x1ZGUvZHJtL2RybV9wbGFuZS5oIGIvaW5jbHVkZS9kcm0vZHJt X3BsYW5lLmgKPiBpbmRleCBmN2JmNGE0OGIxYzMuLjZiMWI1MTY0NWY3NSAxMDA2NDQKPiAtLS0g YS9pbmNsdWRlL2RybS9kcm1fcGxhbmUuaAo+ICsrKyBiL2luY2x1ZGUvZHJtL2RybV9wbGFuZS5o Cj4gQEAgLTQyMCw3ICs0MjAsMTAgQEAgc3RydWN0IGRybV9wbGFuZV9mdW5jcyB7Cj4gIAkgKiBU aGlzIG9wdGlvbmFsIGhvb2sgaXMgdXNlZCBmb3IgdGhlIERSTSB0byBkZXRlcm1pbmUgaWYgdGhl IGdpdmVuCj4gIAkgKiBmb3JtYXQvbW9kaWZpZXIgY29tYmluYXRpb24gaXMgdmFsaWQgZm9yIHRo ZSBwbGFuZS4gVGhpcyBhbGxvd3MgdGhlCj4gIAkgKiBEUk0gdG8gZ2VuZXJhdGUgdGhlIGNvcnJl Y3QgZm9ybWF0IGJpdG1hc2sgKHdoaWNoIGZvcm1hdHMgYXBwbHkgdG8KPiAtCSAqIHdoaWNoIG1v ZGlmaWVyKS4KPiArCSAqIHdoaWNoIG1vZGlmaWVyKSwgYW5kIHRvIHZhbGRpYXRlIG1vZGlmaWVy cyBhdCBhdG9taWNfY2hlY2sgdGltZS4KPiArCSAqCj4gKwkgKiBJZiBub3QgcHJlc2VudCwgdGhl biBhbnkgbW9kaWZpZXIgaW4gdGhlIHBsYW5lJ3MgbW9kaWZpZXIKPiArCSAqIGxpc3QgaXMgYWxs b3dlZCB3aXRoIGFueSBvZiB0aGUgcGxhbmUncyBmb3JtYXRzLgoKVGhpcyBjZXJ0YWlubHkgYXZv aWRzIHRoZSB2ZnVuYyBwcm9maWxlcmF0aW9uIHRoYXQgdXNpbmcgdGhlCm1vZGlmaWVycyBsaXN0 IGNhdXNlcy4gU28geWVhaCBJIGRvIGxpa2UgaXQgZnJvbSB0aGF0IGFuZ2xlLgoKSXQgZG9lcyBw cm9tb3RlIC5mb3JtYXRfbW9kX3N1cHBvcnRlZCgpIHRvIGEgbW9yZSBwcm9taW5lbnQgcm9sZS4g TXkKb3JpZ2luYWwgaWRlYSBmb3IgLmZvcm1hdF9tb2Rfc3VwcG9ydGVkKCkgd2FzIHRoYXQgaXQg Y291bGQgYmUgdmVyeQpsaWdodHdlaWdodDsganVzdCByZWplY3QgdGhlIGV4Y2VwdGlvbmFsIGNh c2VzIGFuZCBhc3N1bWUgZXZlcnl0aGluZwplbHNlIGlzIGZpbmUuIFdpdGggdGhlIGNoYW5nZSBv ZiByb2xlIEkgdGhpbmsgdGhlIGRvY3Mgc2hvdWxkIG1ha2UgaXQKdmVyeSBjbGVhciB0aGF0IF9h bnlfIG1vZGlmaWVyIGNhbiBiZSBwYXNzZWQgdG8gdGhpcyBmdW5jdGlvbiBkaXJlY3RseQpmcm9t IHVzZXJzcGFjZS4gVGhlIGRyaXZlciBtdXN0IGhhbmRsZSB0aGF0IGdyYWNlZnVsbHkgYW5kIHdp dGhvdXQKc3Bld2luZyBhbnkgbm9uLWRlYnVnIHN0dWZmIHRvIGRtZXNnLgoKQWxzbyBJIGRlZmlu aXRlbHkgbmVlZCB0byBjaGFuZ2UgaTkxNS4gQ3VycmVudGx5IHdlJ3JlIHVzaW5nIHRoZQptb2Rp ZmllciBsaXN0IGFzIHRoZSBwcmltYXJ5IG1lYW5zIGJ5IHdoaWNoIHdlIGZpbHRlciBvdXQgdW5z dXBwb3J0ZWQKbW9kaWZpZXJzIGZvciBwbGFuZXMsIGFuZCAuZm9ybWF0X21vZF9zdXBwb3J0KCkg anVzdCBzZXJ2ZXMgYW4Kc3VwcG9ydGluZyByb2xlIGZ1cnRoZXIgcmVtb3ZpbmcgYW55IGZvcm1h dCttb2QgY29tYmluYXRpb24gdGhhdCBpc24ndAp2YWxpZC4gU28gSSBuZWVkIHRvIG1ha2UgLmZv cm1hdF9tb2Rfc3VwcG9ydGVkKCkgdGhlIGxlYWQgYWN0b3IgaGVyZQphbmQgdGhlIG1vZGlmaWVy IGxpc3QganVzdCBzZXJ2ZXMgYXMgYSBzdGFydGluZyBwb2ludCB3aGljaCB3ZSBmaWx0ZXIKZG93 biB0byBzb21ldGhpbmcgd2UgY2FuIHJlcG9ydCB2aWEgdGhlIElOX0ZPUk1BVFMgYmxvYmlmaWVy LgoKSG1tLiBBbmQgbm93IHRoZSBxdWVzdGlvbiBiZWNvbWVzIHNob3VsZCB3ZSBtYWtlIC5mb3Jt YXRfbW9kX3N1cHBvcnRlZCgpCnRha2Ugb3ZlciB0aGUgcm9sZSBvZiB2YWxpZGF0aW5nIHRoZSBw aXhlbCBmb3JtYXQgYXMgd2VsbCBmcm9tIHRoZQpwbGFuZSdzIGZvcm1hdCBsaXN0PyBJdCB3b3Vs ZCBjZXJ0YWlubHkgYmUgbW9yZSBpbiBsaW5lIHdpdGggdGhlCmFwcHJvYWNoIHdlJ3JlIG5vdyBn b2luZyB0byB0YWtlIHdpdGggbW9kaWZpZXJzLgoKLS0gClZpbGxlIFN5cmrDpGzDpApJbnRlbCBP VEMKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRl dmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8v bGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935451AbeCSQdl (ORCPT ); Mon, 19 Mar 2018 12:33:41 -0400 Received: from mga01.intel.com ([192.55.52.88]:43291 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933942AbeCSQdh (ORCPT ); Mon, 19 Mar 2018 12:33:37 -0400 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,331,1517904000"; d="scan'208";a="26995092" Date: Mon, 19 Mar 2018 18:33:33 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Eric Anholt Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/3] drm: Trust format_mod_supported() when it OKs a plane modifier. Message-ID: <20180319163333.GO5453@intel.com> References: <20180316220435.31416-1-eric@anholt.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180316220435.31416-1-eric@anholt.net> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 16, 2018 at 03:04:33PM -0700, Eric Anholt wrote: > For parameterized modifiers (Broadcom's SAND and UIF), we need to > allow the parameter fields to be filled in, while exposing only the > variant of the modifier with the parameter unfilled in the internal > arrays and the format blob. > > Signed-off-by: Eric Anholt > Cc: Ville Syrjälä > --- > drivers/gpu/drm/drm_plane.c | 23 ++++++++++++----------- > include/drm/drm_plane.h | 5 ++++- > 2 files changed, 16 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 46fbd019a337..5bb501f1aae8 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -561,19 +561,20 @@ int drm_plane_check_pixel_format(struct drm_plane *plane, > if (i == plane->format_count) > return -EINVAL; > > - if (!plane->modifier_count) > - return 0; > + if (plane->funcs->format_mod_supported) { > + if (!plane->funcs->format_mod_supported(plane, format, modifier)) > + return -EINVAL; > + } else { > + if (!plane->modifier_count) > + return 0; > > - for (i = 0; i < plane->modifier_count; i++) { > - if (modifier == plane->modifiers[i]) > - break; > + for (i = 0; i < plane->modifier_count; i++) { > + if (modifier == plane->modifiers[i]) > + break; > + } > + if (i == plane->modifier_count) > + return -EINVAL; > } > - if (i == plane->modifier_count) > - return -EINVAL; > - > - if (plane->funcs->format_mod_supported && > - !plane->funcs->format_mod_supported(plane, format, modifier)) > - return -EINVAL; > > return 0; > } > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index f7bf4a48b1c3..6b1b51645f75 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -420,7 +420,10 @@ struct drm_plane_funcs { > * This optional hook is used for the DRM to determine if the given > * format/modifier combination is valid for the plane. This allows the > * DRM to generate the correct format bitmask (which formats apply to > - * which modifier). > + * which modifier), and to valdiate modifiers at atomic_check time. > + * > + * If not present, then any modifier in the plane's modifier > + * list is allowed with any of the plane's formats. This certainly avoids the vfunc profileration that using the modifiers list causes. So yeah I do like it from that angle. It does promote .format_mod_supported() to a more prominent role. My original idea for .format_mod_supported() was that it could be very lightweight; just reject the exceptional cases and assume everything else is fine. With the change of role I think the docs should make it very clear that _any_ modifier can be passed to this function directly from userspace. The driver must handle that gracefully and without spewing any non-debug stuff to dmesg. Also I definitely need to change i915. Currently we're using the modifier list as the primary means by which we filter out unsupported modifiers for planes, and .format_mod_support() just serves an supporting role further removing any format+mod combination that isn't valid. So I need to make .format_mod_supported() the lead actor here and the modifier list just serves as a starting point which we filter down to something we can report via the IN_FORMATS blobifier. Hmm. And now the question becomes should we make .format_mod_supported() take over the role of validating the pixel format as well from the plane's format list? It would certainly be more in line with the approach we're now going to take with modifiers. -- Ville Syrjälä Intel OTC