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

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

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.

> 
> > 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?

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.

> 
> 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.

-- 
Ville Syrjälä
Intel OTC