Op 06-07-18 om 16:10 schreef Ville Syrjälä:
> On Fri, Jul 06, 2018 at 04:05:21PM +0300, Dmitry Osipenko wrote:
>> On 06.07.2018 15:23, Ville Syrjälä wrote:
>>> On Fri, Jul 06, 2018 at 02:11:44PM +0200, Maarten Lankhorst wrote:
>>>> Hey,
>>>>
>>>> Op 04-06-18 om 00:00 schreef Dmitry Osipenko:
>>>>> From: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>>>>>
>>>>> Color keying is the action of replacing pixels matching a given color
>>>>> (or range of colors) with transparent pixels in an overlay when
>>>>> performing blitting. Depending on the hardware capabilities, the
>>>>> matching pixel can either become fully transparent or gain adjustment
>>>>> of the pixels component values.
>>>>>
>>>>> Color keying is found in a large number of devices whose capabilities
>>>>> often differ, but they still have enough common features in range to
>>>>> standardize color key properties. This commit adds three generic DRM plane
>>>>> properties related to the color keying, providing initial color keying
>>>>> support.
>>>>>
>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>>>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>>>>> ---
>>>>> drivers/gpu/drm/drm_atomic.c | 12 +++++
>>>>> drivers/gpu/drm/drm_blend.c | 99 ++++++++++++++++++++++++++++++++++++
>>>>> include/drm/drm_blend.h | 3 ++
>>>>> include/drm/drm_plane.h | 53 +++++++++++++++++++
>>>>> 4 files changed, 167 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>>> index 895741e9cd7d..b322cbed319b 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>> @@ -799,6 +799,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>>> state->rotation = val;
>>>>> } else if (property == plane->zpos_property) {
>>>>> state->zpos = val;
>>>>> + } else if (property == plane->colorkey.mode_property) {
>>>>> + state->colorkey.mode = val;
>>>>> + } else if (property == plane->colorkey.min_property) {
>>>>> + state->colorkey.min = val;
>>>>> + } else if (property == plane->colorkey.max_property) {
>>>>> + state->colorkey.max = val;
>>>>> } else if (property == plane->color_encoding_property) {
>>>>> state->color_encoding = val;
>>>>> } else if (property == plane->color_range_property) {
>>>>> @@ -864,6 +870,12 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>>>> *val = state->rotation;
>>>>> } else if (property == plane->zpos_property) {
>>>>> *val = state->zpos;
>>>>> + } else if (property == plane->colorkey.mode_property) {
>>>>> + *val = state->colorkey.mode;
>>>>> + } else if (property == plane->colorkey.min_property) {
>>>>> + *val = state->colorkey.min;
>>>>> + } else if (property == plane->colorkey.max_property) {
>>>>> + *val = state->colorkey.max;
>>>>> } else if (property == plane->color_encoding_property) {
>>>>> *val = state->color_encoding;
>>>>> } else if (property == plane->color_range_property) {
>>>>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>>>>> index a16a74d7e15e..12fed2ff65c8 100644
>>>>> --- a/drivers/gpu/drm/drm_blend.c
>>>>> +++ b/drivers/gpu/drm/drm_blend.c
>>>>> @@ -107,6 +107,11 @@
>>>>> * planes. Without this property the primary plane is always below the cursor
>>>>> * plane, and ordering between all other planes is undefined.
>>>>> *
>>>>> + * colorkey:
>>>>> + * Color keying is set up with drm_plane_create_colorkey_properties().
>>>>> + * It adds support for replacing a range of colors with a transparent
>>>>> + * color in the plane.
>>>>> + *
>>>>> * Note that all the property extensions described here apply either to the
>>>>> * plane or the CRTC (e.g. for the background color, which currently is not
>>>>> * exposed and assumed to be black).
>>>>> @@ -448,3 +453,97 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>>>>> return 0;
>>>>> }
>>>>> EXPORT_SYMBOL(drm_atomic_normalize_zpos);
>>>>> +
>>>>> +static const char * const plane_colorkey_mode_name[] = {
>>>>> + [DRM_PLANE_COLORKEY_MODE_DISABLED] = "disabled",
>>>>> + [DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP] = "foreground-clip",
>>>>> +};
>>>>> +
>>>>> +/**
>>>>> + * drm_plane_create_colorkey_properties - create colorkey properties
>>>>> + * @plane: drm plane
>>>>> + * @supported_modes: bitmask of supported color keying modes
>>>>> + *
>>>>> + * This function creates the generic color keying properties and attach them to
>>>>> + * the plane to enable color keying control for blending operations.
>>>>> + *
>>>>> + * Color keying is controlled by these properties:
>>>>> + *
>>>>> + * colorkey.mode:
>>>>> + * The mode is an enumerated property that controls how color keying
>>>>> + * operates.
>>>>> + *
>>>>> + * colorkey.min, colorkey.max:
>>>>> + * These two properties specify the colors that are treated as the color
>>>>> + * key. Pixel whose value is in the [min, max] range is the color key
>>>>> + * matching pixel. The minimum and maximum values are expressed as a
>>>>> + * 64-bit integer in ARGB16161616 format, where A is the alpha value and
>>>>> + * R, G and B correspond to the color components. Drivers shall convert
>>>>> + * ARGB16161616 value into appropriate format within planes atomic check.
>>>>> + *
>>>>> + * When a single color key is desired instead of a range, userspace shall
>>>>> + * set the min and max properties to the same value.
>>>>> + *
>>>>> + * Drivers return an error from their plane atomic check if range can't be
>>>>> + * handled.
>>>>> + *
>>>>> + * Returns:
>>>>> + * Zero on success, negative errno on failure.
>>>>> + */
>>>>> +int drm_plane_create_colorkey_properties(struct drm_plane *plane,
>>>>> + u32 supported_modes)
>>>>> +{
>>>>> + struct drm_prop_enum_list modes_list[DRM_PLANE_COLORKEY_MODES_NUM];
>>>>> + struct drm_property *mode_prop;
>>>>> + struct drm_property *min_prop;
>>>>> + struct drm_property *max_prop;
>>>>> + unsigned int modes_num = 0;
>>>>> + unsigned int i;
>>>>> +
>>>>> + /* modes are driver-specific, build the list of supported modes */
>>>>> + for (i = 0; i < DRM_PLANE_COLORKEY_MODES_NUM; i++) {
>>>>> + if (!(supported_modes & BIT(i)))
>>>>> + continue;
>>>>> +
>>>>> + modes_list[modes_num].name = plane_colorkey_mode_name[i];
>>>>> + modes_list[modes_num].type = i;
>>>>> + modes_num++;
>>>>> + }
>>>>> +
>>>>> + /* at least one mode should be supported */
>>>>> + if (!modes_num)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + mode_prop = drm_property_create_enum(plane->dev, 0, "colorkey.mode",
>>>>> + modes_list, modes_num);
>>>>> + if (!mode_prop)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + min_prop = drm_property_create_range(plane->dev, 0, "colorkey.min",
>>>>> + 0, U64_MAX);
>>>>> + if (!min_prop)
>>>>> + goto err_destroy_mode_prop;
>>>>> +
>>>>> + max_prop = drm_property_create_range(plane->dev, 0, "colorkey.max",
>>>>> + 0, U64_MAX);
>>>>> + if (!max_prop)
>>>>> + goto err_destroy_min_prop;
>>>>> +
>>>>> + drm_object_attach_property(&plane->base, mode_prop, 0);
>>>>> + drm_object_attach_property(&plane->base, min_prop, 0);
>>>>> + drm_object_attach_property(&plane->base, max_prop, 0);
>>>>> +
>>>>> + plane->colorkey.mode_property = mode_prop;
>>>>> + plane->colorkey.min_property = min_prop;
>>>>> + plane->colorkey.max_property = max_prop;
>>>>> +
>>>>> + return 0;
>>>>> +
>>>>> +err_destroy_min_prop:
>>>>> + drm_property_destroy(plane->dev, min_prop);
>>>>> +err_destroy_mode_prop:
>>>>> + drm_property_destroy(plane->dev, mode_prop);
>>>>> +
>>>>> + return -ENOMEM;
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_plane_create_colorkey_properties);
>>>>> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
>>>>> index 330c561c4c11..8e80d33b643e 100644
>>>>> --- a/include/drm/drm_blend.h
>>>>> +++ b/include/drm/drm_blend.h
>>>>> @@ -52,4 +52,7 @@ int drm_plane_create_zpos_immutable_property(struct drm_plane *plane,
>>>>> unsigned int zpos);
>>>>> int drm_atomic_normalize_zpos(struct drm_device *dev,
>>>>> struct drm_atomic_state *state);
>>>>> +
>>>>> +int drm_plane_create_colorkey_properties(struct drm_plane *plane,
>>>>> + u32 supported_modes);
>>>>> #endif
>>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>>>>> index 26fa50c2a50e..9a621e1ccc47 100644
>>>>> --- a/include/drm/drm_plane.h
>>>>> +++ b/include/drm/drm_plane.h
>>>>> @@ -32,6 +32,48 @@ struct drm_crtc;
>>>>> struct drm_printer;
>>>>> struct drm_modeset_acquire_ctx;
>>>>>
>>>>> +/**
>>>>> + * enum drm_plane_colorkey_mode - uapi plane colorkey mode enumeration
>>>>> + */
>>>>> +enum drm_plane_colorkey_mode {
>>>>> + /**
>>>>> + * @DRM_PLANE_COLORKEY_MODE_DISABLED:
>>>>> + *
>>>>> + * No color matching performed in this mode.
>>>>> + */
>>>>> + DRM_PLANE_COLORKEY_MODE_DISABLED,
>>>>> +
>>>>> + /**
>>>>> + * @DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP:
>>>>> + *
>>>>> + * This mode is also known as a "green screen". Plane pixels are
>>>>> + * transparent in areas where pixels match a given color key range
>>>>> + * and there is a bottom (background) plane, in other cases plane
>>>>> + * pixels are unaffected.
>>>>> + *
>>>>> + */
>>>>> + DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP,
>>>> Could we add background clip as well?
>> Sure, but I think adding a new mode should be a distinct change made on top of
>> the initial series.
>>
>>> Also could we just name them "src" and "dst" (or some variation of
>>> those). I'm betting no one has any kind of idea what these proposed
>>> names mean without looking up the docs, whereas pretty much everyone
>>> knows immediately what src/dst colorkeying means.
>>>
>> Okay, I'll rename the mode to DRM_PLANE_COLORKEY_MODE_SRC in the next revision.
>>
>>>> Would be nice if we could map i915's legacy ioctl handler to the new color key mode.
>>>>> + /**
>>>>> + * @DRM_PLANE_COLORKEY_MODES_NUM:
>>>>> + *
>>>>> + * Total number of color keying modes.
>>>>> + */
>>>>> + DRM_PLANE_COLORKEY_MODES_NUM,
>>>>> +};
>>>>> +
>>>>> +/**
>>>>> + * struct drm_plane_colorkey_state - plane color keying state
>>>>> + * @colorkey.mode: color keying mode
>>>>> + * @colorkey.min: color key range minimum (in ARGB16161616 format)
>>>>> + * @colorkey.max: color key range maximum (in ARGB16161616 format)
>>>>> + */
>>>>> +struct drm_plane_colorkey_state {
>>>>> + enum drm_plane_colorkey_mode mode;
>>>>> + u64 min;
>>>>> + u64 max;
>>>>> +};
>>>> Could we have some macros to extract the components for min/max?
>>>> A, R, G, B.
>> I'll add the macros in the next revision.
>>
>>> And where did we lose the value+mask?
>>>
>> There is no use for the mask on Tegra. I'd prefer to keep initial patches simple
>> and minimal, other modes and additional properties could be added in the further
>> patches on by as-needed basis. Mask could be added later with the default value
>> of 0xffffffffffffffff. Does it sound good for you?
> IIRC my earlier idea was to have different colorkey modes for the
> min+max and value+mask modes. That way userspace might actually have
> some chance of figuring out which bits of state actually do something.
> Although for Intel hw I think the general rule is that min+max for YUV,
> value+mask for RGB, so it's still not 100% clear what to pick if the
> plane supports both.
>
> I guess one alternative would be to have min+max only, and the driver
> would reject 'min != max' if it only uses a single value?
Sounds sane.
> And maybe we should have the mask always? IIRC Intel hw generally has a
> one bit enable/disable "mask" per channel in the min+max mode (I think
> there's one exception where it has only a 1 bit mask in the value+mask
> mode as well). So we could accept 0 and 0xffff mask values in this case
> and reject everything else. The alternative might be to enable the
> keying for the channel if 'min <= max' and disable it if 'min > max'.
> But not sure if that's slightly too magicy.
Yeah, either a single value or 0/0xffff (don't care about this component) sounds like it could work.
