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). At least that was my takeaway from the discussion yesterday with Paulo, and now reading his second mail I see that he doesn't detail how the intel infoframe packing function could also work. So it reads like the exact same idea as in the first mail unfortunately. Or Paulo and I still have a big disagreement, in which case I need to discuss things a bit more with him about how we could tackle this. Because to reiterate my point from the beginning, I think common sink handling helpers are a Good Thing. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
