On Thu, Dec 06, 2012 at 02:11:00PM -0200, Paulo Zanoni wrote: > Hi > > 2012/12/5 Thierry Reding <thierry.reding@xxxxxxxxxxxxxxxxx>: > > 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
Attachment:
pgp5GAo2pbaOd.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
