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

[Thierry Reding]

On Thu, Dec 06, 2012 at 02:11:00PM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2012/12/5 Thierry Reding <thierry.reding at 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 :)

I don't know this intel_infoframes utility, where can I get it from?

> - Why do we need that kconfig stuff? Why not just put all the code
> inside drivers/gpu/drm?

The reason for this is that it was suggested to make these helpers
generic and not DRM specific in order to allow them to be shared with
other subsystems such as V4L.

Daniel already suggested to move the DRM helper into drm_edid.c to get
rid of the additional Kconfig option.

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

That would completely defeat the purpose of this patch series. The goal
was to unify all implementations and only keep the hardware specifics
in the drivers. Clearly the ECC byte is specific to the i915 hardware
and not part of the HDMI specification.

I don't see how any of the i915 code is complicated by this change. The
only complicated thing about it is handling the ECC byte properly. How
about we try to fix things so that i915 works properly with this series
instead of doing some partial conversion?

> - Instead of "converting our own structs into structs that don't
> exactly fit our usage", what I would really like to see is generic
> _optional_ drm functions to help me fill the infoframe data, like the
> things I did in the past. Examples:

But that's precisely what the DRM helper function is supposed to do. It
is very natural to do this with a generic structure as well. The point
being that this is defined by the HDMI specification, so compliant
hardware needs to implement this in some way or another. If Intel chips
require an extra ECC byte to be transmitted as part of the infoframe
header, then that's something the Intel driver should take care of. The
rest of the code can be shared between all implementations.

> (i) I want a function that reads the EDID and tells me whether the
> monitor is going to respect my "underscan" settings (AVI infoframe
> field S) or not.
> (ii) I want unified overscan/underscan properties between all the
> drivers: we need properties to allow requesting specific
> overscan/underscan properties and also properties to allow the driver
> to force underscan in case the monitor doesn't want to cooperate (as
> far as I remember, radeon has these properties, but we need to have
> the behavior explicitly documented so all the drivers can try to
> implement the exact same thing, and then we also need to ask
> user-space guys to add support for these things in their GUIs).
> (iii) I want a function that helps me properly setting/changing the
> correct pixel and picture aspect ratios. it seems the only difference
> between some modes (with different VICs) is the pixel/aspect ratio,
> how do we expose this to the user-space and let it select the one it
> wants? How does the driver know exactly which one is the selected one?
> (iv) I want code that enables user-space to use those modes with
> variable pixel repetition rates and select exactly the one it wants to
> use with the correct pixel repetition.
> (v) I'd like a quirk list to recognize monitors/devices that don't
> work correctly when they receive infoframes, or when the VIC is 0,
> etc.
> (vi) I want magical functions that help me setting all those
> color-related stuff I did not study yet.

Most of the above require interfaces that we don't have.

> (vii) Ideally, I'd like to accomplish all the above by looking at
> "struct drm_display_mode" and maybe passing it to helper functions
> that I can _optionally_ use if I want.

That's what the DRM helper is used for. I don't see how breaking this up
into smaller pieces would be useful. This is data that is transmitted to
HDMI equipment, so ideally the data would be the same independent of the
HDMI source. Having each driver repeat the same sequence to fill in the
same fields in different structures is exactly what this patchset tries
to fix.

> (viii) I don't want the infoframe code to be a "mid layer" that gets
> in the way, I want it to be a set of optional Lego bricks I can use if
> I need.

That's precisely the reason why there's a split between filling the
infoframe structures and packing them. Filling in the data can be done
either using the DRM helper, or drivers can choose to fill it themselves
even though I don't see the point as I already explained above.

Packing on the other hand is a very generic operation and none of the
drivers in the kernel need to pack the data in any different way. Some
hardware may required the packed data to be written to the registers in
some slighly different ways, but as I also already explained I think
that kind of code belongs in the drivers.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20121207/1fb0407a/attachment-0001.pgp>