Re: [RFC 01/12] exynos-fimc-is: Adding device tree nodes

[Arun Kumar K <arunkk.samsung@gmail.com>]

Hi Sylwester,

Sorry for the late reply.

>>>
>>> OK, thanks for the explanation.
>>>
>>> I can think of at least one possible way to get hold of the fimc-is
>>> context in the subdev. For instance, in subdev's .registered callback
>>> you get a pointer to struct v4l2_device, which is normally embedded
>>> in a top level driver's private data. Then with container_of()
>>> you could get hold of required data at the fimc-is driver.
>>
>>
>> But as per current implementation, it is not the fimc-is driver that is
>> registering the ISP subdevs. It will be registered from the
>> media controller driver. So fimc-is context cannot be obtained by
>> just using container_of().
>
>
> I guess best option would be to have a function to get the IS slave
> interface driver context at the sensor subdev exported by the IS driver
> module, as you suggested previously.

Ok I will make the sensor as i2c device and check what is the best
way to get the IS context in sensor subdev.

>
> You still could obtain the fimc-is object from the media device private
> data structure, since the media device has normally a list of its all
> entities in one form or the other. But the sensor would need to know
> details of the media device, which makes it a bit pointless.
>
> Nevertheless, my main concern is the DT binding. Sharing the sensor
> subdev driver might not be that important at the moment, we are talking
> about 300..500 lines of code per ISP driver currently anyway.
>
> More important is to have the hardware described in a standard way, so
> when the firmware changes there is no need to change the DT bindings.
>

Yes that's right. Need to define the hardware in a generic way regardless
of what the firmware is doing.


>>>
>>
>> In case of ISP subdevs (isp, scc and scp), there is not much configuration
>> that the media device can do. Only control possible is to turn on/off
>> specific scaler DMA outputs which can be done via the video node ioctls.
>> The role of media device here is mostly to convey the pipeline structure
>> to the user. For eg. it is not possible to directly connect isp (sd)
>> -->  scp (sd).
>> In the media controller pipeline1 implementation, we were planning to
>> put immutable links between these subdevs. Is that acceptable?
>
>
> Not sure I understand which links you mean exactly. Could you post the
> media graph generated by media-ctl (--print-dot) ?
>
> If you're talking about the on-the-fly (FIFO) links, then it probably
> makes sense. The media device driver should respond to the link_notify
> events and not to allow data links unsupported in the hardware. If you
> create immutable OTF links, then how would you switch between DMA and
> OTF interfaces ? Or can all processing blocks of the ISP chain work
> simultaneously with the DMA and OTF ? The FD block, for instance, can fed
> data from memory _or_ from previous processing block in the chain, right ?
> You will need a user interface to control which input is used and the
> links configuration seems most natural here.

Yes I agree to that. Though in the current driver, there are no subdevs which
can change between DMA <-> OTF inputs, in future versions we might add it.
So link configuration option will be provided.


>
>
>>> The media driver has a list of media entities (subdevices and video
>>> nodes) and I though it could coordinate any requests involving whole
>>> video/image processing pipeline originating from /dev/video ioctls/fops.
>>>
>>> So for instance if /dev/video in this pipeline is opened
>>>
>>> sensor (sd) ->  mipi-csis (sd) ->  fimc-lite (sd) ->  memory (/dev/video)
>>>
>>> it would call s_power operation on the above subdevs and additionally
>>> on e.g. the isp subdev (or any other we choose as a main subdev
>>> implementing the FIMC-IS slave interface).
>>>
>>> Then couldn't it be done that video node ioctls invoke pipeline
>>> operations, and the media device resolves any dependencies/calls
>>> order, as in case of the exynos4 driver ?
>>
>>
>> On Exynos4 subdevs, it is well and good since all the subdevs are
>> independent IPs. Here in ISP since the same IP can take one input and
>
>
> Not really, there are going to be 2 subdevs exposed by the fimc-is: ISP
> and FD. However FD is still not supported in my last patch series. I was
> planning this for a subsequent kernel release.
>
>
>> provide multiple outputs, we designed them as separate subdevs. So
>> here we cannot make the subdevs independent of each other where only
>> the sequence / dependencies is controlled from the media device.
>
>
> I'm not asking you to make the FIMC-IS subdevs more self-contained,
> it's of course perfectly fine to have multiple (logical) subdevs exposed
> by a complex device like that. I have been thinking only about the
> sensor driver, since the sensors are normally shared across ISPs from
> various chip manufacturers. But let us leave this topic for now.
>
> BTW, in my interpretation FIMC-IS is a collection of IPs/peripheral
> devices, not a single black box, including an image sensor. And the
> firmware should not be the most significant factor how we expose
> the whole subsystem to the user. All elements of the ISP chain, i.e.
> an IP control registers are visible to both, the main CPU and the
> FIMC-IS (Cortex-A5) MCU. Still, it would be possible to create v4l2
> subdevs dynamically, depending on the firmware architecture.
>

Ok. I will address all these comments and work on v2 patchset.

Thanks for the excellent help :)

Regards
Arun