Re: [PATCH v3 33/38] media: ti-vpe: cal: add camerarx locking

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

On 04/06/2021 17:14, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Mon, May 24, 2021 at 02:09:04PM +0300, Tomi Valkeinen wrote:
>> We don't have any locking in camerarx for the subdev ops. We have
>> managed fine so far without locking, but in the future multiple video
>> capture devices can use the same camerarx, and locking is a must.
>>
>> Add a mutex to protect the camerarx subdev ops. Some of the functions
>> were slightly restructured to make lock handling cleaner.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/platform/ti-vpe/cal-camerarx.c | 81 ++++++++++++++------
>>   drivers/media/platform/ti-vpe/cal.h          |  3 +
>>   2 files changed, 61 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c
>> index 82392499e663..b87ffc52feb6 100644
>> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c
>> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c
>> @@ -601,12 +601,18 @@ cal_camerarx_get_pad_format(struct cal_camerarx *phy,
>>   static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable)
>>   {
>>   	struct cal_camerarx *phy = to_cal_camerarx(sd);
>> +	int r = 0;
> 
> The driver uses ret :-)

...fine... ;)

>> +
>> +	mutex_lock(&phy->mutex);
>>   
>>   	if (enable)
>> -		return cal_camerarx_start(phy);
>> +		r = cal_camerarx_start(phy);
>> +	else
>> +		cal_camerarx_stop(phy);
>>   
>> -	cal_camerarx_stop(phy);
>> -	return 0;
>> +	mutex_unlock(&phy->mutex);
>> +
>> +	return r;
>>   }
>>   
>>   static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
>> @@ -614,27 +620,36 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
>>   					  struct v4l2_subdev_mbus_code_enum *code)
>>   {
>>   	struct cal_camerarx *phy = to_cal_camerarx(sd);
>> +	int r = 0;
>> +
>> +	mutex_lock(&phy->mutex);
>>   
>>   	/* No transcoding, source and sink codes must match. */
>>   	if (code->pad == CAL_CAMERARX_PAD_SOURCE) {
>>   		struct v4l2_mbus_framefmt *fmt;
>>   
>> -		if (code->index > 0)
>> -			return -EINVAL;
>> +		if (code->index > 0) {
>> +			r = -EINVAL;
>> +			goto out;
>> +		}
>>   
>>   		fmt = cal_camerarx_get_pad_format(phy, sd_state,
>>   						  CAL_CAMERARX_PAD_SINK,
>>   						  code->which);
>>   		code->code = fmt->code;
>> -		return 0;
>> -	}
>> +	} else {
>> +		if (code->index >= cal_num_formats) {
>> +			r = -EINVAL;
>> +			goto out;
>> +		}
>>   
>> -	if (code->index >= cal_num_formats)
>> -		return -EINVAL;
>> +		code->code = cal_formats[code->index].code;
>> +	}
>>   
>> -	code->code = cal_formats[code->index].code;
>> +out:
>> +	mutex_unlock(&phy->mutex);
>>   
>> -	return 0;
>> +	return r;
>>   }
>>   
>>   static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
>> @@ -643,10 +658,13 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
>>   {
>>   	struct cal_camerarx *phy = to_cal_camerarx(sd);
>>   	const struct cal_format_info *fmtinfo;
>> +	int r = 0;
>>   
>>   	if (fse->index > 0)
>>   		return -EINVAL;
>>   
>> +	mutex_lock(&phy->mutex);
>> +
>>   	/* No transcoding, source and sink formats must match. */
>>   	if (fse->pad == CAL_CAMERARX_PAD_SOURCE) {
>>   		struct v4l2_mbus_framefmt *fmt;
>> @@ -654,27 +672,34 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
>>   		fmt = cal_camerarx_get_pad_format(phy, sd_state,
>>   						  CAL_CAMERARX_PAD_SINK,
>>   						  fse->which);
>> -		if (fse->code != fmt->code)
>> -			return -EINVAL;
>> +		if (fse->code != fmt->code) {
>> +			r = -EINVAL;
>> +			goto out;
>> +		}
>>   
>>   		fse->min_width = fmt->width;
>>   		fse->max_width = fmt->width;
>>   		fse->min_height = fmt->height;
>>   		fse->max_height = fmt->height;
>> +	} else {
>> +		fmtinfo = cal_format_by_code(fse->code);
>> +		if (!fmtinfo) {
>> +			r = -EINVAL;
>> +			goto out;
>> +		}
>>   
>> -		return 0;
>> +		fse->min_width =
>> +			CAL_MIN_WIDTH_BYTES * 8 / ALIGN(fmtinfo->bpp, 8);
>> +		fse->max_width =
>> +			CAL_MAX_WIDTH_BYTES * 8 / ALIGN(fmtinfo->bpp, 8);
> 
> This is a case where I'd write
> 
> 		fse->min_width = CAL_MIN_WIDTH_BYTES * 8
> 			       / ALIGN(fmtinfo->bpp, 8);
> 		fse->max_width = CAL_MAX_WIDTH_BYTES * 8
> 			       / ALIGN(fmtinfo->bpp, 8);
> 
> or go slightly over 80 columns.

Yes. clang-format insist on formatting like that, and I haven't figured 
out how to prevent it from moving everything to next line. It does that 
for some function calls too.

And I'm often relying on clang-format, as my editor doesn't like mixed 
spaces and tabs (googling shows that most of Internet hates mixed spaces 
and tabs...).

But wouldn't the operator usually be left in the earlier line, i.e.

fse->min_width = CAL_MIN_WIDTH_BYTES * 8 /
                  ALIGN(fmtinfo->bpp, 8);

? That's how I would split it.

>> +		fse->min_height = CAL_MIN_HEIGHT_LINES;
>> +		fse->max_height = CAL_MAX_HEIGHT_LINES;
>>   	}
>>   
>> -	fmtinfo = cal_format_by_code(fse->code);
>> -	if (!fmtinfo)
>> -		return -EINVAL;
>> -
>> -	fse->min_width = CAL_MIN_WIDTH_BYTES * 8 / ALIGN(fmtinfo->bpp, 8);
>> -	fse->max_width = CAL_MAX_WIDTH_BYTES * 8 / ALIGN(fmtinfo->bpp, 8);
>> -	fse->min_height = CAL_MIN_HEIGHT_LINES;
>> -	fse->max_height = CAL_MAX_HEIGHT_LINES;
>> +out:
>> +	mutex_unlock(&phy->mutex);
>>   
>> -	return 0;
>> +	return r;
>>   }
>>   
>>   static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd,
>> @@ -684,10 +709,14 @@ static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd,
>>   	struct cal_camerarx *phy = to_cal_camerarx(sd);
>>   	struct v4l2_mbus_framefmt *fmt;
>>   
>> +	mutex_lock(&phy->mutex);
>> +
>>   	fmt = cal_camerarx_get_pad_format(phy, sd_state, format->pad,
>>   					  format->which);
>>   	format->format = *fmt;
>>   
>> +	mutex_unlock(&phy->mutex);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -725,6 +754,8 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
>>   	format->format.field = V4L2_FIELD_NONE;
>>   
>>   	/* Store the format and propagate it to the source pad. */
>> +	mutex_lock(&phy->mutex);
>> +
>>   	fmt = cal_camerarx_get_pad_format(phy, sd_state,
>>   					  CAL_CAMERARX_PAD_SINK,
>>   					  format->which);
>> @@ -735,6 +766,8 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
>>   					  format->which);
>>   	*fmt = format->format;
>>   
>> +	mutex_unlock(&phy->mutex);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -801,6 +834,8 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
>>   	phy->cal = cal;
>>   	phy->instance = instance;
>>   
>> +	mutex_init(&phy->mutex);
> 
> A mutex_destroy() somewhere would be nice.

Right.

>> +
>>   	phy->res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>   						(instance == 0) ?
>>   						"cal_rx_core0" :
>> diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h
>> index ad08c189ad3b..78bd2e041d9a 100644
>> --- a/drivers/media/platform/ti-vpe/cal.h
>> +++ b/drivers/media/platform/ti-vpe/cal.h
>> @@ -163,6 +163,9 @@ struct cal_camerarx {
>>   	struct v4l2_subdev	subdev;
>>   	struct media_pad	pads[2];
>>   	struct v4l2_mbus_framefmt	formats[2];
>> +
>> +	/* mutex for camerarx ops */
>> +	struct mutex		mutex;
> 
> It's best when possible to list the fields protected by a lock, instead
> of the functions. It seems to be cal_camerarx.formats (but would need to
> be updated in subsequent patches).

I've added this.

  Tomi