diff mbox

[RFC,3/3] s5p-fimc: improved pipeline try format routine

Message ID 1353684150-24581-4-git-send-email-a.hajda@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrzej Hajda Nov. 23, 2012, 3:22 p.m. UTC 
Function support variable number of subdevs in pipe-line.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-fimc/fimc-capture.c |  100 +++++++++++++++---------
 1 file changed, 64 insertions(+), 36 deletions(-)

Comments

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Dec. 4, 2012, 3:55 p.m. UTC | #1 
Hi Andrzej,

On 11/23/2012 04:22 PM, Andrzej Hajda wrote:
> Function support variable number of subdevs in pipe-line.

I'm will be applying this patch with description changed to:

Make the pipeline try format routine more generic to support any
number of subdevs in the pipeline, rather than hard coding it for
only a sensor, MIPI-CSIS and FIMC subdevs and the FIMC video node.

> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/platform/s5p-fimc/fimc-capture.c |  100 +++++++++++++++---------
>  1 file changed, 64 insertions(+), 36 deletions(-)
> 
...
>  /**
>   * fimc_pipeline_try_format - negotiate and/or set formats at pipeline
>   *                            elements
> @@ -809,65 +824,78 @@ static int fimc_pipeline_try_format(struct fimc_ctx *ctx,
...
>  		ffmt = fimc_find_format(NULL, mf->code != 0 ? &mf->code : NULL,
>  					FMT_FLAGS_CAM, i++);
> -		if (ffmt == NULL) {
> -			/*
> -			 * Notify user-space if common pixel code for
> -			 * host and sensor does not exist.
> -			 */
> +		if (ffmt == NULL)
>  			return -EINVAL;
> -		}
> +

And as we agreed, with this chunk removed from the patch. Since the comment
still stands.

--

Thank you!
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sakari Ailus Dec. 4, 2012, 11:22 p.m. UTC | #2 
Hi Andrzej,

On Fri, Nov 23, 2012 at 04:22:30PM +0100, Andrzej Hajda wrote:
> Function support variable number of subdevs in pipe-line.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/platform/s5p-fimc/fimc-capture.c |  100 +++++++++++++++---------
>  1 file changed, 64 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c
> index 3acbea3..39c4555 100644
> --- a/drivers/media/platform/s5p-fimc/fimc-capture.c
> +++ b/drivers/media/platform/s5p-fimc/fimc-capture.c
> @@ -794,6 +794,21 @@ static int fimc_cap_enum_fmt_mplane(struct file *file, void *priv,
>  	return 0;
>  }
>  
> +static struct media_entity *fimc_pipeline_get_head(struct media_entity *me)
> +{
> +	struct media_pad *pad = &me->pads[0];
> +
> +	while (!(pad->flags & MEDIA_PAD_FL_SOURCE)) {
> +		pad = media_entity_remote_source(pad);
> +		if (!pad)
> +			break;

Isn't it an error if a sink pad of the entity isn't connected?
media_entity_remote_source(pad) returns NULL if the link is disabled. I'm
just wondering if this is possible.

> +		me = pad->entity;
> +		pad = &me->pads[0];
> +	}
> +
> +	return me;
> +}
> +
Andrzej Hajda Dec. 5, 2012, 8:12 a.m. UTC | #3 
Hi Sakari,

On 05.12.2012 00:22, Sakari Ailus wrote:
> Hi Andrzej,
>
> On Fri, Nov 23, 2012 at 04:22:30PM +0100, Andrzej Hajda wrote:
>> Function support variable number of subdevs in pipe-line.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>   drivers/media/platform/s5p-fimc/fimc-capture.c |  100 +++++++++++++++---------
>>   1 file changed, 64 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c
>> index 3acbea3..39c4555 100644
>> --- a/drivers/media/platform/s5p-fimc/fimc-capture.c
>> +++ b/drivers/media/platform/s5p-fimc/fimc-capture.c
>> @@ -794,6 +794,21 @@ static int fimc_cap_enum_fmt_mplane(struct file *file, void *priv,
>>   	return 0;
>>   }
>>   
>> +static struct media_entity *fimc_pipeline_get_head(struct media_entity *me)
>> +{
>> +	struct media_pad *pad = &me->pads[0];
>> +
>> +	while (!(pad->flags & MEDIA_PAD_FL_SOURCE)) {
>> +		pad = media_entity_remote_source(pad);
>> +		if (!pad)
>> +			break;
> Isn't it an error if a sink pad of the entity isn't connected?
> media_entity_remote_source(pad) returns NULL if the link is disabled. I'm
> just wondering if this is possible.
AFAIK documentation says nothing about it and current media_entity 
implementation
accepts pipelines with pads without active links.
In fact during s5c73m3 sensor development I have successfully used such 
pipeline as a temporary solution.
>
>> +		me = pad->entity;
>> +		pad = &me->pads[0];
>> +	}
>> +
>> +	return me;
>> +}
>> +
Regards
Andrzej

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrzej Hajda Dec. 5, 2012, 8:36 a.m. UTC | #4 
Hi Sylwester,

