From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C0C5E369969; Tue, 24 Feb 2026 09:03:40 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1771923820; cv=none; b=aUz9VNituQ0hbw0wx7jxMIyBhJd/u7grw7zfQdzZe4JgVfkzs9lQZQTLhPMrPgyOmlhy1K71PCMwnjtN+SOpQJGimVN47lSiQL9tgRK7AhdxSEOtkFsQoIcUQF09wkD3Y7gdszNu4EKcX7zyivnAYn/gJvEhMktuPQEq4Fz186Y= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1771923820; c=relaxed/simple; bh=F66MvL8Ia7cFuSm/xIyKma6BJNqk8VAV91xYEJfDrVg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=KJkQkc7DnbtjIuUklrd+33+xvoUDj/1Tw33w5rTYuU49veGl33riqGQgYtCojR/WUUDqehp+JDyyGQxpuYcaywryzpaofBrgUwnyvK31wa7e3LYTA3sCiD1tlu137SvFkVumJpeFLjtrw0n3RhNK2aM1HCYKirJHWYmhmtOCt74= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=EY0o15rc; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="EY0o15rc" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 18FFBC116D0; Tue, 24 Feb 2026 09:03:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1771923820; bh=F66MvL8Ia7cFuSm/xIyKma6BJNqk8VAV91xYEJfDrVg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=EY0o15rcNpeohXzKzHiT9RXLl5kMeW1SU7uJP4j5vdBXUrNRY6a5C+/OcAjng08uR Rl6JkyaqdyQznw5IFlCczaP7rRSb8iAqrPn5VodGC6D/C1Z16P2UZmpcXL3BtEl3ze uenoEDKvbAopscHS02nSxIqf2ehtR7UtybzG2acv1jVBwJ8MQ4Ic1c6hxkhoiQTLSk IYPylxg2vmsIcFlPlIsnXjJ+NOrY3JIC5tvlamDn1EO6oFOtX7+PvItdWwHjUn9V7+ FWNB14pxkA4x1NJNVz5TN6leROlqjT+1c+FYkRVroInPqD8Vumk+fEYE2ycDMpBREq E4Qsy059s6ndQ== Date: Tue, 24 Feb 2026 10:03:37 +0100 From: Maxime Ripard To: Jani Nikula Cc: Nicolas Frattaroli , Harry Wentland , Leo Li , Rodrigo Siqueira , Alex Deucher , Christian =?utf-8?B?S8O2bmln?= , David Airlie , Simona Vetter , Maarten Lankhorst , Thomas Zimmermann , Andrzej Hajda , Neil Armstrong , Robert Foss , Laurent Pinchart , Jonas Karlman , Jernej Skrabec , Sandy Huang , Heiko =?utf-8?Q?St=C3=BCbner?= , Andy Yan , Rodrigo Vivi , Joonas Lahtinen , Tvrtko Ursulin , Dmitry Baryshkov , Sascha Hauer , Rob Herring , Jonathan Corbet , kernel@collabora.com, amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org, intel-gfx@lists.freedesktop.org, intel-xe@lists.freedesktop.org, linux-doc@vger.kernel.org, Andri Yngvason , Werner Sembach , Marius Vlad Subject: Re: [PATCH v8 02/20] drm: Add new general DRM property "color format" Message-ID: <20260224-rustling-provocative-lemming-b2ed2f@houat> References: <20260216-color-format-v8-0-5722ce175dd5@collabora.com> <20260216-color-format-v8-2-5722ce175dd5@collabora.com> <3b5e5af4219671c5b4ffdcb09bd22679332244ac@intel.com> Precedence: bulk X-Mailing-List: linux-doc@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha384; protocol="application/pgp-signature"; boundary="36zlw2efj76bnmz3" Content-Disposition: inline In-Reply-To: <3b5e5af4219671c5b4ffdcb09bd22679332244ac@intel.com> --36zlw2efj76bnmz3 Content-Type: text/plain; protected-headers=v1; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Subject: Re: [PATCH v8 02/20] drm: Add new general DRM property "color format" MIME-Version: 1.0 Hi Jani, On Mon, Feb 23, 2026 at 06:17:23PM +0200, Jani Nikula wrote: > On Mon, 16 Feb 2026, Nicolas Frattaroli wrote: > > +/** > > + * enum drm_color_format_enum - color model description > > + * > > + * This enum is a high-level description of the component makeup of th= e image > > + * data. It says nothing about how the components are ordered or how m= any bits > > + * they take up (i.e. is unlike MEDIA_BUS_FMT\_ or DRM_FORMAT\_), but > > + * describes the type of components (Luminance-Chrominance vs. RGB) an= d the > > + * sub-sampling. > > + * > > + * &enum drm_color_format_enum makes statements about the same attribu= te of > > + * an image as the DRM_COLOR_FORMAT\_ bitfields do. Its purpose is to = inform > > + * choices made by display protocol specific implementations when it c= omes to > > + * translating it to e.g. &enum hdmi_colorspace or &enum dp_pixelforma= t, both > > + * of which also describe the same attribute of the image at the same = level of > > + * specificity. > > + * > > + * In precise terms, this enum describes a color model. It makes no st= atements > > + * about the primaries, gamma, or current phase of the moon used in co= nversion > > + * from one to the other. Furthermore, it also makes no statements abo= ut the > > + * order of components (e.g. RGB vs. BGR), their depth in bits, or the= ir binary > > + * packing. > > + */ > > +enum drm_color_format_enum { >=20 > The enum name should not have "enum" in it. That's just not a style > that's being used. >=20 > > + /** > > + * @DRM_COLOR_FORMAT_ENUM_AUTO: The choice of format is left up to the > > + * display protocol implementation. All implementations of the same > > + * display protocol (e.g. HDMI) are supposed to behave the same way, > > + * though display protocols may choose to behave differently compared= to > > + * each other (e.g. HDMI's "AUTO" does not have to match DP's "AUTO"). > > + * > > + * Implementations may rely on @DRM_COLOR_FORMAT_ENUM_AUTO to be fals= y. > > + */ > > + DRM_COLOR_FORMAT_ENUM_AUTO =3D 0, >=20 > Ditto for the enumeration names, no ENUM in them please. >=20 > > + > > + /** > > + * @DRM_COLOR_FORMAT_ENUM_RGB444: Image components are encoded as RGB > > + * values of equal resolution. > > + */ > > + DRM_COLOR_FORMAT_ENUM_RGB444, > > + > > + /** > > + * @DRM_COLOR_FORMAT_ENUM_YCBCR444: Image components are encoded as > > + * luminance and chrominance of equal resolution. > > + */ > > + DRM_COLOR_FORMAT_ENUM_YCBCR444, > > + > > + /** > > + * @DRM_COLOR_FORMAT_ENUM_YCBCR422: Image components are encoded as > > + * luminance and chrominance with the chrominance components having h= alf > > + * the horizontal resolution. > > + */ > > + DRM_COLOR_FORMAT_ENUM_YCBCR422, > > + > > + /** > > + * @DRM_COLOR_FORMAT_ENUM_YCBCR420: Image components are encoded as > > + * luminance and chrominance with the chrominance components having h= alf > > + * the horizontal and vertical resolution. > > + */ > > + DRM_COLOR_FORMAT_ENUM_YCBCR420, > > + > > + /** > > + * @DRM_COLOR_FORMAT_ENUM_NUM: The number of valid color format values > > + * in this enum. Itself not a valid color format. > > + */ > > + DRM_COLOR_FORMAT_ENUM_NUM, > > + > > + /** > > + * @DRM_COLOR_FORMAT_ENUM_INVALID: Error return value for conversion > > + * functions encountering unexpected inputs. > > + */ > > + DRM_COLOR_FORMAT_ENUM_INVALID =3D -EINVAL, >=20 > Please don't hide negative error codes inside enums. If you need to > return one from a function, please return the negative error code > directly instead. >=20 > > +}; > > + > > +/* > > + * Constants for specifying bit masks for e.g. providing a list of sup= ported > > + * color formats as a single integer. > > + */ > > +#define DRM_COLOR_FORMAT_RGB444 BIT(0) > > +#define DRM_COLOR_FORMAT_YCBCR444 BIT(1) > > +#define DRM_COLOR_FORMAT_YCBCR422 BIT(2) > > +#define DRM_COLOR_FORMAT_YCBCR420 BIT(3) >=20 > I don't think we should define both enum and mask. One or the > other. Moreover, now you have two independent definitions for the same > thing, with nothing to ensure they keep matching. It's a bug waiting to > happen. >=20 > I think the problem is that they were originally defined as bits even > though most places actually use them as single values only. It's > confusing. It would probably have been better to just use enums and > BIT(DRM_COLOR_FORMAT_*) where a mask is needed. >=20 > Maybe that's what should be done as the first step anyway. I largely agree with the sentiment, and can extend it to the HDMI_COLORSPACE used in drm_connector_hdmi_state. I've been working since yesterday on fixing that up to make Nicolas' life easier. I'll post it sometime today. Maxime --36zlw2efj76bnmz3 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iJUEABMJAB0WIQTkHFbLp4ejekA/qfgnX84Zoj2+dgUCaZ1paQAKCRAnX84Zoj2+ dhj+AYCJxRTvr7o7P0A0JR9ICn8LRGV7zoUZ6CpFzW+1A23VNzGrMHwlhRmfhyfX tIXqxK4BgKdTepytTJVLjkyW62yHxmDFSIGl3zjxOOEMlvvU+yKc/TvA/GTUDyqw nc1ZuwKhKg== =86U0 -----END PGP SIGNATURE----- --36zlw2efj76bnmz3--