linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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

  reply	other threads:[~2020-09-12 10:39 UTC|newest]

Thread overview: 30+ 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-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 \
    --subject='Re: [PATCH v3 7/9] media: vimc: Dynamically allocate stream struct' \
    /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

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).