From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Jacopo Mondi <jacopo@jmondi.org>,
linux-media@vger.kernel.org, sakari.ailus@linux.intel.com,
Jacopo Mondi <jacopo+renesas@jmondi.org>,
niklas.soderlund+renesas@ragnatech.se,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Hans Verkuil <hverkuil-cisco@xs4all.nl>,
Pratyush Yadav <p.yadav@ti.com>,
Lokesh Vutla <lokeshvutla@ti.com>
Subject: Re: [PATCH v8 03/36] media: subdev: add 'which' to subdev state
Date: Mon, 27 Sep 2021 11:55:38 +0300 [thread overview]
Message-ID: <9d80e73b-acd0-9439-8447-6c2a66686ec3@ideasonboard.com> (raw)
In-Reply-To: <YVEUxunGwevzoy5f@pendragon.ideasonboard.com>
On 27/09/2021 03:48, Laurent Pinchart wrote:
> Hi Tomi,
>
> On Thu, Sep 16, 2021 at 04:24:19PM +0300, Tomi Valkeinen wrote:
>> On 16/09/2021 16:07, Jacopo Mondi wrote:
>>
>>> Also note that operations like s_stream do not take a state as
>>> parameter. The driver has to fetch it from the subdev anyway
>>> (this in reply to the idea of having the active state as parameter vs
>>> retrieving it from the subdev if ACTIVE)
>>>
>>> While porting the R-Car drivers on top of this series I found myself
>>> in the need to (in the s_stream call chain)
>>>
>>> static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>>> {
>>> const struct v4l2_subdev_state *state = priv->subdev.state;
>>> const struct v4l2_subdev_stream_configs *configs = &state->stream_configs;
>>>
>>> ...
>>>
>>> /*
>>> * Configure field handling inspecting the formats of the
>>> * single sink pad streams.
>>> */
>>> for (i = 0; i < configs->num_configs; ++i) {
>>> const struct v4l2_subdev_stream_config *config = configs->configs;
>>> if (config->pad != RCAR_CSI2_SINK)
>>> continue;
>>>
>>> if (config->fmt.field != V4L2_FIELD_ALTERNATE)
>>> continue;
>>>
>>> fld |= FLD_DET_SEL(1);
>>> fld |= FLD_FLD_EN(config->stream);
>>>
>>> /* PAL vs NTSC. */
>>> if (config->fmt.height == 240)
>>> fld |= FLD_FLD_NUM(0);
>>> else
>>> fld |= FLD_FLD_NUM(1);
>>> }
>>>
>>> ...
>>>
>>> }
>>>
>>> Am I doing it wrong, or is this a case for the subdev to have to
>>> directly access sd->state ?
>>
>> In s_stream path you should:
>>
>> state = v4l2_subdev_lock_active_state(sd);
>>
>> <do the work with the state>
>>
>> v4l2_subdev_unlock_state(state);
>>
>> If you already have the state, e.g. in set_fmt:
>>
>> state = v4l2_subdev_validate_and_lock_state(sd, state);
>>
>> <do the work with the state>
>>
>> v4l2_subdev_unlock_state(state);
>>
>> Accessing the stream_configs directly is fine but not that nice. I did
>> think about some helpers, perhaps for_each_stream_config(), but I didn't
>> add that as I didn't have the need.
>>
>> There's v4l2_state_get_stream_format() which can be used in many cases,
>> but we probably need something else if you need to iterate over all the
>> configs.
>
> I really like forcing drivers to call functions that will lock the
> state, at least until we can move the locks to the core (if ever). We
> should move the fields of v4l2_subdev that drivers are not supposed to
> access directly under a big PRIVATE comment.
Can you clarify what you mean here? Did you mean you like functions that
will _check_ the lock? Or did you just mean that you like
v4l2_subdev_lock_state() better than mutex_lock(state->lock)?
Well, in any case, I think my series does both =). I can add the private
comment to subdev. In fact, at one time I did not have sd->state, but
sd->_state, just to make sure no one accesses it.
Tomi
next prev parent reply other threads:[~2021-09-27 8:55 UTC|newest]
Thread overview: 86+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-30 11:00 [PATCH v8 00/36] v4l: subdev internal routing and streams Tomi Valkeinen
2021-08-30 11:00 ` [PATCH v8 01/36] media: subdev: rename subdev-state alloc & free Tomi Valkeinen
2021-09-26 23:06 ` Laurent Pinchart
2021-09-27 6:38 ` Tomi Valkeinen
2021-08-30 11:00 ` [PATCH v8 02/36] media: subdev: add active state to struct v4l2_subdev Tomi Valkeinen
2021-09-13 10:57 ` Jacopo Mondi
2021-09-13 12:00 ` Tomi Valkeinen
2021-09-15 9:44 ` Jacopo Mondi
2021-09-16 6:17 ` Tomi Valkeinen
2021-09-16 6:52 ` Tomi Valkeinen
2021-09-16 8:08 ` Jacopo Mondi
2021-09-16 9:36 ` Tomi Valkeinen
2021-09-26 23:58 ` Laurent Pinchart
2021-09-27 7:05 ` Tomi Valkeinen
2021-09-27 9:39 ` Laurent Pinchart
2021-09-28 5:14 ` Tomi Valkeinen
2021-09-28 12:33 ` Tomi Valkeinen
2021-09-29 15:41 ` Laurent Pinchart
2021-08-30 11:00 ` [PATCH v8 03/36] media: subdev: add 'which' to subdev state Tomi Valkeinen
2021-09-13 11:41 ` Jacopo Mondi
2021-09-13 12:17 ` Tomi Valkeinen
2021-09-13 13:38 ` Jacopo Mondi
2021-09-13 14:26 ` Tomi Valkeinen
2021-09-16 13:07 ` Jacopo Mondi
2021-09-16 13:24 ` Tomi Valkeinen
2021-09-27 0:48 ` Laurent Pinchart
2021-09-27 8:55 ` Tomi Valkeinen [this message]
2021-09-27 10:49 ` Laurent Pinchart
2021-09-27 0:46 ` Laurent Pinchart
2021-09-27 8:35 ` Tomi Valkeinen
2021-09-27 10:01 ` Laurent Pinchart
2021-08-30 11:00 ` [PATCH v8 04/36] media: subdev: pass also the active state to subdevs from ioctls Tomi Valkeinen
2021-09-15 10:17 ` Jacopo Mondi
2021-09-16 6:44 ` Tomi Valkeinen
2021-09-16 8:02 ` Jacopo Mondi
2021-09-16 8:43 ` Tomi Valkeinen
2021-09-27 1:13 ` Laurent Pinchart
2021-08-30 11:00 ` [PATCH v8 05/36] media: subdev: add subdev state locking Tomi Valkeinen
2021-09-27 1:35 ` Laurent Pinchart
2021-09-27 9:49 ` Tomi Valkeinen
2021-09-27 10:06 ` Laurent Pinchart
2021-08-30 11:00 ` [PATCH v8 06/36] media: subdev: Add v4l2_subdev_validate(_and_lock)_state() Tomi Valkeinen
2021-09-27 1:45 ` Laurent Pinchart
2021-09-28 5:02 ` Tomi Valkeinen
2021-09-28 7:52 ` Laurent Pinchart
2021-09-29 15:35 ` Tomi Valkeinen
2021-09-29 15:39 ` Laurent Pinchart
2021-08-30 11:00 ` [PATCH v8 07/36] media: Documentation: add documentation about subdev state Tomi Valkeinen
2021-08-30 11:00 ` [PATCH v8 08/36] media: entity: Use pad as a starting point for graph walk Tomi Valkeinen
2021-08-30 11:00 ` [PATCH v8 09/36] media: entity: Use pads instead of entities in the media graph walk stack Tomi Valkeinen
2021-08-30 11:00 ` [PATCH v8 10/36] media: entity: Walk the graph based on pads Tomi Valkeinen
2021-08-30 11:00 ` [PATCH v8 11/36] media: mc: Start walk from a specific pad in use count calculation Tomi Valkeinen
2021-08-30 11:00 ` [PATCH v8 12/36] media: entity: Add iterator helper for entity pads Tomi Valkeinen
2021-08-30 11:00 ` [PATCH v8 13/36] media: entity: Move the pipeline from entity to pads Tomi Valkeinen
2021-08-30 11:00 ` [PATCH v8 14/36] media: entity: Use pad as the starting point for a pipeline Tomi Valkeinen
2021-08-30 11:00 ` [PATCH v8 15/36] media: entity: Add has_route entity operation Tomi Valkeinen
2021-08-30 11:00 ` [PATCH v8 16/36] media: entity: Add media_entity_has_route() function Tomi Valkeinen
2021-08-30 11:00 ` [PATCH v8 17/36] media: entity: Use routing information during graph traversal Tomi Valkeinen
2021-08-30 11:00 ` [PATCH v8 18/36] media: entity: Skip link validation for pads to which there is no route Tomi Valkeinen
2021-08-30 11:00 ` [PATCH v8 19/36] media: entity: Add an iterator helper for connected pads Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 20/36] media: entity: Add only connected pads to the pipeline Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 21/36] media: entity: Add debug information in graph walk route check Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 22/36] media: Add bus type to frame descriptors Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 23/36] media: Add CSI-2 bus configuration " Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 24/36] media: Add stream to frame descriptor Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 25/36] media: subdev: increase V4L2_FRAME_DESC_ENTRY_MAX to 8 Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 26/36] media: add V4L2_SUBDEV_FL_MULTIPLEXED Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 27/36] media: Documentation: Add GS_ROUTING documentation Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 28/36] media: subdev: Add [GS]_ROUTING subdev ioctls and operations Tomi Valkeinen
2021-09-15 16:10 ` Jacopo Mondi
2021-09-16 6:57 ` Tomi Valkeinen
2021-10-03 19:52 ` Dafna Hirschfeld
2021-10-04 5:15 ` Tomi Valkeinen
2021-10-05 10:19 ` Dafna Hirschfeld
2021-10-05 10:54 ` Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 29/36] media: subdev: add v4l2_subdev_has_route() Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 30/36] media: subdev: add v4l2_subdev_set_routing helper() Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 31/36] media: subdev: add stream based configuration Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 32/36] media: subdev: use streams in v4l2_subdev_link_validate() Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 33/36] media: subdev: add "opposite" stream helper funcs Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 34/36] media: subdev: add v4l2_subdev_get_fmt() helper function Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 35/36] media: subdev: add v4l2_subdev_set_routing_with_fmt() helper Tomi Valkeinen
2021-08-30 11:01 ` [PATCH v8 36/36] media: subdev: add v4l2_routing_simple_verify() helper Tomi Valkeinen
2021-09-20 10:19 ` [PATCH v8 00/36] v4l: subdev internal routing and streams Tomi Valkeinen
2021-09-27 1:24 ` Laurent Pinchart
2021-09-28 7:59 ` 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=9d80e73b-acd0-9439-8447-6c2a66686ec3@ideasonboard.com \
--to=tomi.valkeinen@ideasonboard.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=jacopo+renesas@jmondi.org \
--cc=jacopo@jmondi.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=lokeshvutla@ti.com \
--cc=mchehab@kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
--cc=p.yadav@ti.com \
--cc=sakari.ailus@linux.intel.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;
as well as URLs for NNTP newsgroup(s).