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

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1.1: Type: text/plain, Size: 2370 bytes --]

On Thu, Dec 06, 2012 at 02:55:23PM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2012/12/6 Paulo Zanoni <przanoni@gmail.com>:
> > Hi
> >
> > 2012/12/5 Thierry Reding <thierry.reding@avionic-design.de>:
> >> Use the generic HDMI infoframe helpers to get rid of the duplicate
> >> implementation in the i915 driver.
> >
> > A few comments:
> >
> > - I've compiled your patches and now "intel_infoframes -d" tells my my
> > infoframes are full of zeroes. So there's a bug somewhere... I have to
> > say that the i915 infoframe registers are quite complicated and it
> > took me a long time to fix a lot of its problems, so please don't
> > break it and read below for a suggestion :)
> >
> > - Why do we need that kconfig stuff? Why not just put all the code
> > inside drivers/gpu/drm?
> >
> > - I really like having "common code between drivers merged", but I
> > really don't see how the i915 driver is benefiting from this change.
> > We're just basically complicating intel_hdmi.c to use a new struct
> > that doesn't fit our needs due to the lack of the ECC byte. My idea:
> > Instead of calling hdmi_avi_infoframe_pack, why don't we just
> > implement intel_hdmi_avi_infoframe_pack that converts a "struct
> > hdmi_avi_infoframe" into our own good-old "struct dip_infoframe" that
> > has the cool ECC value in the right place? This would probably
> > drastically reduce your chances of introducing bugs in our code :)
> 
> Just to be a little bit more clear about this paragraph since Daniel
> asked me a few questions:
> 
> 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.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel