From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [RFC v2 5/5] drm/i915: Use generic HDMI infoframe helpers Date: Fri, 7 Dec 2012 09:49:12 +0100 Message-ID: <20121207084912.GA9427@avionic-0098.adnet.avionic-design.de> References: <1354725944-1862-1-git-send-email-thierry.reding@avionic-design.de> <1354725944-1862-6-git-send-email-thierry.reding@avionic-design.de> <20121207072810.GB3058@avionic-0098.adnet.avionic-design.de> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1521401618==" Return-path: Received: from moutng.kundenserver.de (moutng.kundenserver.de [212.227.126.187]) by gabe.freedesktop.org (Postfix) with ESMTP id 89C66E5C3B for ; Fri, 7 Dec 2012 00:49:16 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1521401618== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="YZ5djTAD1cGYuMQK" Content-Disposition: inline --YZ5djTAD1cGYuMQK Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Dec 07, 2012 at 09:30:34AM +0100, Daniel Vetter wrote: > On Fri, Dec 7, 2012 at 8:28 AM, Thierry Reding > wrote: > > On Thu, Dec 06, 2012 at 02:55:23PM -0200, Paulo Zanoni wrote: > >> 2012/12/6 Paulo Zanoni : > >> 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. >=20 > 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 ;-) >=20 > 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 --YZ5djTAD1cGYuMQK Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (GNU/Linux) iQIcBAEBAgAGBQJQwa2IAAoJEN0jrNd/PrOhLY4P/0W6TBLRVqUZdYQ8R1+Q9a8j eJ9/p92mrdhrhtnwKLwBhzS6pcOYAONPahbe8Po9Uyoml9wFufZHCmxSQOSw5rGL hQkElJcQjZgpDk8UAnMzq0zev5Ztc5tg1bq0KUf20Wn2K3jX+pAQ/DC5wa6phcae bzfqMM07wcDfWswUE/dxNuLDv9AjRUFyfWk5SLEsgAE2ThBECEVpswxzkgAAuLdh 8JbKIIqvQvnIobGGIWcjabXzdYZYdVsCFBDqIDIpmfQqWhjt6cdmbx4giE/4AcwU 2WsPUwgPfLeKsDRek3g0sVq0tLfLnADUirxRU8XZX082n+QPms4Bm9ztq28Ppp07 VyXh+RAKcwq+bKfgOD+G3NX0ZDskP7ValTZqGNg80XRbsYMTHmj2JT8Tm5MteXKu TIwnL8FWCJ4bNzsGg9ytAG9+JziOskV4QS3LfzZiMZu8ImuGPArMkVZ3r/+Zte+9 7B+zg7uSmiCy3QTzoHjivtvitUiejx0XXyWd3EhrebDXLZXrCPMYcsqw2q+DikuG f8aja7DVprX5Q4ATWW5Yuk67Y9BsU6KuxkxTZcL9nugovojDxD/5Y0qRX2Sg21BI qFRZ5im51PkH/Nu4XaDaPlWP2m6e2kZuyxSiyY5jctA7ekw2IEmFD87i43UivIii dOwxc8sxHeuFovfBb7Iq =85m+ -----END PGP SIGNATURE----- --YZ5djTAD1cGYuMQK-- --===============1521401618== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============1521401618==--