Hi Andy, On 4/2/23 08:06, Andy Shevchenko wrote: > 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. > ... I agree I have added a note about this to my list of possible code cleanups (which is quite long) but there are many higher priority issues to solve first. Regards, Hans
