From: Kaaira Gupta <kgupta@es.iitr.ac.in>
To: Kieran Bingham <kieran.bingham@ideasonboard.com>
Cc: Helen Koike <helen.koike@collabora.com>,
Shuah Khan <skhan@linuxfoundation.org>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 7/9] media: vimc: Dynamically allocate stream struct
Date: Sat, 12 Sep 2020 16:09:16 +0530 [thread overview]
Message-ID: <20200912103916.GE5022@kaaira-HP-Pavilion-Notebook> (raw)
In-Reply-To: <7300d7ab-2be0-a6c6-b506-5af8b4a9b893@ideasonboard.com>
On Wed, Sep 02, 2020 at 11:29:31AM +0100, Kieran Bingham wrote:
> Hi Kaaira,
>
> On 19/08/2020 19:04, Kaaira Gupta wrote:
> > Multiple streams will share same stream struct if we want them to run on
> > same thread. So remove it from vimc_cap struct so that it doesn't get
> > destroyed when one of the capture does, and allocate it memory
> > dynamically. Use kref with it as it will be used by multiple captures.
> >
>
> Is the vimc_stream stuct the context of the streamer? or of each 'stream'?
of streamer
>
> If it's the streamer - then can't we store this (non-dynamically) as
> part of the Sensor node, to avoid kzalloc/freeing it ?
I tried this after we discussed, but I kept on having some memory
issues..so I used this method as an alternate. If making vimc_streamer
struct dynamically is an issue with others as well (I understand why it
might be an issue, since it should not be dependent on which capture
calls initialised it), I can work on moving it to the sensor instead
>
>
> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > ---
> > drivers/media/test-drivers/vimc/vimc-capture.c | 15 +++++++++++----
> > drivers/media/test-drivers/vimc/vimc-streamer.c | 17 ++++++-----------
> > drivers/media/test-drivers/vimc/vimc-streamer.h | 2 ++
> > 3 files changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> > index 93418cb5a139..73e5bdd17c57 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> > @@ -28,7 +28,6 @@ struct vimc_cap_device {
> > spinlock_t qlock;
> > struct mutex lock;
> > u32 sequence;
> > - struct vimc_stream stream;
> > struct media_pad pad;
> > };
> >
> > @@ -241,19 +240,25 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
> > {
> > struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
> > struct media_entity *entity = &vcap->vdev.entity;
> > + struct media_pipeline *pipe = NULL;
> > + struct vimc_stream *stream;
> > int ret;
> >
> > atomic_inc(&vcap->ved.use_count);
> > vcap->sequence = 0;
> >
> > + stream = kzalloc(sizeof(*stream), GFP_ATOMIC);
> > + kref_init(&stream->refcount);
> > + pipe = &stream->pipe;
> > +
> > /* Start the media pipeline */
> > - ret = media_pipeline_start(entity, &vcap->stream.pipe);
> > + ret = media_pipeline_start(entity, pipe);
> > if (ret) {
> > vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
> > return ret;
> > }
> >
> > - ret = vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 1);
> > + ret = vimc_streamer_s_stream(stream, &vcap->ved, 1);
> > if (ret) {
> > media_pipeline_stop(entity);
> > vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
> > @@ -270,9 +275,11 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
> > static void vimc_cap_stop_streaming(struct vb2_queue *vq)
> > {
> > struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
> > + struct media_pipeline *pipe = vcap->ved.ent->pipe;
> > + struct vimc_stream *stream = container_of(pipe, struct vimc_stream, pipe);
>
> In fact, I see it's stored as part of the 'pipe' so there is one
> vimc_stream per pipeline ...
>
> So it could just be a structure on the pipe rather than obtaining a
> pointer here.
>
> I think it's probably 'ok' to have one streamer per pipe currently as
> the raw input node is not functional, but I also thought that by having
> the streamer as part of the sensor entity then there is one streamer
> ('one thread') per video source ... which would prevent this having to
> be changed if someone later deals with the video node that allows
> re-processing raw frames ?
>
>
>
> > atomic_dec(&vcap->ved.use_count);
> > - vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 0);
> > + vimc_streamer_s_stream(stream, &vcap->ved, 0);
> >
> > /* Stop the media pipeline */
> > media_pipeline_stop(&vcap->vdev.entity);
> > diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
> > index f5c9e2f3bbcb..fade37bee26d 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> > @@ -20,18 +20,13 @@
> > * Erases values of stream struct and terminates the thread
> > *
> > */
> > -static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
> > +static void vimc_streamer_pipeline_terminate(struct kref *ref)
> > {
> > - struct vimc_ent_device *ved;
> > -
> > - while (stream->pipe_size) {
> > - stream->pipe_size--;
> > - ved = stream->ved_pipeline[stream->pipe_size];
> > - stream->ved_pipeline[stream->pipe_size] = NULL;
> > - }
> > + struct vimc_stream *stream = container_of(ref, struct vimc_stream, refcount);
> >
> > kthread_stop(stream->kthread);
> > stream->kthread = NULL;
> > + kfree(stream);
> > }
> >
> > /**
> > @@ -202,7 +197,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> > }
> >
> > vimc_streamer_stream_terminate(cved);
> > - vimc_streamer_pipeline_terminate(stream);
> > + kref_put(&stream->refcount, vimc_streamer_pipeline_terminate);
> > return -EINVAL;
> > }
> >
> > @@ -298,7 +293,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
> > ret = PTR_ERR(stream->kthread);
> > dev_err(ved->dev, "kthread_run failed with %d\n", ret);
> > vimc_streamer_stream_terminate(ved);
> > - vimc_streamer_pipeline_terminate(stream);
> > + kref_put(&stream->refcount, vimc_streamer_pipeline_terminate);
> > }
> >
> > } else {
> > @@ -306,7 +301,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
> > goto out;
> >
> > vimc_streamer_stream_terminate(ved);
> > - vimc_streamer_pipeline_terminate(stream);
> > + kref_put(&stream->refcount, vimc_streamer_pipeline_terminate);
> > }
> > out:
> > mutex_unlock(&vimc_streamer_lock);
> > diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.h b/drivers/media/test-drivers/vimc/vimc-streamer.h
> > index 3bb6731b8d4d..533c88675362 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-streamer.h
> > +++ b/drivers/media/test-drivers/vimc/vimc-streamer.h
> > @@ -18,6 +18,7 @@
> > /**
> > * struct vimc_stream - struct that represents a stream in the pipeline
> > *
> > + * @refcount: kref object associated with stream struct
>
> What does it track though?
>
> We know it's associated with the stream struct because it's in the
> vimc_stream struct.
Vimc_streamer should be destroyed only when both the streams (if there
are two) have been terminated. So, it takes care of that.
>
>
>
> > * @pipe: the media pipeline object associated with this stream
> > * @ved_pipeline: array containing all the entities participating in the
> > * stream. The order is from a video device (usually a
>
> The fact that this comment still says "all entities participating in the
> stream" worries me a little, as I think now the Streamer is dealing with
> multiple streams.
>
> I think we need to be really clear with the differences of objects and
> terminology.
Yes, 'stream' here should be replaced with pipeline I think? As it
represents all the entities in the entire path connected with the sensor
>
> For instance I think the current terms are something like this:
>
> Streamer: Responsible for managing processing from a sensor device
> through all entities.
>
> Stream: A flow of frames to a single capture video device node.
>
> Pipeline: All entities used within the vimc streamer ?
>
> (I'm not sure if those are right, I'm writing down what my current
> interpretations are, so if someone wants to/can clarify - please do).
>
>
>
> > @@ -32,6 +33,7 @@
> > * process frames for the stream.
> > */
> > struct vimc_stream {
> > + struct kref refcount;
> > struct media_pipeline pipe;
> > struct vimc_ent_device *ved_pipeline[VIMC_STREAMER_PIPELINE_MAX_SIZE];
> > unsigned int pipe_size;
> >
>
> --
> Regards
> --
> Kieran
next prev parent reply other threads:[~2020-09-12 10:39 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-19 18:04 [PATCH v3 0/9] media: vimc: Multiple stream support in vimc Kaaira Gupta
2020-08-19 18:04 ` [PATCH v3 1/9] media: vimc: Move get_source_entity to vimc-common Kaaira Gupta
2020-08-20 15:32 ` Kieran Bingham
2020-08-19 18:04 ` [PATCH v3 2/9] media: vimc: Add get_frame callback Kaaira Gupta
2020-08-20 15:36 ` Kieran Bingham
2020-09-12 10:01 ` Kaaira Gupta
2020-10-02 11:08 ` Dafna Hirschfeld
2020-08-19 18:04 ` [PATCH v3 3/9] media: vimc: Add usage count to subdevices Kaaira Gupta
2020-10-02 11:13 ` Dafna Hirschfeld
2020-08-19 18:04 ` [PATCH v3 4/9] media: vimc: Separate starting stream from pipeline initialisation Kaaira Gupta
2020-08-21 15:11 ` Dafna Hirschfeld
2020-08-21 21:01 ` Kaaira Gupta
2020-08-28 20:37 ` Dafna Hirschfeld
2020-09-02 9:56 ` Kieran Bingham
2020-09-12 10:21 ` Kaaira Gupta
2020-10-02 12:18 ` Dafna Hirschfeld
2020-09-12 10:16 ` Kaaira Gupta
2020-08-19 18:04 ` [PATCH v3 5/9] media: vimc: Separate closing of stream and thread Kaaira Gupta
2020-09-02 10:13 ` Kieran Bingham
2020-09-12 10:28 ` Kaaira Gupta
2020-08-19 18:04 ` [PATCH v3 6/9] media: vimc: Serialize vimc_streamer_s_stream() Kaaira Gupta
2020-08-19 18:04 ` [PATCH v3 7/9] media: vimc: Dynamically allocate stream struct Kaaira Gupta
2020-09-02 10:29 ` Kieran Bingham
2020-09-12 10:39 ` Kaaira Gupta [this message]
2020-08-19 18:04 ` [PATCH v3 8/9] media: vimc: Join pipeline if one already exists Kaaira Gupta
2020-09-02 10:40 ` Kieran Bingham
2020-08-19 18:04 ` [PATCH v3 9/9] media: vimc: Run multiple captures on same thread Kaaira Gupta
2020-09-02 10:46 ` Kieran Bingham
2020-09-12 10:45 ` Kaaira Gupta
2020-09-02 10:51 ` [PATCH v3 0/9] media: vimc: Multiple stream support in vimc Kieran Bingham
2020-09-12 10:49 ` Kaaira Gupta
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=20200912103916.GE5022@kaaira-HP-Pavilion-Notebook \
--to=kgupta@es.iitr.ac.in \
--cc=helen.koike@collabora.com \
--cc=kieran.bingham@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=skhan@linuxfoundation.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).