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