From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 Date: Tue, 3 Feb 2015 16:36:17 +0100 Message-ID: <20150203153614.GJ15068@ulmo.nvidia.com> References: <1422550881-1002-1-git-send-email-daniel.vetter@ffwll.ch> <1422634103-19665-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1827607314==" Return-path: In-Reply-To: <1422634103-19665-1-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: Tvrtko Ursulin , Michel =?utf-8?Q?D=C3=A4nzer?= , DRI Development , Laurent Pinchart , Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org --===============1827607314== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="u3bvv0EcKsvvYeex" Content-Disposition: inline --u3bvv0EcKsvvYeex Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jan 30, 2015 at 05:08:23PM +0100, Daniel Vetter wrote: > From: Rob Clark >=20 > In DRM/KMS we are lacking a good way to deal with tiled/compressed > formats. Especially in the case of dmabuf/prime buffer sharing, where > we cannot always rely on under-the-hood flags passed to driver specific > gem-create ioctl to pass around these extra flags. >=20 > The proposal is to add a per-plane format modifier. This allows to, if > necessary, use different tiling patters for sub-sampled planes, etc. "patterns". Alternatively perhaps "modes", which is how I've heard it referred to most commonly. > The format modifiers are added at the end of the ioctl struct, so for > legacy userspace it will be zero padded. >=20 > v1: original > v1.5: increase modifier to 64b >=20 > v2: Incorporate review comments from the big thread, plus a few more. >=20 > - Add a getcap so that userspace doesn't have to jump through hoops. > - Allow modifiers only when a flag is set. That way drivers know when > they're dealing with old userspace and need to fish out e.g. tiling > from other information. > - After rolling out checks for ->modifier to all drivers I've decided > that this is way too fragile and needs an explicit opt-in flag. So > do that instead. > - Add a define (just for documentation really) for the "NONE" > modifier. Imo we don't need to add mask #defines since drivers > really should only do exact matches against values defined with > fourcc_mod_code. > - Drop the Samsung tiling modifier on Rob's request since he's not yet > sure whether that one is accurate. >=20 > v3: > - Also add a new ->modifier[] array to struct drm_framebuffer and fill > it in drm_helper_mode_fill_fb_struct. Requested by Tvrtko Uruslin. > - Remove TODO in comment and add code comment that modifiers should be > properly documented, requested by Rob. >=20 > v4: Balance parens, spotted by Tvrtko. >=20 > Cc: Rob Clark > Cc: Tvrtko Ursulin > Cc: Laurent Pinchart > Cc: Daniel Stone > Cc: Ville Syrj=C3=A4l=C3=A4 > Cc: Michel D=C3=A4nzer > Signed-off-by: Rob Clark (v1.5) > Reviewed-by: Rob Clark > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/drm_crtc.c | 14 +++++++++++++- > drivers/gpu/drm/drm_crtc_helper.c | 1 + > drivers/gpu/drm/drm_ioctl.c | 3 +++ > include/drm/drm_crtc.h | 4 ++++ > include/uapi/drm/drm.h | 1 + > include/uapi/drm/drm_fourcc.h | 32 ++++++++++++++++++++++++++++++++ > include/uapi/drm/drm_mode.h | 9 +++++++++ > 7 files changed, 63 insertions(+), 1 deletion(-) Also as discussed on IRC, I think this would be better in a non-DRM specific header so that we can have a central, cross-subsystem authority. > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h > index 646ae5f39f42..622109677747 100644 > --- a/include/uapi/drm/drm_fourcc.h > +++ b/include/uapi/drm/drm_fourcc.h > @@ -132,4 +132,36 @@ > #define DRM_FORMAT_YUV444 fourcc_code('Y', 'U', '2', '4') /* non-subsamp= led Cb (1) and Cr (2) planes */ > #define DRM_FORMAT_YVU444 fourcc_code('Y', 'V', '2', '4') /* non-subsamp= led Cr (1) and Cb (2) planes */ > =20 > + Possibly unintented extra blank line? > +/* > + * Format Modifiers: > + * > + * Format modifiers describe, typically, a re-ordering or modification > + * of the data in a plane of an FB. This can be used to express tiled/ > + * swizzled formats, or compression, or a combination of the two. > + * > + * The upper 8 bits of the format modifier are a vendor-id as assigned > + * below. The lower 56 bits are assigned as vendor sees fit. > + */ > + > +/* Vendor Ids: */ > +#define DRM_FORMAT_MOD_NONE 0 > +#define DRM_FORMAT_MOD_VENDOR_INTEL 0x01 > +#define DRM_FORMAT_MOD_VENDOR_AMD 0x02 > +#define DRM_FORMAT_MOD_VENDOR_NV 0x03 I think this should be NVIDIA for consistency with other naming in the kernel, at least on Tegra. Otherwise: Reviewed-by: Thierry Reding --u3bvv0EcKsvvYeex Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJU0OruAAoJEN0jrNd/PrOh9ggP/Ruia6dBebeQY6c5du1cI1tY kzCjhNduccfqnAbFrYra7Xakxb3N467DedCvjFh8VdjKuWA6pKTi2o3OGxrcwm7C SJo9YLhH3YghIfLjI5J+M/Xx6X6aDd6kLXT7xx0Y9JLkMUztv/Ec1aTqqtYbxVGC Woy9CE+2MNPZ926emDDRBGF566lehqsn5bk/YXBRJ41hIXhhMY6BIJVgycOM4WCe Sj6ElzV3spUijQGWuS9x7QmetCTfg3+JLGooqTWQ4acKra20Er7lttOVCC1jXPT4 IAO1czZQvnN30ytpXS1kmAcJLGQinj20MnWCvrJqrj9HpdhBRGHQT+2+GkN/4VYK TS8+v/9w9PVXhqRdEyZLzx8eJ3Oo7TzXgLUAw2z067qA2XTcJLllYm/ubzT2TPme sh8HC0Ghn1fHjluDFkQMZ+aBTA5+PpL3u+6Xn8tYz98pu/swlTC0CC2nQkaSw4Gf wDIZgd73nY0iE6X7LM+WU74vQjdmfBFPueXwhzVWjwKeaZ1Vf6eQOKiXesguKJ6u zss7QDcJn6l0LeIQx0NSNIyhrECOyjWL5zGaQlsikbdUiTmhPHo5V+4jyS6diFiG 673ETTt26+FqByezqmwFl4dZE7/Azk8+5bKpDLg435R50bB5UTl5Me7naPrBKhVE P8yqXM3pXLg7O3s6t22i =3fnj -----END PGP SIGNATURE----- --u3bvv0EcKsvvYeex-- --===============1827607314== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============1827607314==--