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@xxxxxxxxxxxxxxxxx> wrote: > > On Thu, Dec 06, 2012 at 02:55:23PM -0200, Paulo Zanoni wrote: > >> 2012/12/6 Paulo Zanoni <przanoni@xxxxxxxxx>: > >> 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
Attachment:
pgpsPxQ135FEv.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