On 04.12.2012 16:55, Sylwester Nawrocki wrote:
> Hi Andrzej,
>
> On 11/23/2012 04:22 PM, Andrzej Hajda wrote:
>> Function support variable number of subdevs in pipe-line.
> I'm will be applying this patch with description changed to:
>
> Make the pipeline try format routine more generic to support any
> number of subdevs in the pipeline, rather than hard coding it for
> only a sensor, MIPI-CSIS and FIMC subdevs and the FIMC video node.
Seems better :)
>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>   drivers/media/platform/s5p-fimc/fimc-capture.c |  100 +++++++++++++++---------
>>   1 file changed, 64 insertions(+), 36 deletions(-)
>>
> ...
>>   /**
>>    * fimc_pipeline_try_format - negotiate and/or set formats at pipeline
>>    *                            elements
>> @@ -809,65 +824,78 @@ static int fimc_pipeline_try_format(struct fimc_ctx *ctx,
> ...
>>   		ffmt = fimc_find_format(NULL, mf->code != 0 ? &mf->code : NULL,
>>   					FMT_FLAGS_CAM, i++);
>> -		if (ffmt == NULL) {
>> -			/*
>> -			 * Notify user-space if common pixel code for
>> -			 * host and sensor does not exist.
>> -			 */
>> +		if (ffmt == NULL)
>>   			return -EINVAL;
>> -		}
>> +
> And as we agreed, with this chunk removed from the patch. Since the comment
> still stands.
OK.
>
> --
>
> Thank you!
> Sylwester
>

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Dec. 5, 2012, 9:57 a.m. UTC | #5 
Hi Sakari,

Thank you for the comments.

