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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
