Message-ID: <f31ce849-7f1e-99d7-191e-112798f1ffb5@gmail.com>
Date:   Thu, 20 Sep 2018 19:04:43 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Ville Syrjälä <ville.syrjala@...ux.intel.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Cc:     Neil Armstrong <narmstrong@...libre.com>,
        dri-devel@...ts.freedesktop.org, linux-media@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org, Ben Skeggs <bskeggs@...hat.com>,
        Sinclair Yeh <syeh@...are.com>,
        Thomas Hellstrom <thellstrom@...are.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for
 display planes

On 8/16/18 2:42 PM, Maarten Lankhorst wrote:
> Op 08-08-18 om 16:30 schreef Dmitry Osipenko:
>> On Wednesday, 8 August 2018 11:16:09 MSK Russell King - ARM Linux wrote:
>>> On Tue, Aug 07, 2018 at 08:22:01PM +0300, Dmitry Osipenko wrote:
>>>> + * Glossary:
>>>> + *
>>>> + * Destination plane:
>>>> + *	Plane to which color keying properties are applied, this planes takes
>>>> + *	the effect of color keying operation. The effect is determined by a
>>>> + *	given color keying mode.
>>>> + *
>>>> + * Source plane:
>>>> + *	Pixels of this plane are the source for color key matching operation.
>>> ...
>>>
>>>> +	/**
>>>> +	 * @DRM_PLANE_COLORKEY_MODE_TRANSPARENT:
>>>> +	 *
>>>> +	 * Destination plane pixels are completely transparent in areas
>>>> +	 * where pixels of a source plane are matching a given color key
>>>> +	 * range, in other cases pixels of a destination plane are
>> unaffected.
>>>> +	 * In areas where two or more source planes overlap, the topmost
>>>> +	 * plane takes precedence.
>>>> +	 */
>>> This seems confusing to me.
>>>
>>> What you seem to be saying is that the "destination" plane would be the
>>> one which is (eg0 the graphic plane, and the "source" plane would be the
>>> the plane containing (eg) the video.  You seem to be saying that the
>>> colorkey matches the video and determines whether the pixels in the
>>> graphic plane are opaque or transparent.
>> Your example is correct.
>>
>> With the "plane_mask" property we can specify any plane as the "source" for
>> color key, so it can been either a video plane or graphic plane and even both
>> at the same time.
> I'm not sure we should specify plane mask from userspace.

It looks like a quite flexible approach. Do you have any other suggestions?

> Can't we make major loops? How do you want to handle those?

It's up to a specific driver to accept the mask or reject it. You could make a 
loop if HW allows to do that, I don't see what's the problem.

>>> Surely that is the wrong way round - in video overlay, you want to
>>> colorkey match the contents of the graphic plane to determine which
>>> pixels from the video plane to overlay.
>> The "transparent" mode makes the color-matched pixels to become transparent,
>> you want the inversion effect and hence that should be called something like a
>> "transparent-inverted" mode. Maarten Lankhorst was asking for that mode in his
>> comment to v3, I'm leaving for somebody else to add that mode later since
>> there is no real use for it on Tegra right now.
> I would like it to be described and included, so I can convert the existing intel_sprite_set_colorkey_ioctl to atomic.

Okay, I can add it. Though probably better to call that mode "opaque" rather 
than "transparent-inverted".

> Then again, could transparent-inverted also be handled by setting transparent on the primary?

If HW allows to do that, then yes.

> 
>> So in your case the graphic plane will be the "source" plane (specified via
>> the colorkey.plane_mask property), video plane will be the "destination" plane
>> (plane to which the colorkey properties are applied) and the colorkey.mode
>> will be "transparent-inverted". Pixels of the "source" plane are being matched
>> and "destination" plane takes the effect of color keying operation, i.e. the
>> color-matched pixels of graphic plane leave the video plane pixels unaffected
>> and the unmatched pixels make the video plane pixels transparent.
>>
>>> If it's the other way around (source is the graphic, destination is the
>>> video) it makes less sense to use the "source" and "destination" terms,
>>> I can't see how you could describe a plane that is being overlaid on
>>> top of another plane as a "destination".
>> Tegra has a bit annoying limitations in regard to a reduced plane blending
>> functionality when color keying is enabled. I found that the best variant to
>> work around the limitations is to move the graphic plane on top of the video
>> plane and to make the graphic plane to match itself. I.e. the matched pixels
>> of graphic plane become transparent and hence poked by video plane.
>>
>>> I guess the terminology has come from a thought about using a GPU to
>>> physically do the colorkeying when combining two planes - if the GPU
>>> were to write to the "destination" plane, then this would be the wrong
>>> way around.  For starters, taking the above example, the video plane
>>> may well be smaller than the graphic plane.  If it's the other way
>>> around, that has other problems, like destroying the colorkey in the
>>> graphic plane when writing the video plane's contents to it.
>> It all depends on a use-case scenario. It won't be easy for userspace to
>> generalize the usage of color keying, at best the color keying interface could
>> be generalized and then userspace may choose the best fitting variant based on
>> available HW capabilities.
> There's TEST_ONLY for a reason, though I guess it makes generic color keying slightly more invovled for userspace. :)

It is also quite involved for kernel to present a non-standard feature as 
something generic, pleasing everyone in the same time.

Powered by blists - more mailing lists