Hi Guennadi, Thanks for the review. On Thursday 28 July 2011 10:31:24 Guennadi Liakhovetski wrote: > On Mon, 11 Jul 2011, Laurent Pinchart wrote: > > On Friday 24 June 2011 21:45:57 Florian Tobias Schandinat wrote: > > > On 06/24/2011 06:55 PM, Geert Uytterhoeven wrote: > > > > On Fri, Jun 24, 2011 at 08:19, Paul Mundt wrote: > > > >> On Thu, Jun 23, 2011 at 06:08:03PM +0200, Geert Uytterhoeven wrote: > > > >>> On Wed, Jun 22, 2011 at 07:45, Florian Tobias Schandinat wrote: > > > >>>> On 06/21/2011 10:31 PM, Laurent Pinchart wrote: > > > >>>>> On Tuesday 21 June 2011 22:49:14 Geert Uytterhoeven wrote: > > > >>>>>> As FOURCC values are always 4 ASCII characters (hence all 4 > > > >>>>>> bytes must be non-zero), I don't think there are any conflicts > > > >>>>>> with existing values of > > > >>>>>> nonstd. To make it even safer and easier to parse, you could set > > > >>>>>> bit 31 of > > > >>>>>> nonstd as a FOURCC indicator. > > > >>>>> > > > >>>>> I would then create a union between nonstd and fourcc, and > > > >>>>> document nonstd as > > > >>>>> being used for the legacy API only. Most existing drivers use a > > > >>>>> couple of nonstd bits only. The driver that (ab)uses nonstd the > > > >>>>> most is pxafb and uses > > > >>>>> bits 22:0. Bits 31:24 are never used as far as I can tell, so > > > >>>>> nonstd& 0xff000000 != 0 could be used as a FOURCC mode test. > > > >>>>> > > > >>>>> This assumes that FOURCCs will never have their last character > > > >>>>> set to '\0'. Is > > > >>>>> that a safe assumption for the future ? > > > >>>> > > > >>>> Yes, I think. The information I found indicates that space should > > > >>>> be used for padding, so a \0 shouldn't exist. > > > >>>> I think using only the nonstd field and requiring applications to > > > >>>> check the capabilities would be possible, although not fool proof > > > >>>> ;) > > > >>> > > > >>> So we can declare the 8 msb bits of nonstd reserved, and assume > > > >>> FOURCC if any of them is set. > > > >>> > > > >>> Nicely backwards compatible, as sane drivers should reject nonstd > > > >>> values they don't support (apps _will_ start filling in FOURCC > > > >>> values ignoring capabilities, won't they?). > > > >> > > > >> That seems like a reasonable case, but if we're going to do that > > > >> then certainly the nonstd bit encoding needs to be documented and > > > >> treated as a hard ABI. > > > >> > > > >> I'm not so sure about the if any bit in the upper byte is set assume > > > >> FOURCC case though, there will presumably be other users in the > > > >> future that will want bits for themselves, too. What exactly was > > > >> the issue with having a FOURCC capability bit in the upper byte? > > > > > > > > That indeed gives less issues (as long as you don't stuff 8-bit ASCII > > > > in the MSB) and more bits for tradiditional nonstd users. > > > > > > The only disadvantage I can see is that it requires adding this bit in > > > the application and stripping it when interpreting it. But I think the > > > 24 lower bits should be enough for driver specific behavior (as the > > > current values really can only be interpreted per driver). > > > > I'm also not keen on adding/stripping the MSB. It would be easier for > > applications to use FOURCCs directly. > > > > > > BTW, after giving it some more thought: what does the FB_CAP_FOURCC > > > > buys us? It's not like all drivers will support whatever random > > > > FOURCC mode you give them, so you have to check whether it's > > > > supported by doing a set_var first. > > > > > > Because any solution purely based on the nonstd field is insane. The > > > abuse of that field is just too widespread. Drivers that use nonstd > > > usually only check whether it is zero or nonzero and others will just > > > interpret parts of nonstd and ignore the rest. They will happily > > > accept FOURCC values in the nonstd and just doing the wrong thing. > > > Even if we would fix those the problems applications will run into > > > with older kernels will remain. > > > > I agree with Florian here. Many drivers currently check whether nonstd != > > 0. Who knows what kind of values applications feed them ? > > FWIW, I prefer the original Laurent's proposal, with a slight uncertainty > about whether we need the .capability field, or whether the try-and-check > approach is sufficient. Try-and-check would be better, but unfortunately I don't think it would work. Existing drivers will either ignore the new FB_VMODE_FOURCC flag or fail. I expect many drivers that ignore it to still accept it blindly, and not clear it, so applications won't be able to find out whether the flag is supported. > As for struct fb_var_screeninfo fields to support switching to a FOURCC > mode, I also prefer an explicit dedicated flag to specify switching to it. > Even though using FOURCC doesn't fit under the notion of a videomode, using > one of .vmode bits is too tempting, so, I would actually take the plunge and > use FB_VMODE_FOURCC. Another option would be to consider any grayscale > 1 value as a FOURCC. I've briefly checked the in-tree drivers: they only assign grayscale with 0 or 1, and check whether grayscale is 0 or different than 0. If a userspace application only sets grayscale > 1 when talking to a driver that supports the FOURCC-based API, we could get rid of the flag. What can't be easily found out is whether existing applications set grayscale to a > 1 value. They would break when used with FOURCC-aware drivers if we consider any grayscale > 1 value as a FOURCC. Is that a risk we can take ? > As for the actual location of the fourcc code, I would leave .nonstd > alone: who knows, maybe drivers will need both, whereas using grayscale > and fourcc certainly doesn't make any sense. And I personally don't see a > problem with using a union: buggy applications are, well, buggy... I agree with this. > Actually, since in FOURCC mode we don't need any of > > __u32 bits_per_pixel; /* guess what */ > __u32 grayscale; /* != 0 Graylevels instead of colors */ > > struct fb_bitfield red; /* bitfield in fb mem if true color, */ > struct fb_bitfield green; /* else only length is significant */ > struct fb_bitfield blue; > struct fb_bitfield transp; /* transparency */ > > so, we could put them all in the union for the case, if we need anything > else for the fourcc configuration in the future. Good point. It might make sense to exclude bits_per_pixel from the union though, as that's interesting information for applications. The red, green, blue and transp fields are less useful. > > I'd like to reach an agreement on the API, and implement it. I'm fine > > with either grayscale or nonstd to store the FOURCC (with a slight > > preference for grayscale), and with either a vmode flag or using the > > most significant byte of the grayscale/nonstd field to detect FOURCC > > mode. I believe FB_CAP_FOURCC (or something similar) is needed. -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
