Adding support to overlay type in addition to primary and cursor plane. The planes composition relies on the z order of the active planes and only occurs if there is a primary plane (as in the current behavior).
The first patch switches the function of initializing planes from drm_universal_plane_init to drmm_universal_plane_alloc. It aims to improve aspects of allocation and cleanup operations, leaving it to the DRM infrastructure.
The second patch generalizes variables and functions names to refer to any kind of plane, not only cursor. The goal is to reuse them for blending overlay and cursor planes to primary.
The third patch enables the plane composition to select a pixel blend operation according to the plane format (XRGB8888 or ARGB8888).
The last patch creates a module option to enable overlay, and includes overlay to supported types of plane. When the overlay option is enabled, one overlay plane is initialized (plus primary and cursor) and it is included in the planes composition.
This work preserves the current results of IGT tests: kms_cursor_crc; kms_flip and kms_writeback. In addition, subtests related to overlay in kms_atomic and kms_plane_cursor start to pass (pointed out in the commit message).
v2:
- Drop unnecessary changes that init crtc without cursor (Daniel) - Replace function to initialize planes (Daniel) - Add proper pixel blending op according to the plane format (Daniel)
v3:
- Proper use of the variable funcs (kernel bot) - Adjust the patch series format
v4: - use better names for functions of plane composition (Daniel) - clear alpha channel (0xff) after blend color values by pixel - improve comments on blend ops to reflect the current state - describe in the commit message future improvements for plane composition
Melissa Wen (4): drm/vkms: init plane using drmm_universal_plane_alloc drm/vkms: rename cursor to plane on ops of planes composition drm/vkms: add XRGB planes composition drm/vkms: add overlay support
drivers/gpu/drm/vkms/vkms_composer.c | 104 ++++++++++++++++++--------- drivers/gpu/drm/vkms/vkms_drv.c | 5 ++ drivers/gpu/drm/vkms/vkms_drv.h | 9 ++- drivers/gpu/drm/vkms/vkms_output.c | 28 ++++---- drivers/gpu/drm/vkms/vkms_plane.c | 51 ++++++------- 5 files changed, 125 insertions(+), 72 deletions(-)
By using drmm_universal_plane_alloc instead of drm_universal_plane_init, we let the DRM infrastructure handles resource allocation and cleanup. We can also get rid of some code repetitions for plane cleanup, improving code maintainability in vkms.
Signed-off-by: Melissa Wen melissa.srw@gmail.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/vkms/vkms_drv.h | 8 ++++++-- drivers/gpu/drm/vkms/vkms_output.c | 19 +++++-------------- drivers/gpu/drm/vkms/vkms_plane.c | 29 +++++++++++------------------ 3 files changed, 22 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 35540c7c4416..70fb79621617 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -37,6 +37,10 @@ struct vkms_plane_state { struct vkms_composer *composer; };
+struct vkms_plane { + struct drm_plane base; +}; + /** * vkms_crtc_state - Driver specific CRTC state * @base: base CRTC state @@ -114,8 +118,8 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
int vkms_output_init(struct vkms_device *vkmsdev, int index);
-struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev, - enum drm_plane_type type, int index); +struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, + enum drm_plane_type type, int index);
/* CRC Support */ const char *const *vkms_get_crc_sources(struct drm_crtc *crtc, diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c index f5f6f15c362c..6979fbc7f821 100644 --- a/drivers/gpu/drm/vkms/vkms_output.c +++ b/drivers/gpu/drm/vkms/vkms_output.c @@ -39,7 +39,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) struct drm_connector *connector = &output->connector; struct drm_encoder *encoder = &output->encoder; struct drm_crtc *crtc = &output->crtc; - struct drm_plane *primary, *cursor = NULL; + struct vkms_plane *primary, *cursor = NULL; int ret; int writeback;
@@ -49,15 +49,13 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
if (vkmsdev->config->cursor) { cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index); - if (IS_ERR(cursor)) { - ret = PTR_ERR(cursor); - goto err_cursor; - } + if (IS_ERR(cursor)) + return PTR_ERR(cursor); }
- ret = vkms_crtc_init(dev, crtc, primary, cursor); + ret = vkms_crtc_init(dev, crtc, &primary->base, &cursor->base); if (ret) - goto err_crtc; + return ret;
ret = drm_connector_init(dev, connector, &vkms_connector_funcs, DRM_MODE_CONNECTOR_VIRTUAL); @@ -100,12 +98,5 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) err_connector: drm_crtc_cleanup(crtc);
-err_crtc: - if (vkmsdev->config->cursor) - drm_plane_cleanup(cursor); - -err_cursor: - drm_plane_cleanup(primary); - return ret; } diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index 6d310d31b75d..135140f8e87a 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -86,7 +86,6 @@ static void vkms_plane_reset(struct drm_plane *plane) static const struct drm_plane_funcs vkms_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, - .destroy = drm_plane_cleanup, .reset = vkms_plane_reset, .atomic_duplicate_state = vkms_plane_duplicate_state, .atomic_destroy_state = vkms_plane_destroy_state, @@ -191,18 +190,14 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = { .cleanup_fb = vkms_cleanup_fb, };
-struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev, - enum drm_plane_type type, int index) +struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, + enum drm_plane_type type, int index) { struct drm_device *dev = &vkmsdev->drm; const struct drm_plane_helper_funcs *funcs; - struct drm_plane *plane; + struct vkms_plane *plane; const u32 *formats; - int ret, nformats; - - plane = kzalloc(sizeof(*plane), GFP_KERNEL); - if (!plane) - return ERR_PTR(-ENOMEM); + int nformats;
if (type == DRM_PLANE_TYPE_CURSOR) { formats = vkms_cursor_formats; @@ -214,16 +209,14 @@ struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev, funcs = &vkms_primary_helper_funcs; }
- ret = drm_universal_plane_init(dev, plane, 1 << index, - &vkms_plane_funcs, - formats, nformats, - NULL, type, NULL); - if (ret) { - kfree(plane); - return ERR_PTR(ret); - } + plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 1 << index, + &vkms_plane_funcs, + formats, nformats, + NULL, type, NULL); + if (IS_ERR(plane)) + return plane;
- drm_plane_helper_add(plane, funcs); + drm_plane_helper_add(&plane->base, funcs);
return plane; }
Generalize variables and function names used for planes composition (from cursor to plane), since we will reuse the operations for both cursor and overlay types.
No functional change.
v4: - use better names for functions of plane composition (Daniel)
Signed-off-by: Melissa Wen melissa.srw@gmail.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/vkms/vkms_composer.c | 31 ++++++++++++++-------------- 1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 66c6842d70db..02642801735d 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -125,26 +125,26 @@ static void blend(void *vaddr_dst, void *vaddr_src, } }
-static void compose_cursor(struct vkms_composer *cursor_composer, - struct vkms_composer *primary_composer, - void *vaddr_out) +static void compose_plane(struct vkms_composer *primary_composer, + struct vkms_composer *plane_composer, + void *vaddr_out) { - struct drm_gem_object *cursor_obj; - struct drm_gem_shmem_object *cursor_shmem_obj; + struct drm_gem_object *plane_obj; + struct drm_gem_shmem_object *plane_shmem_obj;
- cursor_obj = drm_gem_fb_get_obj(&cursor_composer->fb, 0); - cursor_shmem_obj = to_drm_gem_shmem_obj(cursor_obj); + plane_obj = drm_gem_fb_get_obj(&plane_composer->fb, 0); + plane_shmem_obj = to_drm_gem_shmem_obj(plane_obj);
- if (WARN_ON(!cursor_shmem_obj->vaddr)) + if (WARN_ON(!plane_shmem_obj->vaddr)) return;
- blend(vaddr_out, cursor_shmem_obj->vaddr, - primary_composer, cursor_composer); + blend(vaddr_out, plane_shmem_obj->vaddr, + primary_composer, plane_composer); }
-static int compose_planes(void **vaddr_out, - struct vkms_composer *primary_composer, - struct vkms_composer *cursor_composer) +static int compose_active_planes(void **vaddr_out, + struct vkms_composer *primary_composer, + struct vkms_composer *cursor_composer) { struct drm_framebuffer *fb = &primary_composer->fb; struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0); @@ -164,7 +164,7 @@ static int compose_planes(void **vaddr_out, memcpy(*vaddr_out, shmem_obj->vaddr, shmem_obj->base.size);
if (cursor_composer) - compose_cursor(cursor_composer, primary_composer, *vaddr_out); + compose_plane(primary_composer, cursor_composer, *vaddr_out);
return 0; } @@ -222,7 +222,8 @@ void vkms_composer_worker(struct work_struct *work) if (wb_pending) vaddr_out = crtc_state->active_writeback;
- ret = compose_planes(&vaddr_out, primary_composer, cursor_composer); + ret = compose_active_planes(&vaddr_out, primary_composer, + cursor_composer); if (ret) { if (ret == -EINVAL && !wb_pending) kfree(vaddr_out);
Add support for composing XRGB888 planes in addition to the ARGB8888 format. In the case of an XRGB plane at the top, the composition consists of copying the RGB values of a pixel from src to dst and clearing alpha channel, without the need for alpha blending operations for each pixel.
Blend equations assume a completely opaque background, i.e., primary plane is not cleared before pixel blending but alpha channel is explicitly opaque (a = 0xff). Also, there is room for performance evaluation in switching pixel blend operation according to the plane format.
v4: - clear alpha channel (0xff) after blend color values by pixel - improve comments on blend ops to reflect the current state - describe in the commit message future improvements for plane composition
Signed-off-by: Melissa Wen melissa.srw@gmail.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/vkms/vkms_composer.c | 56 ++++++++++++++++++++++------ drivers/gpu/drm/vkms/vkms_plane.c | 7 ++-- 2 files changed, 48 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 02642801735d..7e01bc39d2a1 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -4,6 +4,7 @@
#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_shmem_helper.h> #include <drm/drm_vblank.h> @@ -64,7 +65,17 @@ static u8 blend_channel(u8 src, u8 dst, u8 alpha) return new_color; }
-static void alpha_blending(const u8 *argb_src, u8 *argb_dst) +/** + * alpha_blend - alpha blending equation + * @argb_src: src pixel on premultiplied alpha mode + * @argb_dst: dst pixel completely opaque + * + * blend pixels using premultiplied blend formula. The current DRM assumption + * is that pixel color values have been already pre-multiplied with the alpha + * channel values. See more drm_plane_create_blend_mode_property(). Also, this + * formula assumes a completely opaque background. + */ +static void alpha_blend(const u8 *argb_src, u8 *argb_dst) { u8 alpha;
@@ -72,8 +83,16 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst) argb_dst[0] = blend_channel(argb_src[0], argb_dst[0], alpha); argb_dst[1] = blend_channel(argb_src[1], argb_dst[1], alpha); argb_dst[2] = blend_channel(argb_src[2], argb_dst[2], alpha); - /* Opaque primary */ - argb_dst[3] = 0xFF; +} + +/** + * x_blend - blending equation that ignores the pixel alpha + * + * overwrites RGB color value from src pixel to dst pixel. + */ +static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst) +{ + memcpy(xrgb_dst, xrgb_src, sizeof(u8) * 3); }
/** @@ -82,16 +101,20 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst) * @vaddr_src: source address * @dst_composer: destination framebuffer's metadata * @src_composer: source framebuffer's metadata + * @pixel_blend: blending equation based on plane format * - * Blend the vaddr_src value with the vaddr_dst value using the pre-multiplied - * alpha blending equation, since DRM currently assumes that the pixel color - * values have already been pre-multiplied with the alpha channel values. See - * more drm_plane_create_blend_mode_property(). This function uses buffer's - * metadata to locate the new composite values at vaddr_dst. + * Blend the vaddr_src value with the vaddr_dst value using a pixel blend + * equation according to the plane format and clearing alpha channel to an + * completely opaque background. This function uses buffer's metadata to locate + * the new composite values at vaddr_dst. + * + * TODO: completely clear the primary plane (a = 0xff) before starting to blend + * pixel color values */ static void blend(void *vaddr_dst, void *vaddr_src, struct vkms_composer *dst_composer, - struct vkms_composer *src_composer) + struct vkms_composer *src_composer, + void (*pixel_blend)(const u8 *, u8 *)) { int i, j, j_dst, i_dst; int offset_src, offset_dst; @@ -119,7 +142,9 @@ static void blend(void *vaddr_dst, void *vaddr_src,
pixel_src = (u8 *)(vaddr_src + offset_src); pixel_dst = (u8 *)(vaddr_dst + offset_dst); - alpha_blending(pixel_src, pixel_dst); + pixel_blend(pixel_src, pixel_dst); + /* clearing alpha channel (0xff)*/ + memset(vaddr_dst + offset_dst + 3, 0xff, 1); } i_dst++; } @@ -131,6 +156,8 @@ static void compose_plane(struct vkms_composer *primary_composer, { struct drm_gem_object *plane_obj; struct drm_gem_shmem_object *plane_shmem_obj; + struct drm_framebuffer *fb = &plane_composer->fb; + void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
plane_obj = drm_gem_fb_get_obj(&plane_composer->fb, 0); plane_shmem_obj = to_drm_gem_shmem_obj(plane_obj); @@ -138,8 +165,13 @@ static void compose_plane(struct vkms_composer *primary_composer, if (WARN_ON(!plane_shmem_obj->vaddr)) return;
- blend(vaddr_out, plane_shmem_obj->vaddr, - primary_composer, plane_composer); + if (fb->format->format == DRM_FORMAT_ARGB8888) + pixel_blend = &alpha_blend; + else + pixel_blend = &x_blend; + + blend(vaddr_out, plane_shmem_obj->vaddr, primary_composer, + plane_composer, pixel_blend); }
static int compose_active_planes(void **vaddr_out, diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index 135140f8e87a..da4251aff67f 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -16,8 +16,9 @@ static const u32 vkms_formats[] = { DRM_FORMAT_XRGB8888, };
-static const u32 vkms_cursor_formats[] = { +static const u32 vkms_plane_formats[] = { DRM_FORMAT_ARGB8888, + DRM_FORMAT_XRGB8888 };
static struct drm_plane_state * @@ -200,8 +201,8 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, int nformats;
if (type == DRM_PLANE_TYPE_CURSOR) { - formats = vkms_cursor_formats; - nformats = ARRAY_SIZE(vkms_cursor_formats); + formats = vkms_plane_formats; + nformats = ARRAY_SIZE(vkms_plane_formats); funcs = &vkms_primary_helper_funcs; } else { formats = vkms_formats;
On Sat, 24 Apr 2021 05:25:31 -0300 Melissa Wen melissa.srw@gmail.com wrote:
Add support for composing XRGB888 planes in addition to the ARGB8888 format. In the case of an XRGB plane at the top, the composition consists of copying the RGB values of a pixel from src to dst and clearing alpha channel, without the need for alpha blending operations for each pixel.
Blend equations assume a completely opaque background, i.e., primary plane is not cleared before pixel blending but alpha channel is explicitly opaque (a = 0xff). Also, there is room for performance evaluation in switching pixel blend operation according to the plane format.
v4:
- clear alpha channel (0xff) after blend color values by pixel
- improve comments on blend ops to reflect the current state
- describe in the commit message future improvements for plane composition
Signed-off-by: Melissa Wen melissa.srw@gmail.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/vkms/vkms_composer.c | 56 ++++++++++++++++++++++------ drivers/gpu/drm/vkms/vkms_plane.c | 7 ++-- 2 files changed, 48 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 02642801735d..7e01bc39d2a1 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -4,6 +4,7 @@
#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_shmem_helper.h> #include <drm/drm_vblank.h> @@ -64,7 +65,17 @@ static u8 blend_channel(u8 src, u8 dst, u8 alpha) return new_color; }
-static void alpha_blending(const u8 *argb_src, u8 *argb_dst) +/**
- alpha_blend - alpha blending equation
- @argb_src: src pixel on premultiplied alpha mode
- @argb_dst: dst pixel completely opaque
- blend pixels using premultiplied blend formula. The current DRM assumption
- is that pixel color values have been already pre-multiplied with the alpha
- channel values. See more drm_plane_create_blend_mode_property(). Also, this
- formula assumes a completely opaque background.
- */
+static void alpha_blend(const u8 *argb_src, u8 *argb_dst) { u8 alpha;
@@ -72,8 +83,16 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst) argb_dst[0] = blend_channel(argb_src[0], argb_dst[0], alpha); argb_dst[1] = blend_channel(argb_src[1], argb_dst[1], alpha); argb_dst[2] = blend_channel(argb_src[2], argb_dst[2], alpha);
- /* Opaque primary */
- argb_dst[3] = 0xFF;
+}
+/**
- x_blend - blending equation that ignores the pixel alpha
- overwrites RGB color value from src pixel to dst pixel.
- */
+static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst) +{
- memcpy(xrgb_dst, xrgb_src, sizeof(u8) * 3);
Hi,
this function very clearly assumes a very specific pixel format on both source and destination. I think it would be good if the code comments called out exactly which DRM_FORMAT_* they assume. This would be good to do on almost every function that makes such assumptions. I believe that would help code readability, and also point out explicitly which things need to be fixed when you add support for even more pixel formats.
"xrgb" and "argb" are IMO too vague. You might be referring to DRM_FORMAT_XRGB* and DRM_FORMAT_ARGB*, or maybe you are referring to any pixel format that happens to have or not have an alpha channel in addition to the three RGB channels in some order and width.
Being explicit that these refer to specific DRM_FORMAT_* should also help understanding how things work on big-endian CPUs. My current understanding is that this memcpy is correct also on big-endian, given DRM_FORMAT_XRGB8888.
Hmm, or rather, is this particular function intended to be general in the sense that the order of RGB channels does not matter as long as it's the same in both source and destination? Which would mean I had a wrong assumption from the start.
}
/** @@ -82,16 +101,20 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst)
- @vaddr_src: source address
- @dst_composer: destination framebuffer's metadata
- @src_composer: source framebuffer's metadata
- @pixel_blend: blending equation based on plane format
- Blend the vaddr_src value with the vaddr_dst value using the pre-multiplied
- alpha blending equation, since DRM currently assumes that the pixel color
- values have already been pre-multiplied with the alpha channel values. See
- more drm_plane_create_blend_mode_property(). This function uses buffer's
- metadata to locate the new composite values at vaddr_dst.
- Blend the vaddr_src value with the vaddr_dst value using a pixel blend
- equation according to the plane format and clearing alpha channel to an
- completely opaque background. This function uses buffer's metadata to locate
- the new composite values at vaddr_dst.
- TODO: completely clear the primary plane (a = 0xff) before starting to blend
*/
- pixel color values
static void blend(void *vaddr_dst, void *vaddr_src, struct vkms_composer *dst_composer,
struct vkms_composer *src_composer)
struct vkms_composer *src_composer,void (*pixel_blend)(const u8 *, u8 *)){ int i, j, j_dst, i_dst; int offset_src, offset_dst; @@ -119,7 +142,9 @@ static void blend(void *vaddr_dst, void *vaddr_src,
pixel_src = (u8 *)(vaddr_src + offset_src); pixel_dst = (u8 *)(vaddr_dst + offset_dst);
alpha_blending(pixel_src, pixel_dst);
pixel_blend(pixel_src, pixel_dst);/* clearing alpha channel (0xff)*/memset(vaddr_dst + offset_dst + 3, 0xff, 1);
A one byte memset?
Wouldn't pixel_dst[3] = 0xff; be more clear?
Thanks, pq
} i_dst++;} @@ -131,6 +156,8 @@ static void compose_plane(struct vkms_composer *primary_composer, { struct drm_gem_object *plane_obj; struct drm_gem_shmem_object *plane_shmem_obj;
struct drm_framebuffer *fb = &plane_composer->fb;
void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
plane_obj = drm_gem_fb_get_obj(&plane_composer->fb, 0); plane_shmem_obj = to_drm_gem_shmem_obj(plane_obj);
@@ -138,8 +165,13 @@ static void compose_plane(struct vkms_composer *primary_composer, if (WARN_ON(!plane_shmem_obj->vaddr)) return;
- blend(vaddr_out, plane_shmem_obj->vaddr,
primary_composer, plane_composer);
- if (fb->format->format == DRM_FORMAT_ARGB8888)
pixel_blend = &alpha_blend;- else
pixel_blend = &x_blend;- blend(vaddr_out, plane_shmem_obj->vaddr, primary_composer,
plane_composer, pixel_blend);}
static int compose_active_planes(void **vaddr_out, diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index 135140f8e87a..da4251aff67f 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -16,8 +16,9 @@ static const u32 vkms_formats[] = { DRM_FORMAT_XRGB8888, };
-static const u32 vkms_cursor_formats[] = { +static const u32 vkms_plane_formats[] = { DRM_FORMAT_ARGB8888,
- DRM_FORMAT_XRGB8888
};
static struct drm_plane_state * @@ -200,8 +201,8 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, int nformats;
if (type == DRM_PLANE_TYPE_CURSOR) {
formats = vkms_cursor_formats;nformats = ARRAY_SIZE(vkms_cursor_formats);
formats = vkms_plane_formats;nformats = ARRAY_SIZE(vkms_plane_formats);
On Mon, Apr 26, 2021 at 11:03:15AM +0300, Pekka Paalanen wrote:
On Sat, 24 Apr 2021 05:25:31 -0300 Melissa Wen melissa.srw@gmail.com wrote:
Add support for composing XRGB888 planes in addition to the ARGB8888 format. In the case of an XRGB plane at the top, the composition consists of copying the RGB values of a pixel from src to dst and clearing alpha channel, without the need for alpha blending operations for each pixel.
Blend equations assume a completely opaque background, i.e., primary plane is not cleared before pixel blending but alpha channel is explicitly opaque (a = 0xff). Also, there is room for performance evaluation in switching pixel blend operation according to the plane format.
v4:
- clear alpha channel (0xff) after blend color values by pixel
- improve comments on blend ops to reflect the current state
- describe in the commit message future improvements for plane composition
Signed-off-by: Melissa Wen melissa.srw@gmail.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/vkms/vkms_composer.c | 56 ++++++++++++++++++++++------ drivers/gpu/drm/vkms/vkms_plane.c | 7 ++-- 2 files changed, 48 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 02642801735d..7e01bc39d2a1 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -4,6 +4,7 @@
#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_shmem_helper.h> #include <drm/drm_vblank.h> @@ -64,7 +65,17 @@ static u8 blend_channel(u8 src, u8 dst, u8 alpha) return new_color; }
-static void alpha_blending(const u8 *argb_src, u8 *argb_dst) +/**
- alpha_blend - alpha blending equation
- @argb_src: src pixel on premultiplied alpha mode
- @argb_dst: dst pixel completely opaque
- blend pixels using premultiplied blend formula. The current DRM assumption
- is that pixel color values have been already pre-multiplied with the alpha
- channel values. See more drm_plane_create_blend_mode_property(). Also, this
- formula assumes a completely opaque background.
- */
+static void alpha_blend(const u8 *argb_src, u8 *argb_dst) { u8 alpha;
@@ -72,8 +83,16 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst) argb_dst[0] = blend_channel(argb_src[0], argb_dst[0], alpha); argb_dst[1] = blend_channel(argb_src[1], argb_dst[1], alpha); argb_dst[2] = blend_channel(argb_src[2], argb_dst[2], alpha);
- /* Opaque primary */
- argb_dst[3] = 0xFF;
+}
+/**
- x_blend - blending equation that ignores the pixel alpha
- overwrites RGB color value from src pixel to dst pixel.
- */
+static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst) +{
- memcpy(xrgb_dst, xrgb_src, sizeof(u8) * 3);
Hi,
this function very clearly assumes a very specific pixel format on both source and destination. I think it would be good if the code comments called out exactly which DRM_FORMAT_* they assume. This would be good to do on almost every function that makes such assumptions. I believe that would help code readability, and also point out explicitly which things need to be fixed when you add support for even more pixel formats.
"xrgb" and "argb" are IMO too vague. You might be referring to DRM_FORMAT_XRGB* and DRM_FORMAT_ARGB*, or maybe you are referring to any pixel format that happens to have or not have an alpha channel in addition to the three RGB channels in some order and width.
Being explicit that these refer to specific DRM_FORMAT_* should also help understanding how things work on big-endian CPUs. My current understanding is that this memcpy is correct also on big-endian, given DRM_FORMAT_XRGB8888.
Hmm, or rather, is this particular function intended to be general in the sense that the order of RGB channels does not matter as long as it's the same in both source and destination? Which would mean I had a wrong assumption from the start.
Atm all vkms supports is X/ARGB8888, and even there we throw around random limits. Add support for more pixel formats is definitely on the list, and then all the blend/compose stuff needs to be quite drastically rearchitected.
I think until we're there documenting what's already documented in the todo list feels like overkill. -Daniel
}
/** @@ -82,16 +101,20 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst)
- @vaddr_src: source address
- @dst_composer: destination framebuffer's metadata
- @src_composer: source framebuffer's metadata
- @pixel_blend: blending equation based on plane format
- Blend the vaddr_src value with the vaddr_dst value using the pre-multiplied
- alpha blending equation, since DRM currently assumes that the pixel color
- values have already been pre-multiplied with the alpha channel values. See
- more drm_plane_create_blend_mode_property(). This function uses buffer's
- metadata to locate the new composite values at vaddr_dst.
- Blend the vaddr_src value with the vaddr_dst value using a pixel blend
- equation according to the plane format and clearing alpha channel to an
- completely opaque background. This function uses buffer's metadata to locate
- the new composite values at vaddr_dst.
- TODO: completely clear the primary plane (a = 0xff) before starting to blend
*/
- pixel color values
static void blend(void *vaddr_dst, void *vaddr_src, struct vkms_composer *dst_composer,
struct vkms_composer *src_composer)
struct vkms_composer *src_composer,void (*pixel_blend)(const u8 *, u8 *)){ int i, j, j_dst, i_dst; int offset_src, offset_dst; @@ -119,7 +142,9 @@ static void blend(void *vaddr_dst, void *vaddr_src,
pixel_src = (u8 *)(vaddr_src + offset_src); pixel_dst = (u8 *)(vaddr_dst + offset_dst);
alpha_blending(pixel_src, pixel_dst);
pixel_blend(pixel_src, pixel_dst);/* clearing alpha channel (0xff)*/memset(vaddr_dst + offset_dst + 3, 0xff, 1);A one byte memset?
Wouldn't pixel_dst[3] = 0xff; be more clear?
Thanks, pq
} i_dst++;} @@ -131,6 +156,8 @@ static void compose_plane(struct vkms_composer *primary_composer, { struct drm_gem_object *plane_obj; struct drm_gem_shmem_object *plane_shmem_obj;
struct drm_framebuffer *fb = &plane_composer->fb;
void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
plane_obj = drm_gem_fb_get_obj(&plane_composer->fb, 0); plane_shmem_obj = to_drm_gem_shmem_obj(plane_obj);
@@ -138,8 +165,13 @@ static void compose_plane(struct vkms_composer *primary_composer, if (WARN_ON(!plane_shmem_obj->vaddr)) return;
- blend(vaddr_out, plane_shmem_obj->vaddr,
primary_composer, plane_composer);
- if (fb->format->format == DRM_FORMAT_ARGB8888)
pixel_blend = &alpha_blend;- else
pixel_blend = &x_blend;- blend(vaddr_out, plane_shmem_obj->vaddr, primary_composer,
plane_composer, pixel_blend);}
static int compose_active_planes(void **vaddr_out, diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index 135140f8e87a..da4251aff67f 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -16,8 +16,9 @@ static const u32 vkms_formats[] = { DRM_FORMAT_XRGB8888, };
-static const u32 vkms_cursor_formats[] = { +static const u32 vkms_plane_formats[] = { DRM_FORMAT_ARGB8888,
- DRM_FORMAT_XRGB8888
};
static struct drm_plane_state * @@ -200,8 +201,8 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, int nformats;
if (type == DRM_PLANE_TYPE_CURSOR) {
formats = vkms_cursor_formats;nformats = ARRAY_SIZE(vkms_cursor_formats);
formats = vkms_plane_formats;nformats = ARRAY_SIZE(vkms_plane_formats);
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 04/26, Daniel Vetter wrote:
On Mon, Apr 26, 2021 at 11:03:15AM +0300, Pekka Paalanen wrote:
On Sat, 24 Apr 2021 05:25:31 -0300 Melissa Wen melissa.srw@gmail.com wrote:
Add support for composing XRGB888 planes in addition to the ARGB8888 format. In the case of an XRGB plane at the top, the composition consists of copying the RGB values of a pixel from src to dst and clearing alpha channel, without the need for alpha blending operations for each pixel.
Blend equations assume a completely opaque background, i.e., primary plane is not cleared before pixel blending but alpha channel is explicitly opaque (a = 0xff). Also, there is room for performance evaluation in switching pixel blend operation according to the plane format.
v4:
- clear alpha channel (0xff) after blend color values by pixel
- improve comments on blend ops to reflect the current state
- describe in the commit message future improvements for plane composition
Signed-off-by: Melissa Wen melissa.srw@gmail.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/vkms/vkms_composer.c | 56 ++++++++++++++++++++++------ drivers/gpu/drm/vkms/vkms_plane.c | 7 ++-- 2 files changed, 48 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 02642801735d..7e01bc39d2a1 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -4,6 +4,7 @@
#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_shmem_helper.h> #include <drm/drm_vblank.h> @@ -64,7 +65,17 @@ static u8 blend_channel(u8 src, u8 dst, u8 alpha) return new_color; }
-static void alpha_blending(const u8 *argb_src, u8 *argb_dst) +/**
- alpha_blend - alpha blending equation
- @argb_src: src pixel on premultiplied alpha mode
- @argb_dst: dst pixel completely opaque
- blend pixels using premultiplied blend formula. The current DRM assumption
- is that pixel color values have been already pre-multiplied with the alpha
- channel values. See more drm_plane_create_blend_mode_property(). Also, this
- formula assumes a completely opaque background.
- */
+static void alpha_blend(const u8 *argb_src, u8 *argb_dst) { u8 alpha;
@@ -72,8 +83,16 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst) argb_dst[0] = blend_channel(argb_src[0], argb_dst[0], alpha); argb_dst[1] = blend_channel(argb_src[1], argb_dst[1], alpha); argb_dst[2] = blend_channel(argb_src[2], argb_dst[2], alpha);
- /* Opaque primary */
- argb_dst[3] = 0xFF;
+}
+/**
- x_blend - blending equation that ignores the pixel alpha
- overwrites RGB color value from src pixel to dst pixel.
- */
+static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst) +{
- memcpy(xrgb_dst, xrgb_src, sizeof(u8) * 3);
Hi,
this function very clearly assumes a very specific pixel format on both source and destination. I think it would be good if the code comments called out exactly which DRM_FORMAT_* they assume. This would be good to do on almost every function that makes such assumptions. I believe that would help code readability, and also point out explicitly which things need to be fixed when you add support for even more pixel formats.
"xrgb" and "argb" are IMO too vague. You might be referring to DRM_FORMAT_XRGB* and DRM_FORMAT_ARGB*, or maybe you are referring to any pixel format that happens to have or not have an alpha channel in addition to the three RGB channels in some order and width.
Being explicit that these refer to specific DRM_FORMAT_* should also help understanding how things work on big-endian CPUs. My current understanding is that this memcpy is correct also on big-endian, given DRM_FORMAT_XRGB8888.
This endianess issue seems a little tricky to me. I remember we have already discussed something similar when introducing alpha blend ops. I took little endian as default by a code comment on include/drm/drm_fourcc.h: DRM formats are little endian. But also, I am not sure if I got it well.
Hmm, or rather, is this particular function intended to be general in the sense that the order of RGB channels does not matter as long as it's the same in both source and destination? Which would mean I had a wrong assumption from the start.
Atm all vkms supports is X/ARGB8888, and even there we throw around random limits. Add support for more pixel formats is definitely on the list, and then all the blend/compose stuff needs to be quite drastically rearchitected.
yes, currently, we only have on vkms these two formats listed as supported (X/ARGB8888), so, I think it is ok, since we do not expected anything other than these two.
I think until we're there documenting what's already documented in the todo list feels like overkill. -Daniel
}
/** @@ -82,16 +101,20 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst)
- @vaddr_src: source address
- @dst_composer: destination framebuffer's metadata
- @src_composer: source framebuffer's metadata
- @pixel_blend: blending equation based on plane format
- Blend the vaddr_src value with the vaddr_dst value using the pre-multiplied
- alpha blending equation, since DRM currently assumes that the pixel color
- values have already been pre-multiplied with the alpha channel values. See
- more drm_plane_create_blend_mode_property(). This function uses buffer's
- metadata to locate the new composite values at vaddr_dst.
- Blend the vaddr_src value with the vaddr_dst value using a pixel blend
- equation according to the plane format and clearing alpha channel to an
- completely opaque background. This function uses buffer's metadata to locate
- the new composite values at vaddr_dst.
- TODO: completely clear the primary plane (a = 0xff) before starting to blend
*/
- pixel color values
static void blend(void *vaddr_dst, void *vaddr_src, struct vkms_composer *dst_composer,
struct vkms_composer *src_composer)
struct vkms_composer *src_composer,void (*pixel_blend)(const u8 *, u8 *)){ int i, j, j_dst, i_dst; int offset_src, offset_dst; @@ -119,7 +142,9 @@ static void blend(void *vaddr_dst, void *vaddr_src,
pixel_src = (u8 *)(vaddr_src + offset_src); pixel_dst = (u8 *)(vaddr_dst + offset_dst);
alpha_blending(pixel_src, pixel_dst);
pixel_blend(pixel_src, pixel_dst);/* clearing alpha channel (0xff)*/memset(vaddr_dst + offset_dst + 3, 0xff, 1);A one byte memset?
Wouldn't pixel_dst[3] = 0xff; be more clear?
yes, I will change it.
Thanks for these suggestions,
Melissa
Thanks, pq
} i_dst++;} @@ -131,6 +156,8 @@ static void compose_plane(struct vkms_composer *primary_composer, { struct drm_gem_object *plane_obj; struct drm_gem_shmem_object *plane_shmem_obj;
struct drm_framebuffer *fb = &plane_composer->fb;
void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
plane_obj = drm_gem_fb_get_obj(&plane_composer->fb, 0); plane_shmem_obj = to_drm_gem_shmem_obj(plane_obj);
@@ -138,8 +165,13 @@ static void compose_plane(struct vkms_composer *primary_composer, if (WARN_ON(!plane_shmem_obj->vaddr)) return;
- blend(vaddr_out, plane_shmem_obj->vaddr,
primary_composer, plane_composer);
- if (fb->format->format == DRM_FORMAT_ARGB8888)
pixel_blend = &alpha_blend;- else
pixel_blend = &x_blend;- blend(vaddr_out, plane_shmem_obj->vaddr, primary_composer,
plane_composer, pixel_blend);}
static int compose_active_planes(void **vaddr_out, diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index 135140f8e87a..da4251aff67f 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -16,8 +16,9 @@ static const u32 vkms_formats[] = { DRM_FORMAT_XRGB8888, };
-static const u32 vkms_cursor_formats[] = { +static const u32 vkms_plane_formats[] = { DRM_FORMAT_ARGB8888,
- DRM_FORMAT_XRGB8888
};
static struct drm_plane_state * @@ -200,8 +201,8 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, int nformats;
if (type == DRM_PLANE_TYPE_CURSOR) {
formats = vkms_cursor_formats;nformats = ARRAY_SIZE(vkms_cursor_formats);
formats = vkms_plane_formats;nformats = ARRAY_SIZE(vkms_plane_formats);
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Mon, 26 Apr 2021 14:31:28 -0300 Melissa Wen melissa.srw@gmail.com wrote:
On 04/26, Daniel Vetter wrote:
On Mon, Apr 26, 2021 at 11:03:15AM +0300, Pekka Paalanen wrote:
On Sat, 24 Apr 2021 05:25:31 -0300 Melissa Wen melissa.srw@gmail.com wrote:
Add support for composing XRGB888 planes in addition to the ARGB8888 format. In the case of an XRGB plane at the top, the composition consists of copying the RGB values of a pixel from src to dst and clearing alpha channel, without the need for alpha blending operations for each pixel.
Blend equations assume a completely opaque background, i.e., primary plane is not cleared before pixel blending but alpha channel is explicitly opaque (a = 0xff). Also, there is room for performance evaluation in switching pixel blend operation according to the plane format.
v4:
- clear alpha channel (0xff) after blend color values by pixel
- improve comments on blend ops to reflect the current state
- describe in the commit message future improvements for plane composition
Signed-off-by: Melissa Wen melissa.srw@gmail.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/vkms/vkms_composer.c | 56 ++++++++++++++++++++++------ drivers/gpu/drm/vkms/vkms_plane.c | 7 ++-- 2 files changed, 48 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 02642801735d..7e01bc39d2a1 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -4,6 +4,7 @@
#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_shmem_helper.h> #include <drm/drm_vblank.h> @@ -64,7 +65,17 @@ static u8 blend_channel(u8 src, u8 dst, u8 alpha) return new_color; }
-static void alpha_blending(const u8 *argb_src, u8 *argb_dst) +/**
- alpha_blend - alpha blending equation
- @argb_src: src pixel on premultiplied alpha mode
- @argb_dst: dst pixel completely opaque
- blend pixels using premultiplied blend formula. The current DRM assumption
- is that pixel color values have been already pre-multiplied with the alpha
- channel values. See more drm_plane_create_blend_mode_property(). Also, this
- formula assumes a completely opaque background.
- */
+static void alpha_blend(const u8 *argb_src, u8 *argb_dst) { u8 alpha;
@@ -72,8 +83,16 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst) argb_dst[0] = blend_channel(argb_src[0], argb_dst[0], alpha); argb_dst[1] = blend_channel(argb_src[1], argb_dst[1], alpha); argb_dst[2] = blend_channel(argb_src[2], argb_dst[2], alpha);
- /* Opaque primary */
- argb_dst[3] = 0xFF;
+}
+/**
- x_blend - blending equation that ignores the pixel alpha
- overwrites RGB color value from src pixel to dst pixel.
- */
+static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst) +{
- memcpy(xrgb_dst, xrgb_src, sizeof(u8) * 3);
Hi,
this function very clearly assumes a very specific pixel format on both source and destination. I think it would be good if the code comments called out exactly which DRM_FORMAT_* they assume. This would be good to do on almost every function that makes such assumptions. I believe that would help code readability, and also point out explicitly which things need to be fixed when you add support for even more pixel formats.
"xrgb" and "argb" are IMO too vague. You might be referring to DRM_FORMAT_XRGB* and DRM_FORMAT_ARGB*, or maybe you are referring to any pixel format that happens to have or not have an alpha channel in addition to the three RGB channels in some order and width.
Being explicit that these refer to specific DRM_FORMAT_* should also help understanding how things work on big-endian CPUs. My current understanding is that this memcpy is correct also on big-endian, given DRM_FORMAT_XRGB8888.
This endianess issue seems a little tricky to me. I remember we have already discussed something similar when introducing alpha blend ops. I took little endian as default by a code comment on include/drm/drm_fourcc.h: DRM formats are little endian. But also, I am not sure if I got it well.
DRM format *definitions* are written on a little-endian CPU. When you have a big-endian CPU, the byte-to-byte memory contents still remain the same. That means if you have a uint32_t pixel in a certain DRM_FORMAT_*, you must always access the bits of it like a little-endian CPU would.
I think this was the "recently" agreed definition, and drivers who do not follow this still exist because fixing them would break userspace?
So if you make the assumption that your machine is little-endian, you have no worries, but you might want to document that you are making this assumption, so that people know it might not be correct on big-endian. It is important to document that it is *unknown* if the code is correct on big-endian, to make people think rather than blindly add a #ifdef big-endian then swap bytes, because the code might be correct already - you just don't know yet.
I wouldn't personally bother thinking about big-endian, other than acknowledging that I don't think about big-endian when writing code, and noticing places where it might make a difference (prime example: accessing pixel components via bytes vs. bits-of-uint32).
Hmm, or rather, is this particular function intended to be general in the sense that the order of RGB channels does not matter as long as it's the same in both source and destination? Which would mean I had a wrong assumption from the start.
Atm all vkms supports is X/ARGB8888, and even there we throw around random limits. Add support for more pixel formats is definitely on the list, and then all the blend/compose stuff needs to be quite drastically rearchitected.
If there are arbitrary limitations, then IMO those are especially important to mention.
yes, currently, we only have on vkms these two formats listed as supported (X/ARGB8888), so, I think it is ok, since we do not expected anything other than these two.
I think until we're there documenting what's already documented in the todo list feels like overkill.
I'm literally asking for single-sentence comments added, like:
/* DRM_FORMAT_XRGB8888 */
It makes all the difference to anyone seeing the code for the first time. Particularly if people want to review patches into this area, because patches are sent via email and therefore completely lack the context of the surrounding code at large and knowledge of which kernel tree they apply to (I'm not a kernel dev), not to mention the trouble of having to apply a patch to be able to look at more context.
Thanks for mentioning https://lkml.org/lkml/2020/8/30/163 in the other email!
Thanks, pq
-Daniel
}
/** @@ -82,16 +101,20 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst)
- @vaddr_src: source address
- @dst_composer: destination framebuffer's metadata
- @src_composer: source framebuffer's metadata
- @pixel_blend: blending equation based on plane format
- Blend the vaddr_src value with the vaddr_dst value using the pre-multiplied
- alpha blending equation, since DRM currently assumes that the pixel color
- values have already been pre-multiplied with the alpha channel values. See
- more drm_plane_create_blend_mode_property(). This function uses buffer's
- metadata to locate the new composite values at vaddr_dst.
- Blend the vaddr_src value with the vaddr_dst value using a pixel blend
- equation according to the plane format and clearing alpha channel to an
- completely opaque background. This function uses buffer's metadata to locate
- the new composite values at vaddr_dst.
- TODO: completely clear the primary plane (a = 0xff) before starting to blend
*/
- pixel color values
static void blend(void *vaddr_dst, void *vaddr_src, struct vkms_composer *dst_composer,
struct vkms_composer *src_composer)
struct vkms_composer *src_composer,void (*pixel_blend)(const u8 *, u8 *)){ int i, j, j_dst, i_dst; int offset_src, offset_dst; @@ -119,7 +142,9 @@ static void blend(void *vaddr_dst, void *vaddr_src,
pixel_src = (u8 *)(vaddr_src + offset_src); pixel_dst = (u8 *)(vaddr_dst + offset_dst);
alpha_blending(pixel_src, pixel_dst);
pixel_blend(pixel_src, pixel_dst);/* clearing alpha channel (0xff)*/memset(vaddr_dst + offset_dst + 3, 0xff, 1);A one byte memset?
Wouldn't pixel_dst[3] = 0xff; be more clear?
yes, I will change it.
Thanks for these suggestions,
Melissa
Thanks, pq
} i_dst++;} @@ -131,6 +156,8 @@ static void compose_plane(struct vkms_composer *primary_composer, { struct drm_gem_object *plane_obj; struct drm_gem_shmem_object *plane_shmem_obj;
struct drm_framebuffer *fb = &plane_composer->fb;
void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
plane_obj = drm_gem_fb_get_obj(&plane_composer->fb, 0); plane_shmem_obj = to_drm_gem_shmem_obj(plane_obj);
@@ -138,8 +165,13 @@ static void compose_plane(struct vkms_composer *primary_composer, if (WARN_ON(!plane_shmem_obj->vaddr)) return;
- blend(vaddr_out, plane_shmem_obj->vaddr,
primary_composer, plane_composer);
- if (fb->format->format == DRM_FORMAT_ARGB8888)
pixel_blend = &alpha_blend;- else
pixel_blend = &x_blend;- blend(vaddr_out, plane_shmem_obj->vaddr, primary_composer,
plane_composer, pixel_blend);}
static int compose_active_planes(void **vaddr_out, diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index 135140f8e87a..da4251aff67f 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -16,8 +16,9 @@ static const u32 vkms_formats[] = { DRM_FORMAT_XRGB8888, };
-static const u32 vkms_cursor_formats[] = { +static const u32 vkms_plane_formats[] = { DRM_FORMAT_ARGB8888,
- DRM_FORMAT_XRGB8888
};
static struct drm_plane_state * @@ -200,8 +201,8 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, int nformats;
if (type == DRM_PLANE_TYPE_CURSOR) {
formats = vkms_cursor_formats;nformats = ARRAY_SIZE(vkms_cursor_formats);
formats = vkms_plane_formats;nformats = ARRAY_SIZE(vkms_plane_formats);
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On 04/27, Pekka Paalanen wrote:
On Mon, 26 Apr 2021 14:31:28 -0300 Melissa Wen melissa.srw@gmail.com wrote:
On 04/26, Daniel Vetter wrote:
On Mon, Apr 26, 2021 at 11:03:15AM +0300, Pekka Paalanen wrote:
On Sat, 24 Apr 2021 05:25:31 -0300 Melissa Wen melissa.srw@gmail.com wrote:
Add support for composing XRGB888 planes in addition to the ARGB8888 format. In the case of an XRGB plane at the top, the composition consists of copying the RGB values of a pixel from src to dst and clearing alpha channel, without the need for alpha blending operations for each pixel.
Blend equations assume a completely opaque background, i.e., primary plane is not cleared before pixel blending but alpha channel is explicitly opaque (a = 0xff). Also, there is room for performance evaluation in switching pixel blend operation according to the plane format.
v4:
- clear alpha channel (0xff) after blend color values by pixel
- improve comments on blend ops to reflect the current state
- describe in the commit message future improvements for plane composition
Signed-off-by: Melissa Wen melissa.srw@gmail.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/vkms/vkms_composer.c | 56 ++++++++++++++++++++++------ drivers/gpu/drm/vkms/vkms_plane.c | 7 ++-- 2 files changed, 48 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 02642801735d..7e01bc39d2a1 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -4,6 +4,7 @@
#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_shmem_helper.h> #include <drm/drm_vblank.h> @@ -64,7 +65,17 @@ static u8 blend_channel(u8 src, u8 dst, u8 alpha) return new_color; }
-static void alpha_blending(const u8 *argb_src, u8 *argb_dst) +/**
- alpha_blend - alpha blending equation
- @argb_src: src pixel on premultiplied alpha mode
- @argb_dst: dst pixel completely opaque
- blend pixels using premultiplied blend formula. The current DRM assumption
- is that pixel color values have been already pre-multiplied with the alpha
- channel values. See more drm_plane_create_blend_mode_property(). Also, this
- formula assumes a completely opaque background.
- */
+static void alpha_blend(const u8 *argb_src, u8 *argb_dst) { u8 alpha;
@@ -72,8 +83,16 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst) argb_dst[0] = blend_channel(argb_src[0], argb_dst[0], alpha); argb_dst[1] = blend_channel(argb_src[1], argb_dst[1], alpha); argb_dst[2] = blend_channel(argb_src[2], argb_dst[2], alpha);
- /* Opaque primary */
- argb_dst[3] = 0xFF;
+}
+/**
- x_blend - blending equation that ignores the pixel alpha
- overwrites RGB color value from src pixel to dst pixel.
- */
+static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst) +{
- memcpy(xrgb_dst, xrgb_src, sizeof(u8) * 3);
Hi,
this function very clearly assumes a very specific pixel format on both source and destination. I think it would be good if the code comments called out exactly which DRM_FORMAT_* they assume. This would be good to do on almost every function that makes such assumptions. I believe that would help code readability, and also point out explicitly which things need to be fixed when you add support for even more pixel formats.
"xrgb" and "argb" are IMO too vague. You might be referring to DRM_FORMAT_XRGB* and DRM_FORMAT_ARGB*, or maybe you are referring to any pixel format that happens to have or not have an alpha channel in addition to the three RGB channels in some order and width.
Being explicit that these refer to specific DRM_FORMAT_* should also help understanding how things work on big-endian CPUs. My current understanding is that this memcpy is correct also on big-endian, given DRM_FORMAT_XRGB8888.
This endianess issue seems a little tricky to me. I remember we have already discussed something similar when introducing alpha blend ops. I took little endian as default by a code comment on include/drm/drm_fourcc.h: DRM formats are little endian. But also, I am not sure if I got it well.
DRM format *definitions* are written on a little-endian CPU. When you have a big-endian CPU, the byte-to-byte memory contents still remain the same. That means if you have a uint32_t pixel in a certain DRM_FORMAT_*, you must always access the bits of it like a little-endian CPU would.
I think this was the "recently" agreed definition, and drivers who do not follow this still exist because fixing them would break userspace?
So if you make the assumption that your machine is little-endian, you have no worries, but you might want to document that you are making this assumption, so that people know it might not be correct on big-endian. It is important to document that it is *unknown* if the code is correct on big-endian, to make people think rather than blindly add a #ifdef big-endian then swap bytes, because the code might be correct already - you just don't know yet.
I wouldn't personally bother thinking about big-endian, other than acknowledging that I don't think about big-endian when writing code, and noticing places where it might make a difference (prime example: accessing pixel components via bytes vs. bits-of-uint32).
Hmm, or rather, is this particular function intended to be general in the sense that the order of RGB channels does not matter as long as it's the same in both source and destination? Which would mean I had a wrong assumption from the start.
Atm all vkms supports is X/ARGB8888, and even there we throw around random limits. Add support for more pixel formats is definitely on the list, and then all the blend/compose stuff needs to be quite drastically rearchitected.
If there are arbitrary limitations, then IMO those are especially important to mention.
yes, currently, we only have on vkms these two formats listed as supported (X/ARGB8888), so, I think it is ok, since we do not expected anything other than these two.
I think until we're there documenting what's already documented in the todo list feels like overkill.
I'm literally asking for single-sentence comments added, like:
/* DRM_FORMAT_XRGB8888 */
It makes all the difference to anyone seeing the code for the first time. Particularly if people want to review patches into this area, because patches are sent via email and therefore completely lack the context of the surrounding code at large and knowledge of which kernel tree they apply to (I'm not a kernel dev), not to mention the trouble of having to apply a patch to be able to look at more context.
Hi,
Thanks for the feedback. These are good points to consider, I will improve comments to make things clear.
Melissa
Thanks for mentioning https://lkml.org/lkml/2020/8/30/163 in the other email!
Thanks, pq
-Daniel
}
/** @@ -82,16 +101,20 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst)
- @vaddr_src: source address
- @dst_composer: destination framebuffer's metadata
- @src_composer: source framebuffer's metadata
- @pixel_blend: blending equation based on plane format
- Blend the vaddr_src value with the vaddr_dst value using the pre-multiplied
- alpha blending equation, since DRM currently assumes that the pixel color
- values have already been pre-multiplied with the alpha channel values. See
- more drm_plane_create_blend_mode_property(). This function uses buffer's
- metadata to locate the new composite values at vaddr_dst.
- Blend the vaddr_src value with the vaddr_dst value using a pixel blend
- equation according to the plane format and clearing alpha channel to an
- completely opaque background. This function uses buffer's metadata to locate
- the new composite values at vaddr_dst.
- TODO: completely clear the primary plane (a = 0xff) before starting to blend
*/
- pixel color values
static void blend(void *vaddr_dst, void *vaddr_src, struct vkms_composer *dst_composer,
struct vkms_composer *src_composer)
struct vkms_composer *src_composer,void (*pixel_blend)(const u8 *, u8 *)){ int i, j, j_dst, i_dst; int offset_src, offset_dst; @@ -119,7 +142,9 @@ static void blend(void *vaddr_dst, void *vaddr_src,
pixel_src = (u8 *)(vaddr_src + offset_src); pixel_dst = (u8 *)(vaddr_dst + offset_dst);
alpha_blending(pixel_src, pixel_dst);
pixel_blend(pixel_src, pixel_dst);/* clearing alpha channel (0xff)*/memset(vaddr_dst + offset_dst + 3, 0xff, 1);A one byte memset?
Wouldn't pixel_dst[3] = 0xff; be more clear?
yes, I will change it.
Thanks for these suggestions,
Melissa
Thanks, pq
} i_dst++;} @@ -131,6 +156,8 @@ static void compose_plane(struct vkms_composer *primary_composer, { struct drm_gem_object *plane_obj; struct drm_gem_shmem_object *plane_shmem_obj;
struct drm_framebuffer *fb = &plane_composer->fb;
void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
plane_obj = drm_gem_fb_get_obj(&plane_composer->fb, 0); plane_shmem_obj = to_drm_gem_shmem_obj(plane_obj);
@@ -138,8 +165,13 @@ static void compose_plane(struct vkms_composer *primary_composer, if (WARN_ON(!plane_shmem_obj->vaddr)) return;
- blend(vaddr_out, plane_shmem_obj->vaddr,
primary_composer, plane_composer);
- if (fb->format->format == DRM_FORMAT_ARGB8888)
pixel_blend = &alpha_blend;- else
pixel_blend = &x_blend;- blend(vaddr_out, plane_shmem_obj->vaddr, primary_composer,
plane_composer, pixel_blend);}
static int compose_active_planes(void **vaddr_out, diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index 135140f8e87a..da4251aff67f 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -16,8 +16,9 @@ static const u32 vkms_formats[] = { DRM_FORMAT_XRGB8888, };
-static const u32 vkms_cursor_formats[] = { +static const u32 vkms_plane_formats[] = { DRM_FORMAT_ARGB8888,
- DRM_FORMAT_XRGB8888
};
static struct drm_plane_state * @@ -200,8 +201,8 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, int nformats;
if (type == DRM_PLANE_TYPE_CURSOR) {
formats = vkms_cursor_formats;nformats = ARRAY_SIZE(vkms_cursor_formats);
formats = vkms_plane_formats;nformats = ARRAY_SIZE(vkms_plane_formats);
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Tue, Apr 27, 2021 at 11:10:59AM +0300, Pekka Paalanen wrote:
On Mon, 26 Apr 2021 14:31:28 -0300 Melissa Wen melissa.srw@gmail.com wrote:
On 04/26, Daniel Vetter wrote:
On Mon, Apr 26, 2021 at 11:03:15AM +0300, Pekka Paalanen wrote:
On Sat, 24 Apr 2021 05:25:31 -0300 Melissa Wen melissa.srw@gmail.com wrote:
Add support for composing XRGB888 planes in addition to the ARGB8888 format. In the case of an XRGB plane at the top, the composition consists of copying the RGB values of a pixel from src to dst and clearing alpha channel, without the need for alpha blending operations for each pixel.
Blend equations assume a completely opaque background, i.e., primary plane is not cleared before pixel blending but alpha channel is explicitly opaque (a = 0xff). Also, there is room for performance evaluation in switching pixel blend operation according to the plane format.
v4:
- clear alpha channel (0xff) after blend color values by pixel
- improve comments on blend ops to reflect the current state
- describe in the commit message future improvements for plane composition
Signed-off-by: Melissa Wen melissa.srw@gmail.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/vkms/vkms_composer.c | 56 ++++++++++++++++++++++------ drivers/gpu/drm/vkms/vkms_plane.c | 7 ++-- 2 files changed, 48 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 02642801735d..7e01bc39d2a1 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -4,6 +4,7 @@
#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_shmem_helper.h> #include <drm/drm_vblank.h> @@ -64,7 +65,17 @@ static u8 blend_channel(u8 src, u8 dst, u8 alpha) return new_color; }
-static void alpha_blending(const u8 *argb_src, u8 *argb_dst) +/**
- alpha_blend - alpha blending equation
- @argb_src: src pixel on premultiplied alpha mode
- @argb_dst: dst pixel completely opaque
- blend pixels using premultiplied blend formula. The current DRM assumption
- is that pixel color values have been already pre-multiplied with the alpha
- channel values. See more drm_plane_create_blend_mode_property(). Also, this
- formula assumes a completely opaque background.
- */
+static void alpha_blend(const u8 *argb_src, u8 *argb_dst) { u8 alpha;
@@ -72,8 +83,16 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst) argb_dst[0] = blend_channel(argb_src[0], argb_dst[0], alpha); argb_dst[1] = blend_channel(argb_src[1], argb_dst[1], alpha); argb_dst[2] = blend_channel(argb_src[2], argb_dst[2], alpha);
- /* Opaque primary */
- argb_dst[3] = 0xFF;
+}
+/**
- x_blend - blending equation that ignores the pixel alpha
- overwrites RGB color value from src pixel to dst pixel.
- */
+static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst) +{
- memcpy(xrgb_dst, xrgb_src, sizeof(u8) * 3);
Hi,
this function very clearly assumes a very specific pixel format on both source and destination. I think it would be good if the code comments called out exactly which DRM_FORMAT_* they assume. This would be good to do on almost every function that makes such assumptions. I believe that would help code readability, and also point out explicitly which things need to be fixed when you add support for even more pixel formats.
"xrgb" and "argb" are IMO too vague. You might be referring to DRM_FORMAT_XRGB* and DRM_FORMAT_ARGB*, or maybe you are referring to any pixel format that happens to have or not have an alpha channel in addition to the three RGB channels in some order and width.
Being explicit that these refer to specific DRM_FORMAT_* should also help understanding how things work on big-endian CPUs. My current understanding is that this memcpy is correct also on big-endian, given DRM_FORMAT_XRGB8888.
This endianess issue seems a little tricky to me. I remember we have already discussed something similar when introducing alpha blend ops. I took little endian as default by a code comment on include/drm/drm_fourcc.h: DRM formats are little endian. But also, I am not sure if I got it well.
DRM format *definitions* are written on a little-endian CPU. When you have a big-endian CPU, the byte-to-byte memory contents still remain the same. That means if you have a uint32_t pixel in a certain DRM_FORMAT_*, you must always access the bits of it like a little-endian CPU would.
I think this was the "recently" agreed definition, and drivers who do not follow this still exist because fixing them would break userspace?
Legacy AddFb might give you a big endia drm_fourcc on some drivers. AddFb2 will not play such tricks.
Also big-endian is dead, imo if someone cares enough about it they could make "fix vkms for big-endian" a nice project :-) -Daniel
So if you make the assumption that your machine is little-endian, you have no worries, but you might want to document that you are making this assumption, so that people know it might not be correct on big-endian. It is important to document that it is *unknown* if the code is correct on big-endian, to make people think rather than blindly add a #ifdef big-endian then swap bytes, because the code might be correct already - you just don't know yet.
I wouldn't personally bother thinking about big-endian, other than acknowledging that I don't think about big-endian when writing code, and noticing places where it might make a difference (prime example: accessing pixel components via bytes vs. bits-of-uint32).
Hmm, or rather, is this particular function intended to be general in the sense that the order of RGB channels does not matter as long as it's the same in both source and destination? Which would mean I had a wrong assumption from the start.
Atm all vkms supports is X/ARGB8888, and even there we throw around random limits. Add support for more pixel formats is definitely on the list, and then all the blend/compose stuff needs to be quite drastically rearchitected.
If there are arbitrary limitations, then IMO those are especially important to mention.
yes, currently, we only have on vkms these two formats listed as supported (X/ARGB8888), so, I think it is ok, since we do not expected anything other than these two.
I think until we're there documenting what's already documented in the todo list feels like overkill.
I'm literally asking for single-sentence comments added, like:
/* DRM_FORMAT_XRGB8888 */
It makes all the difference to anyone seeing the code for the first time. Particularly if people want to review patches into this area, because patches are sent via email and therefore completely lack the context of the surrounding code at large and knowledge of which kernel tree they apply to (I'm not a kernel dev), not to mention the trouble of having to apply a patch to be able to look at more context.
Thanks for mentioning https://lkml.org/lkml/2020/8/30/163 in the other email!
Thanks, pq
-Daniel
}
/** @@ -82,16 +101,20 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst)
- @vaddr_src: source address
- @dst_composer: destination framebuffer's metadata
- @src_composer: source framebuffer's metadata
- @pixel_blend: blending equation based on plane format
- Blend the vaddr_src value with the vaddr_dst value using the pre-multiplied
- alpha blending equation, since DRM currently assumes that the pixel color
- values have already been pre-multiplied with the alpha channel values. See
- more drm_plane_create_blend_mode_property(). This function uses buffer's
- metadata to locate the new composite values at vaddr_dst.
- Blend the vaddr_src value with the vaddr_dst value using a pixel blend
- equation according to the plane format and clearing alpha channel to an
- completely opaque background. This function uses buffer's metadata to locate
- the new composite values at vaddr_dst.
- TODO: completely clear the primary plane (a = 0xff) before starting to blend
*/
- pixel color values
static void blend(void *vaddr_dst, void *vaddr_src, struct vkms_composer *dst_composer,
struct vkms_composer *src_composer)
struct vkms_composer *src_composer,void (*pixel_blend)(const u8 *, u8 *)){ int i, j, j_dst, i_dst; int offset_src, offset_dst; @@ -119,7 +142,9 @@ static void blend(void *vaddr_dst, void *vaddr_src,
pixel_src = (u8 *)(vaddr_src + offset_src); pixel_dst = (u8 *)(vaddr_dst + offset_dst);
alpha_blending(pixel_src, pixel_dst);
pixel_blend(pixel_src, pixel_dst);/* clearing alpha channel (0xff)*/memset(vaddr_dst + offset_dst + 3, 0xff, 1);A one byte memset?
Wouldn't pixel_dst[3] = 0xff; be more clear?
yes, I will change it.
Thanks for these suggestions,
Melissa
Thanks, pq
} i_dst++;} @@ -131,6 +156,8 @@ static void compose_plane(struct vkms_composer *primary_composer, { struct drm_gem_object *plane_obj; struct drm_gem_shmem_object *plane_shmem_obj;
struct drm_framebuffer *fb = &plane_composer->fb;
void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
plane_obj = drm_gem_fb_get_obj(&plane_composer->fb, 0); plane_shmem_obj = to_drm_gem_shmem_obj(plane_obj);
@@ -138,8 +165,13 @@ static void compose_plane(struct vkms_composer *primary_composer, if (WARN_ON(!plane_shmem_obj->vaddr)) return;
- blend(vaddr_out, plane_shmem_obj->vaddr,
primary_composer, plane_composer);
- if (fb->format->format == DRM_FORMAT_ARGB8888)
pixel_blend = &alpha_blend;- else
pixel_blend = &x_blend;- blend(vaddr_out, plane_shmem_obj->vaddr, primary_composer,
plane_composer, pixel_blend);}
static int compose_active_planes(void **vaddr_out, diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index 135140f8e87a..da4251aff67f 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -16,8 +16,9 @@ static const u32 vkms_formats[] = { DRM_FORMAT_XRGB8888, };
-static const u32 vkms_cursor_formats[] = { +static const u32 vkms_plane_formats[] = { DRM_FORMAT_ARGB8888,
- DRM_FORMAT_XRGB8888
};
static struct drm_plane_state * @@ -200,8 +201,8 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, int nformats;
if (type == DRM_PLANE_TYPE_CURSOR) {
formats = vkms_cursor_formats;nformats = ARRAY_SIZE(vkms_cursor_formats);
formats = vkms_plane_formats;nformats = ARRAY_SIZE(vkms_plane_formats);
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Add support to overlay plane, in addition to primary and cursor planes. In this approach, the plane composition still requires an active primary plane and planes are composed associatively in the order: (primary <- overlay) <- cursor
It enables to run the following IGT tests successfully: - kms_plane_cursor: - pipe-A-[overlay, primary, viewport]-size-[64, 128, 256] - kms_atomic: - plane-overlay-legacy and preserves the successful execution of kms_cursor_crc, kms_writeback and kms_flip
Signed-off-by: Melissa Wen melissa.srw@gmail.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/vkms/vkms_composer.c | 27 +++++++++++++++++---------- drivers/gpu/drm/vkms/vkms_drv.c | 5 +++++ drivers/gpu/drm/vkms/vkms_drv.h | 1 + drivers/gpu/drm/vkms/vkms_output.c | 11 ++++++++++- drivers/gpu/drm/vkms/vkms_plane.c | 15 ++++++++++++--- 5 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 7e01bc39d2a1..1b510f3dbcbf 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -176,11 +176,12 @@ static void compose_plane(struct vkms_composer *primary_composer,
static int compose_active_planes(void **vaddr_out, struct vkms_composer *primary_composer, - struct vkms_composer *cursor_composer) + struct vkms_crtc_state *crtc_state) { struct drm_framebuffer *fb = &primary_composer->fb; struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0); struct drm_gem_shmem_object *shmem_obj = to_drm_gem_shmem_obj(gem_obj); + int i;
if (!*vaddr_out) { *vaddr_out = kzalloc(shmem_obj->base.size, GFP_KERNEL); @@ -195,8 +196,14 @@ static int compose_active_planes(void **vaddr_out,
memcpy(*vaddr_out, shmem_obj->vaddr, shmem_obj->base.size);
- if (cursor_composer) - compose_plane(primary_composer, cursor_composer, *vaddr_out); + /* If there are other planes besides primary, we consider the active + * planes should be in z-order and compose them associatively: + * ((primary <- overlay) <- cursor) + */ + for (i = 1; i < crtc_state->num_active_planes; i++) + compose_plane(primary_composer, + crtc_state->active_planes[i]->composer, + *vaddr_out);
return 0; } @@ -218,7 +225,7 @@ void vkms_composer_worker(struct work_struct *work) struct drm_crtc *crtc = crtc_state->base.crtc; struct vkms_output *out = drm_crtc_to_vkms_output(crtc); struct vkms_composer *primary_composer = NULL; - struct vkms_composer *cursor_composer = NULL; + struct vkms_plane_state *act_plane = NULL; bool crc_pending, wb_pending; void *vaddr_out = NULL; u32 crc32 = 0; @@ -242,11 +249,11 @@ void vkms_composer_worker(struct work_struct *work) if (!crc_pending) return;
- if (crtc_state->num_active_planes >= 1) - primary_composer = crtc_state->active_planes[0]->composer; - - if (crtc_state->num_active_planes == 2) - cursor_composer = crtc_state->active_planes[1]->composer; + if (crtc_state->num_active_planes >= 1) { + act_plane = crtc_state->active_planes[0]; + if (act_plane->base.plane->type == DRM_PLANE_TYPE_PRIMARY) + primary_composer = act_plane->composer; + }
if (!primary_composer) return; @@ -255,7 +262,7 @@ void vkms_composer_worker(struct work_struct *work) vaddr_out = crtc_state->active_writeback;
ret = compose_active_planes(&vaddr_out, primary_composer, - cursor_composer); + crtc_state); if (ret) { if (ret == -EINVAL && !wb_pending) kfree(vaddr_out); diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index 2173b82606f6..027ffe759440 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -44,6 +44,10 @@ static bool enable_writeback = true; module_param_named(enable_writeback, enable_writeback, bool, 0444); MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector support");
+static bool enable_overlay; +module_param_named(enable_overlay, enable_overlay, bool, 0444); +MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support"); + DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
static void vkms_release(struct drm_device *dev) @@ -198,6 +202,7 @@ static int __init vkms_init(void)
config->cursor = enable_cursor; config->writeback = enable_writeback; + config->overlay = enable_overlay;
return vkms_create(config); } diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 70fb79621617..ac8c9c2fa4ed 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -89,6 +89,7 @@ struct vkms_device; struct vkms_config { bool writeback; bool cursor; + bool overlay; /* only set when instantiated */ struct vkms_device *dev; }; diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c index 6979fbc7f821..04406bd3ff02 100644 --- a/drivers/gpu/drm/vkms/vkms_output.c +++ b/drivers/gpu/drm/vkms/vkms_output.c @@ -39,7 +39,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) struct drm_connector *connector = &output->connector; struct drm_encoder *encoder = &output->encoder; struct drm_crtc *crtc = &output->crtc; - struct vkms_plane *primary, *cursor = NULL; + struct vkms_plane *primary, *cursor = NULL, *overlay = NULL; int ret; int writeback;
@@ -47,6 +47,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) if (IS_ERR(primary)) return PTR_ERR(primary);
+ if (vkmsdev->config->overlay) { + overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, index); + if (IS_ERR(overlay)) + return PTR_ERR(overlay); + + if (!overlay->base.possible_crtcs) + overlay->base.possible_crtcs = drm_crtc_mask(crtc); + } + if (vkmsdev->config->cursor) { cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index); if (IS_ERR(cursor)) diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index da4251aff67f..107521ace597 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -133,7 +133,7 @@ static int vkms_plane_atomic_check(struct drm_plane *plane, if (IS_ERR(crtc_state)) return PTR_ERR(crtc_state);
- if (plane->type == DRM_PLANE_TYPE_CURSOR) + if (plane->type != DRM_PLANE_TYPE_PRIMARY) can_position = true;
ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state, @@ -200,14 +200,23 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, const u32 *formats; int nformats;
- if (type == DRM_PLANE_TYPE_CURSOR) { + switch (type) { + case DRM_PLANE_TYPE_PRIMARY: + formats = vkms_formats; + nformats = ARRAY_SIZE(vkms_formats); + funcs = &vkms_primary_helper_funcs; + break; + case DRM_PLANE_TYPE_CURSOR: + case DRM_PLANE_TYPE_OVERLAY: formats = vkms_plane_formats; nformats = ARRAY_SIZE(vkms_plane_formats); funcs = &vkms_primary_helper_funcs; - } else { + break; + default: formats = vkms_formats; nformats = ARRAY_SIZE(vkms_formats); funcs = &vkms_primary_helper_funcs; + break; }
plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 1 << index,
On Sat, 24 Apr 2021 05:26:10 -0300 Melissa Wen melissa.srw@gmail.com wrote:
Add support to overlay plane, in addition to primary and cursor planes. In this approach, the plane composition still requires an active primary plane and planes are composed associatively in the order: (primary <- overlay) <- cursor
It enables to run the following IGT tests successfully:
- kms_plane_cursor:
- pipe-A-[overlay, primary, viewport]-size-[64, 128, 256]
- kms_atomic:
- plane-overlay-legacy
and preserves the successful execution of kms_cursor_crc, kms_writeback and kms_flip
Signed-off-by: Melissa Wen melissa.srw@gmail.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Hi,
just curious, when you need to compute a CRC without having a writeback connector output, where do you write the blended result in order to compute CRC?
Thanks, pq
drivers/gpu/drm/vkms/vkms_composer.c | 27 +++++++++++++++++---------- drivers/gpu/drm/vkms/vkms_drv.c | 5 +++++ drivers/gpu/drm/vkms/vkms_drv.h | 1 + drivers/gpu/drm/vkms/vkms_output.c | 11 ++++++++++- drivers/gpu/drm/vkms/vkms_plane.c | 15 ++++++++++++--- 5 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 7e01bc39d2a1..1b510f3dbcbf 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -176,11 +176,12 @@ static void compose_plane(struct vkms_composer *primary_composer,
static int compose_active_planes(void **vaddr_out, struct vkms_composer *primary_composer,
struct vkms_composer *cursor_composer)
struct vkms_crtc_state *crtc_state){ struct drm_framebuffer *fb = &primary_composer->fb; struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0); struct drm_gem_shmem_object *shmem_obj = to_drm_gem_shmem_obj(gem_obj);
int i;
if (!*vaddr_out) { *vaddr_out = kzalloc(shmem_obj->base.size, GFP_KERNEL);
@@ -195,8 +196,14 @@ static int compose_active_planes(void **vaddr_out,
memcpy(*vaddr_out, shmem_obj->vaddr, shmem_obj->base.size);
- if (cursor_composer)
compose_plane(primary_composer, cursor_composer, *vaddr_out);
/* If there are other planes besides primary, we consider the active
* planes should be in z-order and compose them associatively:* ((primary <- overlay) <- cursor)*/for (i = 1; i < crtc_state->num_active_planes; i++)
compose_plane(primary_composer,crtc_state->active_planes[i]->composer,*vaddr_out);return 0;
} @@ -218,7 +225,7 @@ void vkms_composer_worker(struct work_struct *work) struct drm_crtc *crtc = crtc_state->base.crtc; struct vkms_output *out = drm_crtc_to_vkms_output(crtc); struct vkms_composer *primary_composer = NULL;
- struct vkms_composer *cursor_composer = NULL;
- struct vkms_plane_state *act_plane = NULL; bool crc_pending, wb_pending; void *vaddr_out = NULL; u32 crc32 = 0;
@@ -242,11 +249,11 @@ void vkms_composer_worker(struct work_struct *work) if (!crc_pending) return;
- if (crtc_state->num_active_planes >= 1)
primary_composer = crtc_state->active_planes[0]->composer;- if (crtc_state->num_active_planes == 2)
cursor_composer = crtc_state->active_planes[1]->composer;
if (crtc_state->num_active_planes >= 1) {
act_plane = crtc_state->active_planes[0];if (act_plane->base.plane->type == DRM_PLANE_TYPE_PRIMARY)primary_composer = act_plane->composer;}
if (!primary_composer) return;
@@ -255,7 +262,7 @@ void vkms_composer_worker(struct work_struct *work) vaddr_out = crtc_state->active_writeback;
ret = compose_active_planes(&vaddr_out, primary_composer,
cursor_composer);
crtc_state);diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index 2173b82606f6..027ffe759440 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -44,6 +44,10 @@ static bool enable_writeback = true; module_param_named(enable_writeback, enable_writeback, bool, 0444); MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector support");
+static bool enable_overlay; +module_param_named(enable_overlay, enable_overlay, bool, 0444); +MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
static void vkms_release(struct drm_device *dev) @@ -198,6 +202,7 @@ static int __init vkms_init(void)
config->cursor = enable_cursor; config->writeback = enable_writeback;
config->overlay = enable_overlay;
return vkms_create(config);
} diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 70fb79621617..ac8c9c2fa4ed 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -89,6 +89,7 @@ struct vkms_device; struct vkms_config { bool writeback; bool cursor;
- bool overlay; /* only set when instantiated */ struct vkms_device *dev;
}; diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c index 6979fbc7f821..04406bd3ff02 100644 --- a/drivers/gpu/drm/vkms/vkms_output.c +++ b/drivers/gpu/drm/vkms/vkms_output.c @@ -39,7 +39,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) struct drm_connector *connector = &output->connector; struct drm_encoder *encoder = &output->encoder; struct drm_crtc *crtc = &output->crtc;
- struct vkms_plane *primary, *cursor = NULL;
- struct vkms_plane *primary, *cursor = NULL, *overlay = NULL; int ret; int writeback;
@@ -47,6 +47,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) if (IS_ERR(primary)) return PTR_ERR(primary);
- if (vkmsdev->config->overlay) {
overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, index);if (IS_ERR(overlay))return PTR_ERR(overlay);if (!overlay->base.possible_crtcs)overlay->base.possible_crtcs = drm_crtc_mask(crtc);- }
- if (vkmsdev->config->cursor) { cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index); if (IS_ERR(cursor))
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index da4251aff67f..107521ace597 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -133,7 +133,7 @@ static int vkms_plane_atomic_check(struct drm_plane *plane, if (IS_ERR(crtc_state)) return PTR_ERR(crtc_state);
- if (plane->type == DRM_PLANE_TYPE_CURSOR)
if (plane->type != DRM_PLANE_TYPE_PRIMARY) can_position = true;
ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
@@ -200,14 +200,23 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, const u32 *formats; int nformats;
- if (type == DRM_PLANE_TYPE_CURSOR) {
- switch (type) {
- case DRM_PLANE_TYPE_PRIMARY:
formats = vkms_formats;nformats = ARRAY_SIZE(vkms_formats);funcs = &vkms_primary_helper_funcs;break;- case DRM_PLANE_TYPE_CURSOR:
- case DRM_PLANE_TYPE_OVERLAY: formats = vkms_plane_formats; nformats = ARRAY_SIZE(vkms_plane_formats); funcs = &vkms_primary_helper_funcs;
- } else {
break;default: formats = vkms_formats; nformats = ARRAY_SIZE(vkms_formats); funcs = &vkms_primary_helper_funcs;
break;}
plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 1 << index,
On 04/26, Pekka Paalanen wrote:
On Sat, 24 Apr 2021 05:26:10 -0300 Melissa Wen melissa.srw@gmail.com wrote:
Add support to overlay plane, in addition to primary and cursor planes. In this approach, the plane composition still requires an active primary plane and planes are composed associatively in the order: (primary <- overlay) <- cursor
It enables to run the following IGT tests successfully:
- kms_plane_cursor:
- pipe-A-[overlay, primary, viewport]-size-[64, 128, 256]
- kms_atomic:
- plane-overlay-legacy
and preserves the successful execution of kms_cursor_crc, kms_writeback and kms_flip
Signed-off-by: Melissa Wen melissa.srw@gmail.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Hi,
just curious, when you need to compute a CRC without having a writeback connector output, where do you write the blended result in order to compute CRC?
Hi,
so, when no writeback output, an output frame is allocated in the compose_planes functions to get the plane composition there, as shown here: https://lkml.org/lkml/2020/8/30/163
Melissa
Thanks, pq
drivers/gpu/drm/vkms/vkms_composer.c | 27 +++++++++++++++++---------- drivers/gpu/drm/vkms/vkms_drv.c | 5 +++++ drivers/gpu/drm/vkms/vkms_drv.h | 1 + drivers/gpu/drm/vkms/vkms_output.c | 11 ++++++++++- drivers/gpu/drm/vkms/vkms_plane.c | 15 ++++++++++++--- 5 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 7e01bc39d2a1..1b510f3dbcbf 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -176,11 +176,12 @@ static void compose_plane(struct vkms_composer *primary_composer,
static int compose_active_planes(void **vaddr_out, struct vkms_composer *primary_composer,
struct vkms_composer *cursor_composer)
struct vkms_crtc_state *crtc_state){ struct drm_framebuffer *fb = &primary_composer->fb; struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0); struct drm_gem_shmem_object *shmem_obj = to_drm_gem_shmem_obj(gem_obj);
int i;
if (!*vaddr_out) { *vaddr_out = kzalloc(shmem_obj->base.size, GFP_KERNEL);
@@ -195,8 +196,14 @@ static int compose_active_planes(void **vaddr_out,
memcpy(*vaddr_out, shmem_obj->vaddr, shmem_obj->base.size);
- if (cursor_composer)
compose_plane(primary_composer, cursor_composer, *vaddr_out);
/* If there are other planes besides primary, we consider the active
* planes should be in z-order and compose them associatively:* ((primary <- overlay) <- cursor)*/for (i = 1; i < crtc_state->num_active_planes; i++)
compose_plane(primary_composer,crtc_state->active_planes[i]->composer,*vaddr_out);return 0;
} @@ -218,7 +225,7 @@ void vkms_composer_worker(struct work_struct *work) struct drm_crtc *crtc = crtc_state->base.crtc; struct vkms_output *out = drm_crtc_to_vkms_output(crtc); struct vkms_composer *primary_composer = NULL;
- struct vkms_composer *cursor_composer = NULL;
- struct vkms_plane_state *act_plane = NULL; bool crc_pending, wb_pending; void *vaddr_out = NULL; u32 crc32 = 0;
@@ -242,11 +249,11 @@ void vkms_composer_worker(struct work_struct *work) if (!crc_pending) return;
- if (crtc_state->num_active_planes >= 1)
primary_composer = crtc_state->active_planes[0]->composer;- if (crtc_state->num_active_planes == 2)
cursor_composer = crtc_state->active_planes[1]->composer;
if (crtc_state->num_active_planes >= 1) {
act_plane = crtc_state->active_planes[0];if (act_plane->base.plane->type == DRM_PLANE_TYPE_PRIMARY)primary_composer = act_plane->composer;}
if (!primary_composer) return;
@@ -255,7 +262,7 @@ void vkms_composer_worker(struct work_struct *work) vaddr_out = crtc_state->active_writeback;
ret = compose_active_planes(&vaddr_out, primary_composer,
cursor_composer);
crtc_state);diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index 2173b82606f6..027ffe759440 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -44,6 +44,10 @@ static bool enable_writeback = true; module_param_named(enable_writeback, enable_writeback, bool, 0444); MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector support");
+static bool enable_overlay; +module_param_named(enable_overlay, enable_overlay, bool, 0444); +MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
static void vkms_release(struct drm_device *dev) @@ -198,6 +202,7 @@ static int __init vkms_init(void)
config->cursor = enable_cursor; config->writeback = enable_writeback;
config->overlay = enable_overlay;
return vkms_create(config);
} diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 70fb79621617..ac8c9c2fa4ed 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -89,6 +89,7 @@ struct vkms_device; struct vkms_config { bool writeback; bool cursor;
- bool overlay; /* only set when instantiated */ struct vkms_device *dev;
}; diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c index 6979fbc7f821..04406bd3ff02 100644 --- a/drivers/gpu/drm/vkms/vkms_output.c +++ b/drivers/gpu/drm/vkms/vkms_output.c @@ -39,7 +39,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) struct drm_connector *connector = &output->connector; struct drm_encoder *encoder = &output->encoder; struct drm_crtc *crtc = &output->crtc;
- struct vkms_plane *primary, *cursor = NULL;
- struct vkms_plane *primary, *cursor = NULL, *overlay = NULL; int ret; int writeback;
@@ -47,6 +47,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) if (IS_ERR(primary)) return PTR_ERR(primary);
- if (vkmsdev->config->overlay) {
overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, index);if (IS_ERR(overlay))return PTR_ERR(overlay);if (!overlay->base.possible_crtcs)overlay->base.possible_crtcs = drm_crtc_mask(crtc);- }
- if (vkmsdev->config->cursor) { cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index); if (IS_ERR(cursor))
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index da4251aff67f..107521ace597 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -133,7 +133,7 @@ static int vkms_plane_atomic_check(struct drm_plane *plane, if (IS_ERR(crtc_state)) return PTR_ERR(crtc_state);
- if (plane->type == DRM_PLANE_TYPE_CURSOR)
if (plane->type != DRM_PLANE_TYPE_PRIMARY) can_position = true;
ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
@@ -200,14 +200,23 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, const u32 *formats; int nformats;
- if (type == DRM_PLANE_TYPE_CURSOR) {
- switch (type) {
- case DRM_PLANE_TYPE_PRIMARY:
formats = vkms_formats;nformats = ARRAY_SIZE(vkms_formats);funcs = &vkms_primary_helper_funcs;break;- case DRM_PLANE_TYPE_CURSOR:
- case DRM_PLANE_TYPE_OVERLAY: formats = vkms_plane_formats; nformats = ARRAY_SIZE(vkms_plane_formats); funcs = &vkms_primary_helper_funcs;
- } else {
break;default: formats = vkms_formats; nformats = ARRAY_SIZE(vkms_formats); funcs = &vkms_primary_helper_funcs;
break;}
plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 1 << index,
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Melissa Wen -
Pekka Paalanen