[RFC v2 5/5] drm/i915: Use generic HDMI infoframe helpers

[Thierry Reding]

On Fri, Dec 07, 2012 at 09:30:34AM +0100, Daniel Vetter wrote:
> On Fri, Dec 7, 2012 at 8:28 AM, Thierry Reding
> <thierry.reding at avionic-design.de> wrote:
> > On Thu, Dec 06, 2012 at 02:55:23PM -0200, Paulo Zanoni wrote:
> >> 2012/12/6 Paulo Zanoni <przanoni at gmail.com>:
> >> For the changes inside intel_hdmi.c, I'd really like your patch to not
> >> touch any of the "write_infoframe" or "set_infoframes" callbacks. I
> >> think you can do everything by just patching intel_set_infoframe,
> >> intel_hdmi_set_avi_infoframe and intel_hdmi_set_spd_infoframe. In one
> >> of those functions you could call something like
> >> "intel_hdmi_infoframe_pack(struct hdmi_avi_infoframe, struct
> >> dip_infoframe)" and then proceed. This way you don 't need to touch
> >> the code that actually writes stuff to the hardware.
> >
> > I think I already explained in my previous mail how IMO this completely
> > defeats the purpose of this patch series. Furthermore the functions that
> > write the infoframes to the hardware themselves could use some
> > refactoring of their own. There is a lot of duplication there.
> 
> I agree with Paulo that we shouldn't touch the hw interfacing
> functions - we've had too many bugs and regressions in there, so
> burned child et al applies. Now I also disagreed with Paulo's idea to
> keep around the infoframe stuff completed for intel_hdmi.c - common
> frameworks for sink handling are useful, if only that if forces people
> to yell at each another and discovered that way how utterly broken
> real world hw is ;-)
> 
> I think the idea Paulo proposed in the first reply of keeping our own
> infoframe packing code, but using the new data structures to construct
> it is worse than the current code: Other people will extend the
> helpers, but totally forget about the special intel-packing function.
> The idea I've discussed a bit with Paulo is to have our own wrapper
> function which intel_hdmi_set_spd_infoframe and
> intel_hdmi_set_avi_infoframe can call. The wrapper allocates a new
> temporary buffer in the common layout without the ECC byte, calls the
> common packing function with that temporary bufffer and then copies
> things over into the intel layout. That way we can use the common
> infoframe construction&packing stuff, without running the risk of
> blowing up the dip write functions right away (now that they seem to
> actually work).

So looking at the differences between the standard HDMI infoframe and
the dip infoframe structures, this would mean copying 3 bytes of header,
add a 0 byte, then copying the remaining bytes. I think all of this can
be done much more easily when writing to the hardware.

Maybe it would help to do something similar to what I did on Tegra to
validate that the same infoframe ends up in the registers as for the old
code. I used a #ifdef HDMI_GENERIC to easily switch between both code
paths and added some debug output to the register writes so that I could
compare both at the register level. If we do the same for Intel hardware
we should be able to fix this properly.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20121207/f6661041/attachment-0001.pgp>