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

[Daniel Vetter]

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).

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