Re: [PATCH v7 06/27] media: entity: Move the pipeline from entity to pads

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

On 08/07/2021 16:11, Jacopo Mondi wrote:
> Hello again,
> 
> On Mon, May 24, 2021 at 01:43:47PM +0300, Tomi Valkeinen wrote:
>> From: Sakari Ailus <sakari.ailus@linux.intel.com>
>>
>> This moves the pipe and stream_count fields from struct media_entity to
>> struct media_pad. Effectively streams become pad-specific rather than
>> being entity specific, allowing several independent streams to traverse a
>> single entity and an entity to be part of several streams.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>
>> - Update documentation to use 'pads'
>> - Use the media pad iterator in media_entity.c
>> - Update rcar-dma.c to use the new per-pad stream count
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>
>> - Fix cleanup in the error path of __media_pipeline_start()
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>   drivers/media/mc/mc-entity.c                  | 68 +++++++++++--------
>>   drivers/media/platform/exynos4-is/fimc-isp.c  |  2 +-
>>   drivers/media/platform/exynos4-is/fimc-lite.c |  2 +-
>>   drivers/media/platform/omap3isp/isp.c         |  2 +-
>>   drivers/media/platform/omap3isp/ispvideo.c    |  2 +-
>>   drivers/media/platform/omap3isp/ispvideo.h    |  2 +-
>>   drivers/media/platform/rcar-vin/rcar-core.c   | 16 +++--
>>   drivers/media/platform/rcar-vin/rcar-dma.c    |  2 +-
>>   drivers/media/platform/xilinx/xilinx-dma.c    |  2 +-
>>   drivers/media/platform/xilinx/xilinx-dma.h    |  2 +-
>>   drivers/staging/media/imx/imx-media-utils.c   |  2 +-
>>   drivers/staging/media/omap4iss/iss.c          |  2 +-
>>   drivers/staging/media/omap4iss/iss_video.c    |  2 +-
>>   drivers/staging/media/omap4iss/iss_video.h    |  2 +-
>>   include/media/media-entity.h                  | 21 +++---
>>   15 files changed, 73 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
>> index 40ae9b6bac47..ea1cf7f63ae8 100644
>> --- a/drivers/media/mc/mc-entity.c
>> +++ b/drivers/media/mc/mc-entity.c
>> @@ -424,24 +424,28 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
>>
>>   	while ((pad = media_graph_walk_next(graph))) {
>>   		struct media_entity *entity = pad->entity;
>> +		bool skip_validation = pad->pipe != NULL;
>> +		struct media_pad *iter;
>>
>>   		DECLARE_BITMAP(active, MEDIA_ENTITY_MAX_PADS);
>>   		DECLARE_BITMAP(has_no_links, MEDIA_ENTITY_MAX_PADS);
>>
>> -		entity->stream_count++;
>> -
>> -		if (entity->pipe && entity->pipe != pipe) {
>> -			pr_err("Pipe active for %s. Can't start for %s\n",
>> -				entity->name,
>> -				pad_err->entity->name);
>> -			ret = -EBUSY;
>> -			goto error;
>> +		media_entity_for_each_pad(entity, iter) {
>> +			if (iter->pipe && iter->pipe != pipe) {
>> +				pr_err("Pipe active for %s. Can't start for %s\n",
>> +				       entity->name, iter->entity->name);
>> +				ret = -EBUSY;
>> +			} else {
>> +				iter->pipe = pipe;
>> +			}
>> +			iter->stream_count++;
>>   		}
>>
>> -		entity->pipe = pipe;
>> +		if (ret)
> 
> ret is not initialized when declared and there is a code path that
> could lead here without assigning it. I would initialize it to 0 when
> declaring it, or even re-assign it to 0 at the beginning of this while
> loop.

Good catch! I'll fix that.

  Tomi