From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pekka Paalanen Subject: Re: [PATCH] modesetting: Support native primary plane rotation Date: Thu, 10 Jul 2014 21:09:27 +0300 Message-ID: <20140710210927.62254c9a@farn.lan> References: <1404889221-6549-1-git-send-email-chris@chris-wilson.co.uk> <1404893948-18819-1-git-send-email-chris@chris-wilson.co.uk> <20140709125712.11e3da22@farn.lan> <20140709100259.GB8986@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140709100259.GB8986-aII6DKEyn0pWYbfKqPwjAkR8Iwp7RQ6xAL8bYrjMMd8@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: xorg-devel-bounces-go0+a7rfsptAfugRpC6u6w@public.gmane.org Sender: "xorg-devel" To: Chris Wilson Cc: xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, walter harms List-Id: dri-devel@lists.freedesktop.org On Wed, 9 Jul 2014 11:02:59 +0100 Chris Wilson wrote: > On Wed, Jul 09, 2014 at 12:57:12PM +0300, Pekka Paalanen wrote: > > On Wed, 9 Jul 2014 09:19:08 +0100 > > Chris Wilson 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 > > > Cc: Pekka Paalanen > > > Cc: walter harms > > > > My concerns have been addressed. On a second read, I found another > > suspicious thing below. > > > > > + 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. > > It doesn't take the enum.value but 1 << enum.value. Aah, it is not an 'enum', it is a 'bitmask'! Okay, I see it now, I think. 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