Re: [PATCH 02/10] media: atomisp: Remove continuous mode support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andy,

Thank you for the reviews!

On 2/21/23 16:57, Andy Shevchenko wrote:
> On Tue, Feb 21, 2023 at 03:58:58PM +0100, Hans de Goede wrote:
>> Continues mode is a special mode where 2 /dev/video devices can be active
>> at the same time. Either the video-preview + video nodes or the
>> viewfinder (for still capture) + capture nodes.
>>
>> For the video-preview + video-recording case modern userspace will
>> use a single stream multiplexed by pipewire.
>>
>> The still-capture case is extra special only starting the preview
>> stream and then relying on a custom ATOMISP_IOC_S_CONT_CAPTURE_CONFIG
>> ioctl to set things up followed by a second stream on to capture
>> the amount of configured still pictures. While running the sensor
>> at full resolution all the time. This case too is better handled
>> with dma-buf + GPU downscaling for the view-finder rather then all this
>> custom special code. Besises this the ioctl expects a bunch of special
>> non error checked conditions to be met otherwise things will crash/hang.
>>
>> The continues mode also involves a special cases all over the code
>> getting in the way of further cleanups and simplifying the code to
>> using just 1 /dev/video# node. So lets remove it and the
>> related custom ATOMISP_IOC_S_CONT_CAPTURE_CONFIG ioctl.
> 
> ...
> 
>> +	ret = atomisp_set_fmt_to_snr(vdev, &s_fmt,
>> +				     f->fmt.pix.pixelformat, padding_w,
> 
> At least one parameter can be moved to the previous line.

Ack, fixed in my local tree which I will push to:

https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp

When I'm done processing your other review remarks.

> 
>> +				     padding_h, dvs_env_w, dvs_env_h);
>> +	if (ret) {
>> +		dev_warn(isp->dev,
>> +			 "Set format to sensor failed with %d\n", ret);
>> +		return -EINVAL;
> 
> ...
> 
>>  	case ATOMISP_RUN_MODE_PREVIEW:
>> -		if (!asd->continuous_mode->val) {
>> -			if (pipe_id == IA_CSS_PIPE_ID_PREVIEW)
>> -				return true;
>> +		if (pipe_id == IA_CSS_PIPE_ID_PREVIEW)
>> +			return true;
>>  
>> -			return false;
>> -		}
>> -		fallthrough;
>> +		return false;
> 
> 		return pipe_id == IA_CSS_PIPE_ID_PREVIEW;

I agree that that is cleaner, but there are a bunch of other cases
in this switch case which are not touched by this patch and
they all follow the same pattern as which the modified cases
use after this patch, e.g. :

        case ATOMISP_RUN_MODE_STILL_CAPTURE:
                if (pipe_id == IA_CSS_PIPE_ID_CAPTURE)
                        return true;

                return false;

So this patch basically makes all of them consistent with
each other. So I'm going to keep this as is.

Regards,

Hans



> 
> ...
> 
>>  	case ATOMISP_RUN_MODE_VIDEO:
>> -		if (!asd->continuous_mode->val) {
>> -			if (pipe_id == IA_CSS_PIPE_ID_VIDEO ||
>> -			    pipe_id == IA_CSS_PIPE_ID_YUVPP)
>> -				return true;
>> -			else
>> -				return false;
>> -		}
>> -		fallthrough;
>> +		if (pipe_id == IA_CSS_PIPE_ID_VIDEO || pipe_id == IA_CSS_PIPE_ID_YUVPP)
>> +			return true;
>> +
>> +		return false;
> 
> Similar.
> 





[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux