From: Hans Verkuil <hverkuil@xs4all.nl>
To: Eugen.Hristev@microchip.com, linux-media@vger.kernel.org,
jacopo@jmondi.org
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
Claudiu.Beznea@microchip.com, robh+dt@kernel.org,
Nicolas.Ferre@microchip.com
Subject: Re: [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
Date: Fri, 29 Apr 2022 10:33:47 +0200 [thread overview]
Message-ID: <75a667ca-7c12-ff61-dab4-bdea03c16754@xs4all.nl> (raw)
In-Reply-To: <9111ee7f-9eb9-5da5-f65b-6e868f2e72f4@microchip.com>
On 29/04/2022 10:20, Eugen.Hristev@microchip.com wrote:
> On 4/29/22 11:05 AM, Hans Verkuil wrote:
>> Hi Eugen,
>>
>> On 10/03/2022 10:51, Eugen Hristev wrote:
>>> This series is the v9 series that attempts to support media controller in the
>>> atmel ISC and XISC drivers.
>>> The CSI2DC driver was accepted thus removed from the patch series, together with
>>> other patches.
>>>
>>> Important note: this series applies on top of current media_staging tree, as it
>>> relies on previous patches in the series which were accepted.
>>>
>>> Thanks to everyone who reviewed my work !
>>>
>>> Eugen
>>>
>>> Changes in v9:
>>> -> kernel robot reported isc_link_validate is not static, changed to static.
>>>
>>> Changes in v8:
>>> -> scaler: modified crop bounds to have the exact source size
>>>
>>> Changes in v7:
>>> -> scaler: modified crop bounds to have maximum isc size
>>> -> format propagation: did small changes as per Jacopo review
>>>
>>>
>>> Changes in v6:
>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
>>>
>>> Changes in v5:
>>> -> removed patch that removed the 'stop' variable as it was still required
>>> -> added two new trivial patches
>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
>>>
>>>
>>> Changes in v4:
>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
>>> one patch that was using it
>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
>>>
>>>
>>> Changes in v3:
>>> - change in bindings, small fixes in csi2dc driver and conversion to mc
>>> for the isc-base.
>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
>>>
>>> Changes in v2:
>>> - integrated many changes suggested by Jacopo in the review of the v1 series.
>>> - add a few new patches
>>>
>>>
>>>
>>> Eugen Hristev (13):
>>> media: atmel: atmel-isc-base: use streaming status when queueing
>>> buffers
>>> media: atmel: atmel-isc-base: replace is_streaming call in
>>> s_fmt_vid_cap
>>> media: atmel: atmel-isc: remove redundant comments
>>> media: atmel: atmel-isc: implement media controller
>>> media: atmel: atmel-sama5d2-isc: fix wrong mask in YUYV format check
>>> media: atmel: atmel-isc-base: use mutex to lock awb workqueue from
>>> streaming
>>> media: atmel: atmel-isc: compact the controller formats list
>>> media: atmel: atmel-isc: change format propagation to subdev into only
>>> verification
>>> media: atmel: atmel-sama7g5-isc: remove stray line
>>> dt-bindings: media: microchip,xisc: add bus-width of 14
>>
>> I'm a bit unhappy with the order of these patches. Mostly these are fixes,
>> except for patches 4 and 8, which are the meat of this series and actually
>> switching on the MC support.
>>
>> Can those be moved to the end? That also makes it easier to merge the earlier
>> patches if some more work is needed for the MC part.
>>
>> I'm also not sure whether patches 4 and 8 shouldn't be a single patch,
>> since patch 4 leaves the driver in an inconsistent state since it is
>> missing the link validation code that patch 8 adds. Unless I missed
>> something?
>
> Hello Hans,
>
> The difference that patch 8 is making is that the 'old way' of
> configuring the ISC is no longer possible.
>
> Patch 4 makes the ISC 'mc-ready' with all entities, links, but the old
> way still works (meaning that the top driver will call s_fmt down to the
> subdev ).
> After patch 8, the driver no longer has this support at all, and
> validates links at start_streaming, and no longer sets anything to the
> subdev, just validates the config that the subdev already has.
> So one reason that I had things in two patches was that patch 8 makes
> this big change that also makes userspace behave differently and has to
> configure all the subdevs and media pipeline.
>
> If you feel patch 4 and patch 8 should be squashed, I can do it, definitely.
>
> Let me know how to proceed ?
Ah, that was your intention. The problem in patch 4 is that you set
V4L2_CAP_IO_MC, which indicates to applications that they have to use
the MC and configure the pipeline correctly. Moving that to patch 8
should resolve that, I think.
Regards,
Hans
>
> Thanks,
> Eugen
>
>>
>> Regards,
>>
>> Hans
>>
>>> ARM: dts: at91: sama7g5: add nodes for video capture
>>> ARM: configs: at91: sama7: add xisc and csi2dc
>>> ARM: multi_v7_defconfig: add atmel video pipeline modules
>>>
>>> .../bindings/media/microchip,xisc.yaml | 2 +-
>>> arch/arm/boot/dts/sama7g5.dtsi | 49 ++
>>> arch/arm/configs/multi_v7_defconfig | 3 +
>>> arch/arm/configs/sama7_defconfig | 2 +
>>> drivers/media/platform/atmel/Makefile | 2 +-
>>> drivers/media/platform/atmel/atmel-isc-base.c | 518 ++++++++++--------
>>> .../media/platform/atmel/atmel-isc-scaler.c | 267 +++++++++
>>> drivers/media/platform/atmel/atmel-isc.h | 58 +-
>>> .../media/platform/atmel/atmel-sama5d2-isc.c | 87 +--
>>> .../media/platform/atmel/atmel-sama7g5-isc.c | 93 ++--
>>> 10 files changed, 754 insertions(+), 327 deletions(-)
>>> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
>>>
>>
>
next prev parent reply other threads:[~2022-04-29 8:33 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-10 9:51 [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
2022-03-10 9:51 ` [PATCH v9 01/13] media: atmel: atmel-isc-base: use streaming status when queueing buffers Eugen Hristev
2022-03-10 9:51 ` [PATCH v9 02/13] media: atmel: atmel-isc-base: replace is_streaming call in s_fmt_vid_cap Eugen Hristev
2022-03-10 9:51 ` [PATCH v9 03/13] media: atmel: atmel-isc: remove redundant comments Eugen Hristev
2022-03-10 9:51 ` [PATCH v9 04/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
2022-03-10 9:51 ` [PATCH v9 05/13] media: atmel: atmel-sama5d2-isc: fix wrong mask in YUYV format check Eugen Hristev
2022-03-10 9:51 ` [PATCH v9 06/13] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming Eugen Hristev
2022-04-29 7:41 ` Hans Verkuil
2022-04-29 8:08 ` Eugen.Hristev
2022-04-29 8:18 ` Hans Verkuil
2022-03-10 9:51 ` [PATCH v9 07/13] media: atmel: atmel-isc: compact the controller formats list Eugen Hristev
2022-03-10 9:51 ` [PATCH v9 08/13] media: atmel: atmel-isc: change format propagation to subdev into only verification Eugen Hristev
2022-04-29 8:17 ` Hans Verkuil
2022-04-29 8:28 ` Eugen.Hristev
2022-04-29 8:43 ` Hans Verkuil
2022-04-29 9:58 ` Jacopo Mondi
2022-04-29 10:03 ` Hans Verkuil
2022-04-29 10:09 ` Eugen.Hristev
2022-04-29 10:07 ` Laurent Pinchart
2022-04-29 10:13 ` Hans Verkuil
2022-04-29 10:20 ` Laurent Pinchart
2022-04-29 11:17 ` Hans Verkuil
2022-04-29 12:02 ` Laurent Pinchart
2022-04-29 12:23 ` Hans Verkuil
2022-04-29 12:28 ` Laurent Pinchart
2022-04-29 12:38 ` Jacopo Mondi
2022-04-29 12:47 ` Laurent Pinchart
2022-04-29 12:50 ` Hans Verkuil
2022-04-29 12:59 ` Eugen.Hristev
2022-03-10 9:51 ` [PATCH v9 09/13] media: atmel: atmel-sama7g5-isc: remove stray line Eugen Hristev
2022-03-10 9:51 ` [PATCH v9 10/13] dt-bindings: media: microchip,xisc: add bus-width of 14 Eugen Hristev
2022-03-10 9:52 ` [PATCH v9 11/13] ARM: dts: at91: sama7g5: add nodes for video capture Eugen Hristev
2022-03-10 9:52 ` [PATCH v9 12/13] ARM: configs: at91: sama7: add xisc and csi2dc Eugen Hristev
2022-03-10 9:52 ` [PATCH v9 13/13] ARM: multi_v7_defconfig: add atmel video pipeline modules Eugen Hristev
2022-04-29 8:05 ` [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller Hans Verkuil
2022-04-29 8:20 ` Eugen.Hristev
2022-04-29 8:33 ` Hans Verkuil [this message]
2022-04-29 8:39 ` Eugen.Hristev
2022-04-29 8:45 ` Hans Verkuil
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=75a667ca-7c12-ff61-dab4-bdea03c16754@xs4all.nl \
--to=hverkuil@xs4all.nl \
--cc=Claudiu.Beznea@microchip.com \
--cc=Eugen.Hristev@microchip.com \
--cc=Nicolas.Ferre@microchip.com \
--cc=devicetree@vger.kernel.org \
--cc=jacopo@jmondi.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=robh+dt@kernel.org \
/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;
as well as URLs for NNTP newsgroup(s).