LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jose Abreu <Jose.Abreu@synopsys.com>
To: Hans Verkuil <hverkuil@xs4all.nl>, <linux-media@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Cc: Joao Pinto <Joao.Pinto@synopsys.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hans.verkuil@cisco.com>,
	"Sylwester Nawrocki" <snawrocki@kernel.org>,
	Sakari Ailus <sakari.ailus@iki.fi>,
	Philippe Ombredanne <pombredanne@nexb.com>
Subject: Re: [PATCH v10 4/4] [media] platform: Add Synopsys DesignWare HDMI RX Controller Driver
Date: Wed, 13 Dec 2017 14:00:08 +0000	[thread overview]
Message-ID: <f5341c4b-43e2-12f6-9c9f-2385d47bb2fd@synopsys.com> (raw)
In-Reply-To: <ca5b3cf7-c7d0-36d4-08ac-32a7a00afd7d@xs4all.nl>

Hi Hans,

On 13-12-2017 10:00, Hans Verkuil wrote:
> On 12/12/17 17:02, Jose Abreu wrote:
>>
>>>> +static int dw_hdmi_s_routing(struct v4l2_subdev *sd, u32 input, u32 output,
>>>> +		u32 config)
>>>> +{
>>>> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
>>>> +
>>>> +	if (!has_signal(dw_dev, input))
>>>> +		return -EINVAL;
>>> Why would this be a reason to reject this? There may be no signal now, but a signal
>>> might appear later.
>> I would expect s_routing to only be called if there is an input
>> connected, otherwise we are just wasting resources by trying to
>> equalize an input that is not present ... I can remove the "if"
>> as there are other safe guards for this though (for example g_fmt
>> will return an error) ...
> No, s_routing is typically called as a result of a VIDIOC_S_INPUT
> call, and that can come whether or not there is a signal on an
> input. In fact, initially the first input is always selected anyway,
> whether or not there is a signal.
>
> g_fmt will just return the current configured format, this is unrelated
> to whether or not there is a signal.
>
> The only times the driver checks whether or not there is a signal (and
> what that is) are:
>
> 1) g_input_status
> 2) query_dv_timings
> 3) when the irq detects a signal change and sends V4L2_EVENT_SOURCE_CHANGE

Ok, I will remove the checks then.

>
>>>> +	msleep(50); /* Wait for 1 field */
>>> How do you know this waits for 1 field? Or is this: "Wait for at least 1 field"?
>> Its wait at least for 1 field. This is over-generous because its
>> assuming the frame rate is 20fps (which in HDMI does not happen).
> With custom timings it can happen (i.e. a 15 fps stream). Admittedly, it's not
> common, but people sometimes use it.
>
>>> I don't know exactly how the IP does this, but it looks fishy to me. If it is
>>> correct, then it could use a few comments about what is going on here as it is
>>> not obvious.
>> The IP updates the values at each field but I need to change this
>> register to populate all values in the bt struct.
> How do you know which field (top or bottom) you've captured? How do you know you
> didn't miss e.g. the bottom field and instead end up with two top field measurements?
>
> The top and bottom field are almost, but not quite the same. Typically the vertical
> backporch of the fields differs by 1 where the second field's backporch is larger by 1 line.
>
>>> And what happens if the framerate is even slower? You know the pixelclock and
>>> total width+height, so you can calculate the framerate from that.
>> Hmm, but then I have to consider pixelclk error, msleep error, ...
> But you have that now as well.
>
> An alternative is to measure a single field and deduce the backporch values from that.
>
> At least for all the common HDMI interlaced formats il_vbackporch is an even value and
> vbackporch is il_vbackporch - 1.
>
> So if you get an even backporch, then you found il_vbackporch, and if it is odd, then
> you found vbackporch.
>
>>>> +	bt->vsync = hdmi_readl(dw_dev, HDMI_MD_VOL);
>>>> +
>>>> +	hdmi_mask_writel(dw_dev, 0x0, HDMI_MD_VCTRL,
>>>> +			HDMI_MD_VCTRL_V_OFFS_LIN_MODE_OFFSET,
>>>> +			HDMI_MD_VCTRL_V_OFFS_LIN_MODE_MASK);
>>>> +	msleep(50); /* Wait for 1 field */
>>>> +	bt->vbackporch = hdmi_readl(dw_dev, HDMI_MD_VOL);
>>>> +	bt->vfrontporch = vtot - bt->height - bt->vsync - bt->vbackporch;
>>>> +
>>>> +	if (bt->interlaced == V4L2_DV_INTERLACED) {
>>>> +		hdmi_mask_writel(dw_dev, 0x1, HDMI_MD_VCTRL,
>>>> +				HDMI_MD_VCTRL_V_MODE_OFFSET,
>>>> +				HDMI_MD_VCTRL_V_MODE_MASK);
>>>> +		msleep(100); /* Wait for 2 fields */
>>>> +
>>>> +		vtot = hdmi_readl(dw_dev, HDMI_MD_VTL);
>>>> +		hdmi_mask_writel(dw_dev, 0x1, HDMI_MD_VCTRL,
>>>> +				HDMI_MD_VCTRL_V_OFFS_LIN_MODE_OFFSET,
>>>> +				HDMI_MD_VCTRL_V_OFFS_LIN_MODE_MASK);
>>>> +		msleep(50); /* Wait for 1 field */
>>>> +		bt->il_vsync = hdmi_readl(dw_dev, HDMI_MD_VOL);
>>>> +
>>>> +		hdmi_mask_writel(dw_dev, 0x0, HDMI_MD_VCTRL,
>>>> +				HDMI_MD_VCTRL_V_OFFS_LIN_MODE_OFFSET,
>>>> +				HDMI_MD_VCTRL_V_OFFS_LIN_MODE_MASK);
>>>> +		msleep(50);
>>>> +		bt->il_vbackporch = hdmi_readl(dw_dev, HDMI_MD_VOL);
>>>> +		bt->il_vfrontporch = vtot - bt->height - bt->il_vsync -
>>>> +			bt->il_vbackporch;
>>>> +
>>>> +		hdmi_mask_writel(dw_dev, 0x0, HDMI_MD_VCTRL,
>>>> +				HDMI_MD_VCTRL_V_MODE_OFFSET,
>>>> +				HDMI_MD_VCTRL_V_MODE_MASK);
>>> Same here, I'm not sure this is correct. What is the output of
>>> 'v4l2-ctl --query-dv-timings' when you feed it a standard interlaced format?
>> I used v4l2-ctl --log-status with interlaced format and
>> everything seemed correct ...
> Can you show a few examples? Is vbackport odd? And is il_vbackporch equal to vbackporch + 1?
>
> Interlaced is tricky :-)

Indeed. I compared the values with the spec and they are not
correct. Even hsync is wrong. I already corrected in the code the
hsync but regarding interlace I'm not seeing an easy way to do
this without using interrupts in each vsync because the register
I was toggling does not behave as I expected (I misunderstood the
databook). Maybe we should not detect interlaced modes for now?
Or not fill the il_ fields?

Best Regards,
Jose Miguel Abreu

>
> Regards,
>
> 	Hans
>

  parent reply	other threads:[~2017-12-13 14:00 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-11 17:41 [PATCH v10 0/4] Synopsys DesignWare HDMI Video Capture Controller + PHY Jose Abreu
2017-12-11 17:41 ` [PATCH v10 1/4] dt-bindings: media: Document Synopsys DesignWare HDMI RX Jose Abreu
2017-12-15 13:34   ` Hans Verkuil
2017-12-11 17:41 ` [PATCH v10 2/4] MAINTAINERS: Add entry for Synopsys DesignWare HDMI drivers Jose Abreu
2017-12-15 13:35   ` Hans Verkuil
2017-12-15 13:35     ` Hans Verkuil
2017-12-11 17:41 ` [PATCH v10 3/4] [media] platform: Add Synopsys DesignWare HDMI RX PHY e405 Driver Jose Abreu
2017-12-11 19:05   ` Philippe Ombredanne
2017-12-15 13:46   ` Hans Verkuil
2017-12-11 17:41 ` [PATCH v10 4/4] [media] platform: Add Synopsys DesignWare HDMI RX Controller Driver Jose Abreu
2017-12-11 19:06   ` Philippe Ombredanne
2017-12-12 15:47   ` Hans Verkuil
2017-12-12 16:02     ` Jose Abreu
2017-12-13 10:00       ` Hans Verkuil
2017-12-13 10:04         ` Philippe Ombredanne
2017-12-13 14:00         ` Jose Abreu [this message]
2017-12-13 20:49           ` Hans Verkuil
2017-12-15 11:23             ` Jose Abreu
2018-06-08  7:41               ` Hans Verkuil
2018-06-12 13:45                 ` Jose Abreu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f5341c4b-43e2-12f6-9c9f-2385d47bb2fd@synopsys.com \
    --to=jose.abreu@synopsys.com \
    --cc=Joao.Pinto@synopsys.com \
    --cc=hans.verkuil@cisco.com \
    --cc=hverkuil@xs4all.nl \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=pombredanne@nexb.com \
    --cc=sakari.ailus@iki.fi \
    --cc=snawrocki@kernel.org \
    --subject='Re: [PATCH v10 4/4] [media] platform: Add Synopsys DesignWare HDMI RX Controller Driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).