[PATCH 2/2] xfree86/modes: Make cursor position transform a helper function

[Aaron Plattner]

On 09/29/2011 09:47 AM, Jamey Sharp wrote:
> I think this patch is totally sensible.
>
> Reviewed-by: Jamey Sharp <jamey at minilop.net>

Thanks for looking at these, Jamey.

> For the other patch in the series, "xfree86/modes: Let the driver handle
> the transform", I don't think I can review it. I don't know the relevant
> code and the patch touches quite a bit.
>
> I tried to see if I could write a different patch that might get you the
> same effect with fewer core server changes. I couldn't, but maybe the
> approach I had in mind will help somehow.
>
> I imagined adding a "crtc->funcs->handle_transform" callback that, if
> it's present and returns True for a given transform, indicates that the
> driver is handling that transform and the server should render without
> any transformation. In that case, I'd let crtc->transform_in_use be
> False, and I hoped most of the code paths your patch had to change would
> already have the right behavior then.

I thought about that, but actually handling the transform in the 
hardware is pretty much something that needs to happen during a modeset, 
which means the driver needs to take the transform into consideration 
during set_mode_major.  At that point, it was easier to let the driver 
just set a flag instead of making it move some of its transform code 
into a new function.

Unfortunately, some of the existing code uses transform_in_use to mean 
that an actual transformation matrix was specified, so you can have 
crtc->rotation != RR_Rotate_0 but crtc->transform_in_use == FALSE if 
there's just a rotation.  The fact that the rotation & reflection is not 
part of the transformation matrix is a weird legacy thing... maybe we 
should consider trying to hide that in the DIX implementation and have 
it just feed a matrix into the DDX interface.

> I got confused by how to handle LeaveVT/EnterVT or modesetting
> failure--and whether transform was even the right thing to look at, or
> if crtc->rotation is what you're after.

LeaveVT and EnterVT are controlled by the driver, which is responsible 
for clearing and setting the desired modes, so the driver can do 
whatever it needs to to restore the right configuration on EnterVT. 
Just calling xf86SetDesiredModes(pScrn) should be enough most of the 
time, I would imagine.

The way it's written in this change, the driver is responsible for 
making sure driverIsPerformingTransform doesn't change if set_mode_major 
fails.  I didn't try to wrap my head around what would happen if various 
parts of the old-school modesetting path failed and the driver were 
using driverIsPerformingTransform.  I think the advice I'd give would 
just be, "implement set_mode_major already."

