Re: [RESEND PATCH v5 4/4] media: platform: synopsys: Add support for HDMI input driver

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 2/10/25 16:46, Hans Verkuil wrote:
...
>> +static bool hdmirx_check_timing_valid(struct v4l2_bt_timings *bt)
>> +{
>> +	if (bt->width < 100 || bt->width > 5000 ||
>> +	    bt->height < 100 || bt->height > 5000)
>> +		return false;
>> +
>> +	if (!bt->hsync || bt->hsync > 200 ||
>> +	    !bt->vsync || bt->vsync > 100)
>> +		return false;
>> +
>> +	if (!bt->hbackporch || bt->hbackporch > 2000 ||
>> +	    !bt->vbackporch || bt->vbackporch > 2000)
>> +		return false;
>> +
>> +	if (!bt->hfrontporch || bt->hfrontporch > 2000 ||
>> +	    !bt->vfrontporch || bt->vfrontporch > 2000)
>> +		return false;
> 
> I asked before, but are these hardware limitations or just sanity checks?
> Please document.

These should be sanity checks. We added comment explaining what this
code does, telling that we need to repeat checking the timing values
until they become valid. This is borrowed from downstream driver,
whether these values are tied to hw limits is unknown, we couldn't find
the exact limits documented in TRM.

...
>> +static int hdmirx_set_edid(struct file *file, void *fh, struct v4l2_edid *edid)
>> +{
>> +	struct hdmirx_stream *stream = video_drvdata(file);
>> +	struct snps_hdmirx_dev *hdmirx_dev = stream->hdmirx_dev;
>> +	u16 phys_addr;
>> +	int ret;
>> +
>> +	if (edid->pad)
>> +		return -EINVAL;
>> +
>> +	if (edid->start_block)
>> +		return -EINVAL;
>> +
>> +	if (edid->blocks > EDID_NUM_BLOCKS_MAX) {
>> +		edid->blocks = EDID_NUM_BLOCKS_MAX;
>> +		return -E2BIG;
>> +	}
>> +
>> +	if (!edid->blocks) {
>> +		cec_phys_addr_invalidate(hdmirx_dev->cec->adap);
>> +		hdmirx_dev->edid_blocks_written = 0;
> 
> This must pull the HPD low, but that doesn't happen.

Good catch, will fix.

...
>> +static void hdmirx_get_avi_infoframe(struct snps_hdmirx_dev *hdmirx_dev)
>> +{
>> +	struct v4l2_device *v4l2_dev = &hdmirx_dev->v4l2_dev;
>> +	union hdmi_infoframe frame = {};
>> +	int err, i, b, itr = 0;
>> +	u8 aviif[3 + 7 * 4];
>> +	u32 val;
>> +
>> +	aviif[itr++] = HDMI_INFOFRAME_TYPE_AVI;
>> +	val = hdmirx_readl(hdmirx_dev, PKTDEC_AVIIF_PH2_1);
>> +	aviif[itr++] = val & 0xff;
>> +	aviif[itr++] = (val >> 8) & 0xff;
>> +
>> +	for (i = 0; i < 7; i++) {
>> +		val = hdmirx_readl(hdmirx_dev, PKTDEC_AVIIF_PB3_0 + 4 * i);
>> +
>> +		for (b = 0; b < 4; b++)
>> +			aviif[itr++] = (val >> (8 * b)) & 0xff;
>> +	}
>> +
>> +	err = hdmi_infoframe_unpack(&frame, aviif, sizeof(aviif));
>> +	if (err) {
>> +		v4l2_err(v4l2_dev, "failed to unpack AVI infoframe\n");
>> +		return;
>> +	}
>> +
>> +	v4l2_ctrl_s_ctrl(hdmirx_dev->rgb_range, frame.avi.quantization_range);
>> +
>> +	if (frame.avi.itc)
>> +		v4l2_ctrl_s_ctrl(hdmirx_dev->content_type,
>> +				 frame.avi.content_type);
>> +	else
>> +		v4l2_ctrl_s_ctrl(hdmirx_dev->content_type,
>> +				 V4L2_DV_IT_CONTENT_TYPE_NO_ITC);
> 
> InfoFrames must now also be exported to debugfs. See e.g. drivers/media/i2c/tc358743.c
> and include/media/v4l2-dv-timings.h (search for V4L2_DEBUGFS_IF_MAX_LEN).
> 
> This is a recent change: both drm and v4l2 can now export InfoFrames to debugfs,
> and this is handy for debugging issues.
> 
> The edid-decode utility (part of v4l-utils) can parse these InfoFrames.

Ack, will add the InfoFrame debugfs.

...
>> +static void hdmirx_load_default_edid(struct snps_hdmirx_dev *hdmirx_dev)
>> +{
>> +	struct v4l2_edid def_edid;
>> +
>> +	hdmirx_hpd_ctrl(hdmirx_dev, false);
>> +
>> +	/* disable hpd and write edid */
>> +	def_edid.pad = 0;
>> +	def_edid.start_block = 0;
>> +	def_edid.blocks = EDID_NUM_BLOCKS_MAX;
>> +
>> +	if (IS_ENABLED(CONFIG_VIDEO_SYNOPSYS_HDMIRX_LOAD_DEFAULT_EDID))
>> +		def_edid.edid = edid_default;
>> +	else
>> +		def_edid.edid = hdmirx_dev->edid;
> 
> So if you are not loading the default EDID, then you load a zeroed EDID?
> 
> This is much too complicated: if there is no default EDID, then just
> pull the HPD low (you do that already at the start) and return.
> 
> BTW, please check with CONFIG_VIDEO_SYNOPSYS_HDMIRX_LOAD_DEFAULT_EDID set
> and unset and make sure there are no compile warnings/errors relating to that.

Now I see what you meant previously. Keeping HPD pulled low if there is
no valid EDID will work. Will change it in the next iteration.

Thanks for the review!

-- 
Best regards,
Dmitry

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip