Re: [RESEND PATCH v3 06/11] drm: add DT bindings documentation for atmel-hlcdc-dc driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 ?

> Okay, in this case, using a list is a better idea, don't you think ?

I'd prefer an array if possible, as that would be easier to use for drivers.

In any case, we need to define who allocates and frees the array or the list 
elements, how and when.

-- 
Regards,

Laurent Pinchart