On Sat, Apr 1, 2023 at 12:59 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > 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. So, I haven't checked the patch series of 28, but perhaps the above is a good candidate to have across the entire code, so we reduce the codebase. ... > >> 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. -- With Best Regards, Andy Shevchenko
