Re: [PATCH] modesetting: Support native primary plane rotation

[Pekka Paalanen <ppaalanen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Wed,  9 Jul 2014 08:00:21 +0100
Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org> wrote:

> With the advent of universal drm planes and the introduction of generic
> plane properties for rotations, we can query and program the hardware
> for native rotation support.
> 
> NOTE: this depends upon the next release of libdrm to remove some
> opencoded defines.
> 
> Signed-off-by: Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>
> ---
>  configure.ac          |   2 +-
>  src/drmmode_display.c | 223 +++++++++++++++++++++++++++++++++++++++++++-------
>  src/drmmode_display.h |   7 +-
>  3 files changed, 199 insertions(+), 33 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 1c1a36d..0b4e857 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -74,7 +74,7 @@ AM_CONDITIONAL(HAVE_XEXTPROTO_71, [ test "$HAVE_XEXTPROTO_71" = "yes" ])
>  # Checks for header files.
>  AC_HEADER_STDC
>  
> -PKG_CHECK_MODULES(DRM, [libdrm >= 2.4.46])
> +PKG_CHECK_MODULES(DRM, [libdrm >= 2.4.54]) #.55 required for universal planes
>  PKG_CHECK_MODULES([PCIACCESS], [pciaccess >= 0.10])
>  AM_CONDITIONAL(DRM, test "x$DRM" = xyes)
>  
> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index c533324..aaeda39 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -56,6 +56,11 @@
>  
>  #include "driver.h"
>  
> +#define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
> +#define DRM_PLANE_TYPE_OVERLAY 0
> +#define DRM_PLANE_TYPE_PRIMARY 1
> +#define DRM_PLANE_TYPE_CURSOR  2

Hi,

is this really something that is guaranteed to be kernel ABI stable?

I mean, the 'type' property is an enum. I have never seen the enum
(numerical) values being defined in any public ABI header. Instead,
the property system has a mechanism for listing the enum values by
name string.

I have assumed that the name string is what is guaranteed ABI, and
the numerical value is just an arbitrary handle. When I added
universal planes support to Weston (not merged yet), I look up the
numerical value by the name, instead of hardcoding the numerical
value.

Should you do the same here, or are the numerical values really
(going to be) part of the ABI?


Thanks,
pq


> +static void
> +rotation_init(xf86CrtcPtr crtc)
> +{
> +	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
> +	drmmode_ptr drmmode = drmmode_crtc->drmmode;
> +	drmModePlaneRes *plane_resources;
> +	int i, j, k;
> +
> +	drmSetClientCap(drmmode->fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
> +
> +	plane_resources = drmModeGetPlaneResources(drmmode->fd);
> +	if (plane_resources == NULL)
> +		return;
> +
> +	for (i = 0; i < plane_resources->count_planes; i++) {
> +		drmModePlane *drm_plane;
> +		drmModeObjectPropertiesPtr proplist;
> +		int type = -1;
> +
> +		drm_plane = drmModeGetPlane(drmmode->fd,
> +					    plane_resources->planes[i]);
> +		if (drm_plane == NULL)
> +			continue;
> +
> +		if (!(drm_plane->possible_crtcs & (1 << drmmode_crtc->index)))
> +			goto free_plane;
> +
> +		proplist = drmModeObjectGetProperties(drmmode->fd,
> +						      drm_plane->plane_id,
> +						      DRM_MODE_OBJECT_PLANE);
> +		if (proplist == NULL)
> +			goto free_plane;
> +
> +		for (j = 0; type == -1 && j < proplist->count_props; j++) {
> +			drmModePropertyPtr prop;
> +
> +			prop = drmModeGetProperty(drmmode->fd, proplist->props[j]);
> +			if (!prop)
> +				continue;
> +
> +			if (strcmp(prop->name, "type") == 0)
> +				type = proplist->prop_values[j];
> +
> +			drmModeFreeProperty(prop);
> +		}
> +
> +		if (type == DRM_PLANE_TYPE_PRIMARY) {
> +			drmmode_crtc->primary_plane_id = drm_plane->plane_id;
> +
> +			for (j = 0; drmmode_crtc->rotation_prop_id == 0 && j < proplist->count_props; j++) {
> +				drmModePropertyPtr prop;
> +
> +				prop = drmModeGetProperty(drmmode->fd, proplist->props[j]);
> +				if (!prop)
> +					continue;
> +
> +				if (strcmp(prop->name, "rotation") == 0) {
> +					drmmode_crtc->rotation_prop_id = proplist->props[j];
> +					drmmode_crtc->current_rotation = proplist->prop_values[j];
> +					for (k = 0; k < prop->count_enums; k++) {
> +						int rr = -1;
> +						if (strcmp(prop->enums[k].name, "rotate-0") == 0)
> +							rr = RR_Rotate_0;
> +						else if (strcmp(prop->enums[k].name, "rotate-90") == 0)
> +							rr = RR_Rotate_90;
> +						else if (strcmp(prop->enums[k].name, "rotate-180") == 0)
> +							rr = RR_Rotate_180;
> +						else if (strcmp(prop->enums[k].name, "rotate-270") == 0)
> +							rr = RR_Rotate_270;
> +						else if (strcmp(prop->enums[k].name, "reflect-x") == 0)
> +							rr = RR_Reflect_X;
> +						else if (strcmp(prop->enums[k].name, "reflect-y") == 0)
> +							rr = RR_Reflect_Y;
> +						if (rr != -1) {
> +							drmmode_crtc->map_rotations[rotation_index(rr)] = 1 << prop->enums[k].value;
> +							drmmode_crtc->supported_rotations |= rr;
> +						}
> +					}
> +				}
> +
> +				drmModeFreeProperty(prop);
> +			}
> +		}
> +
> +		drmModeFreeObjectProperties(proplist);
> +free_plane:
> +		drmModeFreePlane(drm_plane);
> +	}
> +}
_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel