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

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

 



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





[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