Re: [RFC 0/2] Add HDMI helpers

["Christian König" <deathsimple@vodafone.de>]

On 23.11.2012 10:38, Thierry Reding wrote:
> On Fri, Nov 23, 2012 at 10:24:22AM +0100, Christian König wrote:
>> Hi Thierry,
>>
>> On 21.11.2012 16:01, Thierry Reding wrote:
>>> This small series is very much work in progress, but I still wanted to
>>> get feedback in this early stage to gather requirements from the folks
>>> working on the display drivers that these helpers target.
>>>
>>> Patch 1 in the series adds a generic helper to pack a structure that
>>> describes an HDMI AVI infoframe into the binary format as specified in
>>> the HDMI specification. The resulting binary buffer should be easily
>>> programmable into the HDMI controller.
>>>
>>> Patch 2 provides a helper to fill an HDMI AVI infoframe with data from
>>> a struct drm_display_mode.
>>>
>>> This is all pretty rough right now, but I think some feedback would be
>>> good at this point, to see if the design is at all sensible. I should
>>> also mention that I haven't actually tested this on real hardware yet.
>>> Furthermore I have plans to add something similar for the other types
>>> of infoframes specified by HDMI once the direction becomes clearer.
>> In general I like the idea of storing the informations in a C struct
>> and only packing it into the binary form when needed.
>>
>> I would rather like to see a complete implementation of all the
>> interesting HDMI packets, including the necessary
>> calculations/tables for audio timing recovery etc before it gets
>> committed upstream.
> I'm afraid I'm not very familiar with those packets. Also I haven't seen
> any actual users of audio timing recovery packets so far, so I'm not
> sure that adding support for them would be very useful.

Take a look at: drivers/gpu/drm/radeon/r600_hdmi.c There we have 
r600_hdmi_predefined_acr, r600_hdmi_calc_cts and r600_hdmi_acr doing the 
audio clock recovery calculation/table manually. Those values are only 
used as fallback, since the hardware can do the calculation on their own.

I'm not sure if anybody else is doing this calculation in any driver 
code, but it definitely isn't driver specific and part of the HDMI 
specification, so I would say that it belongs into the general purpose 
file as well.

> What I have added so far is support for audio and vendor-specific
> infoframes, since those have actual users. I also noticed that i915
> implements SPD infoframes as well, so I can extract that code into the
> generic helpers as well.
>
>> Not sure about the separate configuration option. I'm not so much
>> into the config/build system of linux (I know that it is rather
>> complicated), but in general I would like to see that activated
>> automatically as soon as any driver starts using it (or at least the
>> driver depending on that option to be active).
> The configuration option isn't supposed to be user visible. Drivers that
> make use of this are supposed to select HDMI or DRM_HDMI to cause the
> generic code to be pulled in.
>
>> Also two notes about the code in itself:
>> 1. hdmi_avi_infoframe_pack: gets the size of the target buffer, but
>> unfortunately doesn't checks it.
> That should already be fixed in the latest code in my local working
> branch.
>
>> 2. Separate the CRC calculation, we probably need that more than once.
> I've also pulled that out into a separate function. I haven't made it a
> public function yet, but I don't think that'll be necessary either since
> it is used internally by the various infoframe packing functions.
>

Sounds good, let me know when you need a review of the code.

Christian.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel