Linux Media Controller development
 help / color / mirror / Atom feed
From: Ezequiel Garcia <ezequiel@collabora.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Philipp Zabel <p.zabel@pengutronix.de>
Cc: Steve Longerbeam <slongerbeam@gmail.com>,
	kernel@collabora.com, Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	linux-media@vger.kernel.org,
	Hans Verkuil <hans.verkuil@cisco.com>,
	Nicolas Dufresne <nicolas.dufresne@collabora.com>
Subject: Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out
Date: Wed, 26 Jun 2019 15:05:56 -0300	[thread overview]
Message-ID: <f0504a6430e192db1495cf165f93df1aa5cd9f79.camel@collabora.com> (raw)
In-Reply-To: <20190626080027.GA4776@pendragon.ideasonboard.com>

Hi Laurent, Philipp,

Thank you for the prompt review! I was pretty sure this would
raise your wise eyebrows :-)

On Wed, 2019-06-26 at 11:00 +0300, Laurent Pinchart wrote:
> Hi Ezequiel,
> 
> Thank you for the patch.
> 
> On Tue, Jun 25, 2019 at 05:39:45PM -0300, Ezequiel Garcia wrote:
> > Not all sensors will be able to guarantee a proper initial state.
> > This may be either because the driver is not properly written,
> > or (probably unlikely) because the hardware won't support it.
> > 
> > While the right solution in the former case is to fix the sensor
> > driver, the real world not always allows right solutions, due to lack
> > of available documentation and support on these sensors.
> > 
> > Let's relax this requirement, and allow the driver to support stream start,
> > even if the sensor initial sequence wasn't the expected.
> > A warning is still emitted, so users should be hinted that something is off.
> 
> I'm not sure this is a very good idea. Failure to detect the LP-11 state
> may mean that the sensor is completely powered off, but it may also mean
> that it is already streaming data. I don't know how the CSI-2 receiver
> state machine will operate in the first case, but in the second case it
> will not be able to synchronise to the incoming stream, so it won't work
> anyway.
> 
> I think you should instead fix the problem in the sensor driver, as you
> hinted. 

Sure, we all agree that the sensor fix is the right solution.

> Relaxing the requirement here will only make it more confusing,
> it's a hack, and isn't portable across CSI-2 receivers. 

We can emit a warning as suggested by Philipp, stating that the sensor
is buggy and needs fixing.

> The same buggy
> sensor driver won't work with other CSI-2 receivers whose internal state
> machine require starting in the LP-11 state.
> 

Right. But the same buggy sensor driver probaly does work already with other
CSI-2 receivers, hence why it hasn't been detected when it was submitted.

> Which sensor are you using ?
> 

I noticed this problem on OV5645 on a Wandoboard. Looks to be
basically the same issue as the one Jacopo fixed here:

Author: Jacopo Mondi <jacopo@jmondi.org>
Date:   Fri Jul 6 05:51:52 2018 -0400

    media: ov5640: Re-work MIPI startup sequence

I fixed it for OV5645 (and will submit soon), but that is
not the point.

The point is that buggy drivers exist, and while I'd love
to fix them all, it won't be always possible (due to lack of
datasheets, and due to some of these sensors having no active
maintainer).

Without this commit, such a buggy sensor will not work for sure;
while with the commit, there is a chance it will work.

Why would we prevent the latter from happening?

Thanks,
Eze


  reply	other threads:[~2019-06-26 18:06 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-25 20:39 [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out Ezequiel Garcia
2019-06-26  0:45 ` Steve Longerbeam
2019-06-26  7:45 ` Philipp Zabel
2019-06-26 19:53   ` Fabio Estevam
2019-06-26 21:19     ` Steve Longerbeam
2019-06-26 23:22       ` Fabio Estevam
2019-06-26 23:29         ` Steve Longerbeam
2019-06-27  8:43           ` Philipp Zabel
2019-06-27 12:38             ` Fabio Estevam
2019-06-27 12:56               ` Philipp Zabel
2019-06-27 18:45                 ` Ezequiel Garcia
2019-06-27 22:16                   ` Fabio Estevam
2019-06-27 22:12                 ` Steve Longerbeam
2019-07-01  6:48                   ` Philipp Zabel
2019-06-27 12:42             ` Ezequiel Garcia
2019-06-26  8:00 ` Laurent Pinchart
2019-06-26 18:05   ` Ezequiel Garcia [this message]
2019-06-26 18:16   ` Steve Longerbeam
2019-06-27  7:39 ` Jacopo Mondi

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=f0504a6430e192db1495cf165f93df1aa5cd9f79.camel@collabora.com \
    --to=ezequiel@collabora.com \
    --cc=festevam@gmail.com \
    --cc=hans.verkuil@cisco.com \
    --cc=kernel@collabora.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=nicolas.dufresne@collabora.com \
    --cc=p.zabel@pengutronix.de \
    --cc=s.hauer@pengutronix.de \
    --cc=shawnguo@kernel.org \
    --cc=slongerbeam@gmail.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox