[RFC][PATCH] drm/radeon/hdmi: define struct for AVI infoframe

[Rafał Miłecki]

2012/5/6 Daniel Vetter <daniel at ffwll.ch>:
> On Sun, May 06, 2012 at 05:22:59PM +0100, Dave Airlie wrote:
>> On Sun, May 6, 2012 at 5:19 PM, Rafał Miłecki <zajec5 at gmail.com> wrote:
>> > 2012/5/6 Rafał Miłecki <zajec5 at gmail.com>:
>> >> diff --git a/drivers/gpu/drm/radeon/r600_hdmi.c b/drivers/gpu/drm/radeon/r600_hdmi.c
>> >> index c308432..b14c90a 100644
>> >> --- a/drivers/gpu/drm/radeon/r600_hdmi.c
>> >> +++ b/drivers/gpu/drm/radeon/r600_hdmi.c
>> >> @@ -134,78 +134,22 @@ static void r600_hdmi_infoframe_checksum(uint8_t packetType,
>> >>  }
>> >>
>> >>  /*
>> >> - * build a HDMI Video Info Frame
>> >> + * Upload a HDMI AVI Infoframe
>> >>  */
>> >> -static void r600_hdmi_videoinfoframe(
>> >> -       struct drm_encoder *encoder,
>> >> -       enum r600_hdmi_color_format color_format,
>> >> -       int active_information_present,
>> >> -       uint8_t active_format_aspect_ratio,
>> >> -       uint8_t scan_information,
>> >> -       uint8_t colorimetry,
>> >> -       uint8_t ex_colorimetry,
>> >> -       uint8_t quantization,
>> >> -       int ITC,
>> >> -       uint8_t picture_aspect_ratio,
>> >> -       uint8_t video_format_identification,
>> >> -       uint8_t pixel_repetition,
>> >> -       uint8_t non_uniform_picture_scaling,
>> >> -       uint8_t bar_info_data_valid,
>> >> -       uint16_t top_bar,
>> >> -       uint16_t bottom_bar,
>> >> -       uint16_t left_bar,
>> >> -       uint16_t right_bar
>> >> -)
>> >
>> > In case someone wonders about the reason: I think it's really ugly to
>> > have a function taking 18 arguments, 17 of them related to the
>> > infoframe. It makes much more sense for me to use struct for that.
>> > While working on that I though it's reasonable to prepare nice
>> > bitfield __packed struct ready-to-be-written to the GPU registers.
>>
>> won't this screw up on other endian machines?
>
> ... and can we have this in a slightly generic way maybe? We have copies
> of this in i915 and nouveau.

More or less...

I know Intel defines struct dip_infoframe.

Unfortunately it uses things like:
	uint8_t type;		/* HB0 */
	uint8_t ver;		/* HB1 */
	uint8_t len;		/* HB2 - body len, not including checksum */
	uint8_t ecc;		/* Header ECC */
which we don't need in radeon. But maybe we could just ignore that
additional fields in radeon and live with just a little bigger struct.

However Intel still has fields like:
/* PB1 - Y 6:5, A 4:4, B 3:2, S 1:0 */
uint8_t Y_A_B_S;
/* PB2 - C 7:6, M 5:4, R 3:0 */
uint8_t C_M_R;

That means it still requires setting some fields with bitshifting and
ORing. If this is possible to make it endian-safe I've love to split
such a fields into separated-and-shorter ones.

-- 
Rafał