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

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

On Wed,  9 Jul 2014 09:19:08 +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 one
> opencoded define.
> 
> v2: Use enum to determine primary plane, suggested by Pekka Paalanen.
>     Use libobj for replacement ffs(), suggested by walter harms
> 
> Signed-off-by: Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>
> Cc: Pekka Paalanen <ppaalanen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: walter harms <wharms-fPG8STNUNVg@public.gmane.org>

My concerns have been addressed. On a second read, I found another
suspicious thing below.

> ---
>  configure.ac          |   5 +-
>  libobj/ffs.c          |  14 ++++
>  src/drmmode_display.c | 216 ++++++++++++++++++++++++++++++++++++++++++--------
>  src/drmmode_display.h |  10 ++-
>  4 files changed, 212 insertions(+), 33 deletions(-)
>  create mode 100644 libobj/ffs.c
> 
> diff --git a/configure.ac b/configure.ac
> index 1c1a36d..1694465 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -74,10 +74,13 @@ 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.47])
>  PKG_CHECK_MODULES([PCIACCESS], [pciaccess >= 0.10])
>  AM_CONDITIONAL(DRM, test "x$DRM" = xyes)
>  
> +AC_CONFIG_LIBOBJ_DIR(libobj)
> +AC_REPLACE_FUNCS(ffs)
> +
>  PKG_CHECK_MODULES(UDEV, [libudev], [udev=yes], [udev=no])
>  if test x"$udev" = xyes; then
>          AC_DEFINE(HAVE_UDEV,1,[Enable udev-based monitor hotplug detection])
> diff --git a/libobj/ffs.c b/libobj/ffs.c
> new file mode 100644
> index 0000000..2d44dcc
> --- /dev/null
> +++ b/libobj/ffs.c
> @@ -0,0 +1,14 @@
> +extern int ffs(unsigned int value);
> +
> +int ffs(unsigned int value)
> +{
> +	int bit;
> +
> +	if (value == 0)
> +		return 0;
> +
> +	bit = 0;
> +	while ((value & (1 << bit++)) == 0)
> +		;
> +	return bit;
> +}
> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index c533324..e854502 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -56,6 +56,8 @@
>  
>  #include "driver.h"
>  
> +#define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2 /* from libdrm 2.4.55 */
> +
>  static struct dumb_bo *dumb_bo_create(int fd,
>  			  const unsigned width, const unsigned height,
>  			  const unsigned bpp)
> @@ -300,6 +302,132 @@ create_pixmap_for_fbcon(drmmode_ptr drmmode,
>  
>  #endif
>  
> +static unsigned
> +rotation_index(unsigned rotation)
> +{
> +	return ffs(rotation) - 1;
> +}
> +
> +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; drmmode_crtc->primary_plane_id == 0 && i < plane_resources->count_planes; i++) {
> +		drmModePlane *drm_plane;
> +		drmModeObjectPropertiesPtr proplist;
> +		int is_primary = -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; is_primary == -1 && j < proplist->count_props; j++) {
> +			drmModePropertyPtr prop;
> +
> +			prop = drmModeGetProperty(drmmode->fd, proplist->props[j]);
> +			if (!prop)
> +				continue;
> +
> +			if (strcmp(prop->name, "type") == 0) {
> +				for (k = 0; k < prop->count_enums; k++) {
> +					if (prop->enums[k].value != proplist->prop_values[j])
> +						continue;
> +
> +					is_primary = strcmp(prop->enums[k].name, "Primary") == 0;
> +					break;
> +				}
> +			}
> +
> +			drmModeFreeProperty(prop);
> +		}
> +
> +		if (is_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;

Comparing the above assignments to...

> +static Bool
> +rotation_set(xf86CrtcPtr crtc, unsigned rotation)
> +{
> +	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
> +	drmmode_ptr drmmode = drmmode_crtc->drmmode;
> +
> +	if (drmmode_crtc->current_rotation == rotation)
> +		return TRUE;
> +
> +	if ((drmmode_crtc->supported_rotations & rotation) == 0)
> +		return FALSE;
> +
> +	if (drmModeObjectSetProperty(drmmode->fd,
> +				     drmmode_crtc->primary_plane_id,
> +				     DRM_MODE_OBJECT_PLANE,
> +				     drmmode_crtc->rotation_prop_id,
> +				     drmmode_crtc->map_rotations[rotation_index(rotation)]))

...the use here, it does not look like you are passing
prop->enums[k].value here. It is like you are missing ffs() here or
having a 1<< too much in the assignment.

Btw. would it be possible to do e.g. rotate-90 with reflect?


Thanks,
pq
_______________________________________________
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