Re: [PATCH 00/11] media: rcar-vin: fix OPS and format/pad index issues

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 02/13/2017 04:31 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Monday 13 Feb 2017 15:19:13 Hans Verkuil wrote:
>> Hi Niklas,
>>
>> One general remark: in many commit logs you mistype 'subdeivce'. Can you
>> fix that for the v2?
>>
>> On 01/31/2017 04:40 PM, Niklas Söderlund wrote:
>>> Hi,
>>>
>>> This series address issues with the R-Car Gen2 VIN driver. The most
>>> serious issue is the OPS when unbind and rebinding the i2c driver for
>>> the video source subdevice which have popped up as a blocker for other
>>> work.
>>>
>>> This series is broken out of my much larger R-Car Gen3 enablement series
>>> '[PATCHv2 00/32] rcar-vin: Add Gen3 with media controller support'. I
>>> plan to remove that series form patchwork and focus on these fixes first
>>> as they are blocking other development. Once the blocking issues are
>>> removed I will rebase and repost the larger Gen3 series.
>>>
>>> Patch 1-4 fix simple problems found while testing
>>>
>>>     1-2 Fix format problems when the format is (re)set.
>>>     3   Fix media pad errors
>>>     4   Fix standard enumeration problem
>>>
>>> Patch 5 adds a wrapper function to retrieve the active video source
>>> subdevice. This is strictly not needed on Gen2 which only have one
>>> possible video source per VIN instance (This will change on Gen3). But
>>> patch 6-8,11 which fixes real issues on Gen2 make use of this wrapper, as
>>> not risk breaking things by removing this wrapper in this series and
>>> then readding it in a later Gen3 series I have chosen to keep the patch.
>>> Please let me know if I should drop it and rewrite patch 6-11 (if
>>> possible I would like to avoid that).
>>>
>>> Patch 6-8 deals with video source subdevice pad index handling by moving
>>> the information from struct rvin_dev to struct rvin_graph_entity and
>>> moving the pad index probing to the struct v4l2_async_notifier complete
>>> callback. This is needed to allow the bind/unbind fix in patch 10-11.
>>>
>>> Patch 9 use the pad information when calling enum_mbus_code.
>>>
>>> Patch 10-11 fix a OPS when unbinding/binding the video source subdevice.
>>
>> This will not help: you can unbind a subdev at any time, including when
>> it is in use.
>>
>> But why do you need this at all? You can also set suppress_bind_attrs in
>> the adv7180 driver to prevent the bind/unbind files from appearing.
>>
>> It really makes no sense for subdevs. In fact, all subdevs should set this
>> flag since in the current implementation this is completely impossible to
>> implement safely.
>>
>> I suggest you drop the patches relating to this and instead set the suppress
>> flag.
> 
> The adv7180 is connected to an I2C controller that can be unbound. Setting the 
> suppress_bind_attrs flag in the driver thus won't prevent the device from 
> being unbound. suppress_bind_attrs is not a good solution for I2C drivers.

Then just drop these patches, since those don't solve anything either. Without
some of the things we discussed during the refcounting meeting (i.e. a locking
scheme before calling into subdev ops) anything you do will at best just give
you an illusion that you're safe.

Regards,

	Hans




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux