| Message ID | 20240827-iris_v3-v3-17-c5fdbbe65e70@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Qualcomm iris video decoder driver | expand |
On 27/08/2024 11:05, Dikshita Agarwal via B4 Relay wrote: > From: Vedang Nagar <quic_vnagar@quicinc.com> > > Implement query_cap, query_ctrl and query_menu ioctls in the > driver with necessary hooks. > > Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com> > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > --- > drivers/media/platform/qcom/iris/iris_vidc.c | 89 ++++++++++++++++++++++++++++ > 1 file changed, 89 insertions(+) > > diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c > index 7d5da30cb1d1..1dd612b7cec5 100644 > --- a/drivers/media/platform/qcom/iris/iris_vidc.c > +++ b/drivers/media/platform/qcom/iris/iris_vidc.c > @@ -362,6 +362,92 @@ static int iris_enum_framesizes(struct file *filp, void *fh, > return ret; > } > > +static int iris_querycap(struct file *filp, void *fh, struct v4l2_capability *cap) > +{ > + struct iris_inst *inst; > + int ret = 0; > + > + inst = iris_get_inst(filp, fh); > + if (!inst) > + return -EINVAL; > + > + mutex_lock(&inst->lock); > + strscpy(cap->driver, IRIS_DRV_NAME, sizeof(cap->driver)); > + strscpy(cap->bus_info, IRIS_BUS_NAME, sizeof(cap->bus_info)); > + memset(cap->reserved, 0, sizeof(cap->reserved)); > + strscpy(cap->card, "iris_decoder", sizeof(cap->card)); > + mutex_unlock(&inst->lock); Locking is a good thing but, this seems very rote. What's being protected here ? Please take a critical - no pun intended - look at your locking strategy here. I mentioned previously taking a core lock and releasing it with a level of granularity that didn't make a ton of sense to me, here's another example of locking for locking's sake. Please go through your code, look at your locks with a critical eye and say "what's this for, why are doing this, what is the lock supposed to guarantee here". I appreciate that can be difficult with a progressive patchset so recommend jumping in at the end and doing that analysis. --- bod
On 9/24/2024 8:19 PM, Bryan O'Donoghue wrote: > On 27/08/2024 11:05, Dikshita Agarwal via B4 Relay wrote: >> From: Vedang Nagar <quic_vnagar@quicinc.com> >> >> Implement query_cap, query_ctrl and query_menu ioctls in the >> driver with necessary hooks. >> >> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com> >> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >> --- >> drivers/media/platform/qcom/iris/iris_vidc.c | 89 >> ++++++++++++++++++++++++++++ >> 1 file changed, 89 insertions(+) >> >> diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c >> b/drivers/media/platform/qcom/iris/iris_vidc.c >> index 7d5da30cb1d1..1dd612b7cec5 100644 >> --- a/drivers/media/platform/qcom/iris/iris_vidc.c >> +++ b/drivers/media/platform/qcom/iris/iris_vidc.c >> @@ -362,6 +362,92 @@ static int iris_enum_framesizes(struct file *filp, >> void *fh, >> return ret; >> } >> +static int iris_querycap(struct file *filp, void *fh, struct >> v4l2_capability *cap) >> +{ >> + struct iris_inst *inst; >> + int ret = 0; >> + >> + inst = iris_get_inst(filp, fh); >> + if (!inst) >> + return -EINVAL; >> + >> + mutex_lock(&inst->lock); >> + strscpy(cap->driver, IRIS_DRV_NAME, sizeof(cap->driver)); >> + strscpy(cap->bus_info, IRIS_BUS_NAME, sizeof(cap->bus_info)); >> + memset(cap->reserved, 0, sizeof(cap->reserved)); >> + strscpy(cap->card, "iris_decoder", sizeof(cap->card)); >> + mutex_unlock(&inst->lock); > > Locking is a good thing but, this seems very rote. > > What's being protected here ? > > Please take a critical - no pun intended - look at your locking strategy here. > > I mentioned previously taking a core lock and releasing it with a level of > granularity that didn't make a ton of sense to me, here's another example > of locking for locking's sake. > > Please go through your code, look at your locks with a critical eye and say > "what's this for, why are doing this, what is the lock supposed to > guarantee here". > > I appreciate that can be difficult with a progressive patchset so recommend > jumping in at the end and doing that analysis. > sure, will revisit the usage of inst->lock and improve as needed. > --- > bod
diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c index 7d5da30cb1d1..1dd612b7cec5 100644 --- a/drivers/media/platform/qcom/iris/iris_vidc.c +++ b/drivers/media/platform/qcom/iris/iris_vidc.c @@ -362,6 +362,92 @@ static int iris_enum_framesizes(struct file *filp, void *fh, return ret; } +static int iris_querycap(struct file *filp, void *fh, struct v4l2_capability *cap) +{ + struct iris_inst *inst; + int ret = 0; + + inst = iris_get_inst(filp, fh); + if (!inst) + return -EINVAL; + + mutex_lock(&inst->lock); + strscpy(cap->driver, IRIS_DRV_NAME, sizeof(cap->driver)); + strscpy(cap->bus_info, IRIS_BUS_NAME, sizeof(cap->bus_info)); + memset(cap->reserved, 0, sizeof(cap->reserved)); + strscpy(cap->card, "iris_decoder", sizeof(cap->card)); + mutex_unlock(&inst->lock); + + return ret; +} + +static int iris_queryctrl(struct file *filp, void *fh, struct v4l2_queryctrl *q_ctrl) +{ + struct v4l2_ctrl *ctrl; + struct iris_inst *inst; + int ret = 0; + + inst = iris_get_inst(filp, fh); + if (!inst || !q_ctrl) + return -EINVAL; + + mutex_lock(&inst->lock); + ctrl = v4l2_ctrl_find(&inst->ctrl_handler, q_ctrl->id); + if (!ctrl) { + ret = -EINVAL; + goto unlock; + } + + q_ctrl->minimum = ctrl->minimum; + q_ctrl->maximum = ctrl->maximum; + q_ctrl->default_value = ctrl->default_value; + q_ctrl->flags = 0; + q_ctrl->step = ctrl->step; + +unlock: + mutex_unlock(&inst->lock); + + return ret; +} + +static int iris_querymenu(struct file *filp, void *fh, struct v4l2_querymenu *qmenu) +{ + struct v4l2_ctrl *ctrl; + struct iris_inst *inst; + int ret = 0; + + inst = iris_get_inst(filp, fh); + if (!inst || !qmenu) + return -EINVAL; + + mutex_lock(&inst->lock); + ctrl = v4l2_ctrl_find(&inst->ctrl_handler, qmenu->id); + if (!ctrl) { + ret = -EINVAL; + goto unlock; + } + + if (ctrl->type != V4L2_CTRL_TYPE_MENU) { + ret = -EINVAL; + goto unlock; + } + + if (qmenu->index < ctrl->minimum || qmenu->index > ctrl->maximum) { + ret = -EINVAL; + goto unlock; + } + + if (ctrl->menu_skip_mask & (1 << qmenu->index)) { + ret = -EINVAL; + goto unlock; + } + +unlock: + mutex_unlock(&inst->lock); + + return ret; +} + static int iris_g_selection(struct file *filp, void *fh, struct v4l2_selection *s) { struct iris_inst *inst; @@ -453,6 +539,9 @@ static const struct v4l2_ioctl_ops iris_v4l2_ioctl_ops = { .vidioc_g_fmt_vid_out_mplane = iris_g_fmt_vid_mplane, .vidioc_enum_framesizes = iris_enum_framesizes, .vidioc_reqbufs = v4l2_m2m_ioctl_reqbufs, + .vidioc_querycap = iris_querycap, + .vidioc_queryctrl = iris_queryctrl, + .vidioc_querymenu = iris_querymenu, .vidioc_g_selection = iris_g_selection, .vidioc_subscribe_event = iris_subscribe_event, .vidioc_unsubscribe_event = iris_unsubscribe_event,