| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200912103916.GE5022@kaaira-HP-Pavilion-Notebook>
Date: Sat, 12 Sep 2020 16:09:16 +0530
From: Kaaira Gupta <kgupta@...iitr.ac.in>
To: Kieran Bingham <kieran.bingham@...asonboard.com>
Cc: Helen Koike <helen.koike@...labora.com>,
Shuah Khan <skhan@...uxfoundation.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 7/9] media: vimc: Dynamically allocate stream struct
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@...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
Powered by blists - more mailing lists