[Intel-gfx] [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Daniel Vetter]

On Tue, Apr 14, 2015 at 05:49:40PM +0530, Jindal, Sonika wrote:
> 
> 
> On 4/14/2015 5:09 AM, Matt Roper wrote:
> >On Mon, Apr 13, 2015 at 01:49:22PM +0300, Ville Syrjälä wrote:
> >>On Mon, Apr 13, 2015 at 03:53:22PM +0530, Jindal, Sonika wrote:
> >>>
> >>>
> >>>On 4/13/2015 3:40 PM, Ville Syrjälä wrote:
> >>>>On Mon, Apr 13, 2015 at 09:36:02AM +0530, Jindal, Sonika wrote:
> >>>>>
> >>>>>
> >>>>>On 4/10/2015 8:14 PM, Ville Syrjälä wrote:
> >>>>>>On Fri, Apr 10, 2015 at 04:17:17PM +0200, Daniel Vetter wrote:
> >>>>>>>On Fri, Apr 10, 2015 at 02:37:29PM +0530, Sonika Jindal wrote:
> >>>>>>>>v2: Moving creation of property in a function, checking for 90/270
> >>>>>>>>rotation simultaneously (Chris)
> >>>>>>>>Letting primary plane to be positioned
> >>>>>>>>v3: Adding if/else for 90/270 and rest params programming, adding check for
> >>>>>>>>pixel_format, some cleanup (review comments)
> >>>>>>>>v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset
> >>>>>>>>and size programming (Ville)
> >>>>>>>>v5: Rebased on -nightly and Tvrtko's series for gtt remapping.
> >>>>>>>>v6: Rebased on -nightly (Tvrtko's series merged)
> >>>>>>>>v7: Moving pixel_format check to intel_atomic_plane_check (Matt)
> >>>>>>>>
> >>>>>>>>Signed-off-by: Sonika Jindal <sonika.jindal at intel.com>
> >>>>>>>>Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
> >>>>>>>
> >>>>>>>Patches are missing the Testcase: tag, please add that. Also, are all the
> >>>>>>>igt committed or not yet? Pulled these two in meanwhile.
> >>>>>>
> >>>>>>Things are going somewhat broken because you didn't apply my plane
> >>>>>>state stuff. Hmm. Actually it sort of looks that it might work by luck
> >>>>>>as long as the primary plane doesn't get clipped since this is bashing
> >>>>>>the user state directly into the hardware registers and not the derived
> >>>>>>state (ie. clipped coordinates).
> >>>>>>
> >>>>>I was hoping your changes get merged, but not sure why they are held up.
> >>>>>
> >>>>>>Also I see my review comment about the 90 vs. 270 rotation mixup was
> >>>>>>ignored at least.
> >>>>>>
> >>>>>I never really got the to understand the need of reversing 90 and 270 :)
> >>>>>The last discussion was not concluded, AFAIR.
> >>>>>Things look correct to me and work fine with the testcase also.
> >>>>>Is there something completely different which I am unable to get?
> >>>>
> >>>>BSpec gives me the impression the hw rotation is cw, whereas the drm one
> >>>>is ccw.
> >>>>
> >>>Yes, HW rotation is cw as per bspec. In drm, where do we consider it as
> >>>anti-cw? I assume it is cw because all the macros work as expected, ie cw.
> >>
> >>The ccw rule was inherited from XRandR. I'm not sure what macros you
> >>mean, but drm_rect_rotate() will certainly give you wrong results if
> >>you assume cw.
> >
> >It seems like this information needs to be added to the property section
> >of the DRM DocBook; continuing to follow XRandR probably makes sense if
> >that's what's agreed on, but there's no indication of that design
> >philosophy in the actual DRM documentation today, so we're in danger of
> >having different driver authors use conflicting interpretations.
> >
> Ok, I will create a DocBook patch with description for 90/270 rotation
> (Anyways 90/270 rotation is not added in the rotation property in
> documentation). Will there be any other place for this or i915's rotation
> property?
> 
> Also, I will send a patch to flip 90/270 in i915. I am just worried, that it
> will look confusing to check for DRM_ROTATE_90 and use PLANE_CTL_ROTATE_270
> for programming. I will add a comment though.

Just add a comment stating that drm is ccw (to stay compatible with
Xrandr) and i915 hw is cw. That should explain it all.

There's other places where the linux kernel and intel hw people disagree
on definitions, it just happens.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch