Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1311590 > unrolled thread

[PATCH v4 0/1] Introduce Innosilicon HDMI driver on Rockchip platforms

Started byYakir Yang <ykk@rock-chips.com>
First post2016-01-18 15:50 +0100
Last post2016-01-19 05:00 +0100
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/1] Introduce Innosilicon HDMI driver on Rockchip platforms Yakir Yang <ykk@rock-chips.com> - 2016-01-18 15:50 +0100
    Re: [PATCH v4] drm/rockchip: hdmi: add Innosilicon HDMI support Russell King - ARM Linux <linux@arm.linux.org.uk> - 2016-01-18 18:20 +0100
      Re: [PATCH v4] drm/rockchip: hdmi: add Innosilicon HDMI support Yakir Yang <ykk@rock-chips.com> - 2016-01-19 05:00 +0100

#1311590 — [PATCH v4 0/1] Introduce Innosilicon HDMI driver on Rockchip platforms

FromYakir Yang <ykk@rock-chips.com>
Date2016-01-18 15:50 +0100
Subject[PATCH v4 0/1] Introduce Innosilicon HDMI driver on Rockchip platforms
Message-ID<qSji2-1ZW-15@gated-at.bofh.it>
Here are a brief introduction to Innosilicon HDMI IP:
 - Support HDMI 1.4a, HDCP 1.2 and DVI 1.0 standard compliant transmitter
 - Support HDMI1.4 a/b 3D function defined in HDMI 1.4 a/b spec
 - Digital video interface supports a pixel size of 24, 30, 36, 48bits color
   depth in RGB
 - S/PDIF output supports PCM, Dolby Digital, DTS digital audio transmission
   (32-192kHz Fs) using IEC60958 and IEC 61937
 - The EDID and CEC function are also supported by Innosilicon HDMI Transmitter
   Controlle


Changes in v4:
- Modify the commit title "drm/rockchip: hdmi: ..." (Mark)
- Correct the "DKMS" to "DPMS" (Mark)
- Fix over 80 characters problems (Mark)
- Remove encoder .prepare/.commit helper functions, and move the vop mode
  configure function into encoder .enable helper functions. (Mark)

Changes in v3:
- Use encoder enable/disable function, and remove the encoder DPMS function
- Keep HDMI PLL power on in standby mode

Changes in v2:
- Using DRM atomic helper functions for connector init (Mark)
- Remove "hdmi->connector.encoder = encoder;" (Mark)

Yakir Yang (1):
  drm/rockchip: hdmi: add Innosilicon HDMI support

 drivers/gpu/drm/rockchip/Kconfig     |   8 +
 drivers/gpu/drm/rockchip/Makefile    |   1 +
 drivers/gpu/drm/rockchip/inno_hdmi.c | 992 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/rockchip/inno_hdmi.h | 364 +++++++++++++
 4 files changed, 1365 insertions(+)
 create mode 100644 drivers/gpu/drm/rockchip/inno_hdmi.c
 create mode 100644 drivers/gpu/drm/rockchip/inno_hdmi.h

-- 
1.9.1

[toc] | [next] | [standalone]

#1311686 — Re: [PATCH v4] drm/rockchip: hdmi: add Innosilicon HDMI support

FromRussell King - ARM Linux <linux@arm.linux.org.uk>
Date2016-01-18 18:20 +0100
SubjectRe: [PATCH v4] drm/rockchip: hdmi: add Innosilicon HDMI support
Message-ID<qSlDb-3Oc-3@gated-at.bofh.it>
In reply to#1311590
Hi,

Some comments below...

On Mon, Jan 18, 2016 at 10:42:20PM +0800, Yakir Yang wrote:
> +static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi)
> +{
> +	char info[HDMI_SIZE_AVI_INFOFRAME] = {0};
> +	int avi_color_mode;
> +	int i;
> +
> +	hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_BUF_INDEX, INFOFRAME_AVI);
> +
> +	info[0] = 0x82;
> +	info[1] = 0x02;
> +	info[2] = 0x0D;
> +	info[3] = info[0] + info[1] + info[2];
> +
> +	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB)
> +		avi_color_mode = AVI_COLOR_MODE_RGB;
> +	else if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_YUV444)
> +		avi_color_mode = AVI_COLOR_MODE_YCBCR444;
> +	else if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_YUV422)
> +		avi_color_mode = AVI_COLOR_MODE_YCBCR422;
> +	else
> +		avi_color_mode = AVI_COLOR_MODE_RGB;
> +
> +	info[4] = (avi_color_mode << 5);
> +	info[5] = (AVI_COLORIMETRY_NO_DATA << 6) |
> +		  (AVI_CODED_FRAME_ASPECT_NO_DATA << 4) |
> +		  ACTIVE_ASPECT_RATE_SAME_AS_CODED_FRAME;
> +
> +	info[6] = 0;
> +	info[7] = hdmi->hdmi_data.vic;
> +
> +	if (hdmi->hdmi_data.vic == 6 || hdmi->hdmi_data.vic == 7 ||
> +	    hdmi->hdmi_data.vic == 21 || hdmi->hdmi_data.vic == 22)
> +		info[8] = 1;
> +	else
> +		info[8] = 0;
> +
> +	/* Calculate avi info frame checKsum */
> +	for (i = 4; i < HDMI_SIZE_AVI_INFOFRAME; i++)
> +		info[3] += info[i];
> +	info[3] = 0x100 - info[3];
> +
> +	for (i = 0; i < HDMI_SIZE_AVI_INFOFRAME; i++)
> +		hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i, info[i]);
> +
> +	return 0;

Is there a reason why the helpers in drivers/video/hdmi.c can't be used
to generate this?

hdmi_avi_infoframe_init / hdmi_avi_infoframe_pack and
drm_hdmi_avi_infoframe_from_display_mode ?

> +}
> +
> +static int inno_hdmi_config_video_vsi(struct inno_hdmi *hdmi)
> +{
> +	char info[HDMI_SIZE_VSI_INFOFRAME] = {0};
> +	int i;
> +
> +	hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, m_PACKET_VSI_EN,
> +		  v_PACKET_VSI_EN(0));
> +
> +	hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_BUF_INDEX, INFOFRAME_VSI);
> +
> +	/* Header Bytes */
> +	info[0] = 0x81;
> +	info[1] = 0x01;
> +
> +	/* PB1 - PB3 contain the 24bit IEEE Registration Identifier */
> +	info[4] = 0x03;
> +	info[5] = 0x0c;
> +	info[6] = 0x00;
> +
> +	/* PB4 - HDMI_Video_Format into bits 7:5 */
> +	info[7] = 0;
> +
> +	/*
> +	 * PB5 - Depending on the video format, this byte will contain
> +	 * either the HDMI_VIC code in buts 7:0, OR the 3D_Structure in
> +	 * bits 7:4.
> +	 */
> +	info[2] = 0x06 - 2;
> +	info[8] = 0;
> +	info[9] = 0;
> +
> +	info[3] = info[0] + info[1] + info[2];
> +
> +	/* Calculate info frame checKsum */
> +	for (i = 4; i < HDMI_SIZE_VSI_INFOFRAME; i++)
> +		info[3] += info[i];
> +	info[3] = 0x100 - info[3];
> +
> +	for (i = 0; i < HDMI_SIZE_VSI_INFOFRAME; i++)
> +		hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i, info[i]);
> +
> +	hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, m_PACKET_VSI_EN,
> +		  v_PACKET_VSI_EN(1));
> +
> +	return 0;

hdmi_vendor_infoframe_init / hdmi_vendor_infoframe_pack?

You can probably use the same function to upload the control packet
too - something like:

static int inno_hdmi_upload_frame(struct inno_hdmi *hdmi, int setup_rc,
	union hdmi_infoframe *frame, u32 mask, u32 disable, u32 enable)
{
	if (mask)
		hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, mask, disable);

	if (setup_rc >= 0) {
		u8 packed_frame[YOUR_MAXIMUM_INFO_FRAME_SIZE];
		ssize_t rc, i;

		rc = hdmi_infoframe_pack(frame, packed_frame,
					 sizeof(packed_frame));
		if (rc < 0)
			return rc;

		for (i = 0; i < rc; i++)
			hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i, buf[i]);

		if (mask)
			hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, mask, enable);
	}

	return setup_rc;
}

static int inno_hdmi_config_video_vsi(struct inno_hdmi *hdmi)
{
	union hdmi_infoframe frame;

	rc = drm_hdmi_vendor_infoframe_from_display_mode(&frame.vendor.hdmi,
							 the_drm_display_mode);

	return inno_hdmi_upload_frame(hdmi, rc, &frame, m_PACKET_VSI_EN,
				      v_PACKET_VSI_EN(0), v_PACKET_VSI_EN(1));
}

> +static int inno_hdmi_i2c_wait(struct inno_hdmi *hdmi)
> +{
> +	struct inno_hdmi_i2c *i2c = hdmi->i2c;
> +	int stat;
> +
> +	stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
> +	if (!stat) {
> +		stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
> +		if (!stat)
> +			return -EAGAIN;

What's the reason for this double wait_for_completion?  This probably
needs a comment, otherwise it looks like some weird cut-n-paste error.

> +static int inno_hdmi_i2c_write(struct inno_hdmi *hdmi, struct i2c_msg *msgs)
> +{
> +	struct inno_hdmi_i2c *i2c = hdmi->i2c;

You seem to have a local i2c pointer...

> +
> +	/*
> +	 * The DDC module only support read EDID message, so
> +	 * we assume that each word write to this i2c adapter
> +	 * should be the offset of EDID word address.
> +	 */
> +	if ((msgs->len != 1) ||
> +	    ((msgs->addr != DDC_ADDR) && (msgs->addr != DDC_SEGMENT_ADDR)))
> +		return -EINVAL;
> +
> +	reinit_completion(&i2c->cmp);
> +
> +	if (msgs->addr == DDC_SEGMENT_ADDR)
> +		hdmi->i2c->segment_addr = msgs->buf[0];
> +	if (msgs->addr == DDC_ADDR)
> +		hdmi->i2c->ddc_addr = msgs->buf[0];
> +
> +	/* Set edid fifo first addr */
> +	hdmi_writeb(hdmi, HDMI_EDID_FIFO_OFFSET, 0x00);
> +
> +	/* Set edid word address 0x00/0x80 */
> +	hdmi_writeb(hdmi, HDMI_EDID_WORD_ADDR, hdmi->i2c->ddc_addr);
> +
> +	/* Set edid segment pointer */
> +	hdmi_writeb(hdmi, HDMI_EDID_SEGMENT_POINTER, hdmi->i2c->segment_addr);

but then you don't use it in the four locations above.


-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1311935 — Re: [PATCH v4] drm/rockchip: hdmi: add Innosilicon HDMI support

FromYakir Yang <ykk@rock-chips.com>
Date2016-01-19 05:00 +0100
SubjectRe: [PATCH v4] drm/rockchip: hdmi: add Innosilicon HDMI support
Message-ID<qSvCz-23U-21@gated-at.bofh.it>
In reply to#1311686
Hi Russell,

Thanks for your comments :-D

On 01/19/2016 01:15 AM, Russell King - ARM Linux wrote:
> Hi,
>
> Some comments below...
>
> On Mon, Jan 18, 2016 at 10:42:20PM +0800, Yakir Yang wrote:
>> +static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi)
>> +{
>> +	char info[HDMI_SIZE_AVI_INFOFRAME] = {0};
>> +	int avi_color_mode;
>> +	int i;
>> +
>> +	hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_BUF_INDEX, INFOFRAME_AVI);
>> +
>> +	info[0] = 0x82;
>> +	info[1] = 0x02;
>> +	info[2] = 0x0D;
>> +	info[3] = info[0] + info[1] + info[2];
>> +
>> +	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB)
>> +		avi_color_mode = AVI_COLOR_MODE_RGB;
>> +	else if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_YUV444)
>> +		avi_color_mode = AVI_COLOR_MODE_YCBCR444;
>> +	else if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_YUV422)
>> +		avi_color_mode = AVI_COLOR_MODE_YCBCR422;
>> +	else
>> +		avi_color_mode = AVI_COLOR_MODE_RGB;
>> +
>> +	info[4] = (avi_color_mode << 5);
>> +	info[5] = (AVI_COLORIMETRY_NO_DATA << 6) |
>> +		  (AVI_CODED_FRAME_ASPECT_NO_DATA << 4) |
>> +		  ACTIVE_ASPECT_RATE_SAME_AS_CODED_FRAME;
>> +
>> +	info[6] = 0;
>> +	info[7] = hdmi->hdmi_data.vic;
>> +
>> +	if (hdmi->hdmi_data.vic == 6 || hdmi->hdmi_data.vic == 7 ||
>> +	    hdmi->hdmi_data.vic == 21 || hdmi->hdmi_data.vic == 22)
>> +		info[8] = 1;
>> +	else
>> +		info[8] = 0;
>> +
>> +	/* Calculate avi info frame checKsum */
>> +	for (i = 4; i < HDMI_SIZE_AVI_INFOFRAME; i++)
>> +		info[3] += info[i];
>> +	info[3] = 0x100 - info[3];
>> +
>> +	for (i = 0; i < HDMI_SIZE_AVI_INFOFRAME; i++)
>> +		hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i, info[i]);
>> +
>> +	return 0;
> Is there a reason why the helpers in drivers/video/hdmi.c can't be used
> to generate this?
>
> hdmi_avi_infoframe_init / hdmi_avi_infoframe_pack and
> drm_hdmi_avi_infoframe_from_display_mode ?

Great, thanks for point out, it would make code much clean with those 
helper functions.

>> +}
>> +
>> +static int inno_hdmi_config_video_vsi(struct inno_hdmi *hdmi)
>> +{
>> +	char info[HDMI_SIZE_VSI_INFOFRAME] = {0};
>> +	int i;
>> +
>> +	hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, m_PACKET_VSI_EN,
>> +		  v_PACKET_VSI_EN(0));
>> +
>> +	hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_BUF_INDEX, INFOFRAME_VSI);
>> +
>> +	/* Header Bytes */
>> +	info[0] = 0x81;
>> +	info[1] = 0x01;
>> +
>> +	/* PB1 - PB3 contain the 24bit IEEE Registration Identifier */
>> +	info[4] = 0x03;
>> +	info[5] = 0x0c;
>> +	info[6] = 0x00;
>> +
>> +	/* PB4 - HDMI_Video_Format into bits 7:5 */
>> +	info[7] = 0;
>> +
>> +	/*
>> +	 * PB5 - Depending on the video format, this byte will contain
>> +	 * either the HDMI_VIC code in buts 7:0, OR the 3D_Structure in
>> +	 * bits 7:4.
>> +	 */
>> +	info[2] = 0x06 - 2;
>> +	info[8] = 0;
>> +	info[9] = 0;
>> +
>> +	info[3] = info[0] + info[1] + info[2];
>> +
>> +	/* Calculate info frame checKsum */
>> +	for (i = 4; i < HDMI_SIZE_VSI_INFOFRAME; i++)
>> +		info[3] += info[i];
>> +	info[3] = 0x100 - info[3];
>> +
>> +	for (i = 0; i < HDMI_SIZE_VSI_INFOFRAME; i++)
>> +		hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i, info[i]);
>> +
>> +	hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, m_PACKET_VSI_EN,
>> +		  v_PACKET_VSI_EN(1));
>> +
>> +	return 0;
> hdmi_vendor_infoframe_init / hdmi_vendor_infoframe_pack?
>
> You can probably use the same function to upload the control packet
> too - something like:
>
> static int inno_hdmi_upload_frame(struct inno_hdmi *hdmi, int setup_rc,
> 	union hdmi_infoframe *frame, u32 mask, u32 disable, u32 enable)
> {
> 	if (mask)
> 		hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, mask, disable);
>
> 	if (setup_rc >= 0) {
> 		u8 packed_frame[YOUR_MAXIMUM_INFO_FRAME_SIZE];
> 		ssize_t rc, i;
>
> 		rc = hdmi_infoframe_pack(frame, packed_frame,
> 					 sizeof(packed_frame));
> 		if (rc < 0)
> 			return rc;
>
> 		for (i = 0; i < rc; i++)
> 			hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i, buf[i]);
>
> 		if (mask)
> 			hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, mask, enable);
> 	}
>
> 	return setup_rc;
> }
>
> static int inno_hdmi_config_video_vsi(struct inno_hdmi *hdmi)
> {
> 	union hdmi_infoframe frame;
>
> 	rc = drm_hdmi_vendor_infoframe_from_display_mode(&frame.vendor.hdmi,
> 							 the_drm_display_mode);
>
> 	return inno_hdmi_upload_frame(hdmi, rc, &frame, m_PACKET_VSI_EN,
> 				      v_PACKET_VSI_EN(0), v_PACKET_VSI_EN(1));
> }
>

Ah.... appreciate for the code,  it's great suggest, thanks.

Also I found a mistaken with previous "inno_hdmi_config_video_vsi"
function that we don't need to send the HDMI vendor infoframes if
we are not using a 4K or stereoscopic 3D mode.  :)

>> +static int inno_hdmi_i2c_wait(struct inno_hdmi *hdmi)
>> +{
>> +	struct inno_hdmi_i2c *i2c = hdmi->i2c;
>> +	int stat;
>> +
>> +	stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
>> +	if (!stat) {
>> +		stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
>> +		if (!stat)
>> +			return -EAGAIN;
> What's the reason for this double wait_for_completion?  This probably
> needs a comment, otherwise it looks like some weird cut-n-paste error.

Yep, you 're right, don't need double wait here.

>> +static int inno_hdmi_i2c_write(struct inno_hdmi *hdmi, struct i2c_msg *msgs)
>> +{
>> +	struct inno_hdmi_i2c *i2c = hdmi->i2c;
> You seem to have a local i2c pointer...
Ah, got it  ;)
>> +
>> +	/*
>> +	 * The DDC module only support read EDID message, so
>> +	 * we assume that each word write to this i2c adapter
>> +	 * should be the offset of EDID word address.
>> +	 */
>> +	if ((msgs->len != 1) ||
>> +	    ((msgs->addr != DDC_ADDR) && (msgs->addr != DDC_SEGMENT_ADDR)))
>> +		return -EINVAL;
>> +
>> +	reinit_completion(&i2c->cmp);
>> +
>> +	if (msgs->addr == DDC_SEGMENT_ADDR)
>> +		hdmi->i2c->segment_addr = msgs->buf[0];
>> +	if (msgs->addr == DDC_ADDR)
>> +		hdmi->i2c->ddc_addr = msgs->buf[0];
>> +
>> +	/* Set edid fifo first addr */
>> +	hdmi_writeb(hdmi, HDMI_EDID_FIFO_OFFSET, 0x00);
>> +
>> +	/* Set edid word address 0x00/0x80 */
>> +	hdmi_writeb(hdmi, HDMI_EDID_WORD_ADDR, hdmi->i2c->ddc_addr);
>> +
>> +	/* Set edid segment pointer */
>> +	hdmi_writeb(hdmi, HDMI_EDID_SEGMENT_POINTER, hdmi->i2c->segment_addr);
> but then you don't use it in the four locations above.
>
>

Thanks,
- Yakir

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web