Hi Deb,
On 25/04/2023 02:10, Deborah Brouwer wrote:
> Instead of storing video format, width and height separately in each file
> handle, move these fields to the main struct bttv. Use them wherever
> possible in preparation for vb2 conversion which stops using separate bttv
> file handles.
>
> Signed-off-by: Deborah Brouwer <deborah.brouwer@xxxxxxxxxxxxx>
> ---
> drivers/media/pci/bt8xx/bttv-driver.c | 34 +++++++++++++--------------
> drivers/media/pci/bt8xx/bttvp.h | 3 +++
> 2 files changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
> index e59f40dfccc3..7e7658a7ed40 100644
> --- a/drivers/media/pci/bt8xx/bttv-driver.c
> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
> @@ -2066,11 +2066,11 @@ static int bttv_g_fmt_vid_cap(struct file *file, void *priv,
> struct v4l2_format *f)
> {
> struct bttv_fh *fh = priv;
> + struct bttv *btv = video_drvdata(file);
>
> - pix_format_set_size(&f->fmt.pix, fh->fmt,
> - fh->width, fh->height);
> + pix_format_set_size(&f->fmt.pix, btv->fmt, btv->width, btv->height);
> f->fmt.pix.field = fh->cap.field;
> - f->fmt.pix.pixelformat = fh->fmt->fourcc;
> + f->fmt.pix.pixelformat = btv->fmt->fourcc;
> f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
>
> return 0;
> @@ -2190,6 +2190,9 @@ static int bttv_s_fmt_vid_cap(struct file *file, void *priv,
> btv->init.fmt = fmt;
> btv->init.width = f->fmt.pix.width;
> btv->init.height = f->fmt.pix.height;
> + btv->fmt = fmt;
> + btv->width = f->fmt.pix.width;
> + btv->height = f->fmt.pix.height;
>
> return 0;
> }
> @@ -2446,21 +2449,15 @@ static int bttv_s_selection(struct file *file, void *f, struct v4l2_selection *s
>
> fh->do_crop = 1;
>
> - if (fh->width < c.min_scaled_width) {
> - fh->width = c.min_scaled_width;
> - btv->init.width = c.min_scaled_width;
> - } else if (fh->width > c.max_scaled_width) {
> - fh->width = c.max_scaled_width;
> - btv->init.width = c.max_scaled_width;
> - }
> + if (btv->width < c.min_scaled_width)
> + btv->width = c.min_scaled_width;
> + else if (btv->width > c.max_scaled_width)
> + btv->width = c.max_scaled_width;
>
> - if (fh->height < c.min_scaled_height) {
> - fh->height = c.min_scaled_height;
> - btv->init.height = c.min_scaled_height;
> - } else if (fh->height > c.max_scaled_height) {
> - fh->height = c.max_scaled_height;
> - btv->init.height = c.max_scaled_height;
> - }
> + if (btv->height < c.min_scaled_height)
> + btv->height = c.min_scaled_height;
> + else if (btv->height > c.max_scaled_height)
> + btv->height = c.max_scaled_height;
>
> return 0;
> }
> @@ -3636,6 +3633,9 @@ static int bttv_probe(struct pci_dev *dev, const struct pci_device_id *pci_id)
> btv->init.fmt = format_by_fourcc(V4L2_PIX_FMT_BGR24);
> btv->init.width = 320;
> btv->init.height = 240;
> + btv->fmt = format_by_fourcc(V4L2_PIX_FMT_BGR24);
> + btv->width = 320;
> + btv->height = 240;
> btv->input = 0;
>
> v4l2_ctrl_new_std(hdl, &bttv_ctrl_ops,
> diff --git a/drivers/media/pci/bt8xx/bttvp.h b/drivers/media/pci/bt8xx/bttvp.h
> index 717f002a41df..7f02dd5866d7 100644
> --- a/drivers/media/pci/bt8xx/bttvp.h
> +++ b/drivers/media/pci/bt8xx/bttvp.h
> @@ -449,6 +449,9 @@ struct bttv {
>
> unsigned int users;
> struct bttv_fh init;
> + const struct bttv_format *fmt;
> + int width;
> + int height;
>
> /* used to make dvb-bt8xx autoloadable */
> struct work_struct request_module_wk;
It's a bit odd: you add these fields to struct bttv, but they are not
removed from bttv_fh. I suspect you only switched to these new fields
in cases where they are needed for the vb2 conversion and kept the
fields in bttv_fh to avoid having to change more code than is necessary
for that?
If so, that should be clearly documented in the commit log & subject
line since it currently says 'moved', not 'copied'.
The same question applies to patch 5.
Regards,
Hans
