From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 0/9] drm/i915: Plane rotation support Date: Mon, 30 Sep 2013 21:31:45 +0300 Message-ID: <20130930183145.GG9395@intel.com> References: <1380552272-32585-1-git-send-email-ville.syrjala@linux.intel.com> <20130930170949.GE9395@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Rob Clark Cc: Intel Graphics Development , "dri-devel@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Mon, Sep 30, 2013 at 01:46:11PM -0400, Rob Clark wrote: > On Mon, Sep 30, 2013 at 1:09 PM, Ville Syrj=E4l=E4 > wrote: > > On Mon, Sep 30, 2013 at 12:15:06PM -0400, Rob Clark wrote: > >> On Mon, Sep 30, 2013 at 10:44 AM, wro= te: > >> > 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 fo= rmat? 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 goo= d. > >> > >> 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 refl= ection, > > we can expose both 0 and 180 angles and X and Y reflection, and the dri= ver > > 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 redunda= nt > > 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=E4l=E4 Intel OTC