Re: [PATCH 2/3] drm: zynqmp: Make the video plane primary

[Sean Anderson <sean.anderson@linux.dev>]

On 11/13/25 17:45, Klymenko, Anatoliy wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Sean,
> 
> Thanks a lot for the patch (and tackling the alpha issue in general)
> 
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@linux.dev>
>> Sent: Thursday, November 13, 2025 12:37 PM
>> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Tomi Valkeinen
>> <tomi.valkeinen@ideasonboard.com>; dri-devel@lists.freedesktop.org
>> Cc: linux-kernel@vger.kernel.org; Mike Looijmans <mike.looijmans@topic.nl>;
>> David Airlie <airlied@gmail.com>; Thomas Zimmermann
>> <tzimmermann@suse.de>; Maarten Lankhorst
>> <maarten.lankhorst@linux.intel.com>; Klymenko, Anatoliy
>> <Anatoliy.Klymenko@amd.com>; Maxime Ripard <mripard@kernel.org>; linux-
>> arm-kernel@lists.infradead.org; Simona Vetter <simona@ffwll.ch>; Simek,
>> Michal <michal.simek@amd.com>; Sean Anderson
>> <sean.anderson@linux.dev>
>> Subject: [PATCH 2/3] drm: zynqmp: Make the video plane primary
>>
>> Caution: This message originated from an External Source. Use proper caution
>> when opening attachments, clicking links, or responding.
>>
>>
>> The zynqmp has two planes: "video" and "graphics". The video plane
>>
>> - Is on the bottom (zpos=0) (except when chroma keying as the master plane)
>> - Supports "live" input (e.g. from an external source)
>> - Supports RGB, YUV, and YCbCr formats, including XRGB8888
>> - Does not support transparency, except via chroma keying (colorkey)
>> - Must cover the entire screen (translation/resizing not supported)
>>
>> The graphics plane
>>
>> - Is on the top (zpos=1)
>> - Supports "live" input (e.g. from an external source)
>> - Supports RGB and YUV444 formats, but not XRGB8888
>> - Supports transparency either via
>>   - Global alpha channel, which disables per-pixel alpha when enabled
>>   - Per-pixel alpha, which cannot be used with global alpha
>>   - Chroma keying (colorkey)
>> - Must cover the entire screen (translation/resizing not supported)
>>
>> Currently the graphics plane is the primary plane. Make the video plane
>> the primary plane:
>>
>> - The video plane supports XRGB8888, which is the default 24-bit
>>   colorspace for X. This results in improved performance when compared
>>   to RGB565.
>> - The graphics plane can be used as an overlay because it has a higher
>>   z-pos and supports a per-pixel alpha channel. Unfortunately, clients
>>   like weston cannot currently take advantage of this because they
>>   expect overlay planes to support translation/resizing.
>>
>> One downside to this approach could be that the graphics plane has worse
>> support for YUV and YCBCr, so it may be more difficult to compose video
> 
> Not just more difficult but practically impossible:
> 1. GFX (in Xilinx terminology) plane doesn't support pixel upscaling, so
> no support for NV12, YUY2 and other common video pixel formats.
> 2. Both planes are unscalable, this means we can only output native
> display resolution video on the top plane, or display thick black frame
> around the picture. We are losing GFX masking capabilities.

The graphics masking capabilities are the same as they always were.

> 3. We won't be able to render subtitles on top of the video.
> Probably the only practical video player option remains here is to render
> video to a texture and embed it into graphics composition.

OK, but none of this is currently possible either because of the global
alpha setting. So do you have a specific program in mind that plays
video and will do it with an "underlay" plane?

>> streams into the window of a media player. However, no existing software
>> could rely on this because there is no way to enable the per-pixel alpha
>> channel when the graphics plane is enabled. This makes it impossible to
> 
> This situation is a driver bug not a fatum.

And yet the driver bug provides ample evidence that no one is doing this,
so we don't need to worry about it.

--Sean

> Per pixel alpha blending works
> just fine if we disable global alpha. We just need to avoid enabling it when
> we have alpha capable GFX plane format.
> 
>> "carve out" an area in the graphics plane where the video plane shows
>> through. This limitation is addressed in the next patch, but it means we
>> do not need to worry about compatibility in this area.
>>
>> An alternate approach could be to pretend that the graphics plane
>> supports XRGB8888 by using the supported ARGB8888 mode instead and
>> enabling the global alpha channel. However, this would rule out ever
>> using the per-pixel alpha channel.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>>
>>  drivers/gpu/drm/xlnx/zynqmp_kms.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
>> b/drivers/gpu/drm/xlnx/zynqmp_kms.c
>> index c80a2d4034f3..456ada9ac003 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
>> @@ -161,8 +161,8 @@ static int zynqmp_dpsub_create_planes(struct
>> zynqmp_dpsub *dpsub)
>>                 if (!formats)
>>                         return -ENOMEM;
>>
>> -               /* Graphics layer is primary, and video layer is overlay. */
>> -               type = i == ZYNQMP_DPSUB_LAYER_VID
>> +               /* Graphics layer is overlay, and video layer is primary. */
>> +               type = i == ZYNQMP_DPSUB_LAYER_GFX
>>                      ? DRM_PLANE_TYPE_OVERLAY :
>> DRM_PLANE_TYPE_PRIMARY;
>>                 ret = drm_universal_plane_init(&dpsub->drm->dev, plane, 0,
>>                                                &zynqmp_dpsub_plane_funcs,
>> @@ -322,7 +322,7 @@ static const struct drm_crtc_funcs
>> zynqmp_dpsub_crtc_funcs = {
>>
>>  static int zynqmp_dpsub_create_crtc(struct zynqmp_dpsub *dpsub)
>>  {
>> -       struct drm_plane *plane = &dpsub->drm-
>> >planes[ZYNQMP_DPSUB_LAYER_GFX];
>> +       struct drm_plane *plane = &dpsub->drm-
>> >planes[ZYNQMP_DPSUB_LAYER_VID];
>>         struct drm_crtc *crtc = &dpsub->drm->crtc;
>>         int ret;
>>
>> --
>> 2.35.1.1320.gc452695387.dirty
> 
> Thank you,
> Anatoliy