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@avionic-design.de wrote:
On Thu, Dec 06, 2012 at 02:55:23PM -0200, Paulo Zanoni wrote:
2012/12/6 Paulo Zanoni przanoni@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