On 12/05/2012 12:22 AM, Sakari Ailus wrote:
>> diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c
>> index 3acbea3..39c4555 100644
>> --- a/drivers/media/platform/s5p-fimc/fimc-capture.c
>> +++ b/drivers/media/platform/s5p-fimc/fimc-capture.c
>> @@ -794,6 +794,21 @@ static int fimc_cap_enum_fmt_mplane(struct file *file, void *priv,
>>  	return 0;
>>  }
>>  
>> +static struct media_entity *fimc_pipeline_get_head(struct media_entity *me)
>> +{
>> +	struct media_pad *pad = &me->pads[0];
>> +
>> +	while (!(pad->flags & MEDIA_PAD_FL_SOURCE)) {
>> +		pad = media_entity_remote_source(pad);
>> +		if (!pad)
>> +			break;
> 
> Isn't it an error if a sink pad of the entity isn't connected?
> media_entity_remote_source(pad) returns NULL if the link is disabled. I'm
> just wondering if this is possible.

It is not possible to not have all links connected, from video device to
the sensor subdev entity, at the point when this function is called
(from within VIDIOC_TRY_FMT or VIDIOC_S_FMT ioctls). fimc_pipeline_prepare()
walks the graph in direction from video node to the sensor, also using
media_entity_remote_source(). If it doesn't reach the sensor entity and
save a pointer to it for further reference the video node open() will fail.
And there won't be even a chance to invoke VIDIOC_TRY_FMT/VIDIOC_S_FMT 
ioctls. It's true the above function takes some assumptions that could be
explained with a relevant comment.

It's worth to note that all this in-driver setting of format at the 
pipeline is for static default links created by the driver. If userspace 
reconfigures links it should not rely on it in first place. When we finally 
prepare a libv4l support for this driver it should just go away. Regular 
V4L2 applications must use libv4l2 anyway since the driver supports only 
multi-planar API.

--

Regards,
Sylwester

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c
index 3acbea3..39c4555 100644
--- a/drivers/media/platform/s5p-fimc/fimc-capture.c
+++ b/drivers/media/platform/s5p-fimc/fimc-capture.c
@@ -794,6 +794,21 @@  static int fimc_cap_enum_fmt_mplane(struct file *file, void *priv,
 	return 0;
 }
 
+static struct media_entity *fimc_pipeline_get_head(struct media_entity *me)
+{
+	struct media_pad *pad = &me->pads[0];
+
+	while (!(pad->flags & MEDIA_PAD_FL_SOURCE)) {
+		pad = media_entity_remote_source(pad);
+		if (!pad)
+			break;
+		me = pad->entity;
+		pad = &me->pads[0];
+	}
+
+	return me;
+}
+
 /**
  * fimc_pipeline_try_format - negotiate and/or set formats at pipeline
  *                            elements
@@ -809,65 +824,78 @@  static int fimc_pipeline_try_format(struct fimc_ctx *ctx,
 {
 	struct fimc_dev *fimc = ctx->fimc_dev;
 	struct v4l2_subdev *sd = fimc->pipeline.subdevs[IDX_SENSOR];
-	struct v4l2_subdev *csis = fimc->pipeline.subdevs[IDX_CSIS];
 	struct v4l2_subdev_format sfmt;
 	struct v4l2_mbus_framefmt *mf = &sfmt.format;
-	struct fimc_fmt *ffmt = NULL;
-	int ret, i = 0;
+	struct media_entity *me;
+	struct fimc_fmt *ffmt;
+	struct media_pad *pad;
+	int ret, i = 1;
+	u32 fcc;
 
 	if (WARN_ON(!sd || !tfmt))
 		return -EINVAL;
 
 	memset(&sfmt, 0, sizeof(sfmt));
 	sfmt.format = *tfmt;
-
 	sfmt.which = set ? V4L2_SUBDEV_FORMAT_ACTIVE : V4L2_SUBDEV_FORMAT_TRY;
+
+	me = fimc_pipeline_get_head(&sd->entity);
+
 	while (1) {
+
 		ffmt = fimc_find_format(NULL, mf->code != 0 ? &mf->code : NULL,
 					FMT_FLAGS_CAM, i++);
-		if (ffmt == NULL) {
-			/*
-			 * Notify user-space if common pixel code for
-			 * host and sensor does not exist.
-			 */
+		if (ffmt == NULL)
 			return -EINVAL;
-		}
+
 		mf->code = tfmt->code = ffmt->mbus_code;
 
-		ret = v4l2_subdev_call(sd, pad, set_fmt, NULL, &sfmt);
-		if (ret)
-			return ret;
-		if (mf->code != tfmt->code) {
-			mf->code = 0;
-			continue;
+		/* set format on all pipeline subdevs */
+		while (me != &fimc->vid_cap.subdev.entity) {
+			sd = media_entity_to_v4l2_subdev(me);
+
+			sfmt.pad = 0;
+			ret = v4l2_subdev_call(sd, pad, set_fmt, NULL, &sfmt);
+			if (ret)
+				return ret;
+
+			if (me->pads[0].flags & MEDIA_PAD_FL_SINK) {
+				sfmt.pad = me->num_pads - 1;
+				mf->code = tfmt->code;
+				ret = v4l2_subdev_call(sd, pad, set_fmt, NULL,
+									&sfmt);
+				if (ret)
+					return ret;
+			}
+
+			pad = media_entity_remote_source(&me->pads[sfmt.pad]);
+			if (!pad)
+				return -EINVAL;
+			me = pad->entity;
 		}
-		if (mf->width != tfmt->width || mf->height != tfmt->height) {
-			u32 fcc = ffmt->fourcc;
-			tfmt->width  = mf->width;
-			tfmt->height = mf->height;
-			ffmt = fimc_capture_try_format(ctx,
-					       &tfmt->width, &tfmt->height,
-					       NULL, &fcc, FIMC_SD_PAD_SOURCE);
-			if (ffmt && ffmt->mbus_code)
-				mf->code = ffmt->mbus_code;
-			if (mf->width != tfmt->width ||
-			    mf->height != tfmt->height)
-				continue;
-			tfmt->code = mf->code;
-		}
-		if (csis)
-			ret = v4l2_subdev_call(csis, pad, set_fmt, NULL, &sfmt);
 
-		if (mf->code == tfmt->code &&
-		    mf->width == tfmt->width && mf->height == tfmt->height)
-			break;
+		if (mf->code != tfmt->code)
+			continue;
+
+		fcc = ffmt->fourcc;
+		tfmt->width  = mf->width;
+		tfmt->height = mf->height;
+		ffmt = fimc_capture_try_format(ctx, &tfmt->width, &tfmt->height,
+					NULL, &fcc, FIMC_SD_PAD_SINK);
+		ffmt = fimc_capture_try_format(ctx, &tfmt->width, &tfmt->height,
+					NULL, &fcc, FIMC_SD_PAD_SOURCE);
+		if (ffmt && ffmt->mbus_code)
+			mf->code = ffmt->mbus_code;
+		if (mf->width != tfmt->width || mf->height != tfmt->height)
+			continue;
+		tfmt->code = mf->code;
+		break;
 	}
 
 	if (fmt_id && ffmt)
 		*fmt_id = ffmt;
 	*tfmt = *mf;
 
-	dbg("code: 0x%x, %dx%d, %p", mf->code, mf->width, mf->height, ffmt);
 	return 0;
 }