On Wed, 9 Jul 2014 09:19:08 +0100
Chris Wilson <chris at chris-wilson.co.uk> 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 at chris-wilson.co.uk>
> Cc: Pekka Paalanen <ppaalanen at gmail.com>
> Cc: walter harms <wharms at bfs.de>
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
