On Mon, Jul 21, 2014 at 08:30:31PM +0200, Laurent Pinchart wrote: > Hi Boris and Thierry, > > On Monday 21 July 2014 16:21:36 Boris BREZILLON wrote: > > On Mon, 21 Jul 2014 15:54:12 +0200 Thierry Reding wrote: > > > On Mon, Jul 21, 2014 at 03:47:52PM +0200, Laurent Pinchart wrote: > > >> On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote: > > >>> On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote: > > >>>> On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote: > > >>>>> On Monday 21 July 2014 14:55:23 Thierry Reding wrote: > > >>>>>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote: > > >>>>>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote: > > >>>>>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote: > > [snip] > > > >>>>>>>>> Quoting from your paste: > > >>>>>>>>> + const enum video_bus_format *bus_formats; > > >>>>>>>>> + int nbus_formats; > > >>>>>>>>> > > >>>>>>>>> Do we really need more than one? > > >>>>>>>> > > >>>>>>>> We do if we want to replace the color_formats and bpc fields. > > >>>>>>> > > >>>>>>> Yes, that's what I was about to answer :-). > > >>>>>> > > >>>>>> Maybe we don't need to replace color_formats and bpc field > > >>>>>> immediately. That could be done in a follow-up patch. > > >>>>> > > >>>>> We don't need to replace them right now, but we should at least > > >>>>> agree on how to replace them. Introducing a new field that would > > >>>>> need to be replaced in the near future when removing color_formats > > >>>>> and bpc would be a waste of time. > > >>>> > > >>>> Sure. One of the problems I see with replacing color_formats and bpc > > >>>> with the above is that some of the bits within color_formats are set > > >>>> when the EDID is parsed. That implies that if they are replaced with > > >>>> an array of formats, the array would need to be reallocated during > > >>>> EDID parsing. That sounds like ugliness. > > >>>> > > >>>> But if you can find a nice way to make it work that'd be great. > > >>> > > >>> How about using a list instead of an array ? > > >>> This way we can add elements to this list when parsing the EDID. > > >>> > > >>> Or we can just define a maximum size for the bus_formats array when > > >>> retrieving this info from EDID. If I'm correct we have at most 18 bus > > >>> > > >>> formats: > > >>> - 3 color formats: > > >>> * RGB 4:4:4 > > >>> * YCbCr 4:4:4 > > >>> * YCbCr 4:4:2 > > >>> - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color > > >> > > >> bpc isn't a bitmask, so EDID supports up to three formats only. > > >> > > >> The color_formats field is computed in the drm_add_display_info() > > >> function. You could easily turn it into a local variable and allocate > > >> and fill the formats array at the end of the function. > > > > > > But you also need to be careful to keep whatever formats the driver might > > > have set explicitly. > > Do we have drivers that explicitly add formats to the formats parsed from EDID > data ? If so, what's the use case ? Drivers could specifically add them if there's no EDID or if the EDID is known to be broken. If the former this is probably irrelevant. In the latter maybe a better option would be to ignore the EDID-probed ones rather than use the union of those provided by the driver and EDID. Thierry
Attachment:
pgpgt9SttF6sM.pgp
Description: PGP signature
