From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 7C2BCCA0EEB for ; Tue, 19 Aug 2025 15:11:24 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 20C2110E613; Tue, 19 Aug 2025 15:11:24 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.b="ivn6Ou5G"; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by gabe.freedesktop.org (Postfix) with ESMTPS id ABEF210E616 for ; Tue, 19 Aug 2025 15:11:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1755616280; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=pvEEgBbNgsTwxehtfFeqqEpqIgwOhBrJLQl9HVJAuzk=; b=ivn6Ou5GdG5pvoKxaLYrxauqxz+og87mYke3Ops+sLRapSugH/eI7LXkvLydw8ai5pQsIC ud9u3hZik2eybL9nmZAEuz+eeqLx7UObxayFz29kbeA4XmkAmAYF5Grr3gsOFkmsId1mT7 nQFLjWa3VOSBCPhba9lbMgJSmt2ooBk= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-352-o1qC_r56M9Og-hOVGRqw0A-1; Tue, 19 Aug 2025 11:11:19 -0400 X-MC-Unique: o1qC_r56M9Og-hOVGRqw0A-1 X-Mimecast-MFC-AGG-ID: o1qC_r56M9Og-hOVGRqw0A_1755616278 Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-3b9dc5c2c7dso2949083f8f.1 for ; Tue, 19 Aug 2025 08:11:19 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1755616278; x=1756221078; h=in-reply-to:references:to:from:subject:cc:message-id:date :content-transfer-encoding:mime-version:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=ee3u3flTv1kn0zk1T2GKB/BxJ08k470D8YW1cLQnvsk=; b=LzhZHeGrLuNiRHdNhOu8uMRjQcwngR4U0Wj635471nSaUciXQVXIhFJ34BmpzmnaPi 736VmNjWEPgbzMMu1QvevABXYalOvUHLdNa8NTKjH/OHGRFYbN3YjdLkG3OLo7FN2h7R 04FBDmqLzo4yJvuKCE9jRiXZu8f2InnLenEsVLZ8xS++4mS2yjygVhCrH8iJS/ihGtaE kNnd5Y1YOeTvZ2YcVcPnHcwUj0lfKKxweOyV7v/uNuxIQYENRU4ng1unub7pv4zW4Mnz 0wBjdDj/vtFU5m7Fd/fmZifB7fGR8bEvELwzOGuwchq4eE59czzQoLenI3pTAn6ITDOQ tdTQ== X-Forwarded-Encrypted: i=1; AJvYcCVD+w94fE9tersLxnu9+M1Ssxyy5Z9QivxjVrdjtidfXYgLyH3mmGaQlN269SKItlPOHItdOVmQ@lists.freedesktop.org X-Gm-Message-State: AOJu0YzXBbkWIJGmcNMozmaR7bkEYwusr6U3RfjvkIVMJTcvQ9QdIEtv ifBDJTaLOsGKNmRNPqt4Jo6lWDUNQovrxTuai1ihLL7KSkZSeWKDlAOtHhIRgt2Oqz9gWT7Zo18 mYgZFWknFmrJ04a2snT1K3b6nrvxT1RaxTDdvgvjaZ9IHH4+tMDYG2qD9Z3yG5YXGfbI= X-Gm-Gg: ASbGnctkV4rduQyBVlR+SJrN19+xP8vgl82ZyxV5T4TmsxIGcjsWBX0l0IfTNc2W5g+ vuF7kQhHD50heC4UH60RdQkM+oEjywRV0ZcRajkdJ7ZkOsa0hsAkNMIi7WCIxtqlsr60m0VHGhu F5/of2sYxt+dlKqfO9V3iKtZJeKUpdy0SaKK9Wi5NBhFyIq8P5WaxgpI7JHgARnPuvYYhkV9DH6 GgA18IaLc9n5MQRS0znQFRjMWJbOStjWsPd+o8ZX7C5/t+L4GRY9T7jrvJqM4REouXBUgXu/BuL FS4S8LCek9PWboKI4sAw9z2eDb2hIHSuNk4s0vUGeQl4zg== X-Received: by 2002:a05:6000:200d:b0:3b7:8832:fdd5 with SMTP id ffacd0b85a97d-3c0eaf4f6dfmr2225048f8f.16.1755616277696; Tue, 19 Aug 2025 08:11:17 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEYoJJIKVUgdiScKbKjVAGpcWgHzca9rJLqk+FuKY/tpu+fr2yqbqHZ6pYqrduWZp1/thUOYQ== X-Received: by 2002:a05:6000:200d:b0:3b7:8832:fdd5 with SMTP id ffacd0b85a97d-3c0eaf4f6dfmr2225003f8f.16.1755616277086; Tue, 19 Aug 2025 08:11:17 -0700 (PDT) Received: from localhost ([2001:9e8:8986:8500:d724:cc1e:d6eb:bc50]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3c074879fcbsm4157877f8f.9.2025.08.19.08.11.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 19 Aug 2025 08:11:16 -0700 (PDT) Mime-Version: 1.0 Date: Tue, 19 Aug 2025 17:11:15 +0200 Message-Id: Cc: , , , , , , , , , , , , , , , , , , , , , , , , , , , "Daniel Stone" Subject: Re: [PATCH V11 06/47] drm/colorop: Add 1D Curve subtype From: "Sebastian Wick" To: "Alex Hung" , , X-Mailer: aerc 0.20.1 References: <20250815035047.3319284-1-alex.hung@amd.com> <20250815035047.3319284-7-alex.hung@amd.com> In-Reply-To: <20250815035047.3319284-7-alex.hung@amd.com> X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: BNjSr6AAfjyB1qLVRN_AnwVIyxr4137OdWuuxbM43lw_1755616278 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 X-BeenThere: amd-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces@lists.freedesktop.org Sender: "amd-gfx" On Fri Aug 15, 2025 at 5:49 AM CEST, Alex Hung wrote: > From: Harry Wentland > > Add a new drm_colorop with DRM_COLOROP_1D_CURVE with two subtypes: > DRM_COLOROP_1D_CURVE_SRGB_EOTF and DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF. > > Reviewed-by: Simon Ser > Reviewed-by: Louis Chauvet > Signed-off-by: Harry Wentland > Co-developed-by: Alex Hung > Signed-off-by: Alex Hung > Reviewed-by: Daniel Stone > Reviewed-by: Melissa Wen > --- > V9: Specify function names by _plane_ (Chaitanya Kumar Borah) > > v5: > - Add drm_get_colorop_curve_1d_type_name in header > - Add drm_colorop_init > - Set default curve > - Add kernel docs > > v4: > - Use drm_colorop_curve_1d_type_enum_list to get name (Pekka) > - Create separate init function for 1D curve > - Pass supported TFs into 1D curve init function > > drivers/gpu/drm/drm_atomic_uapi.c | 18 ++-- > drivers/gpu/drm/drm_colorop.c | 134 ++++++++++++++++++++++++++++++ > include/drm/drm_colorop.h | 63 ++++++++++++++ > 3 files changed, 210 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atom= ic_uapi.c > index ad2043f16268..52b5a9b5523e 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -650,11 +650,17 @@ static int drm_atomic_colorop_set_property(struct d= rm_colorop *colorop, > =09=09struct drm_colorop_state *state, struct drm_file *file_priv, > =09=09struct drm_property *property, uint64_t val) > { > -=09drm_dbg_atomic(colorop->dev, > -=09=09=09"[COLOROP:%d] unknown property [PROP:%d:%s]]\n", > -=09=09=09colorop->base.id, > -=09=09=09property->base.id, property->name); > -=09return -EINVAL; > +=09if (property =3D=3D colorop->curve_1d_type_property) { > +=09=09state->curve_1d_type =3D val; > +=09} else { > +=09=09drm_dbg_atomic(colorop->dev, > +=09=09=09 "[COLOROP:%d:%d] unknown property [PROP:%d:%s]]\n", > +=09=09=09 colorop->base.id, colorop->type, > +=09=09=09 property->base.id, property->name); > +=09=09return -EINVAL; > +=09} > + > +=09return 0; > } > =20 > static int > @@ -664,6 +670,8 @@ drm_atomic_colorop_get_property(struct drm_colorop *c= olorop, > { > =09if (property =3D=3D colorop->type_property) { > =09=09*val =3D colorop->type; > +=09} else if (property =3D=3D colorop->curve_1d_type_property) { > +=09=09*val =3D state->curve_1d_type; > =09} else { > =09=09return -EINVAL; > =09} > diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.= c > index 1459a28c7e7b..6fbc3c284d33 100644 > --- a/drivers/gpu/drm/drm_colorop.c > +++ b/drivers/gpu/drm/drm_colorop.c > @@ -31,6 +31,123 @@ > =20 > #include "drm_crtc_internal.h" > =20 > +static const struct drm_prop_enum_list drm_colorop_type_enum_list[] =3D = { > +=09{ DRM_COLOROP_1D_CURVE, "1D Curve" }, > +}; > + > +static const char * const colorop_curve_1d_type_names[] =3D { > +=09[DRM_COLOROP_1D_CURVE_SRGB_EOTF] =3D "sRGB EOTF", > +=09[DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF] =3D "sRGB Inverse EOTF", > +}; > + > + > +/* Init Helpers */ > + > +static int drm_plane_colorop_init(struct drm_device *dev, struct drm_col= orop *colorop, > +=09=09=09 struct drm_plane *plane, enum drm_colorop_type type) > +{ > +=09struct drm_mode_config *config =3D &dev->mode_config; > +=09struct drm_property *prop; > +=09int ret =3D 0; > + > +=09ret =3D drm_mode_object_add(dev, &colorop->base, DRM_MODE_OBJECT_COLO= ROP); > +=09if (ret) > +=09=09return ret; > + > +=09colorop->base.properties =3D &colorop->properties; > +=09colorop->dev =3D dev; > +=09colorop->type =3D type; > +=09colorop->plane =3D plane; > + > +=09list_add_tail(&colorop->head, &config->colorop_list); > +=09colorop->index =3D config->num_colorop++; > + > +=09/* add properties */ > + > +=09/* type */ > +=09prop =3D drm_property_create_enum(dev, > +=09=09=09=09=09DRM_MODE_PROP_IMMUTABLE, > +=09=09=09=09=09"TYPE", drm_colorop_type_enum_list, > +=09=09=09=09=09ARRAY_SIZE(drm_colorop_type_enum_list)); > + > +=09if (!prop) > +=09=09return -ENOMEM; > + > +=09colorop->type_property =3D prop; > + > +=09drm_object_attach_property(&colorop->base, > +=09=09=09=09 colorop->type_property, > +=09=09=09=09 colorop->type); > + > +=09return ret; > +} > + > +/** > + * drm_plane_colorop_curve_1d_init - Initialize a DRM_COLOROP_1D_CURVE > + * > + * @dev: DRM device > + * @colorop: The drm_colorop object to initialize > + * @plane: The associated drm_plane > + * @supported_tfs: A bitfield of supported drm_plane_colorop_curve_1d_in= it enum values, > + * created using BIT(curve_type) and combined with the O= R '|' > + * operator. > + * @return zero on success, -E value on failure > + */ > +int drm_plane_colorop_curve_1d_init(struct drm_device *dev, struct drm_c= olorop *colorop, > +=09=09=09=09 struct drm_plane *plane, u64 supported_tfs) > +{ > +=09struct drm_prop_enum_list enum_list[DRM_COLOROP_1D_CURVE_COUNT]; > +=09int i, len; > + > +=09struct drm_property *prop; > +=09int ret; > + > +=09if (!supported_tfs) { > +=09=09drm_err(dev, > +=09=09=09"No supported TFs for new 1D curve colorop on [PLANE:%d:%s]\n", > +=09=09=09plane->base.id, plane->name); > +=09=09return -EINVAL; > +=09} > + > +=09if ((supported_tfs & -BIT(DRM_COLOROP_1D_CURVE_COUNT)) !=3D 0) { > +=09=09drm_err(dev, "Unknown TF provided on [PLANE:%d:%s]\n", > +=09=09=09plane->base.id, plane->name); > +=09=09return -EINVAL; > +=09} > + > +=09ret =3D drm_plane_colorop_init(dev, colorop, plane, DRM_COLOROP_1D_CU= RVE); > +=09if (ret) > +=09=09return ret; > + > +=09len =3D 0; > +=09for (i =3D 0; i < DRM_COLOROP_1D_CURVE_COUNT; i++) { > +=09=09if ((supported_tfs & BIT(i)) =3D=3D 0) > +=09=09=09continue; > + > +=09=09enum_list[len].type =3D i; > +=09=09enum_list[len].name =3D colorop_curve_1d_type_names[i]; > +=09=09len++; > +=09} > + > +=09if (WARN_ON(len <=3D 0)) > +=09=09return -EINVAL; > + > + > +=09/* initialize 1D curve only attribute */ > +=09prop =3D drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, "CURVE_1= D_TYPE", > +=09=09=09=09=09enum_list, len); > +=09if (!prop) > +=09=09return -ENOMEM; > + > +=09colorop->curve_1d_type_property =3D prop; > +=09drm_object_attach_property(&colorop->base, colorop->curve_1d_type_pro= perty, > +=09=09=09=09 enum_list[0].type); > +=09drm_colorop_reset(colorop); > + > +=09return 0; > +} > +EXPORT_SYMBOL(drm_plane_colorop_curve_1d_init); > + > static void __drm_atomic_helper_colorop_duplicate_state(struct drm_color= op *colorop, > =09=09=09=09=09=09=09struct drm_colorop_state *state) > { > @@ -70,7 +187,16 @@ void drm_colorop_atomic_destroy_state(struct drm_colo= rop *colorop, > static void __drm_colorop_state_reset(struct drm_colorop_state *colorop_= state, > =09=09=09=09 struct drm_colorop *colorop) > { > +=09u64 val; > + > =09colorop_state->colorop =3D colorop; > + > +=09if (colorop->curve_1d_type_property) { > +=09=09drm_object_property_get_default_value(&colorop->base, > +=09=09=09=09=09=09colorop->curve_1d_type_property, > +=09=09=09=09=09=09&val); > +=09=09colorop_state->curve_1d_type =3D val; > +=09} > } > =20 > /** > @@ -114,3 +240,11 @@ const char *drm_get_colorop_type_name(enum drm_color= op_type type) > =20 > =09return colorop_type_name[type]; > } > + > +const char *drm_get_colorop_curve_1d_type_name(enum drm_colorop_curve_1d= _type type) > +{ > +=09if (WARN_ON(type >=3D ARRAY_SIZE(colorop_curve_1d_type_names))) > +=09=09return "unknown"; > + > +=09return colorop_curve_1d_type_names[type]; > +} > diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h > index 9c9698545f63..fa167e642e0d 100644 > --- a/include/drm/drm_colorop.h > +++ b/include/drm/drm_colorop.h > @@ -31,6 +31,42 @@ > #include > #include > =20 > + > +/** > + * enum drm_colorop_curve_1d_type - type of 1D curve > + * > + * Describes a 1D curve to be applied by the DRM_COLOROP_1D_CURVE coloro= p. > + */ > +enum drm_colorop_curve_1d_type { > +=09/** > +=09 * @DRM_COLOROP_1D_CURVE_SRGB_EOTF: > +=09 * > +=09 * enum string "sRGB EOTF" > +=09 * > +=09 * sRGB piece-wise electro-optical transfer function. Transfer > +=09 * characteristics as defined by IEC 61966-2-1 sRGB. Equivalent > +=09 * to H.273 TransferCharacteristics code point 13 with > +=09 * MatrixCoefficients set to 0. > +=09 */ We user space folks have been convinced at this point that the sRGB EOTF is actually gamma 2.2, and not the piece-wise function. Now, if the hardware is actually the piece-wise, then that's what should be exposed, but I'm a bit unsure if we should do that under the name sRGB EOTF. Maybe any other alternative is even worse. At least this is clearly documented to be the piece-wise function, so it's only about the naming. > +=09DRM_COLOROP_1D_CURVE_SRGB_EOTF, > + > +=09/** > +=09 * @DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF: > +=09 * > +=09 * enum string "sRGB Inverse EOTF" > +=09 * > +=09 * The inverse of &DRM_COLOROP_1D_CURVE_SRGB_EOTF > +=09 */ > +=09DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF, > + > +=09/** > +=09 * @DRM_COLOROP_1D_CURVE_COUNT: > +=09 * > +=09 * enum value denoting the size of the enum > +=09 */ > +=09DRM_COLOROP_1D_CURVE_COUNT > +}; > + > /** > * struct drm_colorop_state - mutable colorop state > */ > @@ -46,6 +82,13 @@ struct drm_colorop_state { > =09 * information. > =09 */ > =20 > +=09/** > +=09 * @curve_1d_type: > +=09 * > +=09 * Type of 1D curve. > +=09 */ > +=09enum drm_colorop_curve_1d_type curve_1d_type; > + > =09/** @state: backpointer to global drm_atomic_state */ > =09struct drm_atomic_state *state; > }; > @@ -127,6 +170,14 @@ struct drm_colorop { > =09 * this color operation. The type is enum drm_colorop_type. > =09 */ > =09struct drm_property *type_property; > + > +=09/** > +=09 * @curve_1d_type_property: > +=09 * > +=09 * Sub-type for DRM_COLOROP_1D_CURVE type. > +=09 */ > +=09struct drm_property *curve_1d_type_property; > + > }; > =20 > #define obj_to_colorop(x) container_of(x, struct drm_colorop, base) > @@ -151,6 +202,9 @@ static inline struct drm_colorop *drm_colorop_find(st= ruct drm_device *dev, > =09return mo ? obj_to_colorop(mo) : NULL; > } > =20 > +int drm_plane_colorop_curve_1d_init(struct drm_device *dev, struct drm_c= olorop *colorop, > +=09=09=09=09 struct drm_plane *plane, u64 supported_tfs); > + > struct drm_colorop_state * > drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colorop); > =20 > @@ -191,4 +245,13 @@ static inline unsigned int drm_colorop_index(const s= truct drm_colorop *colorop) > */ > const char *drm_get_colorop_type_name(enum drm_colorop_type type); > =20 > +/** > + * drm_get_colorop_curve_1d_type_name - return a string for 1D curve typ= e > + * @type: 1d curve type to compute name of > + * > + * In contrast to the other drm_get_*_name functions this one here retur= ns a > + * const pointer and hence is threadsafe. > + */ > +const char *drm_get_colorop_curve_1d_type_name(enum drm_colorop_curve_1d= _type type); > + > #endif /* __DRM_COLOROP_H__ */