Re: [PATCH 2/2] drm: tegra: Add HDMI support

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Nov 10, 2012 at 10:01:18PM +0100, Thierry Reding wrote:
> On Fri, Nov 09, 2012 at 05:00:54PM +0100, Christian König wrote:
> > On 09.11.2012 16:45, Rafał Miłecki wrote:
> > >2012/11/9 Thierry Reding <thierry.reding@avionic-design.de>:
> > >>+/* all fields little endian */
> > >>+struct hdmi_audio_infoframe {
> > >>+       /* PB0 */
> > >>+       u8 csum;
> > >>+
> > >>+       /* PB1 */
> > >>+       unsigned cc:3; /* channel count */
> > >>+       unsigned res1:1;
> > >>+       unsigned ct:4; /* coding type */
> > >>+
> > >>+       /* PB2 */
> > >>+       unsigned ss:2; /* sample size */
> > >>+       unsigned sf:3; /* sample frequency */
> > >>+       unsigned res2:3;
> > >>+
> > >>+       /* PB3 */
> > >>+       unsigned cxt:5; /* coding extention type */
> > >>+       unsigned res3:3;
> > >>+
> > >>+       /* PB4 */
> > >>+       u8 ca; /* channel/speaker allocation */
> > >>+
> > >>+       /* PB5 */
> > >>+       unsigned res5:3;
> > >>+       unsigned lsv:4; /* level shift value */
> > >>+       unsigned dm_inh:1; /* downmix inhibit */
> > >>+
> > >>+       /* PB6-10 reserved */
> > >>+       u8 res6;
> > >>+       u8 res7;
> > >>+       u8 res8;
> > >>+       u8 res9;
> > >>+       u8 res10;
> > >>+} __packed;
> > >I was told it won't work on different endian devices. See
> > >[RFC][PATCH] drm/radeon/hdmi: define struct for AVI infoframe
> > >http://lists.freedesktop.org/archives/dri-devel/2012-May/022544.html
> > 
> > Yeah, that's indeed true. And honestly adding just another
> > implementation of the HDMI info frames sounds like somebody should
> > finally sit down and implement it in a common drm_hdmi.c
> 
> So I've been looking at what most other implementations do and it seems
> a lot just fill the AVI infoframe with zeroes while only a few actually
> try to put useful information in them. Still in order to plan for a
> generic solution, I thought maybe something like the below set of
> structures and functions could work:
> 
> /*
>  * Structure that contains the infoframe fields in a form that allows them to
>  * be easily accessed from C code.
>  */
> struct hdmi_avi_infoframe;
> 
> /*
>  * DRM helper to fill a struct hdmi_avi_infoframe with information taken from
>  * a struct drm_display_mode. Fields that cannot automatically be derived by
>  * looking at a struct drm_display_mode are set to the default values.
>  */
> int drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> 					     struct drm_display_mode *mode);
> 
> /*
>  * Packs the struct hdmi_avi_infoframe into a binary buffer that can be
>  * programmed to the hardware-specific registers.
>  */
> ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame,
> 				void *buffer, size_t size);
> 
> Such a scheme would allow DRM drivers to call the helper and tweak the
> fields in the structure if the want or need to and call the packing
> function to obtain a buffer that they can write to the controller.
> 
> Does that sound at all reasonable?

Sounds good, especially the disdinction between the infoframe creation and
packing. E.g. on intel sdvo outputs we may not put in one of the ECC bytes
(since the hw creates it), so we need our own packing code there.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch