On 2023-10-20 06:17, Pekka Paalanen wrote:
> On Thu, 19 Oct 2023 10:56:29 -0400
> Harry Wentland <harry.wentland@xxxxxxx> wrote:
>
>> On 2023-09-13 07:29, Pekka Paalanen wrote:
>>> On Fri, 8 Sep 2023 11:02:26 -0400
>>> Harry Wentland <harry.wentland@xxxxxxx> wrote:
>>>
>>>> Signed-off-by: Harry Wentland <harry.wentland@xxxxxxx>
>
> ...
>
>>>> +COLOR_PIPELINE Plane Property
>>>> +=============================
>>>> +
>>>> +Because we don't have existing KMS color properties in the pre-blending
>>>> +portion of display pipelines (i.e. on drm_planes) we are introducing
>>>> +color pipelines here first. Eventually we'll want to use the same
>>>> +concept for the post-blending portion, i.e. drm_crtcs.
>>>
>>> This paragraph might fit better in a cover letter.
>>>
>>>> +
>>>> +Color Pipelines are created by a driver and advertised via a new
>>>> +COLOR_PIPELINE enum property on each plane. Values of the property
>>>> +always include '0', which is the default and means all color processing
>>>> +is disabled. Additional values will be the object IDs of the first
>>>> +drm_colorop in a pipeline. A driver can create and advertise none, one,
>>>> +or more possible color pipelines. A DRM client will select a color
>>>> +pipeline by setting the COLOR PIPELINE to the respective value.
>>>> +
>>>> +In the case where drivers have custom support for pre-blending color
>>>> +processing those drivers shall reject atomic commits that are trying to
>>>> +set both the custom color properties, as well as the COLOR_PIPELINE
>>>
>>> s/set/use/ because one of them could be carried-over state from
>>> previous commits while not literally set in this one.
>>>
>>>> +property.
>>>> +
>>>> +An example of a COLOR_PIPELINE property on a plane might look like this::
>>>> +
>>>> + Plane 10
>>>> + ├─ "type": immutable enum {Overlay, Primary, Cursor} = Primary
>>>> + ├─ …
>>>> + └─ "color_pipeline": enum {0, 42, 52} = 0
>>>
>>> Enum values are string names. I presume the intention here is that the
>>> strings will never need to be parsed, and the uint64_t is always equal
>>> to the string representation, right?
>>>
>>> That needs a statement here. It differs from all previous uses of
>>> enums, and e.g. requires a little bit of new API in libweston's
>>> DRM-backend to handle since it has its own enums referring to the
>>> string names that get mapped to the uint64_t per owning KMS object.
>>>
>>
>> I'm currently putting the DRM object ID in the "value" and use the
>> "name" as a descriptive name.
>
> Would that string name be UAPI? I mean, if userspace hardcodes and
> looks for that name, will that keep working? If it's descriptive then I
> would assume not, but for every enum existing so far the string name is
> UAPI.
>
Yes, it's UAPI, as that's how userspace will set the property. The value
is still important to be able to find out which is the first colorop in
the pipeline.
Harry
>>> struct drm_mode_property_enum {
>>> __u64 value;
>>> char name[DRM_PROP_NAME_LEN];
>>> };
>>
>> This works well in IGT and gives us a nice descriptive name for
>> debugging, but I could consider changing this if it'd simplify
>> people's lives.
>
> It's nice if we can have a description string for each pipeline, but
> according to KMS UAPI conventions so far, userspace would look for the
> string name. I'm worried that could backfire to the kernel.
>
> Or, maybe we do want to make the string UAPI and accept that some
> userspace might look for it rather than an arrangement of colorops?
>
> Matching colorop sequences is "hard". A developer looking at pipelines,
> picking one, and hardcoding its name as a preferred choice would be too
> easy. "Works for my cards." IOW, if there is a useful looking string
> name, we can be sure that someone will depend on it.
>
>
> Thanks,
> pq
>
>>>> +References
>>>> +==========
>>>> +
>>>> +1. https://lore.kernel.org/dri-devel/QMers3awXvNCQlyhWdTtsPwkp5ie9bze_hD5nAccFW7a_RXlWjYB7MoUW_8CKLT2bSQwIXVi5H6VULYIxCdgvryZoAoJnC5lZgyK1QWn488=@emersion.fr/
>>>> \ No newline at end of file
>>>
>>
>
