[Intel-gfx] [PATCH 0/9] drm/i915: Plane rotation support

[Rob Clark]

On Mon, Sep 30, 2013 at 1:09 PM, Ville Syrjälä
<ville.syrjala at 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 at 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)

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

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

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

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.

BR,
-R

> --
> Ville Syrjälä
> Intel OTC