> Jamey
>
> On Thu, Aug 25, 2011 at 04:35:10PM -0700, Aaron Plattner wrote:
>> When the driver can handle the crtc transform in hardware, it sets
>> crtc->driverIsPerformingTransform, which turns off both the shadow
>> layer and the cursor's position-transforming code.  However, some
>> drivers actually do require the cursor position to still be
>> transformed in these cases.  Move the cursor position transform into a
>> helper function that can be called by such drivers.
>>
>> Signed-off-by: Aaron Plattner<aplattner at nvidia.com>
>> ---
>>   hw/xfree86/modes/xf86Crtc.h    |    8 +++++
>>   hw/xfree86/modes/xf86Cursors.c |   57 +++++++++++++++++++++------------------
>>   2 files changed, 39 insertions(+), 26 deletions(-)
>>
>> diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h
>> index 0d7a6a6..ffb2eff 100644
>> --- a/hw/xfree86/modes/xf86Crtc.h
>> +++ b/hw/xfree86/modes/xf86Crtc.h
>> @@ -948,6 +948,14 @@ xf86_hide_cursors (ScrnInfoPtr scrn);
>>   extern _X_EXPORT void
>>   xf86_cursors_fini (ScreenPtr screen);
>>
>> +/**
>> + * Transform the cursor's coordinates based on the crtc transform.  Normally
>> + * this is done by the server, but if crtc->driverIsPerformingTransform is TRUE,
>> + * then the server does not transform the cursor position automatically.
>> + */
>> +extern _X_EXPORT void
>> +xf86CrtcTransformCursorPos (xf86CrtcPtr crtc, int *x, int *y);
>> +
>>   /*
>>    * For overlay video, compute the relevant CRTC and
>>    * clip video to that.
>> diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c
>> index 4281ab3..276bd27 100644
>> --- a/hw/xfree86/modes/xf86Cursors.c
>> +++ b/hw/xfree86/modes/xf86Cursors.c
>> @@ -337,7 +337,36 @@ xf86_show_cursors (ScrnInfoPtr scrn)
>>   	    xf86_crtc_show_cursor (crtc);
>>       }
>>   }
>> -
>> +
>> +void xf86CrtcTransformCursorPos (xf86CrtcPtr crtc, int *x, int *y)
>> +{
>> +    ScrnInfoPtr scrn = crtc->scrn;
>> +    ScreenPtr screen = scrn->pScreen;
>> +    xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
>> +    xf86CursorInfoPtr cursor_info = xf86_config->cursor_info;
>> +    xf86CursorScreenPtr ScreenPriv =
>> +	(xf86CursorScreenPtr)dixLookupPrivate(&screen->devPrivates,
>> +					      xf86CursorScreenKey);
>> +    struct pict_f_vector v;
>> +    int dx, dy;
>> +
>> +    v.v[0] = (*x + ScreenPriv->HotX) + 0.5;
>> +    v.v[1] = (*y + ScreenPriv->HotY) + 0.5;
>> +    v.v[2] = 1;
>> +    pixman_f_transform_point (&crtc->f_framebuffer_to_crtc,&v);
>> +    /* cursor will have 0.5 added to it already so floor is sufficent */
>> +    *x = floor (v.v[0]);
>> +    *y = floor (v.v[1]);
>> +    /*
>> +     * Transform position of cursor upper left corner
>> +     */
>> +    xf86_crtc_rotate_coord_back (crtc->rotation, cursor_info->MaxWidth,
>> +				 cursor_info->MaxHeight, ScreenPriv->HotX,
>> +				 ScreenPriv->HotY,&dx,&dy);
>> +    *x -= dx;
>> +    *y -= dy;
>> +}
>> +
>>   static void
>>   xf86_crtc_set_cursor_position (xf86CrtcPtr crtc, int x, int y)
>>   {
>> @@ -346,36 +375,12 @@ xf86_crtc_set_cursor_position (xf86CrtcPtr crtc, int x, int y)
>>       xf86CursorInfoPtr	cursor_info = xf86_config->cursor_info;
>>       DisplayModePtr	mode =&crtc->mode;
>>       Bool		in_range;
>> -    int			dx, dy;
>>
>>       /*
>>        * Transform position of cursor on screen
>>        */
>>       if (crtc->transform_in_use&&  !crtc->driverIsPerformingTransform)
>> -    {
>> -	ScreenPtr	screen = scrn->pScreen;
>> -	xf86CursorScreenPtr ScreenPriv =
>> -	    (xf86CursorScreenPtr)dixLookupPrivate(&screen->devPrivates,
>> -						  xf86CursorScreenKey);
>> -	struct pict_f_vector   v;
>> -
>> -	v.v[0] = (x + ScreenPriv->HotX) + 0.5;
>> -	v.v[1] = (y + ScreenPriv->HotY) + 0.5;
>> -	v.v[2] = 1;
>> -	pixman_f_transform_point (&crtc->f_framebuffer_to_crtc,&v);
>> -	/* cursor will have 0.5 added to it already so floor is sufficent */
>> -	x = floor (v.v[0]);
>> -	y = floor (v.v[1]);
>> -	/*
>> -	 * Transform position of cursor upper left corner
>> -	 */
>> -	xf86_crtc_rotate_coord_back (crtc->rotation,
>> -				     cursor_info->MaxWidth,
>> -				     cursor_info->MaxHeight,
>> -				     ScreenPriv->HotX, ScreenPriv->HotY,&dx,&dy);
>> -	x -= dx;
>> -	y -= dy;
>> -   }
>> +	xf86CrtcTransformCursorPos(crtc,&x,&y);
>>       else
>>       {
>>   	x -= crtc->x;
>> --
>> 1.7.4.1