Re: [PATCH 0/9] drm/i915: Plane rotation support

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 30, 2013 at 01:46:11PM -0400, Rob Clark wrote:
> On Mon, Sep 30, 2013 at 1:09 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Sep 30, 2013 at 12:15:06PM -0400, Rob Clark wrote:
> >> On Mon, Sep 30, 2013 at 10:44 AM,  <ville.syrjala@linux.intel.com> wrote:
> >> > Recently some people from inside Intel have showed some interest in 180
> >> > degree plane rotation. To avoid a huge mess, I decided that I should
> >> > implement the feature properly.
> >> >
> >> > So I snatched the rotation property from omapdrm, and moved some of the
> >> > code into drm_crtc.c, added a bunch of helper stuff to drm_crtc/rect
> >> > and implemented the relevant bits in i915. I didn't touch cursor or
> >> > primary planes at all. I'm sort of hoping we might do the drm_plane
> >> > conversion sometime soonish and then we'd avoid adding a bunch of
> >> > plane properties to the crtc.
> >>
> >> fwiw, I was leaning towards introducing primary-plane's visible to
> >> userspace after we have atomic modeset (or really, the
> >> propertyification associated with atomic modeset).
> >
> > Less burden of maintaining all the crtc properties if we convert to
> > drm_plane first.
> >
> >>
> >> But that should be independent of drm_plane conversion.  You probably
> >> still want to do something like what I did in omapdrm where you attach
> >> plane properties on the crtc as well for benefit of old userspace.
> >
> > Dunno. There's no old userspace that would use this. There are some
> > folks inside Intel who apparently want rotation for some thing, but
> > I was thinking I'd let them add the properties on the crtc and not
> > upstream any of that.
> 
> for properties that aren't actually handled by the crtc, just make
> your crtc_set_prop() call:
> 
>   intel_plane_set_property(intel_crtc->plane, property, val)
> 
> (or something roughly like that)

Sure it's doable, but I tend to think it's rather pointless since we
don't have any legacy userspace to worry about yet.

> >>
> >> > One thing I don't really like is the current way of stuffing the bit
> >> > number into the enum_list resulting in DRM_ROTATE_FOO being just the bit
> >> > number. I'd prefer to make DRM_ROTATE_FOO the shifted bitmask. But I'm
> >> > not sure if changing that would cause grief to userspace, so I left it
> >> > alone for now.
> >>
> >> I think this shouldn't be visible to userspace.  If I remember
> >> correctly, I just did it this way to make it easier to prevent users
> >> of bitmask property from doing the wrong thing (setting multiple bits,
> >> overlapping bitmask values, etc).
> >
> > Well setting multiple bits should be allowed. If it's not we need to fix
> > it since rotation+reflection sure needs it. Or did you mean users as in
> > driver code in the kernel which registers the bitmask prop?
> 
> yeah, setting multiple bits is allowed.. this is why it isn't an enum
> property ;-)
> 
> I meant users as in driver code rather than userspace.  I guess that
> was worded a bit confusingly.  I blame the cough syrup.

Right.

> 
> > I also just had an idea to expose color keys, bg colors, etc. as bitmask
> > props where each channel is represented by a multi-bit mask, and the
> > whole thing is just one bitmask prop. That would expose some structure
> > to userspace w/o need a new prop type. But maybe we want a specialized
> > type for color props for extra clarty.
> 
> hmm.. should the values be interpreted in terms of current attached fb format?

My current thinking is to go with a fixed 16 bits per channel, and
just throw away any low bits you don't need.

> 
> >>
> >> Anyways, from a really quick look, the core and omapdrm parts look good.
> >>
> >> The drm_rotation_simplify() might be overkill..  or at least userspace
> >> should see what are the supported bitmask flags and not try to ask for
> >> something that is not supported.  Or am I missing something?
> >
> > My main idea was that, for example if the hardware support X and Y reflection,
> > we can expose both 0 and 180 angles and X and Y reflection, and the driver
> > code can then do the simplification to elimnate the 180 degree case. So it's
> > just a convenience tool for the driver authors to eliminate the redundant
> > information. I didn't actually end up using it in i915 since we really
> > don't support anything but 0 and 180 cases, and advertising anything
> > else to userspace would be a bad idea.
> 
> oh, right.. we do have some redundancy in rotation values.  Maybe we
> should have stuck with xyflip/yinvert/xinvert (which was how the omap
> hw worked.. but seemed a bit too hw specific).

That's omap4 or omap5 i take it. Omap3 had 0,90,180,270 + x mirror for DMA
rotation, and 0+90+180+270 + y mirror for VRFB. The mismatch was
interesting since omap angle was clockwise, randr counter-clockwise, and
omap mirroring was post-rotation, randr pre-rotation.

Also the xyflip/etc. is just confusing to me, so I'm happy you went with
randr compatible specification rather than whatever your hw had.

> I guess the main thing I care about is that we don't advertise things
> to userspace that we can't actually do.  I'm not sure what other hw
> out there supports rotation in hw in some form or another, but it
> might be a good time to hear from 'em about whether these property
> values work for them or not.

I think the current stuff is good. In my years  I've seen hardware
that supports everything (omap), just 0+180 (intel gen4+), just
0+180+x+y (intel gen2/3 video overlay), just 0+x (matrox g200+),
just 0 (ati mach64 and r128 era stuff).

-- 
Ville Syrjälä
Intel OTC