Hi Max,
On Wed, Sep 07, 2022 at 03:44:44PM +0200, Maximilian Luz wrote:
> On Thu, 25 Aug 2022 22:03:51 +0300, Sakari Ailus wrote:
> > What the IMGU driver did was that it first acquired the pointers to active
> > and try V4L2 subdev state, and only then figured out which one to use.
> >
> > The problem with that approach and a later patch (see Fixes: tag) is that
> > as sd_state argument to v4l2_subdev_get_try_crop() et al is NULL, there is
> > now an attempt to dereference that.
> >
> > Fix this.
>
> Thank you for this fix, this however only addresses
> imgu_subdev_get_selection(), but the issue is also present in
> imgu_subdev_set_selection(). I assume that a similar fix is needed for that.
> I've added a diff for that below. Feel free to squash that into your patch or
> let me know if I should submit this separately.
This looks like a problem indeed. I'll let Sakari decide what he wants
to do. Adding wrappers along the lines of
static struct v4l2_rect *
imgu_subdev_get_crop(struct imgu_v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state, unsigned int pad,
enum v4l2_subdev_format_whence which)
{
if (which == V4L2_SUBDEV_FORMAT_TRY)
return v4l2_subdev_get_try_crop(&imgu_sd->subdev, sd_state, pad);
else
return &imgu_sd->rect.eff;
}
(same for the selection rectangle) and using them through the code may
help.
> ---
> drivers/staging/media/ipu3/ipu3-v4l2.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
> index 2234bb8d48b3..079f2635c70d 100644
> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
> @@ -223,10 +223,9 @@ static int imgu_subdev_set_selection(struct v4l2_subdev *sd,
> struct v4l2_subdev_selection *sel)
> {
> struct imgu_device *imgu = v4l2_get_subdevdata(sd);
> - struct imgu_v4l2_subdev *imgu_sd = container_of(sd,
> - struct imgu_v4l2_subdev,
> - subdev);
> - struct v4l2_rect *rect, *try_sel;
> + struct imgu_v4l2_subdev *imgu_sd =
> + container_of(sd, struct imgu_v4l2_subdev, subdev);
> + struct v4l2_rect *rect;
>
> dev_dbg(&imgu->pci_dev->dev,
> "set subdev %u sel which %u target 0x%4x rect [%ux%u]",
> @@ -238,22 +237,22 @@ static int imgu_subdev_set_selection(struct v4l2_subdev *sd,
>
> switch (sel->target) {
> case V4L2_SEL_TGT_CROP:
> - try_sel = v4l2_subdev_get_try_crop(sd, sd_state, sel->pad);
> - rect = &imgu_sd->rect.eff;
> + if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> + rect = v4l2_subdev_get_try_crop(sd, sd_state, sel->pad);
> + else
> + rect = &imgu_sd->rect.eff;
> break;
> case V4L2_SEL_TGT_COMPOSE:
> - try_sel = v4l2_subdev_get_try_compose(sd, sd_state, sel->pad);
> - rect = &imgu_sd->rect.bds;
> + if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> + rect = v4l2_subdev_get_try_compose(sd, sd_state, sel->pad);
> + else
> + rect = &imgu_sd->rect.bds;
> break;
> default:
> return -EINVAL;
> }
>
> - if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> - *try_sel = sel->r;
> - else
> - *rect = sel->r;
> -
> + *rect = sel->r;
> return 0;
> }
>
--
Regards,
Laurent Pinchart
