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

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

On Wed, 9 Jul 2014 11:02:59 +0100
Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org> 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 <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.
> > 
> > > +				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