| Message ID | 1471871619-25873-3-git-send-email-stanimir.varbanov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Stanimir, Thanks for this patch series! I have some review comments: On 08/22/2016 03:13 PM, Stanimir Varbanov wrote: <snip> > diff --git a/drivers/media/platform/qcom/vidc/core.h b/drivers/media/platform/qcom/vidc/core.h > new file mode 100644 > index 000000000000..5dc8e05f8c36 > --- /dev/null > +++ b/drivers/media/platform/qcom/vidc/core.h > @@ -0,0 +1,196 @@ > +/* > + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. > + * Copyright (C) 2016 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#ifndef __VIDC_CORE_H_ > +#define __VIDC_CORE_H_ > + > +#include <media/v4l2-device.h> > +#include <media/v4l2-ctrls.h> > +#include <media/videobuf2-core.h> > + > +#include "resources.h" > +#include "hfi.h" > + > +#define VIDC_DRV_NAME "vidc" > + > +struct vidc_list { > + struct list_head list; > + struct mutex lock; > +}; > + > +struct vidc_format { > + u32 pixfmt; > + int num_planes; > + u32 type; > +}; > + > +struct vidc_core { > + struct list_head list; > + void __iomem *base; > + int irq; > + struct clk *clks[VIDC_CLKS_NUM_MAX]; > + struct mutex lock; > + struct hfi_core hfi; > + struct video_device vdev_dec; > + struct video_device vdev_enc; I know that many drivers embed struct video_device, but this can cause subtle refcounting problems. I recommend changing this to a pointer and using video_device_alloc(). I have plans to reorganize the way video_devices are allocated and registered in the near future, and you might just as well prepare this driver for that by switching to a pointer. > + struct v4l2_device v4l2_dev; > + struct list_head instances; > + const struct vidc_resources *res; > + struct rproc *rproc; > + bool rproc_booted; > + struct device *dev; > +}; > + > +struct vdec_controls { > + u32 post_loop_deb_mode; > + u32 profile; > + u32 level; > +}; > + > +struct venc_controls { > + u16 gop_size; > + u32 idr_period; > + u32 num_p_frames; > + u32 num_b_frames; > + u32 bitrate_mode; > + u32 bitrate; > + u32 bitrate_peak; > + > + u32 h264_i_period; > + u32 h264_entropy_mode; > + u32 h264_i_qp; > + u32 h264_p_qp; > + u32 h264_b_qp; > + u32 h264_min_qp; > + u32 h264_max_qp; > + u32 h264_loop_filter_mode; > + u32 h264_loop_filter_alpha; > + u32 h264_loop_filter_beta; > + > + u32 vp8_min_qp; > + u32 vp8_max_qp; > + > + u32 multi_slice_mode; > + u32 multi_slice_max_bytes; > + u32 multi_slice_max_mb; > + > + u32 header_mode; > + > + u32 profile; > + u32 level; > +}; > + > +struct vidc_inst { > + struct list_head list; > + struct mutex lock; > + struct vidc_core *core; > + > + struct vidc_list scratchbufs; > + struct vidc_list persistbufs; > + struct vidc_list registeredbufs; > + > + struct list_head bufqueue; > + struct mutex bufqueue_lock; > + > + int streamoff; > + int streamon; > + struct vb2_queue bufq_out; > + struct vb2_queue bufq_cap; > + > + struct v4l2_ctrl_handler ctrl_handler; > + union { > + struct vdec_controls dec; > + struct venc_controls enc; > + } controls; > + struct v4l2_fh fh; > + > + struct hfi_inst *hfi_inst; > + > + /* session fields */ > + u32 session_type; > + u32 width; > + u32 height; > + u32 out_width; > + u32 out_height; > + u32 colorspace; > + u8 ycbcr_enc; > + u8 quantization; > + u8 xfer_func; > + u64 fps; > + struct v4l2_fract timeperframe; > + const struct vidc_format *fmt_out; > + const struct vidc_format *fmt_cap; > + unsigned int num_input_bufs; > + unsigned int num_output_bufs; > + bool in_reconfig; > + u32 reconfig_width; > + u32 reconfig_height; > + u64 sequence; > +}; > + > +#define ctrl_to_inst(ctrl) \ > + container_of(ctrl->handler, struct vidc_inst, ctrl_handler) > + > +struct vidc_ctrl { > + u32 id; > + enum v4l2_ctrl_type type; > + s32 min; > + s32 max; > + s32 def; > + u32 step; > + u64 menu_skip_mask; > + u32 flags; > + const char * const *qmenu; > +}; > + > +/* > + * Offset base for buffers on the destination queue - used to distinguish > + * between source and destination buffers when mmapping - they receive the same > + * offsets but for different queues > + */ > +#define DST_QUEUE_OFF_BASE (1 << 30) > + > +extern const struct v4l2_file_operations vidc_fops; > + > +static inline void INIT_VIDC_LIST(struct vidc_list *mlist) > +{ > + mutex_init(&mlist->lock); > + INIT_LIST_HEAD(&mlist->list); > +} > + > +static inline struct vidc_inst *to_inst(struct file *filp) > +{ > + return container_of(filp->private_data, struct vidc_inst, fh); > +} > + > +static inline struct hfi_inst *to_hfi_inst(struct file *filp) > +{ > + return to_inst(filp)->hfi_inst; > +} > + > +static inline struct vb2_queue * > +vidc_to_vb2q(struct file *file, enum v4l2_buf_type type) > +{ > + struct vidc_inst *inst = to_inst(file); > + > + if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > + return &inst->bufq_cap; > + else if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > + return &inst->bufq_out; > + > + return NULL; > +} > + > +#endif > diff --git a/drivers/media/platform/qcom/vidc/helpers.c b/drivers/media/platform/qcom/vidc/helpers.c > new file mode 100644 > index 000000000000..81079f2b5ed1 > --- /dev/null > +++ b/drivers/media/platform/qcom/vidc/helpers.c > @@ -0,0 +1,394 @@ > +/* > + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. > + * Copyright (C) 2016 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > +#include <linux/list.h> > +#include <linux/mutex.h> > +#include <linux/pm_runtime.h> > +#include <media/videobuf2-dma-sg.h> > + > +#include "helpers.h" > +#include "int_bufs.h" > +#include "load.h" > +#include "hfi_helper.h" > + > +static int session_set_buf(struct vb2_buffer *vb) > +{ > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > + struct vb2_queue *q = vb->vb2_queue; > + struct vidc_inst *inst = vb2_get_drv_priv(q); > + struct vidc_core *core = inst->core; > + struct device *dev = core->dev; > + struct hfi_core *hfi = &core->hfi; > + struct vidc_buffer *buf = to_vidc_buffer(vbuf); > + struct hfi_frame_data fdata; > + int ret; > + > + memset(&fdata, 0, sizeof(fdata)); > + > + fdata.alloc_len = vb2_plane_size(vb, 0); > + fdata.device_addr = buf->dma_addr; > + fdata.timestamp = vb->timestamp; > + fdata.flags = 0; > + fdata.clnt_data = buf->dma_addr; > + > + if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { > + fdata.buffer_type = HFI_BUFFER_INPUT; > + fdata.filled_len = vb2_get_plane_payload(vb, 0); > + fdata.offset = vb->planes[0].data_offset; > + > + if (vbuf->flags & V4L2_BUF_FLAG_LAST || !fdata.filled_len) > + fdata.flags |= HFI_BUFFERFLAG_EOS; > + > + ret = vidc_hfi_session_etb(hfi, inst->hfi_inst, &fdata); > + } else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { > + fdata.buffer_type = HFI_BUFFER_OUTPUT; > + fdata.filled_len = 0; > + fdata.offset = 0; > + > + ret = vidc_hfi_session_ftb(hfi, inst->hfi_inst, &fdata); > + } else { > + ret = -EINVAL; > + } > + > + if (ret) { > + dev_err(dev, "failed to set session buffer (%d)\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static int session_unregister_bufs(struct vidc_inst *inst) > +{ > + struct device *dev = inst->core->dev; > + struct hfi_core *hfi = &inst->core->hfi; > + struct hfi_buffer_desc *bd; > + struct vidc_buffer *buf, *tmp; > + int ret = 0; > + > + mutex_lock(&inst->registeredbufs.lock); > + list_for_each_entry_safe(buf, tmp, &inst->registeredbufs.list, > + hfi_list) { > + list_del(&buf->hfi_list); > + bd = &buf->bd; > + bd->response_required = 1; > + ret = vidc_hfi_session_unset_buffers(hfi, inst->hfi_inst, bd); > + if (ret) { > + dev_err(dev, "%s: session release buffers failed\n", > + __func__); > + break; > + } > + } > + mutex_unlock(&inst->registeredbufs.lock); > + > + return ret; > +} > + > +static int session_register_bufs(struct vidc_inst *inst) > +{ > + struct device *dev = inst->core->dev; > + struct hfi_core *hfi = &inst->core->hfi; > + struct hfi_buffer_desc *bd; > + struct vidc_buffer *buf, *tmp; > + int ret = 0; > + > + mutex_lock(&inst->registeredbufs.lock); > + list_for_each_entry_safe(buf, tmp, &inst->registeredbufs.list, > + hfi_list) { > + bd = &buf->bd; > + ret = vidc_hfi_session_set_buffers(hfi, inst->hfi_inst, bd); > + if (ret) { > + dev_err(dev, "%s: session: set buffer failed\n", > + __func__); > + break; > + } > + } > + mutex_unlock(&inst->registeredbufs.lock); > + > + return ret; > +} > + > +int vidc_buf_descs(struct vidc_inst *inst, u32 type, > + struct hfi_buffer_requirements *out) > +{ > + struct hfi_core *hfi = &inst->core->hfi; > + u32 ptype = HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS; > + union hfi_get_property hprop; > + int ret, i; > + > + if (out) > + memset(out, 0, sizeof(*out)); > + > + ret = vidc_hfi_session_get_property(hfi, inst->hfi_inst, ptype, &hprop); > + if (ret) > + return ret; > + > + ret = -EINVAL; > + > + for (i = 0; i < HFI_BUFFER_TYPE_MAX; i++) { > + if (hprop.bufreq[i].type != type) > + continue; > + > + if (out) > + memcpy(out, &hprop.bufreq[i], sizeof(*out)); > + ret = 0; > + break; > + } > + > + return ret; > +} > + > +int vidc_set_color_format(struct vidc_inst *inst, u32 type, u32 pixfmt) > +{ > + struct hfi_uncompressed_format_select fmt; > + struct hfi_core *hfi = &inst->core->hfi; > + u32 ptype = HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SELECT; > + int ret; > + > + fmt.buffer_type = type; > + > + switch (pixfmt) { > + case V4L2_PIX_FMT_NV12: > + fmt.format = HFI_COLOR_FORMAT_NV12; > + break; > + case V4L2_PIX_FMT_NV21: > + fmt.format = HFI_COLOR_FORMAT_NV21; > + break; > + default: > + return -ENOTSUPP; I'm not really sure how this error code is used, but normally -EINVAL is returned for invalid pixel formats. -ENOTSUPP is not used by V4L2. > + } > + > + ret = vidc_hfi_session_set_property(hfi, inst->hfi_inst, ptype, &fmt); > + if (ret) > + return ret; > + > + return 0; > +} > + > +struct vb2_v4l2_buffer * > +vidc_vb2_find_buf(struct vidc_inst *inst, dma_addr_t addr) > +{ > + struct vidc_buffer *buf; > + struct vb2_v4l2_buffer *vb = NULL; > + > + mutex_lock(&inst->bufqueue_lock); > + > + list_for_each_entry(buf, &inst->bufqueue, list) { > + if (buf->dma_addr == addr) { > + vb = &buf->vb; > + break; > + } > + } > + > + if (vb) > + list_del(&buf->list); > + > + mutex_unlock(&inst->bufqueue_lock); > + > + return vb; > +} > + > +int vidc_vb2_buf_init(struct vb2_buffer *vb) > +{ > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > + struct vb2_queue *q = vb->vb2_queue; > + struct vidc_inst *inst = vb2_get_drv_priv(q); > + struct vidc_buffer *buf = to_vidc_buffer(vbuf); > + struct hfi_buffer_desc *bd = &buf->bd; > + struct sg_table *sgt; > + > + memset(bd, 0, sizeof(*bd)); > + > + if (q->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > + return 0; > + > + sgt = vb2_dma_sg_plane_desc(vb, 0); > + if (!sgt) > + return -EINVAL; > + > + bd->buffer_size = vb2_plane_size(vb, 0); > + bd->buffer_type = HFI_BUFFER_OUTPUT; > + bd->num_buffers = 1; > + bd->device_addr = sg_dma_address(sgt->sgl); > + > + mutex_lock(&inst->registeredbufs.lock); > + list_add_tail(&buf->hfi_list, &inst->registeredbufs.list); > + mutex_unlock(&inst->registeredbufs.lock); > + > + return 0; > +} > + > +int vidc_vb2_buf_prepare(struct vb2_buffer *vb) > +{ > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > + struct vidc_buffer *buf = to_vidc_buffer(vbuf); > + struct sg_table *sgt; > + > + sgt = vb2_dma_sg_plane_desc(vb, 0); > + if (!sgt) > + return -EINVAL; > + > + buf->dma_addr = sg_dma_address(sgt->sgl); > + > + return 0; > +} > + > +void vidc_vb2_buf_queue(struct vb2_buffer *vb) > +{ > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > + struct vidc_inst *inst = vb2_get_drv_priv(vb->vb2_queue); > + struct vidc_core *core = inst->core; > + struct device *dev = core->dev; > + struct vidc_buffer *buf = to_vidc_buffer(vbuf); > + unsigned int state; > + int ret; > + > + mutex_lock(&inst->hfi_inst->lock); > + state = inst->hfi_inst->state; > + mutex_unlock(&inst->hfi_inst->lock); > + > + if (state == INST_INVALID || state >= INST_STOP) { > + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); > + dev_dbg(dev, "%s: type:%d, invalid instance state\n", __func__, > + vb->type); > + return; > + } > + > + mutex_lock(&inst->bufqueue_lock); > + list_add_tail(&buf->list, &inst->bufqueue); > + mutex_unlock(&inst->bufqueue_lock); > + > + if (!vb2_is_streaming(&inst->bufq_cap) || > + !vb2_is_streaming(&inst->bufq_out)) > + return; > + > + ret = session_set_buf(vb); > + if (ret) > + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); > +} > + > +void vidc_vb2_stop_streaming(struct vb2_queue *q) > +{ > + struct vidc_inst *inst = vb2_get_drv_priv(q); > + struct hfi_inst *hfi_inst = inst->hfi_inst; > + struct vidc_core *core = inst->core; > + struct device *dev = core->dev; > + struct hfi_core *hfi = &core->hfi; > + int ret, streamoff; > + > + mutex_lock(&inst->lock); > + streamoff = inst->streamoff; > + mutex_unlock(&inst->lock); > + > + if (streamoff) > + return; > + > + mutex_lock(&inst->lock); > + if (inst->streamon == 0) { > + mutex_unlock(&inst->lock); > + return; > + } > + mutex_unlock(&inst->lock); > + > + ret = vidc_hfi_session_stop(hfi, inst->hfi_inst); > + if (ret) { > + dev_err(dev, "session: stop failed (%d)\n", ret); > + goto abort; > + } > + > + ret = vidc_hfi_session_unload_res(hfi, inst->hfi_inst); > + if (ret) { > + dev_err(dev, "session: release resources failed (%d)\n", ret); > + goto abort; > + } > + > + ret = session_unregister_bufs(inst); > + if (ret) { > + dev_err(dev, "failed to release capture buffers: %d\n", ret); > + goto abort; > + } > + > + ret = internal_bufs_free(inst); > + > + if (hfi_inst->state == INST_INVALID || hfi->state == CORE_INVALID) { > + ret = -EINVAL; > + goto abort; > + } > + > +abort: > + if (ret) > + vidc_hfi_session_abort(hfi, inst->hfi_inst); > + > + vidc_scale_clocks(inst->core); > + > + ret = vidc_hfi_session_deinit(hfi, inst->hfi_inst); > + > + mutex_lock(&inst->lock); > + inst->streamoff = 1; > + mutex_unlock(&inst->lock); > + > + if (ret) > + dev_err(dev, "stop streaming failed type: %d, ret: %d\n", > + q->type, ret); > + > + ret = pm_runtime_put_sync(dev); > + if (ret < 0) > + dev_err(dev, "%s: pm_runtime_put_sync (%d)\n", __func__, ret); > +} > + > +int vidc_vb2_start_streaming(struct vidc_inst *inst) > +{ > + struct device *dev = inst->core->dev; > + struct hfi_core *hfi = &inst->core->hfi; > + struct vidc_buffer *buf, *n; > + int ret; > + > + ret = session_register_bufs(inst); > + if (ret) > + return ret; > + > + ret = internal_bufs_alloc(inst); > + if (ret) > + return ret; > + > + vidc_scale_clocks(inst->core); > + > + ret = vidc_hfi_session_load_res(hfi, inst->hfi_inst); > + if (ret) { > + dev_err(dev, "session: load resources (%d)\n", ret); > + return ret; > + } > + > + ret = vidc_hfi_session_start(hfi, inst->hfi_inst); > + if (ret) { > + dev_err(dev, "session: start failed (%d)\n", ret); > + return ret; > + } > + > + mutex_lock(&inst->bufqueue_lock); > + list_for_each_entry_safe(buf, n, &inst->bufqueue, list) { > + ret = session_set_buf(&buf->vb.vb2_buf); > + if (ret) > + break; > + } > + mutex_unlock(&inst->bufqueue_lock); > + > + if (!ret) { > + mutex_lock(&inst->lock); > + inst->streamon = 1; > + mutex_unlock(&inst->lock); > + } > + > + return ret; > +} Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Thanks for the express comments! <cut> >> + >> +struct vidc_core { >> + struct list_head list; >> + void __iomem *base; >> + int irq; >> + struct clk *clks[VIDC_CLKS_NUM_MAX]; >> + struct mutex lock; >> + struct hfi_core hfi; >> + struct video_device vdev_dec; >> + struct video_device vdev_enc; > > I know that many drivers embed struct video_device, but this can cause subtle > refcounting problems. I recommend changing this to a pointer and using video_device_alloc(). > > I have plans to reorganize the way video_devices are allocated and registered in > the near future, and you might just as well prepare this driver for that by switching > to a pointer. OK, thanks for the info, I will change to pointers. <cut> >> + >> +int vidc_set_color_format(struct vidc_inst *inst, u32 type, u32 pixfmt) >> +{ >> + struct hfi_uncompressed_format_select fmt; >> + struct hfi_core *hfi = &inst->core->hfi; >> + u32 ptype = HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SELECT; >> + int ret; >> + >> + fmt.buffer_type = type; >> + >> + switch (pixfmt) { >> + case V4L2_PIX_FMT_NV12: >> + fmt.format = HFI_COLOR_FORMAT_NV12; >> + break; >> + case V4L2_PIX_FMT_NV21: >> + fmt.format = HFI_COLOR_FORMAT_NV21; >> + break; >> + default: >> + return -ENOTSUPP; > > I'm not really sure how this error code is used, but normally -EINVAL is returned > for invalid pixel formats. -ENOTSUPP is not used by V4L2. > you are right, I need to change this to EINVAL.
On Mon 22 Aug 06:13 PDT 2016, Stanimir Varbanov wrote: Hi Stan, > This adds core part of the vidc driver common helper functions > used by encoder and decoder specific files. I believe "vidc" is short for "video core" and this is not the only "video core" from Qualcomm. This driver is the v4l2 <-> hfi interface and uses either two ram based fifos _or_ apr tal for communication with the implementation. In the case of apr, the other side is not the venus core but rather the "VIDC" apr service on the Hexagon DSP. In this case the hfi packets are encapsulated in apr packets. Although this is not used in 8916 it would be nice to be able to add this later... But I think we should call this driver "hfi" - or at least venus, as it's not compatible with e.g the "blackbird" found in 8064, which is also called "vidc". > > - core.c has implemented the platform dirver methods, file > operations and v4l2 registration. > > - helpers.c has implemented common helper functions for > buffer management, vb2_ops and functions for format propagation. > > - int_bufs.c implements functions for allocating and freeing > buffers for internal usage. The buffer parameters describing > internal buffers depends on current format, resolution and > codec. > > - load.c consists functions for calculation of current load > of the hardware. Depending on the count of instances and > resolutions it selects the best clock rate for the video > core. > > - mem.c has two functions for memory allocation, currently > those functions are used for internal buffers and to allocate > the shared memory for communication with firmware via HFI > (Host Firmware Interface) interface commands. Please drop this; see comments on mem_alloc() > > - resources.c exports a structure describing the details > specific to platform and SoC. > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- This doesn't compile, as it depends on later patches. Also there are plenty of functions that are related to later patches and would with be better to include there, to keep the size of this patch down. > drivers/media/platform/qcom/vidc/core.c | 548 +++++++++++++++++++++++++++ > drivers/media/platform/qcom/vidc/core.h | 196 ++++++++++ > drivers/media/platform/qcom/vidc/helpers.c | 394 +++++++++++++++++++ > drivers/media/platform/qcom/vidc/helpers.h | 43 +++ > drivers/media/platform/qcom/vidc/int_bufs.c | 325 ++++++++++++++++ > drivers/media/platform/qcom/vidc/int_bufs.h | 23 ++ > drivers/media/platform/qcom/vidc/load.c | 104 +++++ > drivers/media/platform/qcom/vidc/load.h | 22 ++ > drivers/media/platform/qcom/vidc/mem.c | 64 ++++ > drivers/media/platform/qcom/vidc/mem.h | 32 ++ > drivers/media/platform/qcom/vidc/resources.c | 46 +++ > drivers/media/platform/qcom/vidc/resources.h | 46 +++ > 12 files changed, 1843 insertions(+) > create mode 100644 drivers/media/platform/qcom/vidc/core.c > create mode 100644 drivers/media/platform/qcom/vidc/core.h > create mode 100644 drivers/media/platform/qcom/vidc/helpers.c > create mode 100644 drivers/media/platform/qcom/vidc/helpers.h > create mode 100644 drivers/media/platform/qcom/vidc/int_bufs.c > create mode 100644 drivers/media/platform/qcom/vidc/int_bufs.h > create mode 100644 drivers/media/platform/qcom/vidc/load.c > create mode 100644 drivers/media/platform/qcom/vidc/load.h > create mode 100644 drivers/media/platform/qcom/vidc/mem.c > create mode 100644 drivers/media/platform/qcom/vidc/mem.h > create mode 100644 drivers/media/platform/qcom/vidc/resources.c > create mode 100644 drivers/media/platform/qcom/vidc/resources.h > > diff --git a/drivers/media/platform/qcom/vidc/core.c b/drivers/media/platform/qcom/vidc/core.c > new file mode 100644 > index 000000000000..e005be178fc0 > --- /dev/null > +++ b/drivers/media/platform/qcom/vidc/core.c > @@ -0,0 +1,548 @@ > +/* > + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. > + * Copyright (C) 2016 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > +#include <linux/clk.h> > +#include <linux/init.h> > +#include <linux/ioctl.h> > +#include <linux/list.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/types.h> > +#include <linux/remoteproc.h> > +#include <linux/pm_runtime.h> > +#include <media/videobuf2-v4l2.h> > +#include <media/v4l2-ioctl.h> > + > +#include "core.h" > +#include "resources.h" > +#include "vdec.h" > +#include "venc.h" > + > +static void vidc_add_inst(struct vidc_core *core, struct vidc_inst *inst) > +{ > + mutex_lock(&core->lock); > + list_add_tail(&inst->list, &core->instances); There are two different "instances" lists in this implementation, one keeping track of vidc instances and one keeping track of hfi instances, at the same time the vidc instances has a reference to its associated hfi instance. It should be possible to drop one of those lists. > + mutex_unlock(&core->lock); > +} > + > +static void vidc_del_inst(struct vidc_core *core, struct vidc_inst *inst) > +{ > + struct vidc_inst *pos, *n; > + > + mutex_lock(&core->lock); > + list_for_each_entry_safe(pos, n, &core->instances, list) { > + if (pos == inst) > + list_del(&inst->list); > + } > + mutex_unlock(&core->lock); > +} > + > +static int vidc_rproc_boot(struct vidc_core *core) > +{ > + int ret; > + > + if (core->rproc_booted) > + return 0; rproc_boot()/rproc_shutdown() is reference counted, so there is no reason (other than this driver being buggy) to keep track of "rproc_boot". As such, you can drop vidc_rproc_boot() and vidc_rproc_shutdown() and just call the rproc functions directly. > + > + ret = rproc_boot(core->rproc); > + if (ret) > + return ret; > + > + core->rproc_booted = true; > + > + return 0; > +} > + > +static void vidc_rproc_shutdown(struct vidc_core *core) > +{ > + if (!core->rproc_booted) > + return; > + > + rproc_shutdown(core->rproc); > + core->rproc_booted = false; > +} > + > +struct vidc_sys_error { > + struct vidc_core *core; > + struct delayed_work work; > +}; This is cool, but during the 5 second delay we should be able to call remove on the driver and this will dereference a freed hfi instance. Move the worker to hfi_core and you can cancel it on remove. > + > +static void vidc_sys_error_handler(struct work_struct *work) > +{ > + struct vidc_sys_error *handler = > + container_of(work, struct vidc_sys_error, work.work); > + struct vidc_core *core = handler->core; > + struct hfi_core *hfi = &core->hfi; > + struct device *dev = core->dev; > + int ret; > + > + mutex_lock(&hfi->lock); > + if (hfi->state != CORE_INVALID) > + goto exit; > + > + mutex_unlock(&hfi->lock); > + > + ret = vidc_hfi_core_deinit(hfi); > + if (ret) > + dev_err(dev, "core: deinit failed (%d)\n", ret); > + > + mutex_lock(&hfi->lock); > + > + rproc_report_crash(core->rproc, RPROC_FATAL_ERROR); This operation is async, as such I believe this to be fragile. To get the expected result you should be able to simply call rproc_shutdown()/rproc_boot() to restart the core... However, if we at any point would like to be able to get memory dumps from this core (likely a requirement on the Qualcomm side) we need to call rproc_report_crash() and let it collect the resources and then power cycle the core. As the life cycle of the venus driver goes 1:1 with the rproc driver I think it would be more suitable to make the v4l driver a child of the rproc driver and have it probe/remove this driver as the rproc comes and goes. This would allow us to call rproc_report_crash() here, we will be removed and when the crash is handled (sometime in the future) we will be probed again. > + > + vidc_rproc_shutdown(core); > + > + ret = vidc_rproc_boot(core); > + if (ret) > + goto exit; > + > + hfi->state = CORE_INIT; > + > +exit: > + mutex_unlock(&hfi->lock); > + kfree(handler); > +} > + > +static int vidc_event_notify(struct hfi_core *hfi, u32 event) > +{ > + struct vidc_sys_error *handler; > + struct hfi_inst *inst; > + > + switch (event) { > + case EVT_SYS_WATCHDOG_TIMEOUT: > + case EVT_SYS_ERROR: > + break; > + default: > + return -EINVAL; > + } > + > + mutex_lock(&hfi->lock); > + > + hfi->state = CORE_INVALID; > + > + list_for_each_entry(inst, &hfi->instances, list) { > + mutex_lock(&inst->lock); > + inst->state = INST_INVALID; > + mutex_unlock(&inst->lock); > + } > + > + mutex_unlock(&hfi->lock); > + > + handler = kzalloc(sizeof(*handler), GFP_KERNEL); > + if (!handler) > + return -ENOMEM; > + > + handler->core = container_of(hfi, struct vidc_core, hfi); > + INIT_DELAYED_WORK(&handler->work, vidc_sys_error_handler); > + > + /* > + * Sleep for 5 sec to ensure venus has completed any > + * pending cache operations. Without this sleep, we see > + * device reset when firmware is unloaded after a sys > + * error. > + */ > + schedule_delayed_work(&handler->work, msecs_to_jiffies(5000)); > + > + return 0; > +} > + > +static const struct hfi_core_ops vidc_core_ops = { > + .event_notify = vidc_event_notify, > +}; This is an overly generic way of calling vidc_sys_error_handler(). There is no need for having the hfi_core_ops indirections for a single op that will only exist in 1 and only 1 variant. Just replace the two affected event_notify() calls with a direct call to this function (and clean it up a bit). > + > +static int vidc_open(struct file *file) > +{ > + struct video_device *vdev = video_devdata(file); > + struct vidc_core *core = video_drvdata(file); > + struct vidc_inst *inst; > + int ret = 0; > + > + inst = kzalloc(sizeof(*inst), GFP_KERNEL); > + if (!inst) > + return -ENOMEM; > + > + mutex_init(&inst->lock); > + > + INIT_VIDC_LIST(&inst->scratchbufs); Please inline the mutex_init() and INIT_LIST_HEAD() here and drop the custom INIT_VIDC_LIST() wrapper macro. > + INIT_VIDC_LIST(&inst->persistbufs); > + INIT_VIDC_LIST(&inst->registeredbufs); > + > + INIT_LIST_HEAD(&inst->bufqueue); > + mutex_init(&inst->bufqueue_lock); > + > + if (vdev == &core->vdev_dec) > + inst->session_type = VIDC_SESSION_TYPE_DEC; > + else > + inst->session_type = VIDC_SESSION_TYPE_ENC; > + > + inst->core = core; > + > + if (inst->session_type == VIDC_SESSION_TYPE_DEC) > + ret = vdec_open(inst); > + else > + ret = venc_open(inst); > + > + if (ret) > + goto err_free_inst; > + > + if (inst->session_type == VIDC_SESSION_TYPE_DEC) > + v4l2_fh_init(&inst->fh, &core->vdev_dec); > + else > + v4l2_fh_init(&inst->fh, &core->vdev_enc); Here we have three sequential conditionals testing for the same thing, please join them into one. > + > + inst->fh.ctrl_handler = &inst->ctrl_handler; > + > + v4l2_fh_add(&inst->fh); > + > + file->private_data = &inst->fh; > + > + vidc_add_inst(core, inst); > + > + return 0; > + > +err_free_inst: > + kfree(inst); > + return ret; > +} > + > +static int vidc_close(struct file *file) > +{ > + struct vidc_inst *inst = to_inst(file); > + struct vidc_core *core = inst->core; > + > + if (inst->session_type == VIDC_SESSION_TYPE_DEC) > + vdec_close(inst); > + else > + venc_close(inst); > + > + vidc_del_inst(core, inst); > + > + mutex_destroy(&inst->bufqueue_lock); > + mutex_destroy(&inst->scratchbufs.lock); > + mutex_destroy(&inst->persistbufs.lock); > + mutex_destroy(&inst->registeredbufs.lock); Here's a good reason for dropping the INIT_VIDC_LIST() macro > + > + v4l2_fh_del(&inst->fh); > + v4l2_fh_exit(&inst->fh); > + > + kfree(inst); > + return 0; > +} > + > +static unsigned int vidc_poll(struct file *file, struct poll_table_struct *pt) > +{ > + struct vidc_inst *inst = to_inst(file); > + struct vb2_queue *outq = &inst->bufq_out; > + struct vb2_queue *capq = &inst->bufq_cap; > + unsigned int ret; > + > + ret = vb2_poll(outq, file, pt); > + ret |= vb2_poll(capq, file, pt); > + > + return ret; > +} > + > +static int vidc_mmap(struct file *file, struct vm_area_struct *vma) > +{ > + struct vidc_inst *inst = to_inst(file); > + unsigned long offset = vma->vm_pgoff << PAGE_SHIFT; > + int ret; > + > + if (offset < DST_QUEUE_OFF_BASE) { > + ret = vb2_mmap(&inst->bufq_out, vma); > + } else { > + vma->vm_pgoff -= DST_QUEUE_OFF_BASE >> PAGE_SHIFT; > + ret = vb2_mmap(&inst->bufq_cap, vma); > + } This feels hackish, is this really the way to do this? > + > + return ret; > +} > + > +const struct v4l2_file_operations vidc_fops = { > + .owner = THIS_MODULE, > + .open = vidc_open, > + .release = vidc_close, > + .unlocked_ioctl = video_ioctl2, > + .poll = vidc_poll, > + .mmap = vidc_mmap, > +#ifdef CONFIG_COMPAT > + .compat_ioctl32 = v4l2_compat_ioctl32, > +#endif > +}; > + > +static irqreturn_t vidc_isr_thread(int irq, void *dev_id) > +{ > + return vidc_hfi_isr_thread(irq, dev_id); > +} > + > +static irqreturn_t vidc_isr(int irq, void *dev) > +{ > + return vidc_hfi_isr(irq, dev); > +} These two functions indicates that we're requesting the irq in the wrong layer. Also, these two functions arrives in a later patchset, so I assume this doesn't compile... > + > +static int vidc_clks_get(struct vidc_core *core, unsigned int clks_num, > + const char * const *clks_id) > +{ > + struct device *dev = core->dev; > + unsigned int i; > + > + for (i = 0; i < clks_num; i++) { > + core->clks[i] = devm_clk_get(dev, clks_id[i]); > + if (IS_ERR(core->clks[i])) > + return PTR_ERR(core->clks[i]); > + } > + > + return 0; > +} > + > +static int > +vidc_clks_enable(struct vidc_core *core, const struct vidc_resources *res) > +{ > + unsigned int i; > + int ret; > + > + for (i = 0; i < res->clks_num; i++) { > + ret = clk_prepare_enable(core->clks[i]); > + if (ret) > + goto err; > + } > + > + return 0; > +err: > + while (--i) > + clk_disable_unprepare(core->clks[i]); > + > + return ret; > +} > + > +static void > +vidc_clks_disable(struct vidc_core *core, const struct vidc_resources *res) > +{ > + unsigned int i; > + > + for (i = 0; i < res->clks_num; i++) > + clk_disable_unprepare(core->clks[i]); > +} > + > +static const struct of_device_id vidc_dt_match[] = { > + { .compatible = "qcom,vidc-msm8916", .data = &msm8916_res, }, > + { } > +}; > + > +MODULE_DEVICE_TABLE(of, vidc_dt_match); As you're using of_device_get_match_data() you can move this table to the bottom of the file. > + > +static int vidc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct vidc_core *core; > + struct device_node *rproc; > + struct resource *r; > + int ret; > + > + core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL); > + if (!core) > + return -ENOMEM; > + > + core->dev = dev; > + platform_set_drvdata(pdev, core); > + > + rproc = of_parse_phandle(dev->of_node, "rproc", 0); > + if (IS_ERR(rproc)) > + return PTR_ERR(rproc); > + > + core->rproc = rproc_get_by_phandle(rproc->phandle); FYI, We're hoping to land some patches shortly that will replace this with rproc_get(pdev->dev.of_node), looking up an rproc by the standard "rprocs" property... > + if (IS_ERR(core->rproc)) > + return PTR_ERR(core->rproc); > + else if (!core->rproc) > + return -EPROBE_DEFER; We're cleaning up this in the core as well. You need to rproc_put() the rproc pointer after this point. My question still stands though, if this driver should be probed as the remoteproc is booted (or the apr service appearing). I will continue to look at that. > + > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + core->base = devm_ioremap_resource(dev, r); > + if (IS_ERR(core->base)) > + return PTR_ERR(core->base); > + > + core->irq = platform_get_irq(pdev, 0); > + if (core->irq < 0) > + return core->irq; > + > + core->res = of_device_get_match_data(dev); > + if (!core->res) > + return -ENODEV; > + > + ret = vidc_clks_get(core, core->res->clks_num, core->res->clks); > + if (ret) > + return ret; > + > + ret = dma_set_mask_and_coherent(dev, core->res->dma_mask); > + if (ret) > + return ret; > + > + INIT_LIST_HEAD(&core->instances); > + mutex_init(&core->lock); > + > + ret = devm_request_threaded_irq(dev, core->irq, vidc_isr, > + vidc_isr_thread, > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, Drop this IRQF_TRIGGER_HIGH and have this be specified in devicetree. > + "vidc", &core->hfi); > + if (ret) > + return ret; > + > + core->hfi.core_ops = &vidc_core_ops; > + core->hfi.dev = dev; > + > + ret = vidc_hfi_create(&core->hfi, core->res, core->base); > + if (ret) > + return ret; > + > + ret = vidc_clks_enable(core, core->res); > + if (ret) > + goto err_hfi_destroy; > + > + ret = vidc_rproc_boot(core); > + if (ret) { > + vidc_clks_disable(core, core->res); > + goto err_hfi_destroy; > + } > + > + pm_runtime_enable(dev); > + > + ret = pm_runtime_get_sync(dev); > + if (ret < 0) > + goto err_runtime_disable; > + > + ret = vidc_hfi_core_init(&core->hfi); > + if (ret) > + goto err_rproc_shutdown; > + > + ret = pm_runtime_put_sync(dev); > + if (ret) > + goto err_core_deinit; > + > + vidc_clks_disable(core, core->res); These operations follow the general pattern of booting other Qualcomm remoteprocs; acquire and enable some resources, boot the core and disable the resources. Therefor it looks quite likely that these operations are related to the life cycle of the venus core, rather than hfi. > + > + ret = v4l2_device_register(dev, &core->v4l2_dev); > + if (ret) > + goto err_core_deinit; > + > + ret = vdec_init(core, &core->vdev_dec); > + if (ret) > + goto err_dev_unregister; > + > + ret = venc_init(core, &core->vdev_enc); > + if (ret) > + goto err_vdec_deinit; > + > + return 0; > + > +err_vdec_deinit: > + vdec_deinit(core, &core->vdev_dec); > +err_dev_unregister: > + v4l2_device_unregister(&core->v4l2_dev); > +err_core_deinit: > + vidc_hfi_core_deinit(&core->hfi); > +err_rproc_shutdown: > + vidc_rproc_shutdown(core); > +err_runtime_disable: > + pm_runtime_set_suspended(dev); > + pm_runtime_disable(dev); > +err_hfi_destroy: > + vidc_hfi_destroy(&core->hfi); > + return ret; > +} > + > +static int vidc_remove(struct platform_device *pdev) > +{ > + struct vidc_core *core = platform_get_drvdata(pdev); > + int ret; > + > + ret = pm_runtime_get_sync(&pdev->dev); > + if (ret < 0) > + return ret; No-one cares about you returning an error here, so you better move forward and release as much of your resources as possible even though you didn't get your pm. > + > + ret = vidc_hfi_core_deinit(&core->hfi); > + if (ret) { > + pm_runtime_put_sync(&pdev->dev); > + return ret; > + } > + > + vidc_rproc_shutdown(core); > + > + ret = pm_runtime_put_sync(&pdev->dev); > + > + vidc_hfi_destroy(&core->hfi); > + vdec_deinit(core, &core->vdev_dec); > + venc_deinit(core, &core->vdev_enc); > + v4l2_device_unregister(&core->v4l2_dev); > + > + pm_runtime_disable(core->dev); > + > + return ret < 0 ? ret : 0; > +} > + > +static int vidc_runtime_suspend(struct device *dev) > +{ > + struct vidc_core *core = dev_get_drvdata(dev); > + int ret; > + > + ret = vidc_hfi_core_suspend(&core->hfi); > + > + vidc_clks_disable(core, core->res); > + > + return ret; > +} > + > +static int vidc_runtime_resume(struct device *dev) > +{ > + struct vidc_core *core = dev_get_drvdata(dev); > + int ret; > + > + ret = vidc_clks_enable(core, core->res); > + if (ret) > + return ret; > + > + return vidc_hfi_core_resume(&core->hfi); > +} > + > +static int vidc_pm_suspend(struct device *dev) > +{ > + return vidc_runtime_suspend(dev); > +} > + > +static int vidc_pm_resume(struct device *dev) > +{ > + return vidc_runtime_resume(dev); > +} > + > +static const struct dev_pm_ops vidc_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(vidc_pm_suspend, vidc_pm_resume) > + SET_RUNTIME_PM_OPS(vidc_runtime_suspend, vidc_runtime_resume, NULL) > +}; > + > +static struct platform_driver qcom_vidc_driver = { > + .probe = vidc_probe, > + .remove = vidc_remove, > + .driver = { > + .name = "qcom-vidc", > + .of_match_table = vidc_dt_match, > + .pm = &vidc_pm_ops, > + }, > +}; > + > +module_platform_driver(qcom_vidc_driver); > + > +MODULE_ALIAS("platform:qcom-vidc"); > +MODULE_DESCRIPTION("Qualcomm video encoder and decoder driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/media/platform/qcom/vidc/core.h b/drivers/media/platform/qcom/vidc/core.h > new file mode 100644 > index 000000000000..5dc8e05f8c36 > --- /dev/null > +++ b/drivers/media/platform/qcom/vidc/core.h > @@ -0,0 +1,196 @@ > +/* > + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. > + * Copyright (C) 2016 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#ifndef __VIDC_CORE_H_ > +#define __VIDC_CORE_H_ > + > +#include <media/v4l2-device.h> > +#include <media/v4l2-ctrls.h> > +#include <media/videobuf2-core.h> > + > +#include "resources.h" > +#include "hfi.h" > + > +#define VIDC_DRV_NAME "vidc" Unused > + > +struct vidc_list { > + struct list_head list; > + struct mutex lock; > +}; Can't we get away without passing around lockable lists? Does these lists have to be locked independently and should we really pass around their lock with them? > + > +struct vidc_format { > + u32 pixfmt; > + int num_planes; > + u32 type; > +}; > + > +struct vidc_core { > + struct list_head list; This list_head seems unused. > + void __iomem *base; base is acquired and passed by value to vidc_hfi_create(), so no need to keep track of it here. > + int irq; This irq belongs to hfi, so it should probably be kept there. > + struct clk *clks[VIDC_CLKS_NUM_MAX]; > + struct mutex lock; This "lock" seems to be only related the instances list, please name it more appropriately - and place it next to the instances member. > + struct hfi_core hfi; > + struct video_device vdev_dec; > + struct video_device vdev_enc; > + struct v4l2_device v4l2_dev; > + struct list_head instances; > + const struct vidc_resources *res; > + struct rproc *rproc; > + bool rproc_booted; > + struct device *dev; > +}; > + > +struct vdec_controls { > + u32 post_loop_deb_mode; > + u32 profile; > + u32 level; > +}; > + > +struct venc_controls { > + u16 gop_size; > + u32 idr_period; > + u32 num_p_frames; > + u32 num_b_frames; > + u32 bitrate_mode; > + u32 bitrate; > + u32 bitrate_peak; > + > + u32 h264_i_period; > + u32 h264_entropy_mode; > + u32 h264_i_qp; > + u32 h264_p_qp; > + u32 h264_b_qp; > + u32 h264_min_qp; > + u32 h264_max_qp; > + u32 h264_loop_filter_mode; > + u32 h264_loop_filter_alpha; > + u32 h264_loop_filter_beta; > + > + u32 vp8_min_qp; > + u32 vp8_max_qp; > + > + u32 multi_slice_mode; > + u32 multi_slice_max_bytes; > + u32 multi_slice_max_mb; > + > + u32 header_mode; > + > + u32 profile; > + u32 level; > +}; > + > +struct vidc_inst { > + struct list_head list; > + struct mutex lock; > + struct vidc_core *core; > + > + struct vidc_list scratchbufs; > + struct vidc_list persistbufs; > + struct vidc_list registeredbufs; Just inline the list_head and mutex here, as it's done for bufqueue. > + > + struct list_head bufqueue; > + struct mutex bufqueue_lock; > + > + int streamoff; > + int streamon; > + struct vb2_queue bufq_out; > + struct vb2_queue bufq_cap; > + > + struct v4l2_ctrl_handler ctrl_handler; > + union { > + struct vdec_controls dec; > + struct venc_controls enc; > + } controls; > + struct v4l2_fh fh; > + > + struct hfi_inst *hfi_inst; > + > + /* session fields */ > + u32 session_type; > + u32 width; > + u32 height; > + u32 out_width; > + u32 out_height; > + u32 colorspace; > + u8 ycbcr_enc; > + u8 quantization; > + u8 xfer_func; > + u64 fps; > + struct v4l2_fract timeperframe; > + const struct vidc_format *fmt_out; > + const struct vidc_format *fmt_cap; > + unsigned int num_input_bufs; > + unsigned int num_output_bufs; > + bool in_reconfig; > + u32 reconfig_width; > + u32 reconfig_height; > + u64 sequence; > +}; > + > +#define ctrl_to_inst(ctrl) \ > + container_of(ctrl->handler, struct vidc_inst, ctrl_handler) > + > +struct vidc_ctrl { > + u32 id; > + enum v4l2_ctrl_type type; > + s32 min; > + s32 max; > + s32 def; > + u32 step; > + u64 menu_skip_mask; > + u32 flags; > + const char * const *qmenu; > +}; > + > +/* > + * Offset base for buffers on the destination queue - used to distinguish > + * between source and destination buffers when mmapping - they receive the same > + * offsets but for different queues > + */ > +#define DST_QUEUE_OFF_BASE (1 << 30) > + > +extern const struct v4l2_file_operations vidc_fops; Just pass this to v{dec,enc}_init() rather than back-referencing it through a global variable. But on the other hand this is unused in this patchset... > + > +static inline void INIT_VIDC_LIST(struct vidc_list *mlist) > +{ > + mutex_init(&mlist->lock); > + INIT_LIST_HEAD(&mlist->list); > +} > + > +static inline struct vidc_inst *to_inst(struct file *filp) > +{ > + return container_of(filp->private_data, struct vidc_inst, fh); > +} > + > +static inline struct hfi_inst *to_hfi_inst(struct file *filp) Unused > +{ > + return to_inst(filp)->hfi_inst; > +} > + > +static inline struct vb2_queue * > +vidc_to_vb2q(struct file *file, enum v4l2_buf_type type) Unused > +{ > + struct vidc_inst *inst = to_inst(file); > + > + if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > + return &inst->bufq_cap; > + else if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > + return &inst->bufq_out; > + > + return NULL; > +} > + > +#endif > diff --git a/drivers/media/platform/qcom/vidc/helpers.c b/drivers/media/platform/qcom/vidc/helpers.c > new file mode 100644 > index 000000000000..81079f2b5ed1 > --- /dev/null > +++ b/drivers/media/platform/qcom/vidc/helpers.c > @@ -0,0 +1,394 @@ > +/* > + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. > + * Copyright (C) 2016 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > +#include <linux/list.h> > +#include <linux/mutex.h> > +#include <linux/pm_runtime.h> > +#include <media/videobuf2-dma-sg.h> > + > +#include "helpers.h" > +#include "int_bufs.h" > +#include "load.h" > +#include "hfi_helper.h" > + > +static int session_set_buf(struct vb2_buffer *vb) > +{ > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > + struct vb2_queue *q = vb->vb2_queue; > + struct vidc_inst *inst = vb2_get_drv_priv(q); > + struct vidc_core *core = inst->core; > + struct device *dev = core->dev; > + struct hfi_core *hfi = &core->hfi; > + struct vidc_buffer *buf = to_vidc_buffer(vbuf); > + struct hfi_frame_data fdata; > + int ret; > + > + memset(&fdata, 0, sizeof(fdata)); > + > + fdata.alloc_len = vb2_plane_size(vb, 0); > + fdata.device_addr = buf->dma_addr; > + fdata.timestamp = vb->timestamp; > + fdata.flags = 0; > + fdata.clnt_data = buf->dma_addr; > + > + if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { > + fdata.buffer_type = HFI_BUFFER_INPUT; > + fdata.filled_len = vb2_get_plane_payload(vb, 0); > + fdata.offset = vb->planes[0].data_offset; > + > + if (vbuf->flags & V4L2_BUF_FLAG_LAST || !fdata.filled_len) > + fdata.flags |= HFI_BUFFERFLAG_EOS; > + > + ret = vidc_hfi_session_etb(hfi, inst->hfi_inst, &fdata); > + } else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { > + fdata.buffer_type = HFI_BUFFER_OUTPUT; > + fdata.filled_len = 0; > + fdata.offset = 0; > + > + ret = vidc_hfi_session_ftb(hfi, inst->hfi_inst, &fdata); > + } else { > + ret = -EINVAL; > + } > + > + if (ret) { > + dev_err(dev, "failed to set session buffer (%d)\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static int session_unregister_bufs(struct vidc_inst *inst) > +{ > + struct device *dev = inst->core->dev; > + struct hfi_core *hfi = &inst->core->hfi; > + struct hfi_buffer_desc *bd; > + struct vidc_buffer *buf, *tmp; > + int ret = 0; > + > + mutex_lock(&inst->registeredbufs.lock); > + list_for_each_entry_safe(buf, tmp, &inst->registeredbufs.list, > + hfi_list) { > + list_del(&buf->hfi_list); So the hfi_list is the list_head for entries in the _vidc_ instance list? > + bd = &buf->bd; > + bd->response_required = 1; > + ret = vidc_hfi_session_unset_buffers(hfi, inst->hfi_inst, bd); > + if (ret) { > + dev_err(dev, "%s: session release buffers failed\n", > + __func__); > + break; > + } > + } > + mutex_unlock(&inst->registeredbufs.lock); > + > + return ret; > +} > + > +static int session_register_bufs(struct vidc_inst *inst) > +{ > + struct device *dev = inst->core->dev; > + struct hfi_core *hfi = &inst->core->hfi; > + struct hfi_buffer_desc *bd; > + struct vidc_buffer *buf, *tmp; > + int ret = 0; > + > + mutex_lock(&inst->registeredbufs.lock); > + list_for_each_entry_safe(buf, tmp, &inst->registeredbufs.list, > + hfi_list) { > + bd = &buf->bd; > + ret = vidc_hfi_session_set_buffers(hfi, inst->hfi_inst, bd); > + if (ret) { > + dev_err(dev, "%s: session: set buffer failed\n", > + __func__); > + break; > + } > + } > + mutex_unlock(&inst->registeredbufs.lock); > + > + return ret; > +} > + > +int vidc_buf_descs(struct vidc_inst *inst, u32 type, > + struct hfi_buffer_requirements *out) If you call this vidc_get_buf_requirements() it would actually describe what's going on. But why is this hfi wrapper in the core, rather than just have the internal buffer manager call it directly. The call doesn't seem to depend on the parameters or state, can we cache the result? > +{ > + struct hfi_core *hfi = &inst->core->hfi; > + u32 ptype = HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS; > + union hfi_get_property hprop; > + int ret, i; > + > + if (out) > + memset(out, 0, sizeof(*out)); > + > + ret = vidc_hfi_session_get_property(hfi, inst->hfi_inst, ptype, &hprop); > + if (ret) > + return ret; > + > + ret = -EINVAL; > + > + for (i = 0; i < HFI_BUFFER_TYPE_MAX; i++) { > + if (hprop.bufreq[i].type != type) > + continue; > + > + if (out) > + memcpy(out, &hprop.bufreq[i], sizeof(*out)); > + ret = 0; > + break; > + } > + > + return ret; > +} > + [..] > + > +void vidc_vb2_stop_streaming(struct vb2_queue *q) > +{ > + struct vidc_inst *inst = vb2_get_drv_priv(q); > + struct hfi_inst *hfi_inst = inst->hfi_inst; > + struct vidc_core *core = inst->core; > + struct device *dev = core->dev; > + struct hfi_core *hfi = &core->hfi; > + int ret, streamoff; > + > + mutex_lock(&inst->lock); > + streamoff = inst->streamoff; > + mutex_unlock(&inst->lock); > + > + if (streamoff) > + return; > + > + mutex_lock(&inst->lock); > + if (inst->streamon == 0) { > + mutex_unlock(&inst->lock); > + return; > + } > + mutex_unlock(&inst->lock); Why do we keep track of streamon and stream off, why isn't streamoff ever cleared? Why don't we check both conditions in one critical region? > + > + ret = vidc_hfi_session_stop(hfi, inst->hfi_inst); > + if (ret) { > + dev_err(dev, "session: stop failed (%d)\n", ret); > + goto abort; When are we going to relaim the buffers in these cases? > + } > + > + ret = vidc_hfi_session_unload_res(hfi, inst->hfi_inst); > + if (ret) { > + dev_err(dev, "session: release resources failed (%d)\n", ret); > + goto abort; > + } > + > + ret = session_unregister_bufs(inst); > + if (ret) { > + dev_err(dev, "failed to release capture buffers: %d\n", ret); > + goto abort; > + } > + > + ret = internal_bufs_free(inst); > + > + if (hfi_inst->state == INST_INVALID || hfi->state == CORE_INVALID) { > + ret = -EINVAL; > + goto abort; > + } > + > +abort: > + if (ret) > + vidc_hfi_session_abort(hfi, inst->hfi_inst); > + > + vidc_scale_clocks(inst->core); > + > + ret = vidc_hfi_session_deinit(hfi, inst->hfi_inst); > + > + mutex_lock(&inst->lock); > + inst->streamoff = 1; > + mutex_unlock(&inst->lock); > + > + if (ret) > + dev_err(dev, "stop streaming failed type: %d, ret: %d\n", > + q->type, ret); > + > + ret = pm_runtime_put_sync(dev); > + if (ret < 0) > + dev_err(dev, "%s: pm_runtime_put_sync (%d)\n", __func__, ret); > +} > + > +int vidc_vb2_start_streaming(struct vidc_inst *inst) > +{ > + struct device *dev = inst->core->dev; > + struct hfi_core *hfi = &inst->core->hfi; > + struct vidc_buffer *buf, *n; > + int ret; > + > + ret = session_register_bufs(inst); > + if (ret) > + return ret; > + > + ret = internal_bufs_alloc(inst); > + if (ret) > + return ret; > + > + vidc_scale_clocks(inst->core); > + > + ret = vidc_hfi_session_load_res(hfi, inst->hfi_inst); > + if (ret) { > + dev_err(dev, "session: load resources (%d)\n", ret); > + return ret; > + } > + > + ret = vidc_hfi_session_start(hfi, inst->hfi_inst); > + if (ret) { > + dev_err(dev, "session: start failed (%d)\n", ret); > + return ret; > + } > + > + mutex_lock(&inst->bufqueue_lock); > + list_for_each_entry_safe(buf, n, &inst->bufqueue, list) { > + ret = session_set_buf(&buf->vb.vb2_buf); > + if (ret) > + break; > + } > + mutex_unlock(&inst->bufqueue_lock); > + > + if (!ret) { > + mutex_lock(&inst->lock); > + inst->streamon = 1; > + mutex_unlock(&inst->lock); > + } > + > + return ret; > +} > diff --git a/drivers/media/platform/qcom/vidc/helpers.h b/drivers/media/platform/qcom/vidc/helpers.h > new file mode 100644 > index 000000000000..a151c96bf939 > --- /dev/null > +++ b/drivers/media/platform/qcom/vidc/helpers.h > @@ -0,0 +1,43 @@ > +/* > + * Copyright (c) 2012-2014, The Linux Foundation. All rights reserved. > + * Copyright (C) 2016 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > +#ifndef __VIDC_COMMON_H__ > +#define __VIDC_COMMON_H__ s/COMMON/HELPERS/ > + > +#include <linux/list.h> > +#include <media/videobuf2-v4l2.h> > + > +#include "core.h" > + > +struct vidc_buffer { > + struct vb2_v4l2_buffer vb; > + struct list_head list; > + dma_addr_t dma_addr; > + struct list_head hfi_list; This seems to be the list_head used for associating buffers to the _vidc_ instances. > + struct hfi_buffer_desc bd; > +}; > + > +#define to_vidc_buffer(buf) container_of(buf, struct vidc_buffer, vb) > + > +struct vb2_v4l2_buffer * > +vidc_vb2_find_buf(struct vidc_inst *inst, dma_addr_t addr); > +int vidc_vb2_buf_init(struct vb2_buffer *vb); > +int vidc_vb2_buf_prepare(struct vb2_buffer *vb); > +void vidc_vb2_buf_queue(struct vb2_buffer *vb); > +void vidc_vb2_stop_streaming(struct vb2_queue *q); > +int vidc_vb2_start_streaming(struct vidc_inst *inst); > +int vidc_buf_descs(struct vidc_inst *inst, u32 type, > + struct hfi_buffer_requirements *out); > +int vidc_set_color_format(struct vidc_inst *inst, u32 type, u32 fmt); > +#endif > diff --git a/drivers/media/platform/qcom/vidc/int_bufs.c b/drivers/media/platform/qcom/vidc/int_bufs.c [..] > + > +static int internal_alloc_and_set(struct vidc_inst *inst, > + struct hfi_buffer_requirements *bufreq, > + struct vidc_list *buf_list) > +{ > + struct vidc_internal_buf *buf; > + struct vidc_mem *mem; > + unsigned int i; > + int ret = 0; > + > + if (!bufreq->size) > + return 0; > + > + for (i = 0; i < bufreq->count_actual; i++) { > + mem = mem_alloc(inst->core->dev, bufreq->size, 0); Inline mem_alloc here; might need to make sure bufreq->size is 4K aligned. > + if (IS_ERR(mem)) { > + ret = PTR_ERR(mem); > + goto err_no_mem; > + } > + > + buf = kzalloc(sizeof(*buf), GFP_KERNEL); > + if (!buf) { > + ret = -ENOMEM; > + goto fail_kzalloc; > + } > + > + buf->mem = mem; > + buf->type = bufreq->type; > + > + ret = internal_set_buf_on_fw(inst, bufreq->type, mem, false); > + if (ret) > + goto fail_set_buffers; > + > + mutex_lock(&buf_list->lock); > + list_add_tail(&buf->list, &buf_list->list); > + mutex_unlock(&buf_list->lock); > + } > + > + return ret; > + > +fail_set_buffers: > + kfree(buf); > +fail_kzalloc: > + mem_free(mem); > +err_no_mem: > + return ret; > +} > + [..] > + > +static int persist_set_buffer(struct vidc_inst *inst, u32 type) > +{ > + struct hfi_buffer_requirements bufreq; > + int ret; > + > + ret = vidc_buf_descs(inst, type, &bufreq); > + if (ret) > + return 0; > + > + mutex_lock(&inst->persistbufs.lock); > + if (!list_empty(&inst->persistbufs.list)) { This function is called twice, with type HFI_BUFFER_INTERNAL_PERSIST and HFI_BUFFER_INTERNAL_PERSIST_1 respectively. Unless the buffer requirements are missing for HFI_BUFFER_INTERNAL_PERSIST persistbufs won't be empty and we will skip the later allocation. > + mutex_unlock(&inst->persistbufs.lock); > + return 0; > + } > + mutex_unlock(&inst->persistbufs.lock); > + > + return internal_alloc_and_set(inst, &bufreq, &inst->persistbufs); > +} > + [..] > + > +static int scratch_set_buffers(struct vidc_inst *inst) > +{ > + struct device *dev = inst->core->dev; > + int ret; > + > + ret = scratch_unset_buffers(inst, true); > + if (ret) > + dev_warn(dev, "Failed to release scratch buffers\n"); internal_bufs_free() calls scratch_unset_buffers(reuse=false) so we're coming here with an empty scratchbufs either way - meaning that this whole file can be greatly simplified. So instead of trying to fix that I would suggest that you just let internal_bufs_alloc() acquire the buffer requirements and call internal_alloc_and_set() directly, storing the result in a single list. And then inline a free method in internal_bufs_free() as well as drop all reuse-stuff and unused/dead code. That would simplify this file quite a bit and if there actually is a need for the reusing of buffer that can be added at some later time. > + > + ret = scratch_set_buffer(inst, HFI_BUFFER_INTERNAL_SCRATCH); > + if (ret) > + goto error; > + > + ret = scratch_set_buffer(inst, HFI_BUFFER_INTERNAL_SCRATCH_1); > + if (ret) > + goto error; > + > + ret = scratch_set_buffer(inst, HFI_BUFFER_INTERNAL_SCRATCH_2); > + if (ret) > + goto error; > + > + return 0; > +error: > + scratch_unset_buffers(inst, false); > + return ret; > +} > + > +static int persist_set_buffers(struct vidc_inst *inst) > +{ > + int ret; > + > + ret = persist_set_buffer(inst, HFI_BUFFER_INTERNAL_PERSIST); > + if (ret) > + goto error; > + > + ret = persist_set_buffer(inst, HFI_BUFFER_INTERNAL_PERSIST_1); > + if (ret) > + goto error; > + > + return 0; > + > +error: > + persist_unset_buffers(inst); > + return ret; > +} > + > +int internal_bufs_alloc(struct vidc_inst *inst) > +{ > + struct device *dev = inst->core->dev; > + int ret; > + > + ret = scratch_set_buffers(inst); > + if (ret) { > + dev_err(dev, "set scratch buffers (%d)\n", ret); > + return ret; > + } > + > + ret = persist_set_buffers(inst); > + if (ret) { > + dev_err(dev, "set persist buffers (%d)\n", ret); > + goto error; > + } > + > + return 0; > + > +error: > + scratch_unset_buffers(inst, false); > + return ret; > +} > + > +int internal_bufs_free(struct vidc_inst *inst) > +{ > + struct device *dev = inst->core->dev; > + int ret; > + > + ret = scratch_unset_buffers(inst, false); > + if (ret) > + dev_err(dev, "failed to release scratch buffers: %d\n", ret); > + > + ret = persist_unset_buffers(inst); > + if (ret) > + dev_err(dev, "failed to release persist buffers: %d\n", ret); > + > + return ret; > +} > diff --git a/drivers/media/platform/qcom/vidc/int_bufs.h b/drivers/media/platform/qcom/vidc/int_bufs.h > new file mode 100644 > index 000000000000..5f8b2b85839f > --- /dev/null > +++ b/drivers/media/platform/qcom/vidc/int_bufs.h > @@ -0,0 +1,23 @@ > +/* > + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. > + * Copyright (C) 2016 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > +#ifndef __VIDC_INTERNAL_BUFFERS_H__ > +#define __VIDC_INTERNAL_BUFFERS_H__ > + > +struct vidc_inst; > + > +int internal_bufs_alloc(struct vidc_inst *inst); > +int internal_bufs_free(struct vidc_inst *inst); > + > +#endif > diff --git a/drivers/media/platform/qcom/vidc/load.c b/drivers/media/platform/qcom/vidc/load.c > new file mode 100644 > index 000000000000..8ae25fc0e8a5 > --- /dev/null > +++ b/drivers/media/platform/qcom/vidc/load.c > @@ -0,0 +1,104 @@ > +/* > + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved. > + * Copyright (C) 2016 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#include <linux/clk.h> > + > +#include "core.h" > +#include "load.h" > + > +static u32 get_inst_load(struct vidc_inst *inst) > +{ > + int mbs; > + u32 w = inst->width; > + u32 h = inst->height; > + > + if (!inst->hfi_inst || !(inst->hfi_inst->state >= INST_INIT && > + inst->hfi_inst->state < INST_STOP)) > + return 0; > + > + mbs = (ALIGN(w, 16) / 16) * (ALIGN(h, 16) / 16); > + > + return mbs * inst->fps; > +} > + > +static u32 get_load(struct vidc_core *core, u32 session_type) > +{ > + struct vidc_inst *inst = NULL; > + u32 mbs_per_sec = 0; > + > + mutex_lock(&core->lock); > + list_for_each_entry(inst, &core->instances, list) { > + if (inst->session_type != session_type) > + continue; > + > + mbs_per_sec += get_inst_load(inst); > + } > + mutex_unlock(&core->lock); > + > + return mbs_per_sec; > +} > + > +static int scale_clocks_load(struct vidc_core *core, u32 mbs_per_sec) > +{ > + const struct freq_tbl *table = core->res->freq_tbl; > + int num_rows = core->res->freq_tbl_size; > + struct clk *clk = core->clks[0]; Using individual clk pointers instead of this array would make this "core_clk" easier to follow. > + struct device *dev = core->dev; > + unsigned long freq = table[0].freq; > + int ret, i; > + > + if (!mbs_per_sec && num_rows > 1) { > + freq = table[num_rows - 1].freq; > + goto set_freq; > + } Here we will set freq to the last entry in the freq table, potentially table[0] if num_rows == 1, so the second part of the conditional doesn't add any value and you can skip the early initialization above. And you can put the loop below in an else block instead of using a goto. > + > + for (i = 0; i < num_rows; i++) { > + if (mbs_per_sec > table[i].load) > + break; > + freq = table[i].freq; > + } > + > +set_freq: > + > + ret = clk_set_rate(clk, freq); > + if (ret) { > + dev_err(dev, "failed to set clock rate %lu (%d)\n", freq, ret); > + return ret; > + } > + > + return 0; ret will be 0 here, so print the error message conditionally and then just return ret. > +} > + > +int vidc_scale_clocks(struct vidc_core *core) This is only called from helpers.c, drop this file and move the implementation there. > +{ > + struct device *dev = core->dev; > + u32 mbs_per_sec; > + int ret; > + > + mbs_per_sec = get_load(core, VIDC_SESSION_TYPE_ENC) + > + get_load(core, VIDC_SESSION_TYPE_DEC); > + > + if (mbs_per_sec > core->res->max_load) { > + dev_warn(dev, "HW is overloaded, needed: %d max: %d\n", > + mbs_per_sec, core->res->max_load); > + return -EBUSY; > + } > + > + ret = scale_clocks_load(core, mbs_per_sec); > + if (ret) > + dev_warn(dev, "failed to scale clocks, performance might be impacted\n"); > + > + return 0; > +} [..] > diff --git a/drivers/media/platform/qcom/vidc/mem.c b/drivers/media/platform/qcom/vidc/mem.c > new file mode 100644 > index 000000000000..6a83b5784410 > --- /dev/null > +++ b/drivers/media/platform/qcom/vidc/mem.c > @@ -0,0 +1,64 @@ > +/* > + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. > + * Copyright (C) 2016 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#include <linux/device.h> > +#include <linux/dma-direction.h> > +#include <linux/dma-mapping.h> > +#include <linux/err.h> > +#include <linux/slab.h> > + > +#include "mem.h" > + > +struct vidc_mem *mem_alloc(struct device *dev, size_t size, int map_kernel) This is a terrible name for a global function. But I think you can favorably inline this into the two callers. They both have their own tracking objects. So just drop this entire file. > +{ > + struct vidc_mem *mem; > + > + if (!size) > + return ERR_PTR(-EINVAL); > + > + if (IS_ERR(dev)) > + return ERR_CAST(dev); > + > + mem = kzalloc(sizeof(*mem), GFP_KERNEL); > + if (!mem) > + return ERR_PTR(-ENOMEM); > + > + mem->size = ALIGN(size, SZ_4K); > + mem->iommu_dev = dev; > + > + mem->attrs = DMA_ATTR_WRITE_COMBINE; > + > + if (!map_kernel) > + mem->attrs |= DMA_ATTR_NO_KERNEL_MAPPING; > + > + mem->kvaddr = dma_alloc_attrs(mem->iommu_dev, mem->size, &mem->da, > + GFP_KERNEL, mem->attrs); > + if (!mem->kvaddr) { > + kfree(mem); > + return ERR_PTR(-ENOMEM); > + } > + > + return mem; > +} > + > +void mem_free(struct vidc_mem *mem) > +{ > + if (!mem) > + return; > + > + dma_free_attrs(mem->iommu_dev, mem->size, mem->kvaddr, > + mem->da, mem->attrs); > + kfree(mem); > +}; [..] > diff --git a/drivers/media/platform/qcom/vidc/resources.c b/drivers/media/platform/qcom/vidc/resources.c > new file mode 100644 > index 000000000000..e00ed1caa824 > --- /dev/null > +++ b/drivers/media/platform/qcom/vidc/resources.c > @@ -0,0 +1,46 @@ > +/* > + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. > + * Copyright (C) 2016 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > +#include <linux/bug.h> > +#include <linux/types.h> > +#include <linux/kernel.h> > + > +#include "hfi.h" > + > +static const struct freq_tbl msm8916_freq_table[] = { > + { 352800, 228570000 }, /* 1920x1088 @ 30 + 1280x720 @ 30 */ > + { 244800, 160000000 }, /* 1920x1088 @ 30 */ > + { 108000, 100000000 }, /* 1280x720 @ 30 */ > +}; > + > +static const struct reg_val msm8916_reg_preset[] = { > + { 0xe0020, 0x05555556 }, > + { 0xe0024, 0x05555556 }, > + { 0x80124, 0x00000003 }, > +}; > + > +const struct vidc_resources msm8916_res = { > + .freq_tbl = msm8916_freq_table, > + .freq_tbl_size = ARRAY_SIZE(msm8916_freq_table), > + .reg_tbl = msm8916_reg_preset, > + .reg_tbl_size = ARRAY_SIZE(msm8916_reg_preset), > + .clks = {"core", "iface", "bus", }, > + .clks_num = 3, > + .max_load = 352800, /* 720p@30 + 1080p@30 */ > + .hfi_version = 0, Unused > + .vmem_id = VIDC_RESOURCE_NONE, Unused > + .vmem_size = 0, Unused > + .vmem_addr = 0, Unused > + .dma_mask = 0xddc00000 - 1, > +}; These tables could with favor be moved next to the of_table in vidc.c Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, Thanks for the review and comments! On 08/23/2016 05:50 AM, Bjorn Andersson wrote: > On Mon 22 Aug 06:13 PDT 2016, Stanimir Varbanov wrote: > > Hi Stan, > >> This adds core part of the vidc driver common helper functions >> used by encoder and decoder specific files. > > I believe "vidc" is short for "video core" and this is not the only > "video core" from Qualcomm. This driver is the v4l2 <-> hfi interface and What other "video core"s do you know? > uses either two ram based fifos _or_ apr tal for communication with the > implementation. > > In the case of apr, the other side is not the venus core but rather the > "VIDC" apr service on the Hexagon DSP. In this case the hfi packets are > encapsulated in apr packets. Although this is not used in 8916 it would > be nice to be able to add this later... OK, you are talking about q6_hfi.c which file is found in msm-3.10 and maybe older kernel versions. There is a function vidc_hfi_create() which currently creates venus hfi interface but it aways could be extended to call q6 DSP specific function. > > > But I think we should call this driver "hfi" - or at least venus, as > it's not compatible with e.g the "blackbird" found in 8064, which is > also called "vidc". Do you think that vidc driver for 8064 will ever reach the mainline kernel? I personally don't like hfi nor venus other suggestions? Does "vidcore" or "vcore" makes sense? > >> >> - core.c has implemented the platform dirver methods, file >> operations and v4l2 registration. >> >> - helpers.c has implemented common helper functions for >> buffer management, vb2_ops and functions for format propagation. >> >> - int_bufs.c implements functions for allocating and freeing >> buffers for internal usage. The buffer parameters describing >> internal buffers depends on current format, resolution and >> codec. >> >> - load.c consists functions for calculation of current load >> of the hardware. Depending on the count of instances and >> resolutions it selects the best clock rate for the video >> core. >> >> - mem.c has two functions for memory allocation, currently >> those functions are used for internal buffers and to allocate >> the shared memory for communication with firmware via HFI >> (Host Firmware Interface) interface commands. > > Please drop this; see comments on mem_alloc() OK. > >> >> - resources.c exports a structure describing the details >> specific to platform and SoC. >> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- > > This doesn't compile, as it depends on later patches. Also there are > plenty of functions that are related to later patches and would with be > better to include there, to keep the size of this patch down. > >> drivers/media/platform/qcom/vidc/core.c | 548 +++++++++++++++++++++++++++ >> drivers/media/platform/qcom/vidc/core.h | 196 ++++++++++ >> drivers/media/platform/qcom/vidc/helpers.c | 394 +++++++++++++++++++ >> drivers/media/platform/qcom/vidc/helpers.h | 43 +++ >> drivers/media/platform/qcom/vidc/int_bufs.c | 325 ++++++++++++++++ >> drivers/media/platform/qcom/vidc/int_bufs.h | 23 ++ >> drivers/media/platform/qcom/vidc/load.c | 104 +++++ >> drivers/media/platform/qcom/vidc/load.h | 22 ++ >> drivers/media/platform/qcom/vidc/mem.c | 64 ++++ >> drivers/media/platform/qcom/vidc/mem.h | 32 ++ >> drivers/media/platform/qcom/vidc/resources.c | 46 +++ >> drivers/media/platform/qcom/vidc/resources.h | 46 +++ >> 12 files changed, 1843 insertions(+) >> create mode 100644 drivers/media/platform/qcom/vidc/core.c >> create mode 100644 drivers/media/platform/qcom/vidc/core.h >> create mode 100644 drivers/media/platform/qcom/vidc/helpers.c >> create mode 100644 drivers/media/platform/qcom/vidc/helpers.h >> create mode 100644 drivers/media/platform/qcom/vidc/int_bufs.c >> create mode 100644 drivers/media/platform/qcom/vidc/int_bufs.h >> create mode 100644 drivers/media/platform/qcom/vidc/load.c >> create mode 100644 drivers/media/platform/qcom/vidc/load.h >> create mode 100644 drivers/media/platform/qcom/vidc/mem.c >> create mode 100644 drivers/media/platform/qcom/vidc/mem.h >> create mode 100644 drivers/media/platform/qcom/vidc/resources.c >> create mode 100644 drivers/media/platform/qcom/vidc/resources.h >> >> diff --git a/drivers/media/platform/qcom/vidc/core.c b/drivers/media/platform/qcom/vidc/core.c >> new file mode 100644 >> index 000000000000..e005be178fc0 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/vidc/core.c >> @@ -0,0 +1,548 @@ >> +/* >> + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. >> + * Copyright (C) 2016 Linaro Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ >> +#include <linux/clk.h> >> +#include <linux/init.h> >> +#include <linux/ioctl.h> >> +#include <linux/list.h> >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <linux/slab.h> >> +#include <linux/types.h> >> +#include <linux/remoteproc.h> >> +#include <linux/pm_runtime.h> >> +#include <media/videobuf2-v4l2.h> >> +#include <media/v4l2-ioctl.h> >> + >> +#include "core.h" >> +#include "resources.h" >> +#include "vdec.h" >> +#include "venc.h" >> + >> +static void vidc_add_inst(struct vidc_core *core, struct vidc_inst *inst) >> +{ >> + mutex_lock(&core->lock); >> + list_add_tail(&inst->list, &core->instances); > > There are two different "instances" lists in this implementation, one > keeping track of vidc instances and one keeping track of hfi instances, > at the same time the vidc instances has a reference to its associated > hfi instance. > > It should be possible to drop one of those lists. I agree with you. I have thought about this many times during driver development and it should be possible. > >> + mutex_unlock(&core->lock); >> +} >> + >> +static void vidc_del_inst(struct vidc_core *core, struct vidc_inst *inst) >> +{ >> + struct vidc_inst *pos, *n; >> + >> + mutex_lock(&core->lock); >> + list_for_each_entry_safe(pos, n, &core->instances, list) { >> + if (pos == inst) >> + list_del(&inst->list); >> + } >> + mutex_unlock(&core->lock); >> +} >> + >> +static int vidc_rproc_boot(struct vidc_core *core) >> +{ >> + int ret; >> + >> + if (core->rproc_booted) >> + return 0; > > rproc_boot()/rproc_shutdown() is reference counted, so there is no > reason (other than this driver being buggy) to keep track of > "rproc_boot". As such, you can drop vidc_rproc_boot() and > vidc_rproc_shutdown() and just call the rproc functions directly. You are right, this checks are redundant. > >> + >> + ret = rproc_boot(core->rproc); >> + if (ret) >> + return ret; >> + >> + core->rproc_booted = true; >> + >> + return 0; >> +} >> + >> +static void vidc_rproc_shutdown(struct vidc_core *core) >> +{ >> + if (!core->rproc_booted) >> + return; >> + >> + rproc_shutdown(core->rproc); >> + core->rproc_booted = false; >> +} >> + >> +struct vidc_sys_error { >> + struct vidc_core *core; >> + struct delayed_work work; >> +}; > > This is cool, but during the 5 second delay we should be able to call > remove on the driver and this will dereference a freed hfi instance. > > Move the worker to hfi_core and you can cancel it on remove. OK. > >> + >> +static void vidc_sys_error_handler(struct work_struct *work) >> +{ >> + struct vidc_sys_error *handler = >> + container_of(work, struct vidc_sys_error, work.work); >> + struct vidc_core *core = handler->core; >> + struct hfi_core *hfi = &core->hfi; >> + struct device *dev = core->dev; >> + int ret; >> + >> + mutex_lock(&hfi->lock); >> + if (hfi->state != CORE_INVALID) >> + goto exit; >> + >> + mutex_unlock(&hfi->lock); >> + >> + ret = vidc_hfi_core_deinit(hfi); >> + if (ret) >> + dev_err(dev, "core: deinit failed (%d)\n", ret); >> + >> + mutex_lock(&hfi->lock); >> + >> + rproc_report_crash(core->rproc, RPROC_FATAL_ERROR); > > This operation is async, as such I believe this to be fragile. To get > the expected result you should be able to simply call > rproc_shutdown()/rproc_boot() to restart the core... OK will remove crash report for now. > > However, if we at any point would like to be able to get memory dumps > from this core (likely a requirement on the Qualcomm side) we need to > call rproc_report_crash() and let it collect the resources and then > power cycle the core. > > > As the life cycle of the venus driver goes 1:1 with the rproc driver I > think it would be more suitable to make the v4l driver a child of the > rproc driver and have it probe/remove this driver as the rproc comes and > goes. This would allow us to call rproc_report_crash() here, we will be > removed and when the crash is handled (sometime in the future) we will > be probed again. What's the problem with Kconfig "depends on QCOM_VENUS_PIL", isn't that enough? > >> + >> + vidc_rproc_shutdown(core); >> + >> + ret = vidc_rproc_boot(core); >> + if (ret) >> + goto exit; >> + >> + hfi->state = CORE_INIT; >> + >> +exit: >> + mutex_unlock(&hfi->lock); >> + kfree(handler); >> +} >> + >> +static int vidc_event_notify(struct hfi_core *hfi, u32 event) >> +{ >> + struct vidc_sys_error *handler; >> + struct hfi_inst *inst; >> + >> + switch (event) { >> + case EVT_SYS_WATCHDOG_TIMEOUT: >> + case EVT_SYS_ERROR: >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + mutex_lock(&hfi->lock); >> + >> + hfi->state = CORE_INVALID; >> + >> + list_for_each_entry(inst, &hfi->instances, list) { >> + mutex_lock(&inst->lock); >> + inst->state = INST_INVALID; >> + mutex_unlock(&inst->lock); >> + } >> + >> + mutex_unlock(&hfi->lock); >> + >> + handler = kzalloc(sizeof(*handler), GFP_KERNEL); >> + if (!handler) >> + return -ENOMEM; >> + >> + handler->core = container_of(hfi, struct vidc_core, hfi); >> + INIT_DELAYED_WORK(&handler->work, vidc_sys_error_handler); >> + >> + /* >> + * Sleep for 5 sec to ensure venus has completed any >> + * pending cache operations. Without this sleep, we see >> + * device reset when firmware is unloaded after a sys >> + * error. >> + */ >> + schedule_delayed_work(&handler->work, msecs_to_jiffies(5000)); >> + >> + return 0; >> +} >> + >> +static const struct hfi_core_ops vidc_core_ops = { >> + .event_notify = vidc_event_notify, >> +}; > > This is an overly generic way of calling vidc_sys_error_handler(). > There is no need for having the hfi_core_ops indirections for a single > op that will only exist in 1 and only 1 variant. The .event_notify operation is called by hfi part (in hfi_msgs.c) of the driver and I don't want break the interface. My idea was to have HFI part and v4l2 part, and each of these parts taking care of their specifics. The interface between HFI <-> v4l2 should be immutable and shoudn't be changed when every new version of the hardware IP rise up. > > Just replace the two affected event_notify() calls with a direct call to > this function (and clean it up a bit). > >> + >> +static int vidc_open(struct file *file) >> +{ >> + struct video_device *vdev = video_devdata(file); >> + struct vidc_core *core = video_drvdata(file); >> + struct vidc_inst *inst; >> + int ret = 0; >> + >> + inst = kzalloc(sizeof(*inst), GFP_KERNEL); >> + if (!inst) >> + return -ENOMEM; >> + >> + mutex_init(&inst->lock); >> + >> + INIT_VIDC_LIST(&inst->scratchbufs); > > Please inline the mutex_init() and INIT_LIST_HEAD() here and drop the > custom INIT_VIDC_LIST() wrapper macro. OK. I thought I made this already, but seems that I forgot it. Also I think scratchbufs and persistbufs lists can be merged in one common list. > >> + INIT_VIDC_LIST(&inst->persistbufs); >> + INIT_VIDC_LIST(&inst->registeredbufs); >> + >> + INIT_LIST_HEAD(&inst->bufqueue); >> + mutex_init(&inst->bufqueue_lock); >> + >> + if (vdev == &core->vdev_dec) >> + inst->session_type = VIDC_SESSION_TYPE_DEC; >> + else >> + inst->session_type = VIDC_SESSION_TYPE_ENC; >> + >> + inst->core = core; >> + >> + if (inst->session_type == VIDC_SESSION_TYPE_DEC) >> + ret = vdec_open(inst); >> + else >> + ret = venc_open(inst); >> + >> + if (ret) >> + goto err_free_inst; >> + >> + if (inst->session_type == VIDC_SESSION_TYPE_DEC) >> + v4l2_fh_init(&inst->fh, &core->vdev_dec); >> + else >> + v4l2_fh_init(&inst->fh, &core->vdev_enc); > > Here we have three sequential conditionals testing for the same thing, > please join them into one. OK. > >> + >> + inst->fh.ctrl_handler = &inst->ctrl_handler; >> + >> + v4l2_fh_add(&inst->fh); >> + >> + file->private_data = &inst->fh; >> + >> + vidc_add_inst(core, inst); >> + >> + return 0; >> + >> +err_free_inst: >> + kfree(inst); >> + return ret; >> +} >> + >> +static int vidc_close(struct file *file) >> +{ >> + struct vidc_inst *inst = to_inst(file); >> + struct vidc_core *core = inst->core; >> + >> + if (inst->session_type == VIDC_SESSION_TYPE_DEC) >> + vdec_close(inst); >> + else >> + venc_close(inst); >> + >> + vidc_del_inst(core, inst); >> + >> + mutex_destroy(&inst->bufqueue_lock); >> + mutex_destroy(&inst->scratchbufs.lock); >> + mutex_destroy(&inst->persistbufs.lock); >> + mutex_destroy(&inst->registeredbufs.lock); > > Here's a good reason for dropping the INIT_VIDC_LIST() macro yes indeed :) > >> + >> + v4l2_fh_del(&inst->fh); >> + v4l2_fh_exit(&inst->fh); >> + >> + kfree(inst); >> + return 0; >> +} >> + >> +static unsigned int vidc_poll(struct file *file, struct poll_table_struct *pt) >> +{ >> + struct vidc_inst *inst = to_inst(file); >> + struct vb2_queue *outq = &inst->bufq_out; >> + struct vb2_queue *capq = &inst->bufq_cap; >> + unsigned int ret; >> + >> + ret = vb2_poll(outq, file, pt); >> + ret |= vb2_poll(capq, file, pt); >> + >> + return ret; >> +} >> + >> +static int vidc_mmap(struct file *file, struct vm_area_struct *vma) >> +{ >> + struct vidc_inst *inst = to_inst(file); >> + unsigned long offset = vma->vm_pgoff << PAGE_SHIFT; >> + int ret; >> + >> + if (offset < DST_QUEUE_OFF_BASE) { >> + ret = vb2_mmap(&inst->bufq_out, vma); >> + } else { >> + vma->vm_pgoff -= DST_QUEUE_OFF_BASE >> PAGE_SHIFT; >> + ret = vb2_mmap(&inst->bufq_cap, vma); >> + } > > This feels hackish, is this really the way to do this? Yes it looks like a hack but there is no other way (to my knowledge), there are plenty of v4l2 drivers doing like this. > >> + >> + return ret; >> +} >> + >> +const struct v4l2_file_operations vidc_fops = { >> + .owner = THIS_MODULE, >> + .open = vidc_open, >> + .release = vidc_close, >> + .unlocked_ioctl = video_ioctl2, >> + .poll = vidc_poll, >> + .mmap = vidc_mmap, >> +#ifdef CONFIG_COMPAT >> + .compat_ioctl32 = v4l2_compat_ioctl32, >> +#endif >> +}; >> + >> +static irqreturn_t vidc_isr_thread(int irq, void *dev_id) >> +{ >> + return vidc_hfi_isr_thread(irq, dev_id); >> +} >> + >> +static irqreturn_t vidc_isr(int irq, void *dev) >> +{ >> + return vidc_hfi_isr(irq, dev); >> +} > > These two functions indicates that we're requesting the irq in the wrong > layer. IMO the proper place is platform driver .probe method. > > Also, these two functions arrives in a later patchset, so I assume this > doesn't compile... That's why I'm adding Makefiles later on patchset. On the other hand I have splitted the driver by files because I think it is easier for review. But I might be wrong. > >> + >> +static int vidc_clks_get(struct vidc_core *core, unsigned int clks_num, >> + const char * const *clks_id) >> +{ >> + struct device *dev = core->dev; >> + unsigned int i; >> + >> + for (i = 0; i < clks_num; i++) { >> + core->clks[i] = devm_clk_get(dev, clks_id[i]); >> + if (IS_ERR(core->clks[i])) >> + return PTR_ERR(core->clks[i]); >> + } >> + >> + return 0; >> +} >> + >> +static int >> +vidc_clks_enable(struct vidc_core *core, const struct vidc_resources *res) >> +{ >> + unsigned int i; >> + int ret; >> + >> + for (i = 0; i < res->clks_num; i++) { >> + ret = clk_prepare_enable(core->clks[i]); >> + if (ret) >> + goto err; >> + } >> + >> + return 0; >> +err: >> + while (--i) >> + clk_disable_unprepare(core->clks[i]); >> + >> + return ret; >> +} >> + >> +static void >> +vidc_clks_disable(struct vidc_core *core, const struct vidc_resources *res) >> +{ >> + unsigned int i; >> + >> + for (i = 0; i < res->clks_num; i++) >> + clk_disable_unprepare(core->clks[i]); >> +} >> + >> +static const struct of_device_id vidc_dt_match[] = { >> + { .compatible = "qcom,vidc-msm8916", .data = &msm8916_res, }, >> + { } >> +}; >> + >> +MODULE_DEVICE_TABLE(of, vidc_dt_match); > > As you're using of_device_get_match_data() you can move this table to > the bottom of the file. OK. > >> + >> +static int vidc_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct vidc_core *core; >> + struct device_node *rproc; >> + struct resource *r; >> + int ret; >> + >> + core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL); >> + if (!core) >> + return -ENOMEM; >> + >> + core->dev = dev; >> + platform_set_drvdata(pdev, core); >> + >> + rproc = of_parse_phandle(dev->of_node, "rproc", 0); >> + if (IS_ERR(rproc)) >> + return PTR_ERR(rproc); >> + >> + core->rproc = rproc_get_by_phandle(rproc->phandle); > > FYI, We're hoping to land some patches shortly that will replace this > with rproc_get(pdev->dev.of_node), looking up an rproc by the standard > "rprocs" property... OK, that looks good. But shoudn't be rproc_get(pdev->dev)? > >> + if (IS_ERR(core->rproc)) >> + return PTR_ERR(core->rproc); >> + else if (!core->rproc) >> + return -EPROBE_DEFER; > > We're cleaning up this in the core as well. > > You need to rproc_put() the rproc pointer after this point. OK, good. When those changes landed I will rework this part. > > > My question still stands though, if this driver should be probed as the > remoteproc is booted (or the apr service appearing). I will continue to > look at that. I might be misunderstood your point here. Is your concern related to EPROBE_DEFFER or some sort of ordering issue in rproc? Currently the vidc depends on QCOM_VENUS_PIL in Kconfig, so modprobe vidc should modprobe remoteproc driver cause it depends on it. > >> + >> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + core->base = devm_ioremap_resource(dev, r); >> + if (IS_ERR(core->base)) >> + return PTR_ERR(core->base); >> + >> + core->irq = platform_get_irq(pdev, 0); >> + if (core->irq < 0) >> + return core->irq; >> + >> + core->res = of_device_get_match_data(dev); >> + if (!core->res) >> + return -ENODEV; >> + >> + ret = vidc_clks_get(core, core->res->clks_num, core->res->clks); >> + if (ret) >> + return ret; >> + >> + ret = dma_set_mask_and_coherent(dev, core->res->dma_mask); >> + if (ret) >> + return ret; >> + >> + INIT_LIST_HEAD(&core->instances); >> + mutex_init(&core->lock); >> + >> + ret = devm_request_threaded_irq(dev, core->irq, vidc_isr, >> + vidc_isr_thread, >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > > Drop this IRQF_TRIGGER_HIGH and have this be specified in devicetree. OK can do that, but is there a convention about who is populating the flag and what is the precedence dt against the driver? > >> + "vidc", &core->hfi); >> + if (ret) >> + return ret; >> + >> + core->hfi.core_ops = &vidc_core_ops; >> + core->hfi.dev = dev; >> + >> + ret = vidc_hfi_create(&core->hfi, core->res, core->base); >> + if (ret) >> + return ret; >> + >> + ret = vidc_clks_enable(core, core->res); >> + if (ret) >> + goto err_hfi_destroy; >> + >> + ret = vidc_rproc_boot(core); >> + if (ret) { >> + vidc_clks_disable(core, core->res); >> + goto err_hfi_destroy; >> + } >> + >> + pm_runtime_enable(dev); >> + >> + ret = pm_runtime_get_sync(dev); >> + if (ret < 0) >> + goto err_runtime_disable; >> + >> + ret = vidc_hfi_core_init(&core->hfi); >> + if (ret) >> + goto err_rproc_shutdown; >> + >> + ret = pm_runtime_put_sync(dev); >> + if (ret) >> + goto err_core_deinit; >> + >> + vidc_clks_disable(core, core->res); > > These operations follow the general pattern of booting other Qualcomm > remoteprocs; acquire and enable some resources, boot the core and > disable the resources. Therefor it looks quite likely that these > operations are related to the life cycle of the venus core, rather than > hfi. So you saying that > >> + >> + ret = v4l2_device_register(dev, &core->v4l2_dev); >> + if (ret) >> + goto err_core_deinit; >> + >> + ret = vdec_init(core, &core->vdev_dec); >> + if (ret) >> + goto err_dev_unregister; >> + >> + ret = venc_init(core, &core->vdev_enc); >> + if (ret) >> + goto err_vdec_deinit; >> + >> + return 0; >> + >> +err_vdec_deinit: >> + vdec_deinit(core, &core->vdev_dec); >> +err_dev_unregister: >> + v4l2_device_unregister(&core->v4l2_dev); >> +err_core_deinit: >> + vidc_hfi_core_deinit(&core->hfi); >> +err_rproc_shutdown: >> + vidc_rproc_shutdown(core); >> +err_runtime_disable: >> + pm_runtime_set_suspended(dev); >> + pm_runtime_disable(dev); >> +err_hfi_destroy: >> + vidc_hfi_destroy(&core->hfi); >> + return ret; >> +} >> + >> +static int vidc_remove(struct platform_device *pdev) >> +{ >> + struct vidc_core *core = platform_get_drvdata(pdev); >> + int ret; >> + >> + ret = pm_runtime_get_sync(&pdev->dev); >> + if (ret < 0) >> + return ret; > > No-one cares about you returning an error here, so you better move > forward and release as much of your resources as possible even though > you didn't get your pm. Hmm, I don't agree here. The runtime_resume will enable clocks (for example venus iface clk) and if it fails the subsequent call to vidc_rproc_shutdown can crash badly. > >> + >> + ret = vidc_hfi_core_deinit(&core->hfi); >> + if (ret) { >> + pm_runtime_put_sync(&pdev->dev); >> + return ret; >> + } >> + >> + vidc_rproc_shutdown(core); >> + >> + ret = pm_runtime_put_sync(&pdev->dev); >> + >> + vidc_hfi_destroy(&core->hfi); >> + vdec_deinit(core, &core->vdev_dec); >> + venc_deinit(core, &core->vdev_enc); >> + v4l2_device_unregister(&core->v4l2_dev); >> + >> + pm_runtime_disable(core->dev); >> + >> + return ret < 0 ? ret : 0; >> +} >> + >> +static int vidc_runtime_suspend(struct device *dev) >> +{ >> + struct vidc_core *core = dev_get_drvdata(dev); >> + int ret; >> + >> + ret = vidc_hfi_core_suspend(&core->hfi); >> + >> + vidc_clks_disable(core, core->res); >> + >> + return ret; >> +} >> + >> +static int vidc_runtime_resume(struct device *dev) >> +{ >> + struct vidc_core *core = dev_get_drvdata(dev); >> + int ret; >> + >> + ret = vidc_clks_enable(core, core->res); >> + if (ret) >> + return ret; >> + >> + return vidc_hfi_core_resume(&core->hfi); >> +} >> + >> +static int vidc_pm_suspend(struct device *dev) >> +{ >> + return vidc_runtime_suspend(dev); >> +} >> + >> +static int vidc_pm_resume(struct device *dev) >> +{ >> + return vidc_runtime_resume(dev); >> +} >> + >> +static const struct dev_pm_ops vidc_pm_ops = { >> + SET_SYSTEM_SLEEP_PM_OPS(vidc_pm_suspend, vidc_pm_resume) >> + SET_RUNTIME_PM_OPS(vidc_runtime_suspend, vidc_runtime_resume, NULL) >> +}; >> + >> +static struct platform_driver qcom_vidc_driver = { >> + .probe = vidc_probe, >> + .remove = vidc_remove, >> + .driver = { >> + .name = "qcom-vidc", >> + .of_match_table = vidc_dt_match, >> + .pm = &vidc_pm_ops, >> + }, >> +}; >> + >> +module_platform_driver(qcom_vidc_driver); >> + >> +MODULE_ALIAS("platform:qcom-vidc"); >> +MODULE_DESCRIPTION("Qualcomm video encoder and decoder driver"); >> +MODULE_LICENSE("GPL v2"); >> diff --git a/drivers/media/platform/qcom/vidc/core.h b/drivers/media/platform/qcom/vidc/core.h >> new file mode 100644 >> index 000000000000..5dc8e05f8c36 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/vidc/core.h >> @@ -0,0 +1,196 @@ >> +/* >> + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. >> + * Copyright (C) 2016 Linaro Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ >> + >> +#ifndef __VIDC_CORE_H_ >> +#define __VIDC_CORE_H_ >> + >> +#include <media/v4l2-device.h> >> +#include <media/v4l2-ctrls.h> >> +#include <media/videobuf2-core.h> >> + >> +#include "resources.h" >> +#include "hfi.h" >> + >> +#define VIDC_DRV_NAME "vidc" > > Unused used in vdec.c and enc.c. But I can delete this in next submission. > >> + >> +struct vidc_list { >> + struct list_head list; >> + struct mutex lock; >> +}; > > Can't we get away without passing around lockable lists? Does these > lists have to be locked independently and should we really pass around > their lock with them? I guess it is possible but didn't spent to much time on that (I had more important problems to solve with downstream driver). So the answer is yes and I have to re-consider it. > >> + >> +struct vidc_format { >> + u32 pixfmt; >> + int num_planes; >> + u32 type; >> +}; >> + >> +struct vidc_core { >> + struct list_head list; > > This list_head seems unused. Yes, it is. > >> + void __iomem *base; > > base is acquired and passed by value to vidc_hfi_create(), so no need to > keep track of it here. > >> + int irq; > > This irq belongs to hfi, so it should probably be kept there. Sure, will move those two. > >> + struct clk *clks[VIDC_CLKS_NUM_MAX]; >> + struct mutex lock; > > This "lock" seems to be only related the instances list, please name it > more appropriately - and place it next to the instances member. OK. > >> + struct hfi_core hfi; >> + struct video_device vdev_dec; >> + struct video_device vdev_enc; >> + struct v4l2_device v4l2_dev; >> + struct list_head instances; >> + const struct vidc_resources *res; >> + struct rproc *rproc; >> + bool rproc_booted; >> + struct device *dev; >> +}; >> + >> +struct vdec_controls { >> + u32 post_loop_deb_mode; >> + u32 profile; >> + u32 level; >> +}; >> + >> +struct venc_controls { >> + u16 gop_size; >> + u32 idr_period; >> + u32 num_p_frames; >> + u32 num_b_frames; >> + u32 bitrate_mode; >> + u32 bitrate; >> + u32 bitrate_peak; >> + >> + u32 h264_i_period; >> + u32 h264_entropy_mode; >> + u32 h264_i_qp; >> + u32 h264_p_qp; >> + u32 h264_b_qp; >> + u32 h264_min_qp; >> + u32 h264_max_qp; >> + u32 h264_loop_filter_mode; >> + u32 h264_loop_filter_alpha; >> + u32 h264_loop_filter_beta; >> + >> + u32 vp8_min_qp; >> + u32 vp8_max_qp; >> + >> + u32 multi_slice_mode; >> + u32 multi_slice_max_bytes; >> + u32 multi_slice_max_mb; >> + >> + u32 header_mode; >> + >> + u32 profile; >> + u32 level; >> +}; >> + >> +struct vidc_inst { >> + struct list_head list; >> + struct mutex lock; >> + struct vidc_core *core; >> + >> + struct vidc_list scratchbufs; >> + struct vidc_list persistbufs; >> + struct vidc_list registeredbufs; > > Just inline the list_head and mutex here, as it's done for bufqueue. OK. >> + >> + struct list_head bufqueue; >> + struct mutex bufqueue_lock; >> + >> + int streamoff; >> + int streamon; >> + struct vb2_queue bufq_out; >> + struct vb2_queue bufq_cap; >> + >> + struct v4l2_ctrl_handler ctrl_handler; >> + union { >> + struct vdec_controls dec; >> + struct venc_controls enc; >> + } controls; >> + struct v4l2_fh fh; >> + >> + struct hfi_inst *hfi_inst; >> + >> + /* session fields */ >> + u32 session_type; >> + u32 width; >> + u32 height; >> + u32 out_width; >> + u32 out_height; >> + u32 colorspace; >> + u8 ycbcr_enc; >> + u8 quantization; >> + u8 xfer_func; >> + u64 fps; >> + struct v4l2_fract timeperframe; >> + const struct vidc_format *fmt_out; >> + const struct vidc_format *fmt_cap; >> + unsigned int num_input_bufs; >> + unsigned int num_output_bufs; >> + bool in_reconfig; >> + u32 reconfig_width; >> + u32 reconfig_height; >> + u64 sequence; >> +}; >> + >> +#define ctrl_to_inst(ctrl) \ >> + container_of(ctrl->handler, struct vidc_inst, ctrl_handler) >> + >> +struct vidc_ctrl { >> + u32 id; >> + enum v4l2_ctrl_type type; >> + s32 min; >> + s32 max; >> + s32 def; >> + u32 step; >> + u64 menu_skip_mask; >> + u32 flags; >> + const char * const *qmenu; >> +}; >> + >> +/* >> + * Offset base for buffers on the destination queue - used to distinguish >> + * between source and destination buffers when mmapping - they receive the same >> + * offsets but for different queues >> + */ >> +#define DST_QUEUE_OFF_BASE (1 << 30) >> + >> +extern const struct v4l2_file_operations vidc_fops; > > Just pass this to v{dec,enc}_init() rather than back-referencing it > through a global variable. But on the other hand this is unused in this > patchset... Very nice idea, thanks. > >> + >> +static inline void INIT_VIDC_LIST(struct vidc_list *mlist) >> +{ >> + mutex_init(&mlist->lock); >> + INIT_LIST_HEAD(&mlist->list); >> +} >> + >> +static inline struct vidc_inst *to_inst(struct file *filp) >> +{ >> + return container_of(filp->private_data, struct vidc_inst, fh); >> +} >> + >> +static inline struct hfi_inst *to_hfi_inst(struct file *filp) > > Unused Unsed in this patch, but used by vdec.c and enc.c in subsequent patches. > >> +{ >> + return to_inst(filp)->hfi_inst; >> +} >> + >> +static inline struct vb2_queue * >> +vidc_to_vb2q(struct file *file, enum v4l2_buf_type type) > > Unused Same as above comment. > >> +{ >> + struct vidc_inst *inst = to_inst(file); >> + >> + if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) >> + return &inst->bufq_cap; >> + else if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >> + return &inst->bufq_out; >> + >> + return NULL; >> +} >> + >> +#endif >> diff --git a/drivers/media/platform/qcom/vidc/helpers.c b/drivers/media/platform/qcom/vidc/helpers.c >> new file mode 100644 >> index 000000000000..81079f2b5ed1 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/vidc/helpers.c >> @@ -0,0 +1,394 @@ >> +/* >> + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. >> + * Copyright (C) 2016 Linaro Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ >> +#include <linux/list.h> >> +#include <linux/mutex.h> >> +#include <linux/pm_runtime.h> >> +#include <media/videobuf2-dma-sg.h> >> + >> +#include "helpers.h" >> +#include "int_bufs.h" >> +#include "load.h" >> +#include "hfi_helper.h" >> + >> +static int session_set_buf(struct vb2_buffer *vb) >> +{ >> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); >> + struct vb2_queue *q = vb->vb2_queue; >> + struct vidc_inst *inst = vb2_get_drv_priv(q); >> + struct vidc_core *core = inst->core; >> + struct device *dev = core->dev; >> + struct hfi_core *hfi = &core->hfi; >> + struct vidc_buffer *buf = to_vidc_buffer(vbuf); >> + struct hfi_frame_data fdata; >> + int ret; >> + >> + memset(&fdata, 0, sizeof(fdata)); >> + >> + fdata.alloc_len = vb2_plane_size(vb, 0); >> + fdata.device_addr = buf->dma_addr; >> + fdata.timestamp = vb->timestamp; >> + fdata.flags = 0; >> + fdata.clnt_data = buf->dma_addr; >> + >> + if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { >> + fdata.buffer_type = HFI_BUFFER_INPUT; >> + fdata.filled_len = vb2_get_plane_payload(vb, 0); >> + fdata.offset = vb->planes[0].data_offset; >> + >> + if (vbuf->flags & V4L2_BUF_FLAG_LAST || !fdata.filled_len) >> + fdata.flags |= HFI_BUFFERFLAG_EOS; >> + >> + ret = vidc_hfi_session_etb(hfi, inst->hfi_inst, &fdata); >> + } else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { >> + fdata.buffer_type = HFI_BUFFER_OUTPUT; >> + fdata.filled_len = 0; >> + fdata.offset = 0; >> + >> + ret = vidc_hfi_session_ftb(hfi, inst->hfi_inst, &fdata); >> + } else { >> + ret = -EINVAL; >> + } >> + >> + if (ret) { >> + dev_err(dev, "failed to set session buffer (%d)\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int session_unregister_bufs(struct vidc_inst *inst) >> +{ >> + struct device *dev = inst->core->dev; >> + struct hfi_core *hfi = &inst->core->hfi; >> + struct hfi_buffer_desc *bd; >> + struct vidc_buffer *buf, *tmp; >> + int ret = 0; >> + >> + mutex_lock(&inst->registeredbufs.lock); >> + list_for_each_entry_safe(buf, tmp, &inst->registeredbufs.list, >> + hfi_list) { >> + list_del(&buf->hfi_list); > > So the hfi_list is the list_head for entries in the _vidc_ instance > list? yes, registeredbufs.list is used to keep track of the all buffers that will be used during the life-cycle of the current session, i.e. the firmware wants to know all buffer addresses before calling session_start. On the other side bufqueue list is used for v4l2 queue/dequeue side. > >> + bd = &buf->bd; >> + bd->response_required = 1; >> + ret = vidc_hfi_session_unset_buffers(hfi, inst->hfi_inst, bd); >> + if (ret) { >> + dev_err(dev, "%s: session release buffers failed\n", >> + __func__); >> + break; >> + } >> + } >> + mutex_unlock(&inst->registeredbufs.lock); >> + >> + return ret; >> +} >> + >> +static int session_register_bufs(struct vidc_inst *inst) >> +{ >> + struct device *dev = inst->core->dev; >> + struct hfi_core *hfi = &inst->core->hfi; >> + struct hfi_buffer_desc *bd; >> + struct vidc_buffer *buf, *tmp; >> + int ret = 0; >> + >> + mutex_lock(&inst->registeredbufs.lock); >> + list_for_each_entry_safe(buf, tmp, &inst->registeredbufs.list, >> + hfi_list) { >> + bd = &buf->bd; >> + ret = vidc_hfi_session_set_buffers(hfi, inst->hfi_inst, bd); >> + if (ret) { >> + dev_err(dev, "%s: session: set buffer failed\n", >> + __func__); >> + break; >> + } >> + } >> + mutex_unlock(&inst->registeredbufs.lock); >> + >> + return ret; >> +} >> + >> +int vidc_buf_descs(struct vidc_inst *inst, u32 type, >> + struct hfi_buffer_requirements *out) > > If you call this vidc_get_buf_requirements() it would actually describe > what's going on. But why is this hfi wrapper in the core, rather than the original name of this function was similar to what you suggest but I decided in last-minute cleanup to shorten its name. > just have the internal buffer manager call it directly. It is in the core cause I used it on few places to gather buffer count needed depending on parameters (resolution, codec). Good example is vb2_ops::queue_setup where I need to return num_buffers depending on resolution, codec, bitrate, framerate and so on. > > The call doesn't seem to depend on the parameters or state, can we > cache the result? No, we cannot. see above comment. Something more the scratch and prersist buffer sizes can also be changed by the firmware depending on above proparties. > >> +{ >> + struct hfi_core *hfi = &inst->core->hfi; >> + u32 ptype = HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS; >> + union hfi_get_property hprop; >> + int ret, i; >> + >> + if (out) >> + memset(out, 0, sizeof(*out)); >> + >> + ret = vidc_hfi_session_get_property(hfi, inst->hfi_inst, ptype, &hprop); >> + if (ret) >> + return ret; >> + >> + ret = -EINVAL; >> + >> + for (i = 0; i < HFI_BUFFER_TYPE_MAX; i++) { >> + if (hprop.bufreq[i].type != type) >> + continue; >> + >> + if (out) >> + memcpy(out, &hprop.bufreq[i], sizeof(*out)); >> + ret = 0; >> + break; >> + } >> + >> + return ret; >> +} >> + > [..] >> + >> +void vidc_vb2_stop_streaming(struct vb2_queue *q) >> +{ >> + struct vidc_inst *inst = vb2_get_drv_priv(q); >> + struct hfi_inst *hfi_inst = inst->hfi_inst; >> + struct vidc_core *core = inst->core; >> + struct device *dev = core->dev; >> + struct hfi_core *hfi = &core->hfi; >> + int ret, streamoff; >> + >> + mutex_lock(&inst->lock); >> + streamoff = inst->streamoff; >> + mutex_unlock(&inst->lock); >> + >> + if (streamoff) >> + return; >> + >> + mutex_lock(&inst->lock); >> + if (inst->streamon == 0) { >> + mutex_unlock(&inst->lock); >> + return; >> + } >> + mutex_unlock(&inst->lock); > > Why do we keep track of streamon and stream off, why isn't streamoff > ever cleared? Why don't we check both conditions in one critical region? Probably cause its buggy, I will sort it out. > >> + >> + ret = vidc_hfi_session_stop(hfi, inst->hfi_inst); >> + if (ret) { >> + dev_err(dev, "session: stop failed (%d)\n", ret); >> + goto abort; > > When are we going to relaim the buffers in these cases? session_stop will instruct the firmware return buffers to the v4l2 driver through hfi_inst_ops empty_buf_done and fill_buf_done, those operations will call vb2_buffer_done. > >> + } >> + >> + ret = vidc_hfi_session_unload_res(hfi, inst->hfi_inst); >> + if (ret) { >> + dev_err(dev, "session: release resources failed (%d)\n", ret); >> + goto abort; >> + } >> + >> + ret = session_unregister_bufs(inst); >> + if (ret) { >> + dev_err(dev, "failed to release capture buffers: %d\n", ret); >> + goto abort; >> + } >> + >> + ret = internal_bufs_free(inst); >> + >> + if (hfi_inst->state == INST_INVALID || hfi->state == CORE_INVALID) { >> + ret = -EINVAL; >> + goto abort; >> + } >> + >> +abort: >> + if (ret) >> + vidc_hfi_session_abort(hfi, inst->hfi_inst); >> + >> + vidc_scale_clocks(inst->core); >> + >> + ret = vidc_hfi_session_deinit(hfi, inst->hfi_inst); >> + >> + mutex_lock(&inst->lock); >> + inst->streamoff = 1; >> + mutex_unlock(&inst->lock); >> + >> + if (ret) >> + dev_err(dev, "stop streaming failed type: %d, ret: %d\n", >> + q->type, ret); >> + >> + ret = pm_runtime_put_sync(dev); >> + if (ret < 0) >> + dev_err(dev, "%s: pm_runtime_put_sync (%d)\n", __func__, ret); >> +} >> + >> +int vidc_vb2_start_streaming(struct vidc_inst *inst) >> +{ >> + struct device *dev = inst->core->dev; >> + struct hfi_core *hfi = &inst->core->hfi; >> + struct vidc_buffer *buf, *n; >> + int ret; >> + >> + ret = session_register_bufs(inst); >> + if (ret) >> + return ret; >> + >> + ret = internal_bufs_alloc(inst); >> + if (ret) >> + return ret; >> + >> + vidc_scale_clocks(inst->core); >> + >> + ret = vidc_hfi_session_load_res(hfi, inst->hfi_inst); >> + if (ret) { >> + dev_err(dev, "session: load resources (%d)\n", ret); >> + return ret; >> + } >> + >> + ret = vidc_hfi_session_start(hfi, inst->hfi_inst); >> + if (ret) { >> + dev_err(dev, "session: start failed (%d)\n", ret); >> + return ret; >> + } >> + >> + mutex_lock(&inst->bufqueue_lock); >> + list_for_each_entry_safe(buf, n, &inst->bufqueue, list) { >> + ret = session_set_buf(&buf->vb.vb2_buf); >> + if (ret) >> + break; >> + } >> + mutex_unlock(&inst->bufqueue_lock); >> + >> + if (!ret) { >> + mutex_lock(&inst->lock); >> + inst->streamon = 1; >> + mutex_unlock(&inst->lock); >> + } >> + >> + return ret; >> +} >> diff --git a/drivers/media/platform/qcom/vidc/helpers.h b/drivers/media/platform/qcom/vidc/helpers.h >> new file mode 100644 >> index 000000000000..a151c96bf939 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/vidc/helpers.h >> @@ -0,0 +1,43 @@ >> +/* >> + * Copyright (c) 2012-2014, The Linux Foundation. All rights reserved. >> + * Copyright (C) 2016 Linaro Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ >> +#ifndef __VIDC_COMMON_H__ >> +#define __VIDC_COMMON_H__ > > s/COMMON/HELPERS/ > >> + >> +#include <linux/list.h> >> +#include <media/videobuf2-v4l2.h> >> + >> +#include "core.h" >> + >> +struct vidc_buffer { >> + struct vb2_v4l2_buffer vb; >> + struct list_head list; >> + dma_addr_t dma_addr; >> + struct list_head hfi_list; > > This seems to be the list_head used for associating buffers to the > _vidc_ instances. > >> + struct hfi_buffer_desc bd; >> +}; >> + >> +#define to_vidc_buffer(buf) container_of(buf, struct vidc_buffer, vb) >> + >> +struct vb2_v4l2_buffer * >> +vidc_vb2_find_buf(struct vidc_inst *inst, dma_addr_t addr); >> +int vidc_vb2_buf_init(struct vb2_buffer *vb); >> +int vidc_vb2_buf_prepare(struct vb2_buffer *vb); >> +void vidc_vb2_buf_queue(struct vb2_buffer *vb); >> +void vidc_vb2_stop_streaming(struct vb2_queue *q); >> +int vidc_vb2_start_streaming(struct vidc_inst *inst); >> +int vidc_buf_descs(struct vidc_inst *inst, u32 type, >> + struct hfi_buffer_requirements *out); >> +int vidc_set_color_format(struct vidc_inst *inst, u32 type, u32 fmt); >> +#endif >> diff --git a/drivers/media/platform/qcom/vidc/int_bufs.c b/drivers/media/platform/qcom/vidc/int_bufs.c > [..] >> + >> +static int internal_alloc_and_set(struct vidc_inst *inst, >> + struct hfi_buffer_requirements *bufreq, >> + struct vidc_list *buf_list) >> +{ >> + struct vidc_internal_buf *buf; >> + struct vidc_mem *mem; >> + unsigned int i; >> + int ret = 0; >> + >> + if (!bufreq->size) >> + return 0; >> + >> + for (i = 0; i < bufreq->count_actual; i++) { >> + mem = mem_alloc(inst->core->dev, bufreq->size, 0); > > Inline mem_alloc here; might need to make sure bufreq->size is 4K > aligned. OK, I will give it a try. > >> + if (IS_ERR(mem)) { >> + ret = PTR_ERR(mem); >> + goto err_no_mem; >> + } >> + >> + buf = kzalloc(sizeof(*buf), GFP_KERNEL); >> + if (!buf) { >> + ret = -ENOMEM; >> + goto fail_kzalloc; >> + } >> + >> + buf->mem = mem; >> + buf->type = bufreq->type; >> + >> + ret = internal_set_buf_on_fw(inst, bufreq->type, mem, false); >> + if (ret) >> + goto fail_set_buffers; >> + >> + mutex_lock(&buf_list->lock); >> + list_add_tail(&buf->list, &buf_list->list); >> + mutex_unlock(&buf_list->lock); >> + } >> + >> + return ret; >> + >> +fail_set_buffers: >> + kfree(buf); >> +fail_kzalloc: >> + mem_free(mem); >> +err_no_mem: >> + return ret; >> +} >> + > [..] >> + >> +static int persist_set_buffer(struct vidc_inst *inst, u32 type) >> +{ >> + struct hfi_buffer_requirements bufreq; >> + int ret; >> + >> + ret = vidc_buf_descs(inst, type, &bufreq); >> + if (ret) >> + return 0; >> + >> + mutex_lock(&inst->persistbufs.lock); >> + if (!list_empty(&inst->persistbufs.list)) { > > This function is called twice, with type HFI_BUFFER_INTERNAL_PERSIST and > HFI_BUFFER_INTERNAL_PERSIST_1 respectively. Unless the buffer > requirements are missing for HFI_BUFFER_INTERNAL_PERSIST persistbufs > won't be empty and we will skip the later allocation. > >> + mutex_unlock(&inst->persistbufs.lock); >> + return 0; >> + } >> + mutex_unlock(&inst->persistbufs.lock); >> + >> + return internal_alloc_and_set(inst, &bufreq, &inst->persistbufs); >> +} >> + > [..] >> + >> +static int scratch_set_buffers(struct vidc_inst *inst) >> +{ >> + struct device *dev = inst->core->dev; >> + int ret; >> + >> + ret = scratch_unset_buffers(inst, true); >> + if (ret) >> + dev_warn(dev, "Failed to release scratch buffers\n"); > > internal_bufs_free() calls scratch_unset_buffers(reuse=false) so we're > coming here with an empty scratchbufs either way - meaning that this > whole file can be greatly simplified. > > So instead of trying to fix that I would suggest that you just let > internal_bufs_alloc() acquire the buffer requirements and call > internal_alloc_and_set() directly, storing the result in a single list. > > And then inline a free method in internal_bufs_free() as well as drop > all reuse-stuff and unused/dead code. > > That would simplify this file quite a bit and if there actually is a > need for the reusing of buffer that can be added at some later time. > Actially I thought about droping the reuse stuff in the past, so I agree on that cleanup. The thing which worries me is the size of those buffers (the biggest is 10-15MB) and also the allocation time. Currently those buffers are allocate on streamon time, but probably the right place is on request_buf time. >> + >> + ret = scratch_set_buffer(inst, HFI_BUFFER_INTERNAL_SCRATCH); >> + if (ret) >> + goto error; >> + >> + ret = scratch_set_buffer(inst, HFI_BUFFER_INTERNAL_SCRATCH_1); >> + if (ret) >> + goto error; >> + >> + ret = scratch_set_buffer(inst, HFI_BUFFER_INTERNAL_SCRATCH_2); >> + if (ret) >> + goto error; >> + >> + return 0; >> +error: >> + scratch_unset_buffers(inst, false); >> + return ret; >> +} >> + >> +static int persist_set_buffers(struct vidc_inst *inst) >> +{ >> + int ret; >> + >> + ret = persist_set_buffer(inst, HFI_BUFFER_INTERNAL_PERSIST); >> + if (ret) >> + goto error; >> + >> + ret = persist_set_buffer(inst, HFI_BUFFER_INTERNAL_PERSIST_1); >> + if (ret) >> + goto error; >> + >> + return 0; >> + >> +error: >> + persist_unset_buffers(inst); >> + return ret; >> +} >> + >> +int internal_bufs_alloc(struct vidc_inst *inst) >> +{ >> + struct device *dev = inst->core->dev; >> + int ret; >> + >> + ret = scratch_set_buffers(inst); >> + if (ret) { >> + dev_err(dev, "set scratch buffers (%d)\n", ret); >> + return ret; >> + } >> + >> + ret = persist_set_buffers(inst); >> + if (ret) { >> + dev_err(dev, "set persist buffers (%d)\n", ret); >> + goto error; >> + } >> + >> + return 0; >> + >> +error: >> + scratch_unset_buffers(inst, false); >> + return ret; >> +} >> + >> +int internal_bufs_free(struct vidc_inst *inst) >> +{ >> + struct device *dev = inst->core->dev; >> + int ret; >> + >> + ret = scratch_unset_buffers(inst, false); >> + if (ret) >> + dev_err(dev, "failed to release scratch buffers: %d\n", ret); >> + >> + ret = persist_unset_buffers(inst); >> + if (ret) >> + dev_err(dev, "failed to release persist buffers: %d\n", ret); >> + >> + return ret; >> +} >> diff --git a/drivers/media/platform/qcom/vidc/int_bufs.h b/drivers/media/platform/qcom/vidc/int_bufs.h >> new file mode 100644 >> index 000000000000..5f8b2b85839f >> --- /dev/null >> +++ b/drivers/media/platform/qcom/vidc/int_bufs.h >> @@ -0,0 +1,23 @@ >> +/* >> + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. >> + * Copyright (C) 2016 Linaro Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ >> +#ifndef __VIDC_INTERNAL_BUFFERS_H__ >> +#define __VIDC_INTERNAL_BUFFERS_H__ >> + >> +struct vidc_inst; >> + >> +int internal_bufs_alloc(struct vidc_inst *inst); >> +int internal_bufs_free(struct vidc_inst *inst); >> + >> +#endif >> diff --git a/drivers/media/platform/qcom/vidc/load.c b/drivers/media/platform/qcom/vidc/load.c >> new file mode 100644 >> index 000000000000..8ae25fc0e8a5 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/vidc/load.c >> @@ -0,0 +1,104 @@ >> +/* >> + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved. >> + * Copyright (C) 2016 Linaro Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ >> + >> +#include <linux/clk.h> >> + >> +#include "core.h" >> +#include "load.h" >> + >> +static u32 get_inst_load(struct vidc_inst *inst) >> +{ >> + int mbs; >> + u32 w = inst->width; >> + u32 h = inst->height; >> + >> + if (!inst->hfi_inst || !(inst->hfi_inst->state >= INST_INIT && >> + inst->hfi_inst->state < INST_STOP)) >> + return 0; >> + >> + mbs = (ALIGN(w, 16) / 16) * (ALIGN(h, 16) / 16); >> + >> + return mbs * inst->fps; >> +} >> + >> +static u32 get_load(struct vidc_core *core, u32 session_type) >> +{ >> + struct vidc_inst *inst = NULL; >> + u32 mbs_per_sec = 0; >> + >> + mutex_lock(&core->lock); >> + list_for_each_entry(inst, &core->instances, list) { >> + if (inst->session_type != session_type) >> + continue; >> + >> + mbs_per_sec += get_inst_load(inst); >> + } >> + mutex_unlock(&core->lock); >> + >> + return mbs_per_sec; >> +} >> + >> +static int scale_clocks_load(struct vidc_core *core, u32 mbs_per_sec) >> +{ >> + const struct freq_tbl *table = core->res->freq_tbl; >> + int num_rows = core->res->freq_tbl_size; >> + struct clk *clk = core->clks[0]; > > Using individual clk pointers instead of this array would make this > "core_clk" easier to follow. I have decided to use an array of struct clk pointers is that the number of clocks depends on SoC, for example 8096 have 9 clks and I wanted to avoid describing each of them as idividual one. > >> + struct device *dev = core->dev; >> + unsigned long freq = table[0].freq; >> + int ret, i; >> + >> + if (!mbs_per_sec num_row&& s > 1) { >> + freq = table[num_rows - 1].freq; >> + goto set_freq; >> + } > > Here we will set freq to the last entry in the freq table, potentially > table[0] if num_rows == 1, so the second part of the conditional doesn't > add any value and you can skip the early initialization above. Ok I will reconsider this part. > > And you can put the loop below in an else block instead of using a goto. >> + >> + for (i = 0; i < num_rows; i++) { >> + if (mbs_per_sec > table[i].load) >> + break; >> + freq = table[i].freq; >> + } >> + >> +set_freq: >> + >> + ret = clk_set_rate(clk, freq); >> + if (ret) { >> + dev_err(dev, "failed to set clock rate %lu (%d)\n", freq, ret); >> + return ret; >> + } >> + >> + return 0; > > ret will be 0 here, so print the error message conditionally and then > just return ret. > >> +} >> + >> +int vidc_scale_clocks(struct vidc_core *core) > > This is only called from helpers.c, drop this file and move the > implementation there. OK, agreed. > >> +{ >> + struct device *dev = core->dev; >> + u32 mbs_per_sec; >> + int ret; >> + >> + mbs_per_sec = get_load(core, VIDC_SESSION_TYPE_ENC) + >> + get_load(core, VIDC_SESSION_TYPE_DEC); >> + >> + if (mbs_per_sec > core->res->max_load) { >> + dev_warn(dev, "HW is overloaded, needed: %d max: %d\n", >> + mbs_per_sec, core->res->max_load); >> + return -EBUSY; >> + } >> + >> + ret = scale_clocks_load(core, mbs_per_sec); >> + if (ret) >> + dev_warn(dev, "failed to scale clocks, performance might be impacted\n"); >> + >> + return 0; >> +} > [..] >> diff --git a/drivers/media/platform/qcom/vidc/mem.c b/drivers/media/platform/qcom/vidc/mem.c >> new file mode 100644 >> index 000000000000..6a83b5784410 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/vidc/mem.c >> @@ -0,0 +1,64 @@ >> +/* >> + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. >> + * Copyright (C) 2016 Linaro Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ >> + >> +#include <linux/device.h> >> +#include <linux/dma-direction.h> >> +#include <linux/dma-mapping.h> >> +#include <linux/err.h> >> +#include <linux/slab.h> >> + >> +#include "mem.h" >> + >> +struct vidc_mem *mem_alloc(struct device *dev, size_t size, int map_kernel) > > This is a terrible name for a global function. > > But I think you can favorably inline this into the two callers. They > both have their own tracking objects. So just drop this entire file. OK, I will delete it. > >> +{ >> + struct vidc_mem *mem; >> + >> + if (!size) >> + return ERR_PTR(-EINVAL); >> + >> + if (IS_ERR(dev)) >> + return ERR_CAST(dev); >> + >> + mem = kzalloc(sizeof(*mem), GFP_KERNEL); >> + if (!mem) >> + return ERR_PTR(-ENOMEM); >> + >> + mem->size = ALIGN(size, SZ_4K); >> + mem->iommu_dev = dev; >> + >> + mem->attrs = DMA_ATTR_WRITE_COMBINE; >> + >> + if (!map_kernel) >> + mem->attrs |= DMA_ATTR_NO_KERNEL_MAPPING; >> + >> + mem->kvaddr = dma_alloc_attrs(mem->iommu_dev, mem->size, &mem->da, >> + GFP_KERNEL, mem->attrs); >> + if (!mem->kvaddr) { >> + kfree(mem); >> + return ERR_PTR(-ENOMEM); >> + } >> + >> + return mem; >> +} >> + >> +void mem_free(struct vidc_mem *mem) >> +{ >> + if (!mem) >> + return; >> + >> + dma_free_attrs(mem->iommu_dev, mem->size, mem->kvaddr, >> + mem->da, mem->attrs); >> + kfree(mem); >> +}; > [..] >> diff --git a/drivers/media/platform/qcom/vidc/resources.c b/drivers/media/platform/qcom/vidc/resources.c >> new file mode 100644 >> index 000000000000..e00ed1caa824 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/vidc/resources.c >> @@ -0,0 +1,46 @@ >> +/* >> + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. >> + * Copyright (C) 2016 Linaro Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ >> +#include <linux/bug.h> >> +#include <linux/types.h> >> +#include <linux/kernel.h> >> + >> +#include "hfi.h" >> + >> +static const struct freq_tbl msm8916_freq_table[] = { >> + { 352800, 228570000 }, /* 1920x1088 @ 30 + 1280x720 @ 30 */ >> + { 244800, 160000000 }, /* 1920x1088 @ 30 */ >> + { 108000, 100000000 }, /* 1280x720 @ 30 */ >> +}; >> + >> +static const struct reg_val msm8916_reg_preset[] = { >> + { 0xe0020, 0x05555556 }, >> + { 0xe0024, 0x05555556 }, >> + { 0x80124, 0x00000003 }, >> +}; >> + >> +const struct vidc_resources msm8916_res = { >> + .freq_tbl = msm8916_freq_table, >> + .freq_tbl_size = ARRAY_SIZE(msm8916_freq_table), >> + .reg_tbl = msm8916_reg_preset, >> + .reg_tbl_size = ARRAY_SIZE(msm8916_reg_preset), >> + .clks = {"core", "iface", "bus", }, >> + .clks_num = 3, >> + .max_load = 352800, /* 720p@30 + 1080p@30 */ >> + .hfi_version = 0, > > Unused hfi_version is used from vidc_hfi_create() to decide which packetization type "3xx" or "legacy" to use. Currently msm8916 use "legacy" but I guess msm8996 will use "3xx" > >> + .vmem_id = VIDC_RESOURCE_NONE, > > Unused Some of the planed SoCs to support with this driver has this fast video RAM memory, despite that msm8916 has not. > >> + .vmem_size = 0, > > Unused this is for next SoCs which we will support. > >> + .vmem_addr = 0, > > Unused same comment as above. > >> + .dma_mask = 0xddc00000 - 1, >> +}; > > These tables could with favor be moved next to the of_table in vidc.c yes, makes sense.
On Thu 25 Aug 05:59 PDT 2016, Stanimir Varbanov wrote: > Hi Bjorn, > > Thanks for the review and comments! > > On 08/23/2016 05:50 AM, Bjorn Andersson wrote: > > On Mon 22 Aug 06:13 PDT 2016, Stanimir Varbanov wrote: > > > > Hi Stan, > > > >> This adds core part of the vidc driver common helper functions > >> used by encoder and decoder specific files. > > > > I believe "vidc" is short for "video core" and this is not the only > > "video core" from Qualcomm. This driver is the v4l2 <-> hfi interface and > > What other "video core"s do you know? > The blackbird, or "vidc" as we know it in 8064. That one should likely be named "mfc" though. > > uses either two ram based fifos _or_ apr tal for communication with the > > implementation. > > > > In the case of apr, the other side is not the venus core but rather the > > "VIDC" apr service on the Hexagon DSP. In this case the hfi packets are > > encapsulated in apr packets. Although this is not used in 8916 it would > > be nice to be able to add this later... > > OK, you are talking about q6_hfi.c which file is found in msm-3.10 and > maybe older kernel versions. > That's the one. > There is a function vidc_hfi_create() which currently creates venus hfi > interface but it aways could be extended to call q6 DSP specific function. > As the ADSP comes up with a VIDC service (on the applicable platform(s)) we get an APR channel, the concept is similar from then on but rather than putting the messages directly into the venus hfi tx/rx fifos a header is prepended to each hfi message and it's passed to the APR. In the Qualcomm code the hfi ops are picked as vidc_hfi_create() is called, but I think flipping that upside down to have the venus_hfi or q6_hfi be the probe point and then calling the common probe part with an ops struct associated would be more natural. I do however not see a problem with doing such refactoring in the future, I just wanted to bring it up. > > > > > > But I think we should call this driver "hfi" - or at least venus, as > > it's not compatible with e.g the "blackbird" found in 8064, which is > > also called "vidc". > > Do you think that vidc driver for 8064 will ever reach the mainline kernel? > There are strong wishes for it to be supported, so we should take reasonable measures to make sure its possible. > I personally don't like hfi nor venus other suggestions? Does "vidcore" > or "vcore" makes sense? > These names would imply that we intend to have a single driver for all Qualcomm video encoder/decoders. As it's really hard to envision the future, or to argue about 8064 vidc, I would rather see a driver for the hfi based video encoder/decoders. If we in 8996+1 see a non-hfi chip benefit greatly from reusing something from this implementation then those better go into the v4l core or some other common entity. [..] > >> diff --git a/drivers/media/platform/qcom/vidc/core.c b/drivers/media/platform/qcom/vidc/core.c [..] > >> +static void vidc_sys_error_handler(struct work_struct *work) > >> +{ > >> + struct vidc_sys_error *handler = > >> + container_of(work, struct vidc_sys_error, work.work); > >> + struct vidc_core *core = handler->core; > >> + struct hfi_core *hfi = &core->hfi; > >> + struct device *dev = core->dev; > >> + int ret; > >> + > >> + mutex_lock(&hfi->lock); > >> + if (hfi->state != CORE_INVALID) > >> + goto exit; > >> + > >> + mutex_unlock(&hfi->lock); > >> + > >> + ret = vidc_hfi_core_deinit(hfi); > >> + if (ret) > >> + dev_err(dev, "core: deinit failed (%d)\n", ret); > >> + > >> + mutex_lock(&hfi->lock); > >> + > >> + rproc_report_crash(core->rproc, RPROC_FATAL_ERROR); > > > > This operation is async, as such I believe this to be fragile. To get > > the expected result you should be able to simply call > > rproc_shutdown()/rproc_boot() to restart the core... > > OK will remove crash report for now. > > > > > However, if we at any point would like to be able to get memory dumps > > from this core (likely a requirement on the Qualcomm side) we need to > > call rproc_report_crash() and let it collect the resources and then > > power cycle the core. > > > > > > As the life cycle of the venus driver goes 1:1 with the rproc driver I > > think it would be more suitable to make the v4l driver a child of the > > rproc driver and have it probe/remove this driver as the rproc comes and > > goes. This would allow us to call rproc_report_crash() here, we will be > > removed and when the crash is handled (sometime in the future) we will > > be probed again. > > What's the problem with Kconfig "depends on QCOM_VENUS_PIL", isn't that > enough? > I mean in runtime, not compile time. Devices instantiated from this driver does not serve a purpose without either the venus rproc running or the adsp running. Further more as we detect an issue with the remote core these resources (the venus or adsp) will be stopped and restarted - potentially with a long delay inbetween for ramdumping. So either the venus driver must be resilient towards the remote being gone or we should tie the venus driver to the running state of the remoteproc driver. One way to do that would be to add a of_platform_populate() in venus_start() and a of_platform_depopulate() in venus_stop() and represent the hfi-venus driver as a child of the remoteproc driver. > > > >> + > >> + vidc_rproc_shutdown(core); > >> + > >> + ret = vidc_rproc_boot(core); > >> + if (ret) > >> + goto exit; > >> + > >> + hfi->state = CORE_INIT; > >> + > >> +exit: > >> + mutex_unlock(&hfi->lock); > >> + kfree(handler); > >> +} > >> + > >> +static int vidc_event_notify(struct hfi_core *hfi, u32 event) > >> +{ > >> + struct vidc_sys_error *handler; > >> + struct hfi_inst *inst; > >> + > >> + switch (event) { > >> + case EVT_SYS_WATCHDOG_TIMEOUT: > >> + case EVT_SYS_ERROR: > >> + break; > >> + default: > >> + return -EINVAL; > >> + } > >> + > >> + mutex_lock(&hfi->lock); > >> + > >> + hfi->state = CORE_INVALID; > >> + > >> + list_for_each_entry(inst, &hfi->instances, list) { > >> + mutex_lock(&inst->lock); > >> + inst->state = INST_INVALID; > >> + mutex_unlock(&inst->lock); > >> + } > >> + > >> + mutex_unlock(&hfi->lock); > >> + > >> + handler = kzalloc(sizeof(*handler), GFP_KERNEL); > >> + if (!handler) > >> + return -ENOMEM; > >> + > >> + handler->core = container_of(hfi, struct vidc_core, hfi); > >> + INIT_DELAYED_WORK(&handler->work, vidc_sys_error_handler); > >> + > >> + /* > >> + * Sleep for 5 sec to ensure venus has completed any > >> + * pending cache operations. Without this sleep, we see > >> + * device reset when firmware is unloaded after a sys > >> + * error. > >> + */ > >> + schedule_delayed_work(&handler->work, msecs_to_jiffies(5000)); > >> + > >> + return 0; > >> +} > >> + > >> +static const struct hfi_core_ops vidc_core_ops = { > >> + .event_notify = vidc_event_notify, > >> +}; > > > > This is an overly generic way of calling vidc_sys_error_handler(). > > There is no need for having the hfi_core_ops indirections for a single > > op that will only exist in 1 and only 1 variant. > > The .event_notify operation is called by hfi part (in hfi_msgs.c) of the > driver and I don't want break the interface. My idea was to have HFI > part and v4l2 part, and each of these parts taking care of their > specifics. The interface between HFI <-> v4l2 should be immutable and > shoudn't be changed when every new version of the hardware IP rise up. > I'm fine with having the structural split between the two pieces (although I see it serving little purpose), but I don't think it's necessary to use a function pointer interface for something that always only will have a single possible value. > > > > Just replace the two affected event_notify() calls with a direct call to > > this function (and clean it up a bit). > > > >> + [..] > >> + > >> +static irqreturn_t vidc_isr_thread(int irq, void *dev_id) > >> +{ > >> + return vidc_hfi_isr_thread(irq, dev_id); > >> +} > >> + > >> +static irqreturn_t vidc_isr(int irq, void *dev) > >> +{ > >> + return vidc_hfi_isr(irq, dev); > >> +} > > > > These two functions indicates that we're requesting the irq in the wrong > > layer. > > IMO the proper place is platform driver .probe method. > If we squash the vidc-core and hfi-core into one this oddness should go away, right? > > > > Also, these two functions arrives in a later patchset, so I assume this > > doesn't compile... > > That's why I'm adding Makefiles later on patchset. On the other hand I > have splitted the driver by files because I think it is easier for > review. But I might be wrong. > Ok, I'm afraid I don't have any good suggestion to counter this. It's hard to introduce a minimal functional driver for this and then add features - as any "minimal" version is large. Sorry for all the "unused" comments for later patches, I didn't double check those with the end result. > > > >> + [..] > >> + > >> +static int vidc_probe(struct platform_device *pdev) > >> +{ > >> + struct device *dev = &pdev->dev; > >> + struct vidc_core *core; > >> + struct device_node *rproc; > >> + struct resource *r; > >> + int ret; > >> + > >> + core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL); > >> + if (!core) > >> + return -ENOMEM; > >> + > >> + core->dev = dev; > >> + platform_set_drvdata(pdev, core); > >> + > >> + rproc = of_parse_phandle(dev->of_node, "rproc", 0); > >> + if (IS_ERR(rproc)) > >> + return PTR_ERR(rproc); > >> + > >> + core->rproc = rproc_get_by_phandle(rproc->phandle); > > > > FYI, We're hoping to land some patches shortly that will replace this > > with rproc_get(pdev->dev.of_node), looking up an rproc by the standard > > "rprocs" property... > > OK, that looks good. But shoudn't be rproc_get(pdev->dev)? > Sorry, the patches in flight introduces of_rproc_get() - not rproc_get(), which takes a device_node. But it's still pending some DT discussions. > > > >> + if (IS_ERR(core->rproc)) > >> + return PTR_ERR(core->rproc); > >> + else if (!core->rproc) > >> + return -EPROBE_DEFER; > > > > We're cleaning up this in the core as well. > > > > You need to rproc_put() the rproc pointer after this point. > > OK, good. When those changes landed I will rework this part. > > > > > > > My question still stands though, if this driver should be probed as the > > remoteproc is booted (or the apr service appearing). I will continue to > > look at that. > > I might be misunderstood your point here. Is your concern related to > EPROBE_DEFFER or some sort of ordering issue in rproc? > > Currently the vidc depends on QCOM_VENUS_PIL in Kconfig, so modprobe > vidc should modprobe remoteproc driver cause it depends on it. > If we probe the two drivers independently, then you're good. I just don't see how we (sanely) will communicate the information about when the remote goes down and comes up related to a crash. > > > >> + > >> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> + core->base = devm_ioremap_resource(dev, r); > >> + if (IS_ERR(core->base)) > >> + return PTR_ERR(core->base); > >> + > >> + core->irq = platform_get_irq(pdev, 0); > >> + if (core->irq < 0) > >> + return core->irq; > >> + > >> + core->res = of_device_get_match_data(dev); > >> + if (!core->res) > >> + return -ENODEV; > >> + > >> + ret = vidc_clks_get(core, core->res->clks_num, core->res->clks); > >> + if (ret) > >> + return ret; > >> + > >> + ret = dma_set_mask_and_coherent(dev, core->res->dma_mask); > >> + if (ret) > >> + return ret; > >> + > >> + INIT_LIST_HEAD(&core->instances); > >> + mutex_init(&core->lock); > >> + > >> + ret = devm_request_threaded_irq(dev, core->irq, vidc_isr, > >> + vidc_isr_thread, > >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > > > > Drop this IRQF_TRIGGER_HIGH and have this be specified in devicetree. > > OK can do that, but is there a convention about who is populating the > flag and what is the precedence dt against the driver? > It seems there's a push lately for moving to getting the trigger from DT. But I haven't not studied the details or reasoning behind it. > > > >> + "vidc", &core->hfi); > >> + if (ret) > >> + return ret; > >> + > >> + core->hfi.core_ops = &vidc_core_ops; > >> + core->hfi.dev = dev; > >> + > >> + ret = vidc_hfi_create(&core->hfi, core->res, core->base); > >> + if (ret) > >> + return ret; > >> + > >> + ret = vidc_clks_enable(core, core->res); > >> + if (ret) > >> + goto err_hfi_destroy; > >> + > >> + ret = vidc_rproc_boot(core); > >> + if (ret) { > >> + vidc_clks_disable(core, core->res); > >> + goto err_hfi_destroy; > >> + } > >> + > >> + pm_runtime_enable(dev); > >> + > >> + ret = pm_runtime_get_sync(dev); > >> + if (ret < 0) > >> + goto err_runtime_disable; > >> + > >> + ret = vidc_hfi_core_init(&core->hfi); > >> + if (ret) > >> + goto err_rproc_shutdown; > >> + > >> + ret = pm_runtime_put_sync(dev); > >> + if (ret) > >> + goto err_core_deinit; > >> + > >> + vidc_clks_disable(core, core->res); > > > > These operations follow the general pattern of booting other Qualcomm > > remoteprocs; acquire and enable some resources, boot the core and > > disable the resources. Therefor it looks quite likely that these > > operations are related to the life cycle of the venus core, rather than > > hfi. > > So you saying that > I'm saying that I would like for the rproc driver to be able to boot, shutdown and handle a crash (not trigger) without having to depend on the venus driver setting up a bunch of resources for it. Either that or the venus rproc driver should not be a standalone rproc driver - with its own probe function. > > > >> + > >> + ret = v4l2_device_register(dev, &core->v4l2_dev); > >> + if (ret) > >> + goto err_core_deinit; > >> + > >> + ret = vdec_init(core, &core->vdev_dec); > >> + if (ret) > >> + goto err_dev_unregister; > >> + > >> + ret = venc_init(core, &core->vdev_enc); > >> + if (ret) > >> + goto err_vdec_deinit; > >> + > >> + return 0; > >> + > >> +err_vdec_deinit: > >> + vdec_deinit(core, &core->vdev_dec); > >> +err_dev_unregister: > >> + v4l2_device_unregister(&core->v4l2_dev); > >> +err_core_deinit: > >> + vidc_hfi_core_deinit(&core->hfi); > >> +err_rproc_shutdown: > >> + vidc_rproc_shutdown(core); > >> +err_runtime_disable: > >> + pm_runtime_set_suspended(dev); > >> + pm_runtime_disable(dev); > >> +err_hfi_destroy: > >> + vidc_hfi_destroy(&core->hfi); > >> + return ret; > >> +} > >> + > >> +static int vidc_remove(struct platform_device *pdev) > >> +{ > >> + struct vidc_core *core = platform_get_drvdata(pdev); > >> + int ret; > >> + > >> + ret = pm_runtime_get_sync(&pdev->dev); > >> + if (ret < 0) > >> + return ret; > > > > No-one cares about you returning an error here, so you better move > > forward and release as much of your resources as possible even though > > you didn't get your pm. > > Hmm, I don't agree here. The runtime_resume will enable clocks (for > example venus iface clk) and if it fails the subsequent call to > vidc_rproc_shutdown can crash badly. > Sorry for not being clear, that part we care about. What I meant was that in the device core will dismantle your driver regardless of you returning an error here. So you will leak the resources not freed below. > > > >> + > >> + ret = vidc_hfi_core_deinit(&core->hfi); > >> + if (ret) { > >> + pm_runtime_put_sync(&pdev->dev); > >> + return ret; > >> + } > >> + > >> + vidc_rproc_shutdown(core); > >> + > >> + ret = pm_runtime_put_sync(&pdev->dev); > >> + > >> + vidc_hfi_destroy(&core->hfi); > >> + vdec_deinit(core, &core->vdev_dec); > >> + venc_deinit(core, &core->vdev_enc); > >> + v4l2_device_unregister(&core->v4l2_dev); > >> + > >> + pm_runtime_disable(core->dev); > >> + > >> + return ret < 0 ? ret : 0; > >> +} > >> + [..] > >> diff --git a/drivers/media/platform/qcom/vidc/core.h b/drivers/media/platform/qcom/vidc/core.h [..] > > > > >> + > >> +struct vidc_list { > >> + struct list_head list; > >> + struct mutex lock; > >> +}; > > > > Can't we get away without passing around lockable lists? Does these > > lists have to be locked independently and should we really pass around > > their lock with them? > > I guess it is possible but didn't spent to much time on that (I had more > important problems to solve with downstream driver). So the answer is > yes and I have to re-consider it. > We with rework of internal buffers this should be less of a problem, you could with favor grab the one lock, allocate all internal buffers and then unlock the lock again. [..] > >> diff --git a/drivers/media/platform/qcom/vidc/helpers.c b/drivers/media/platform/qcom/vidc/helpers.c [..] > >> +int vidc_buf_descs(struct vidc_inst *inst, u32 type, > >> + struct hfi_buffer_requirements *out) > > > > If you call this vidc_get_buf_requirements() it would actually describe > > what's going on. But why is this hfi wrapper in the core, rather than > > the original name of this function was similar to what you suggest but I > decided in last-minute cleanup to shorten its name. > vidc_get_bufreq() should be short enough :) > > just have the internal buffer manager call it directly. > > It is in the core cause I used it on few places to gather buffer count > needed depending on parameters (resolution, codec). Good example is > vb2_ops::queue_setup where I need to return num_buffers depending on > resolution, codec, bitrate, framerate and so on. > Ok, that makes sense. > > > > The call doesn't seem to depend on the parameters or state, can we > > cache the result? > > No, we cannot. see above comment. Something more the scratch and > prersist buffer sizes can also be changed by the firmware depending on > above proparties. > Okay, I figured that might be the case. > > > >> +{ > >> + struct hfi_core *hfi = &inst->core->hfi; > >> + u32 ptype = HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS; > >> + union hfi_get_property hprop; > >> + int ret, i; > >> + > >> + if (out) > >> + memset(out, 0, sizeof(*out)); > >> + > >> + ret = vidc_hfi_session_get_property(hfi, inst->hfi_inst, ptype, &hprop); > >> + if (ret) > >> + return ret; > >> + > >> + ret = -EINVAL; > >> + > >> + for (i = 0; i < HFI_BUFFER_TYPE_MAX; i++) { > >> + if (hprop.bufreq[i].type != type) > >> + continue; > >> + > >> + if (out) > >> + memcpy(out, &hprop.bufreq[i], sizeof(*out)); > >> + ret = 0; > >> + break; > >> + } > >> + > >> + return ret; > >> +} > >> + > > [..] > >> + > >> +void vidc_vb2_stop_streaming(struct vb2_queue *q) > >> +{ > >> + struct vidc_inst *inst = vb2_get_drv_priv(q); > >> + struct hfi_inst *hfi_inst = inst->hfi_inst; > >> + struct vidc_core *core = inst->core; > >> + struct device *dev = core->dev; > >> + struct hfi_core *hfi = &core->hfi; > >> + int ret, streamoff; > >> + > >> + mutex_lock(&inst->lock); > >> + streamoff = inst->streamoff; > >> + mutex_unlock(&inst->lock); > >> + > >> + if (streamoff) > >> + return; > >> + > >> + mutex_lock(&inst->lock); > >> + if (inst->streamon == 0) { > >> + mutex_unlock(&inst->lock); > >> + return; > >> + } > >> + mutex_unlock(&inst->lock); > > > > Why do we keep track of streamon and stream off, why isn't streamoff > > ever cleared? Why don't we check both conditions in one critical region? > > Probably cause its buggy, I will sort it out. > > > > >> + > >> + ret = vidc_hfi_session_stop(hfi, inst->hfi_inst); > >> + if (ret) { > >> + dev_err(dev, "session: stop failed (%d)\n", ret); > >> + goto abort; > > > > When are we going to relaim the buffers in these cases? > > session_stop will instruct the firmware return buffers to the v4l2 > driver through hfi_inst_ops empty_buf_done and fill_buf_done, those > operations will call vb2_buffer_done. > Okay, but does that include the internal buffers as well? > > > >> + } > >> + > >> + ret = vidc_hfi_session_unload_res(hfi, inst->hfi_inst); > >> + if (ret) { > >> + dev_err(dev, "session: release resources failed (%d)\n", ret); > >> + goto abort; > >> + } > >> + > >> + ret = session_unregister_bufs(inst); > >> + if (ret) { > >> + dev_err(dev, "failed to release capture buffers: %d\n", ret); > >> + goto abort; > >> + } > >> + > >> + ret = internal_bufs_free(inst); > >> + > >> + if (hfi_inst->state == INST_INVALID || hfi->state == CORE_INVALID) { > >> + ret = -EINVAL; > >> + goto abort; > >> + } > >> + > >> +abort: > >> + if (ret) > >> + vidc_hfi_session_abort(hfi, inst->hfi_inst); > >> + > >> + vidc_scale_clocks(inst->core); > >> + > >> + ret = vidc_hfi_session_deinit(hfi, inst->hfi_inst); > >> + > >> + mutex_lock(&inst->lock); > >> + inst->streamoff = 1; > >> + mutex_unlock(&inst->lock); > >> + > >> + if (ret) > >> + dev_err(dev, "stop streaming failed type: %d, ret: %d\n", > >> + q->type, ret); > >> + > >> + ret = pm_runtime_put_sync(dev); > >> + if (ret < 0) > >> + dev_err(dev, "%s: pm_runtime_put_sync (%d)\n", __func__, ret); > >> +} > >> + [..] > >> diff --git a/drivers/media/platform/qcom/vidc/helpers.h b/drivers/media/platform/qcom/vidc/helpers.h [..] > >> +static int scratch_set_buffers(struct vidc_inst *inst) > >> +{ > >> + struct device *dev = inst->core->dev; > >> + int ret; > >> + > >> + ret = scratch_unset_buffers(inst, true); > >> + if (ret) > >> + dev_warn(dev, "Failed to release scratch buffers\n"); > > > > internal_bufs_free() calls scratch_unset_buffers(reuse=false) so we're > > coming here with an empty scratchbufs either way - meaning that this > > whole file can be greatly simplified. > > > > So instead of trying to fix that I would suggest that you just let > > internal_bufs_alloc() acquire the buffer requirements and call > > internal_alloc_and_set() directly, storing the result in a single list. > > > > And then inline a free method in internal_bufs_free() as well as drop > > all reuse-stuff and unused/dead code. > > > > That would simplify this file quite a bit and if there actually is a > > need for the reusing of buffer that can be added at some later time. > > > > Actially I thought about droping the reuse stuff in the past, so I agree > on that cleanup. The thing which worries me is the size of those buffers > (the biggest is 10-15MB) and also the allocation time. Currently those > buffers are allocate on streamon time, but probably the right place is > on request_buf time. > I think this is definitely worth investigating and should be possible to add incrementally once the driver has landed. As far as I can see the code doesn't really work as it is now, so rather than fixing that make it dead-simple and then add to it later. > >> + > >> + ret = scratch_set_buffer(inst, HFI_BUFFER_INTERNAL_SCRATCH); > >> + if (ret) > >> + goto error; > >> + > >> + ret = scratch_set_buffer(inst, HFI_BUFFER_INTERNAL_SCRATCH_1); > >> + if (ret) > >> + goto error; > >> + > >> + ret = scratch_set_buffer(inst, HFI_BUFFER_INTERNAL_SCRATCH_2); > >> + if (ret) > >> + goto error; > >> + > >> + return 0; > >> +error: > >> + scratch_unset_buffers(inst, false); > >> + return ret; > >> +} > >> + Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/platform/qcom/vidc/core.c b/drivers/media/platform/qcom/vidc/core.c new file mode 100644 index 000000000000..e005be178fc0 --- /dev/null +++ b/drivers/media/platform/qcom/vidc/core.c @@ -0,0 +1,548 @@ +/* + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. + * Copyright (C) 2016 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ +#include <linux/clk.h> +#include <linux/init.h> +#include <linux/ioctl.h> +#include <linux/list.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/types.h> +#include <linux/remoteproc.h> +#include <linux/pm_runtime.h> +#include <media/videobuf2-v4l2.h> +#include <media/v4l2-ioctl.h> + +#include "core.h" +#include "resources.h" +#include "vdec.h" +#include "venc.h" + +static void vidc_add_inst(struct vidc_core *core, struct vidc_inst *inst) +{ + mutex_lock(&core->lock); + list_add_tail(&inst->list, &core->instances); + mutex_unlock(&core->lock); +} + +static void vidc_del_inst(struct vidc_core *core, struct vidc_inst *inst) +{ + struct vidc_inst *pos, *n; + + mutex_lock(&core->lock); + list_for_each_entry_safe(pos, n, &core->instances, list) { + if (pos == inst) + list_del(&inst->list); + } + mutex_unlock(&core->lock); +} + +static int vidc_rproc_boot(struct vidc_core *core) +{ + int ret; + + if (core->rproc_booted) + return 0; + + ret = rproc_boot(core->rproc); + if (ret) + return ret; + + core->rproc_booted = true; + + return 0; +} + +static void vidc_rproc_shutdown(struct vidc_core *core) +{ + if (!core->rproc_booted) + return; + + rproc_shutdown(core->rproc); + core->rproc_booted = false; +} + +struct vidc_sys_error { + struct vidc_core *core; + struct delayed_work work; +}; + +static void vidc_sys_error_handler(struct work_struct *work) +{ + struct vidc_sys_error *handler = + container_of(work, struct vidc_sys_error, work.work); + struct vidc_core *core = handler->core; + struct hfi_core *hfi = &core->hfi; + struct device *dev = core->dev; + int ret; + + mutex_lock(&hfi->lock); + if (hfi->state != CORE_INVALID) + goto exit; + + mutex_unlock(&hfi->lock); + + ret = vidc_hfi_core_deinit(hfi); + if (ret) + dev_err(dev, "core: deinit failed (%d)\n", ret); + + mutex_lock(&hfi->lock); + + rproc_report_crash(core->rproc, RPROC_FATAL_ERROR); + + vidc_rproc_shutdown(core); + + ret = vidc_rproc_boot(core); + if (ret) + goto exit; + + hfi->state = CORE_INIT; + +exit: + mutex_unlock(&hfi->lock); + kfree(handler); +} + +static int vidc_event_notify(struct hfi_core *hfi, u32 event) +{ + struct vidc_sys_error *handler; + struct hfi_inst *inst; + + switch (event) { + case EVT_SYS_WATCHDOG_TIMEOUT: + case EVT_SYS_ERROR: + break; + default: + return -EINVAL; + } + + mutex_lock(&hfi->lock); + + hfi->state = CORE_INVALID; + + list_for_each_entry(inst, &hfi->instances, list) { + mutex_lock(&inst->lock); + inst->state = INST_INVALID; + mutex_unlock(&inst->lock); + } + + mutex_unlock(&hfi->lock); + + handler = kzalloc(sizeof(*handler), GFP_KERNEL); + if (!handler) + return -ENOMEM; + + handler->core = container_of(hfi, struct vidc_core, hfi); + INIT_DELAYED_WORK(&handler->work, vidc_sys_error_handler); + + /* + * Sleep for 5 sec to ensure venus has completed any + * pending cache operations. Without this sleep, we see + * device reset when firmware is unloaded after a sys + * error. + */ + schedule_delayed_work(&handler->work, msecs_to_jiffies(5000)); + + return 0; +} + +static const struct hfi_core_ops vidc_core_ops = { + .event_notify = vidc_event_notify, +}; + +static int vidc_open(struct file *file) +{ + struct video_device *vdev = video_devdata(file); + struct vidc_core *core = video_drvdata(file); + struct vidc_inst *inst; + int ret = 0; + + inst = kzalloc(sizeof(*inst), GFP_KERNEL); + if (!inst) + return -ENOMEM; + + mutex_init(&inst->lock); + + INIT_VIDC_LIST(&inst->scratchbufs); + INIT_VIDC_LIST(&inst->persistbufs); + INIT_VIDC_LIST(&inst->registeredbufs); + + INIT_LIST_HEAD(&inst->bufqueue); + mutex_init(&inst->bufqueue_lock); + + if (vdev == &core->vdev_dec) + inst->session_type = VIDC_SESSION_TYPE_DEC; + else + inst->session_type = VIDC_SESSION_TYPE_ENC; + + inst->core = core; + + if (inst->session_type == VIDC_SESSION_TYPE_DEC) + ret = vdec_open(inst); + else + ret = venc_open(inst); + + if (ret) + goto err_free_inst; + + if (inst->session_type == VIDC_SESSION_TYPE_DEC) + v4l2_fh_init(&inst->fh, &core->vdev_dec); + else + v4l2_fh_init(&inst->fh, &core->vdev_enc); + + inst->fh.ctrl_handler = &inst->ctrl_handler; + + v4l2_fh_add(&inst->fh); + + file->private_data = &inst->fh; + + vidc_add_inst(core, inst); + + return 0; + +err_free_inst: + kfree(inst); + return ret; +} + +static int vidc_close(struct file *file) +{ + struct vidc_inst *inst = to_inst(file); + struct vidc_core *core = inst->core; + + if (inst->session_type == VIDC_SESSION_TYPE_DEC) + vdec_close(inst); + else + venc_close(inst); + + vidc_del_inst(core, inst); + + mutex_destroy(&inst->bufqueue_lock); + mutex_destroy(&inst->scratchbufs.lock); + mutex_destroy(&inst->persistbufs.lock); + mutex_destroy(&inst->registeredbufs.lock); + + v4l2_fh_del(&inst->fh); + v4l2_fh_exit(&inst->fh); + + kfree(inst); + return 0; +} + +static unsigned int vidc_poll(struct file *file, struct poll_table_struct *pt) +{ + struct vidc_inst *inst = to_inst(file); + struct vb2_queue *outq = &inst->bufq_out; + struct vb2_queue *capq = &inst->bufq_cap; + unsigned int ret; + + ret = vb2_poll(outq, file, pt); + ret |= vb2_poll(capq, file, pt); + + return ret; +} + +static int vidc_mmap(struct file *file, struct vm_area_struct *vma) +{ + struct vidc_inst *inst = to_inst(file); + unsigned long offset = vma->vm_pgoff << PAGE_SHIFT; + int ret; + + if (offset < DST_QUEUE_OFF_BASE) { + ret = vb2_mmap(&inst->bufq_out, vma); + } else { + vma->vm_pgoff -= DST_QUEUE_OFF_BASE >> PAGE_SHIFT; + ret = vb2_mmap(&inst->bufq_cap, vma); + } + + return ret; +} + +const struct v4l2_file_operations vidc_fops = { + .owner = THIS_MODULE, + .open = vidc_open, + .release = vidc_close, + .unlocked_ioctl = video_ioctl2, + .poll = vidc_poll, + .mmap = vidc_mmap, +#ifdef CONFIG_COMPAT + .compat_ioctl32 = v4l2_compat_ioctl32, +#endif +}; + +static irqreturn_t vidc_isr_thread(int irq, void *dev_id) +{ + return vidc_hfi_isr_thread(irq, dev_id); +} + +static irqreturn_t vidc_isr(int irq, void *dev) +{ + return vidc_hfi_isr(irq, dev); +} + +static int vidc_clks_get(struct vidc_core *core, unsigned int clks_num, + const char * const *clks_id) +{ + struct device *dev = core->dev; + unsigned int i; + + for (i = 0; i < clks_num; i++) { + core->clks[i] = devm_clk_get(dev, clks_id[i]); + if (IS_ERR(core->clks[i])) + return PTR_ERR(core->clks[i]); + } + + return 0; +} + +static int +vidc_clks_enable(struct vidc_core *core, const struct vidc_resources *res) +{ + unsigned int i; + int ret; + + for (i = 0; i < res->clks_num; i++) { + ret = clk_prepare_enable(core->clks[i]); + if (ret) + goto err; + } + + return 0; +err: + while (--i) + clk_disable_unprepare(core->clks[i]); + + return ret; +} + +static void +vidc_clks_disable(struct vidc_core *core, const struct vidc_resources *res) +{ + unsigned int i; + + for (i = 0; i < res->clks_num; i++) + clk_disable_unprepare(core->clks[i]); +} + +static const struct of_device_id vidc_dt_match[] = { + { .compatible = "qcom,vidc-msm8916", .data = &msm8916_res, }, + { } +}; + +MODULE_DEVICE_TABLE(of, vidc_dt_match); + +static int vidc_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct vidc_core *core; + struct device_node *rproc; + struct resource *r; + int ret; + + core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL); + if (!core) + return -ENOMEM; + + core->dev = dev; + platform_set_drvdata(pdev, core); + + rproc = of_parse_phandle(dev->of_node, "rproc", 0); + if (IS_ERR(rproc)) + return PTR_ERR(rproc); + + core->rproc = rproc_get_by_phandle(rproc->phandle); + if (IS_ERR(core->rproc)) + return PTR_ERR(core->rproc); + else if (!core->rproc) + return -EPROBE_DEFER; + + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + core->base = devm_ioremap_resource(dev, r); + if (IS_ERR(core->base)) + return PTR_ERR(core->base); + + core->irq = platform_get_irq(pdev, 0); + if (core->irq < 0) + return core->irq; + + core->res = of_device_get_match_data(dev); + if (!core->res) + return -ENODEV; + + ret = vidc_clks_get(core, core->res->clks_num, core->res->clks); + if (ret) + return ret; + + ret = dma_set_mask_and_coherent(dev, core->res->dma_mask); + if (ret) + return ret; + + INIT_LIST_HEAD(&core->instances); + mutex_init(&core->lock); + + ret = devm_request_threaded_irq(dev, core->irq, vidc_isr, + vidc_isr_thread, + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, + "vidc", &core->hfi); + if (ret) + return ret; + + core->hfi.core_ops = &vidc_core_ops; + core->hfi.dev = dev; + + ret = vidc_hfi_create(&core->hfi, core->res, core->base); + if (ret) + return ret; + + ret = vidc_clks_enable(core, core->res); + if (ret) + goto err_hfi_destroy; + + ret = vidc_rproc_boot(core); + if (ret) { + vidc_clks_disable(core, core->res); + goto err_hfi_destroy; + } + + pm_runtime_enable(dev); + + ret = pm_runtime_get_sync(dev); + if (ret < 0) + goto err_runtime_disable; + + ret = vidc_hfi_core_init(&core->hfi); + if (ret) + goto err_rproc_shutdown; + + ret = pm_runtime_put_sync(dev); + if (ret) + goto err_core_deinit; + + vidc_clks_disable(core, core->res); + + ret = v4l2_device_register(dev, &core->v4l2_dev); + if (ret) + goto err_core_deinit; + + ret = vdec_init(core, &core->vdev_dec); + if (ret) + goto err_dev_unregister; + + ret = venc_init(core, &core->vdev_enc); + if (ret) + goto err_vdec_deinit; + + return 0; + +err_vdec_deinit: + vdec_deinit(core, &core->vdev_dec); +err_dev_unregister: + v4l2_device_unregister(&core->v4l2_dev); +err_core_deinit: + vidc_hfi_core_deinit(&core->hfi); +err_rproc_shutdown: + vidc_rproc_shutdown(core); +err_runtime_disable: + pm_runtime_set_suspended(dev); + pm_runtime_disable(dev); +err_hfi_destroy: + vidc_hfi_destroy(&core->hfi); + return ret; +} + +static int vidc_remove(struct platform_device *pdev) +{ + struct vidc_core *core = platform_get_drvdata(pdev); + int ret; + + ret = pm_runtime_get_sync(&pdev->dev); + if (ret < 0) + return ret; + + ret = vidc_hfi_core_deinit(&core->hfi); + if (ret) { + pm_runtime_put_sync(&pdev->dev); + return ret; + } + + vidc_rproc_shutdown(core); + + ret = pm_runtime_put_sync(&pdev->dev); + + vidc_hfi_destroy(&core->hfi); + vdec_deinit(core, &core->vdev_dec); + venc_deinit(core, &core->vdev_enc); + v4l2_device_unregister(&core->v4l2_dev); + + pm_runtime_disable(core->dev); + + return ret < 0 ? ret : 0; +} + +static int vidc_runtime_suspend(struct device *dev) +{ + struct vidc_core *core = dev_get_drvdata(dev); + int ret; + + ret = vidc_hfi_core_suspend(&core->hfi); + + vidc_clks_disable(core, core->res); + + return ret; +} + +static int vidc_runtime_resume(struct device *dev) +{ + struct vidc_core *core = dev_get_drvdata(dev); + int ret; + + ret = vidc_clks_enable(core, core->res); + if (ret) + return ret; + + return vidc_hfi_core_resume(&core->hfi); +} + +static int vidc_pm_suspend(struct device *dev) +{ + return vidc_runtime_suspend(dev); +} + +static int vidc_pm_resume(struct device *dev) +{ + return vidc_runtime_resume(dev); +} + +static const struct dev_pm_ops vidc_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(vidc_pm_suspend, vidc_pm_resume) + SET_RUNTIME_PM_OPS(vidc_runtime_suspend, vidc_runtime_resume, NULL) +}; + +static struct platform_driver qcom_vidc_driver = { + .probe = vidc_probe, + .remove = vidc_remove, + .driver = { + .name = "qcom-vidc", + .of_match_table = vidc_dt_match, + .pm = &vidc_pm_ops, + }, +}; + +module_platform_driver(qcom_vidc_driver); + +MODULE_ALIAS("platform:qcom-vidc"); +MODULE_DESCRIPTION("Qualcomm video encoder and decoder driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/media/platform/qcom/vidc/core.h b/drivers/media/platform/qcom/vidc/core.h new file mode 100644 index 000000000000..5dc8e05f8c36 --- /dev/null +++ b/drivers/media/platform/qcom/vidc/core.h @@ -0,0 +1,196 @@ +/* + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. + * Copyright (C) 2016 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#ifndef __VIDC_CORE_H_ +#define __VIDC_CORE_H_ + +#include <media/v4l2-device.h> +#include <media/v4l2-ctrls.h> +#include <media/videobuf2-core.h> + +#include "resources.h" +#include "hfi.h" + +#define VIDC_DRV_NAME "vidc" + +struct vidc_list { + struct list_head list; + struct mutex lock; +}; + +struct vidc_format { + u32 pixfmt; + int num_planes; + u32 type; +}; + +struct vidc_core { + struct list_head list; + void __iomem *base; + int irq; + struct clk *clks[VIDC_CLKS_NUM_MAX]; + struct mutex lock; + struct hfi_core hfi; + struct video_device vdev_dec; + struct video_device vdev_enc; + struct v4l2_device v4l2_dev; + struct list_head instances; + const struct vidc_resources *res; + struct rproc *rproc; + bool rproc_booted; + struct device *dev; +}; + +struct vdec_controls { + u32 post_loop_deb_mode; + u32 profile; + u32 level; +}; + +struct venc_controls { + u16 gop_size; + u32 idr_period; + u32 num_p_frames; + u32 num_b_frames; + u32 bitrate_mode; + u32 bitrate; + u32 bitrate_peak; + + u32 h264_i_period; + u32 h264_entropy_mode; + u32 h264_i_qp; + u32 h264_p_qp; + u32 h264_b_qp; + u32 h264_min_qp; + u32 h264_max_qp; + u32 h264_loop_filter_mode; + u32 h264_loop_filter_alpha; + u32 h264_loop_filter_beta; + + u32 vp8_min_qp; + u32 vp8_max_qp; + + u32 multi_slice_mode; + u32 multi_slice_max_bytes; + u32 multi_slice_max_mb; + + u32 header_mode; + + u32 profile; + u32 level; +}; + +struct vidc_inst { + struct list_head list; + struct mutex lock; + struct vidc_core *core; + + struct vidc_list scratchbufs; + struct vidc_list persistbufs; + struct vidc_list registeredbufs; + + struct list_head bufqueue; + struct mutex bufqueue_lock; + + int streamoff; + int streamon; + struct vb2_queue bufq_out; + struct vb2_queue bufq_cap; + + struct v4l2_ctrl_handler ctrl_handler; + union { + struct vdec_controls dec; + struct venc_controls enc; + } controls; + struct v4l2_fh fh; + + struct hfi_inst *hfi_inst; + + /* session fields */ + u32 session_type; + u32 width; + u32 height; + u32 out_width; + u32 out_height; + u32 colorspace; + u8 ycbcr_enc; + u8 quantization; + u8 xfer_func; + u64 fps; + struct v4l2_fract timeperframe; + const struct vidc_format *fmt_out; + const struct vidc_format *fmt_cap; + unsigned int num_input_bufs; + unsigned int num_output_bufs; + bool in_reconfig; + u32 reconfig_width; + u32 reconfig_height; + u64 sequence; +}; + +#define ctrl_to_inst(ctrl) \ + container_of(ctrl->handler, struct vidc_inst, ctrl_handler) + +struct vidc_ctrl { + u32 id; + enum v4l2_ctrl_type type; + s32 min; + s32 max; + s32 def; + u32 step; + u64 menu_skip_mask; + u32 flags; + const char * const *qmenu; +}; + +/* + * Offset base for buffers on the destination queue - used to distinguish + * between source and destination buffers when mmapping - they receive the same + * offsets but for different queues + */ +#define DST_QUEUE_OFF_BASE (1 << 30) + +extern const struct v4l2_file_operations vidc_fops; + +static inline void INIT_VIDC_LIST(struct vidc_list *mlist) +{ + mutex_init(&mlist->lock); + INIT_LIST_HEAD(&mlist->list); +} + +static inline struct vidc_inst *to_inst(struct file *filp) +{ + return container_of(filp->private_data, struct vidc_inst, fh); +} + +static inline struct hfi_inst *to_hfi_inst(struct file *filp) +{ + return to_inst(filp)->hfi_inst; +} + +static inline struct vb2_queue * +vidc_to_vb2q(struct file *file, enum v4l2_buf_type type) +{ + struct vidc_inst *inst = to_inst(file); + + if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) + return &inst->bufq_cap; + else if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) + return &inst->bufq_out; + + return NULL; +} + +#endif diff --git a/drivers/media/platform/qcom/vidc/helpers.c b/drivers/media/platform/qcom/vidc/helpers.c new file mode 100644 index 000000000000..81079f2b5ed1 --- /dev/null +++ b/drivers/media/platform/qcom/vidc/helpers.c @@ -0,0 +1,394 @@ +/* + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. + * Copyright (C) 2016 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ +#include <linux/list.h> +#include <linux/mutex.h> +#include <linux/pm_runtime.h> +#include <media/videobuf2-dma-sg.h> + +#include "helpers.h" +#include "int_bufs.h" +#include "load.h" +#include "hfi_helper.h" + +static int session_set_buf(struct vb2_buffer *vb) +{ + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); + struct vb2_queue *q = vb->vb2_queue; + struct vidc_inst *inst = vb2_get_drv_priv(q); + struct vidc_core *core = inst->core; + struct device *dev = core->dev; + struct hfi_core *hfi = &core->hfi; + struct vidc_buffer *buf = to_vidc_buffer(vbuf); + struct hfi_frame_data fdata; + int ret; + + memset(&fdata, 0, sizeof(fdata)); + + fdata.alloc_len = vb2_plane_size(vb, 0); + fdata.device_addr = buf->dma_addr; + fdata.timestamp = vb->timestamp; + fdata.flags = 0; + fdata.clnt_data = buf->dma_addr; + + if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { + fdata.buffer_type = HFI_BUFFER_INPUT; + fdata.filled_len = vb2_get_plane_payload(vb, 0); + fdata.offset = vb->planes[0].data_offset; + + if (vbuf->flags & V4L2_BUF_FLAG_LAST || !fdata.filled_len) + fdata.flags |= HFI_BUFFERFLAG_EOS; + + ret = vidc_hfi_session_etb(hfi, inst->hfi_inst, &fdata); + } else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { + fdata.buffer_type = HFI_BUFFER_OUTPUT; + fdata.filled_len = 0; + fdata.offset = 0; + + ret = vidc_hfi_session_ftb(hfi, inst->hfi_inst, &fdata); + } else { + ret = -EINVAL; + } + + if (ret) { + dev_err(dev, "failed to set session buffer (%d)\n", ret); + return ret; + } + + return 0; +} + +static int session_unregister_bufs(struct vidc_inst *inst) +{ + struct device *dev = inst->core->dev; + struct hfi_core *hfi = &inst->core->hfi; + struct hfi_buffer_desc *bd; + struct vidc_buffer *buf, *tmp; + int ret = 0; + + mutex_lock(&inst->registeredbufs.lock); + list_for_each_entry_safe(buf, tmp, &inst->registeredbufs.list, + hfi_list) { + list_del(&buf->hfi_list); + bd = &buf->bd; + bd->response_required = 1; + ret = vidc_hfi_session_unset_buffers(hfi, inst->hfi_inst, bd); + if (ret) { + dev_err(dev, "%s: session release buffers failed\n", + __func__); + break; + } + } + mutex_unlock(&inst->registeredbufs.lock); + + return ret; +} + +static int session_register_bufs(struct vidc_inst *inst) +{ + struct device *dev = inst->core->dev; + struct hfi_core *hfi = &inst->core->hfi; + struct hfi_buffer_desc *bd; + struct vidc_buffer *buf, *tmp; + int ret = 0; + + mutex_lock(&inst->registeredbufs.lock); + list_for_each_entry_safe(buf, tmp, &inst->registeredbufs.list, + hfi_list) { + bd = &buf->bd; + ret = vidc_hfi_session_set_buffers(hfi, inst->hfi_inst, bd); + if (ret) { + dev_err(dev, "%s: session: set buffer failed\n", + __func__); + break; + } + } + mutex_unlock(&inst->registeredbufs.lock); + + return ret; +} + +int vidc_buf_descs(struct vidc_inst *inst, u32 type, + struct hfi_buffer_requirements *out) +{ + struct hfi_core *hfi = &inst->core->hfi; + u32 ptype = HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS; + union hfi_get_property hprop; + int ret, i; + + if (out) + memset(out, 0, sizeof(*out)); + + ret = vidc_hfi_session_get_property(hfi, inst->hfi_inst, ptype, &hprop); + if (ret) + return ret; + + ret = -EINVAL; + + for (i = 0; i < HFI_BUFFER_TYPE_MAX; i++) { + if (hprop.bufreq[i].type != type) + continue; + + if (out) + memcpy(out, &hprop.bufreq[i], sizeof(*out)); + ret = 0; + break; + } + + return ret; +} + +int vidc_set_color_format(struct vidc_inst *inst, u32 type, u32 pixfmt) +{ + struct hfi_uncompressed_format_select fmt; + struct hfi_core *hfi = &inst->core->hfi; + u32 ptype = HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SELECT; + int ret; + + fmt.buffer_type = type; + + switch (pixfmt) { + case V4L2_PIX_FMT_NV12: + fmt.format = HFI_COLOR_FORMAT_NV12; + break; + case V4L2_PIX_FMT_NV21: + fmt.format = HFI_COLOR_FORMAT_NV21; + break; + default: + return -ENOTSUPP; + } + + ret = vidc_hfi_session_set_property(hfi, inst->hfi_inst, ptype, &fmt); + if (ret) + return ret; + + return 0; +} + +struct vb2_v4l2_buffer * +vidc_vb2_find_buf(struct vidc_inst *inst, dma_addr_t addr) +{ + struct vidc_buffer *buf; + struct vb2_v4l2_buffer *vb = NULL; + + mutex_lock(&inst->bufqueue_lock); + + list_for_each_entry(buf, &inst->bufqueue, list) { + if (buf->dma_addr == addr) { + vb = &buf->vb; + break; + } + } + + if (vb) + list_del(&buf->list); + + mutex_unlock(&inst->bufqueue_lock); + + return vb; +} + +int vidc_vb2_buf_init(struct vb2_buffer *vb) +{ + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); + struct vb2_queue *q = vb->vb2_queue; + struct vidc_inst *inst = vb2_get_drv_priv(q); + struct vidc_buffer *buf = to_vidc_buffer(vbuf); + struct hfi_buffer_desc *bd = &buf->bd; + struct sg_table *sgt; + + memset(bd, 0, sizeof(*bd)); + + if (q->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) + return 0; + + sgt = vb2_dma_sg_plane_desc(vb, 0); + if (!sgt) + return -EINVAL; + + bd->buffer_size = vb2_plane_size(vb, 0); + bd->buffer_type = HFI_BUFFER_OUTPUT; + bd->num_buffers = 1; + bd->device_addr = sg_dma_address(sgt->sgl); + + mutex_lock(&inst->registeredbufs.lock); + list_add_tail(&buf->hfi_list, &inst->registeredbufs.list); + mutex_unlock(&inst->registeredbufs.lock); + + return 0; +} + +int vidc_vb2_buf_prepare(struct vb2_buffer *vb) +{ + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); + struct vidc_buffer *buf = to_vidc_buffer(vbuf); + struct sg_table *sgt; + + sgt = vb2_dma_sg_plane_desc(vb, 0); + if (!sgt) + return -EINVAL; + + buf->dma_addr = sg_dma_address(sgt->sgl); + + return 0; +} + +void vidc_vb2_buf_queue(struct vb2_buffer *vb) +{ + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); + struct vidc_inst *inst = vb2_get_drv_priv(vb->vb2_queue); + struct vidc_core *core = inst->core; + struct device *dev = core->dev; + struct vidc_buffer *buf = to_vidc_buffer(vbuf); + unsigned int state; + int ret; + + mutex_lock(&inst->hfi_inst->lock); + state = inst->hfi_inst->state; + mutex_unlock(&inst->hfi_inst->lock); + + if (state == INST_INVALID || state >= INST_STOP) { + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); + dev_dbg(dev, "%s: type:%d, invalid instance state\n", __func__, + vb->type); + return; + } + + mutex_lock(&inst->bufqueue_lock); + list_add_tail(&buf->list, &inst->bufqueue); + mutex_unlock(&inst->bufqueue_lock); + + if (!vb2_is_streaming(&inst->bufq_cap) || + !vb2_is_streaming(&inst->bufq_out)) + return; + + ret = session_set_buf(vb); + if (ret) + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); +} + +void vidc_vb2_stop_streaming(struct vb2_queue *q) +{ + struct vidc_inst *inst = vb2_get_drv_priv(q); + struct hfi_inst *hfi_inst = inst->hfi_inst; + struct vidc_core *core = inst->core; + struct device *dev = core->dev; + struct hfi_core *hfi = &core->hfi; + int ret, streamoff; + + mutex_lock(&inst->lock); + streamoff = inst->streamoff; + mutex_unlock(&inst->lock); + + if (streamoff) + return; + + mutex_lock(&inst->lock); + if (inst->streamon == 0) { + mutex_unlock(&inst->lock); + return; + } + mutex_unlock(&inst->lock); + + ret = vidc_hfi_session_stop(hfi, inst->hfi_inst); + if (ret) { + dev_err(dev, "session: stop failed (%d)\n", ret); + goto abort; + } + + ret = vidc_hfi_session_unload_res(hfi, inst->hfi_inst); + if (ret) { + dev_err(dev, "session: release resources failed (%d)\n", ret); + goto abort; + } + + ret = session_unregister_bufs(inst); + if (ret) { + dev_err(dev, "failed to release capture buffers: %d\n", ret); + goto abort; + } + + ret = internal_bufs_free(inst); + + if (hfi_inst->state == INST_INVALID || hfi->state == CORE_INVALID) { + ret = -EINVAL; + goto abort; + } + +abort: + if (ret) + vidc_hfi_session_abort(hfi, inst->hfi_inst); + + vidc_scale_clocks(inst->core); + + ret = vidc_hfi_session_deinit(hfi, inst->hfi_inst); + + mutex_lock(&inst->lock); + inst->streamoff = 1; + mutex_unlock(&inst->lock); + + if (ret) + dev_err(dev, "stop streaming failed type: %d, ret: %d\n", + q->type, ret); + + ret = pm_runtime_put_sync(dev); + if (ret < 0) + dev_err(dev, "%s: pm_runtime_put_sync (%d)\n", __func__, ret); +} + +int vidc_vb2_start_streaming(struct vidc_inst *inst) +{ + struct device *dev = inst->core->dev; + struct hfi_core *hfi = &inst->core->hfi; + struct vidc_buffer *buf, *n; + int ret; + + ret = session_register_bufs(inst); + if (ret) + return ret; + + ret = internal_bufs_alloc(inst); + if (ret) + return ret; + + vidc_scale_clocks(inst->core); + + ret = vidc_hfi_session_load_res(hfi, inst->hfi_inst); + if (ret) { + dev_err(dev, "session: load resources (%d)\n", ret); + return ret; + } + + ret = vidc_hfi_session_start(hfi, inst->hfi_inst); + if (ret) { + dev_err(dev, "session: start failed (%d)\n", ret); + return ret; + } + + mutex_lock(&inst->bufqueue_lock); + list_for_each_entry_safe(buf, n, &inst->bufqueue, list) { + ret = session_set_buf(&buf->vb.vb2_buf); + if (ret) + break; + } + mutex_unlock(&inst->bufqueue_lock); + + if (!ret) { + mutex_lock(&inst->lock); + inst->streamon = 1; + mutex_unlock(&inst->lock); + } + + return ret; +} diff --git a/drivers/media/platform/qcom/vidc/helpers.h b/drivers/media/platform/qcom/vidc/helpers.h new file mode 100644 index 000000000000..a151c96bf939 --- /dev/null +++ b/drivers/media/platform/qcom/vidc/helpers.h @@ -0,0 +1,43 @@ +/* + * Copyright (c) 2012-2014, The Linux Foundation. All rights reserved. + * Copyright (C) 2016 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ +#ifndef __VIDC_COMMON_H__ +#define __VIDC_COMMON_H__ + +#include <linux/list.h> +#include <media/videobuf2-v4l2.h> + +#include "core.h" + +struct vidc_buffer { + struct vb2_v4l2_buffer vb; + struct list_head list; + dma_addr_t dma_addr; + struct list_head hfi_list; + struct hfi_buffer_desc bd; +}; + +#define to_vidc_buffer(buf) container_of(buf, struct vidc_buffer, vb) + +struct vb2_v4l2_buffer * +vidc_vb2_find_buf(struct vidc_inst *inst, dma_addr_t addr); +int vidc_vb2_buf_init(struct vb2_buffer *vb); +int vidc_vb2_buf_prepare(struct vb2_buffer *vb); +void vidc_vb2_buf_queue(struct vb2_buffer *vb); +void vidc_vb2_stop_streaming(struct vb2_queue *q); +int vidc_vb2_start_streaming(struct vidc_inst *inst); +int vidc_buf_descs(struct vidc_inst *inst, u32 type, + struct hfi_buffer_requirements *out); +int vidc_set_color_format(struct vidc_inst *inst, u32 type, u32 fmt); +#endif diff --git a/drivers/media/platform/qcom/vidc/int_bufs.c b/drivers/media/platform/qcom/vidc/int_bufs.c new file mode 100644 index 000000000000..393d75785d7a --- /dev/null +++ b/drivers/media/platform/qcom/vidc/int_bufs.c @@ -0,0 +1,325 @@ +/* + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. + * Copyright (C) 2016 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#include "helpers.h" +#include "int_bufs.h" +#include "mem.h" + +struct vidc_internal_buf { + struct list_head list; + u32 type; + struct vidc_mem *mem; +}; + +static u32 scratch_buf_sufficient(struct vidc_inst *inst, u32 buffer_type) +{ + struct hfi_buffer_requirements bufreq; + struct vidc_internal_buf *buf; + unsigned int count = 0; + int ret; + + ret = vidc_buf_descs(inst, buffer_type, &bufreq); + if (ret) + return 0; + + /* Check if current scratch buffers are sufficient */ + mutex_lock(&inst->scratchbufs.lock); + list_for_each_entry(buf, &inst->scratchbufs.list, list) { + if (buf->type == buffer_type && + buf->mem->size >= bufreq.size) + count++; + } + mutex_unlock(&inst->scratchbufs.lock); + + if (count != bufreq.count_actual) + return 0; + + return buffer_type; +} + +static int internal_set_buf_on_fw(struct vidc_inst *inst, u32 buffer_type, + struct vidc_mem *mem, bool reuse) +{ + struct device *dev = inst->core->dev; + struct hfi_core *hfi = &inst->core->hfi; + struct hfi_buffer_desc bd = {0}; + int ret; + + bd.buffer_size = mem->size; + bd.buffer_type = buffer_type; + bd.num_buffers = 1; + bd.device_addr = mem->da; + + ret = vidc_hfi_session_set_buffers(hfi, inst->hfi_inst, &bd); + if (ret) { + dev_err(dev, "set session buffers failed\n"); + return ret; + } + + return 0; +} + +static int internal_alloc_and_set(struct vidc_inst *inst, + struct hfi_buffer_requirements *bufreq, + struct vidc_list *buf_list) +{ + struct vidc_internal_buf *buf; + struct vidc_mem *mem; + unsigned int i; + int ret = 0; + + if (!bufreq->size) + return 0; + + for (i = 0; i < bufreq->count_actual; i++) { + mem = mem_alloc(inst->core->dev, bufreq->size, 0); + if (IS_ERR(mem)) { + ret = PTR_ERR(mem); + goto err_no_mem; + } + + buf = kzalloc(sizeof(*buf), GFP_KERNEL); + if (!buf) { + ret = -ENOMEM; + goto fail_kzalloc; + } + + buf->mem = mem; + buf->type = bufreq->type; + + ret = internal_set_buf_on_fw(inst, bufreq->type, mem, false); + if (ret) + goto fail_set_buffers; + + mutex_lock(&buf_list->lock); + list_add_tail(&buf->list, &buf_list->list); + mutex_unlock(&buf_list->lock); + } + + return ret; + +fail_set_buffers: + kfree(buf); +fail_kzalloc: + mem_free(mem); +err_no_mem: + return ret; +} + +static bool scratch_reuse_buffer(struct vidc_inst *inst, u32 buffer_type) +{ + struct device *dev = inst->core->dev; + struct vidc_internal_buf *buf; + bool reused = false; + int ret = 0; + + mutex_lock(&inst->scratchbufs.lock); + list_for_each_entry(buf, &inst->scratchbufs.list, list) { + if (buf->type != buffer_type) + continue; + + ret = internal_set_buf_on_fw(inst, buffer_type, buf->mem, true); + if (ret) { + dev_err(dev, "set internal buffers failed\n"); + reused = false; + break; + } + + reused = true; + } + mutex_unlock(&inst->scratchbufs.lock); + + return reused; +} + +static int scratch_set_buffer(struct vidc_inst *inst, u32 type) +{ + struct hfi_buffer_requirements bufreq; + int ret; + + ret = vidc_buf_descs(inst, type, &bufreq); + if (ret) + return 0; + + if (scratch_reuse_buffer(inst, type)) + return 0; + + return internal_alloc_and_set(inst, &bufreq, &inst->scratchbufs); +} + +static int persist_set_buffer(struct vidc_inst *inst, u32 type) +{ + struct hfi_buffer_requirements bufreq; + int ret; + + ret = vidc_buf_descs(inst, type, &bufreq); + if (ret) + return 0; + + mutex_lock(&inst->persistbufs.lock); + if (!list_empty(&inst->persistbufs.list)) { + mutex_unlock(&inst->persistbufs.lock); + return 0; + } + mutex_unlock(&inst->persistbufs.lock); + + return internal_alloc_and_set(inst, &bufreq, &inst->persistbufs); +} + +static int scratch_unset_buffers(struct vidc_inst *inst, bool reuse) +{ + struct hfi_core *hfi = &inst->core->hfi; + struct vidc_internal_buf *buf, *n; + struct hfi_buffer_desc bd = {0}; + u32 sufficient = 0; + int ret = 0; + + mutex_lock(&inst->scratchbufs.lock); + list_for_each_entry_safe(buf, n, &inst->scratchbufs.list, list) { + bd.buffer_size = buf->mem->size; + bd.buffer_type = buf->type; + bd.num_buffers = 1; + bd.device_addr = buf->mem->da; + bd.response_required = true; + + ret = vidc_hfi_session_unset_buffers(hfi, inst->hfi_inst, &bd); + + /* If scratch buffers can be reused, do not free the buffers */ + if (reuse) { + sufficient = scratch_buf_sufficient(inst, buf->type); + if (sufficient == buf->type) + continue; + } + + list_del(&buf->list); + mem_free(buf->mem); + kfree(buf); + } + mutex_unlock(&inst->scratchbufs.lock); + + return ret; +} + +static int persist_unset_buffers(struct vidc_inst *inst) +{ + struct hfi_core *hfi = &inst->core->hfi; + struct vidc_internal_buf *buf, *n; + struct hfi_buffer_desc bd = {0}; + int ret = 0; + + mutex_lock(&inst->persistbufs.lock); + list_for_each_entry_safe(buf, n, &inst->persistbufs.list, list) { + bd.buffer_size = buf->mem->size; + bd.buffer_type = buf->type; + bd.num_buffers = 1; + bd.device_addr = buf->mem->da; + bd.response_required = true; + + ret = vidc_hfi_session_unset_buffers(hfi, inst->hfi_inst, &bd); + + list_del(&buf->list); + mem_free(buf->mem); + kfree(buf); + } + mutex_unlock(&inst->persistbufs.lock); + + return ret; +} + +static int scratch_set_buffers(struct vidc_inst *inst) +{ + struct device *dev = inst->core->dev; + int ret; + + ret = scratch_unset_buffers(inst, true); + if (ret) + dev_warn(dev, "Failed to release scratch buffers\n"); + + ret = scratch_set_buffer(inst, HFI_BUFFER_INTERNAL_SCRATCH); + if (ret) + goto error; + + ret = scratch_set_buffer(inst, HFI_BUFFER_INTERNAL_SCRATCH_1); + if (ret) + goto error; + + ret = scratch_set_buffer(inst, HFI_BUFFER_INTERNAL_SCRATCH_2); + if (ret) + goto error; + + return 0; +error: + scratch_unset_buffers(inst, false); + return ret; +} + +static int persist_set_buffers(struct vidc_inst *inst) +{ + int ret; + + ret = persist_set_buffer(inst, HFI_BUFFER_INTERNAL_PERSIST); + if (ret) + goto error; + + ret = persist_set_buffer(inst, HFI_BUFFER_INTERNAL_PERSIST_1); + if (ret) + goto error; + + return 0; + +error: + persist_unset_buffers(inst); + return ret; +} + +int internal_bufs_alloc(struct vidc_inst *inst) +{ + struct device *dev = inst->core->dev; + int ret; + + ret = scratch_set_buffers(inst); + if (ret) { + dev_err(dev, "set scratch buffers (%d)\n", ret); + return ret; + } + + ret = persist_set_buffers(inst); + if (ret) { + dev_err(dev, "set persist buffers (%d)\n", ret); + goto error; + } + + return 0; + +error: + scratch_unset_buffers(inst, false); + return ret; +} + +int internal_bufs_free(struct vidc_inst *inst) +{ + struct device *dev = inst->core->dev; + int ret; + + ret = scratch_unset_buffers(inst, false); + if (ret) + dev_err(dev, "failed to release scratch buffers: %d\n", ret); + + ret = persist_unset_buffers(inst); + if (ret) + dev_err(dev, "failed to release persist buffers: %d\n", ret); + + return ret; +} diff --git a/drivers/media/platform/qcom/vidc/int_bufs.h b/drivers/media/platform/qcom/vidc/int_bufs.h new file mode 100644 index 000000000000..5f8b2b85839f --- /dev/null +++ b/drivers/media/platform/qcom/vidc/int_bufs.h @@ -0,0 +1,23 @@ +/* + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. + * Copyright (C) 2016 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ +#ifndef __VIDC_INTERNAL_BUFFERS_H__ +#define __VIDC_INTERNAL_BUFFERS_H__ + +struct vidc_inst; + +int internal_bufs_alloc(struct vidc_inst *inst); +int internal_bufs_free(struct vidc_inst *inst); + +#endif diff --git a/drivers/media/platform/qcom/vidc/load.c b/drivers/media/platform/qcom/vidc/load.c new file mode 100644 index 000000000000..8ae25fc0e8a5 --- /dev/null +++ b/drivers/media/platform/qcom/vidc/load.c @@ -0,0 +1,104 @@ +/* + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved. + * Copyright (C) 2016 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#include <linux/clk.h> + +#include "core.h" +#include "load.h" + +static u32 get_inst_load(struct vidc_inst *inst) +{ + int mbs; + u32 w = inst->width; + u32 h = inst->height; + + if (!inst->hfi_inst || !(inst->hfi_inst->state >= INST_INIT && + inst->hfi_inst->state < INST_STOP)) + return 0; + + mbs = (ALIGN(w, 16) / 16) * (ALIGN(h, 16) / 16); + + return mbs * inst->fps; +} + +static u32 get_load(struct vidc_core *core, u32 session_type) +{ + struct vidc_inst *inst = NULL; + u32 mbs_per_sec = 0; + + mutex_lock(&core->lock); + list_for_each_entry(inst, &core->instances, list) { + if (inst->session_type != session_type) + continue; + + mbs_per_sec += get_inst_load(inst); + } + mutex_unlock(&core->lock); + + return mbs_per_sec; +} + +static int scale_clocks_load(struct vidc_core *core, u32 mbs_per_sec) +{ + const struct freq_tbl *table = core->res->freq_tbl; + int num_rows = core->res->freq_tbl_size; + struct clk *clk = core->clks[0]; + struct device *dev = core->dev; + unsigned long freq = table[0].freq; + int ret, i; + + if (!mbs_per_sec && num_rows > 1) { + freq = table[num_rows - 1].freq; + goto set_freq; + } + + for (i = 0; i < num_rows; i++) { + if (mbs_per_sec > table[i].load) + break; + freq = table[i].freq; + } + +set_freq: + + ret = clk_set_rate(clk, freq); + if (ret) { + dev_err(dev, "failed to set clock rate %lu (%d)\n", freq, ret); + return ret; + } + + return 0; +} + +int vidc_scale_clocks(struct vidc_core *core) +{ + struct device *dev = core->dev; + u32 mbs_per_sec; + int ret; + + mbs_per_sec = get_load(core, VIDC_SESSION_TYPE_ENC) + + get_load(core, VIDC_SESSION_TYPE_DEC); + + if (mbs_per_sec > core->res->max_load) { + dev_warn(dev, "HW is overloaded, needed: %d max: %d\n", + mbs_per_sec, core->res->max_load); + return -EBUSY; + } + + ret = scale_clocks_load(core, mbs_per_sec); + if (ret) + dev_warn(dev, "failed to scale clocks, performance might be impacted\n"); + + return 0; +} diff --git a/drivers/media/platform/qcom/vidc/load.h b/drivers/media/platform/qcom/vidc/load.h new file mode 100644 index 000000000000..b8f8dc57e18f --- /dev/null +++ b/drivers/media/platform/qcom/vidc/load.h @@ -0,0 +1,22 @@ +/* + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved. + * Copyright (C) 2016 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ +#ifndef __VIDC_LOAD_H__ +#define __VIDC_LOAD_H__ + +struct vidc_core; + +int vidc_scale_clocks(struct vidc_core *core); + +#endif diff --git a/drivers/media/platform/qcom/vidc/mem.c b/drivers/media/platform/qcom/vidc/mem.c new file mode 100644 index 000000000000..6a83b5784410 --- /dev/null +++ b/drivers/media/platform/qcom/vidc/mem.c @@ -0,0 +1,64 @@ +/* + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. + * Copyright (C) 2016 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#include <linux/device.h> +#include <linux/dma-direction.h> +#include <linux/dma-mapping.h> +#include <linux/err.h> +#include <linux/slab.h> + +#include "mem.h" + +struct vidc_mem *mem_alloc(struct device *dev, size_t size, int map_kernel) +{ + struct vidc_mem *mem; + + if (!size) + return ERR_PTR(-EINVAL); + + if (IS_ERR(dev)) + return ERR_CAST(dev); + + mem = kzalloc(sizeof(*mem), GFP_KERNEL); + if (!mem) + return ERR_PTR(-ENOMEM); + + mem->size = ALIGN(size, SZ_4K); + mem->iommu_dev = dev; + + mem->attrs = DMA_ATTR_WRITE_COMBINE; + + if (!map_kernel) + mem->attrs |= DMA_ATTR_NO_KERNEL_MAPPING; + + mem->kvaddr = dma_alloc_attrs(mem->iommu_dev, mem->size, &mem->da, + GFP_KERNEL, mem->attrs); + if (!mem->kvaddr) { + kfree(mem); + return ERR_PTR(-ENOMEM); + } + + return mem; +} + +void mem_free(struct vidc_mem *mem) +{ + if (!mem) + return; + + dma_free_attrs(mem->iommu_dev, mem->size, mem->kvaddr, + mem->da, mem->attrs); + kfree(mem); +}; diff --git a/drivers/media/platform/qcom/vidc/mem.h b/drivers/media/platform/qcom/vidc/mem.h new file mode 100644 index 000000000000..cab81aa2f550 --- /dev/null +++ b/drivers/media/platform/qcom/vidc/mem.h @@ -0,0 +1,32 @@ +/* + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. + * Copyright (C) 2016 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#ifndef __VIDC_MEM_H__ +#define __VIDC_MEM_H__ + +struct device; + +struct vidc_mem { + size_t size; + void *kvaddr; + dma_addr_t da; + unsigned long attrs; + struct device *iommu_dev; +}; + +struct vidc_mem *mem_alloc(struct device *dev, size_t size, int map_kernel); +void mem_free(struct vidc_mem *mem); + +#endif /* __VIDC_MEM_H__ */ diff --git a/drivers/media/platform/qcom/vidc/resources.c b/drivers/media/platform/qcom/vidc/resources.c new file mode 100644 index 000000000000..e00ed1caa824 --- /dev/null +++ b/drivers/media/platform/qcom/vidc/resources.c @@ -0,0 +1,46 @@ +/* + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved. + * Copyright (C) 2016 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ +#include <linux/bug.h> +#include <linux/types.h> +#include <linux/kernel.h> + +#include "hfi.h" + +static const struct freq_tbl msm8916_freq_table[] = { + { 352800, 228570000 }, /* 1920x1088 @ 30 + 1280x720 @ 30 */ + { 244800, 160000000 }, /* 1920x1088 @ 30 */ + { 108000, 100000000 }, /* 1280x720 @ 30 */ +}; + +static const struct reg_val msm8916_reg_preset[] = { + { 0xe0020, 0x05555556 }, + { 0xe0024, 0x05555556 }, + { 0x80124, 0x00000003 }, +}; + +const struct vidc_resources msm8916_res = { + .freq_tbl = msm8916_freq_table, + .freq_tbl_size = ARRAY_SIZE(msm8916_freq_table), + .reg_tbl = msm8916_reg_preset, + .reg_tbl_size = ARRAY_SIZE(msm8916_reg_preset), + .clks = {"core", "iface", "bus", }, + .clks_num = 3, + .max_load = 352800, /* 720p@30 + 1080p@30 */ + .hfi_version = 0, + .vmem_id = VIDC_RESOURCE_NONE, + .vmem_size = 0, + .vmem_addr = 0, + .dma_mask = 0xddc00000 - 1, +}; diff --git a/drivers/media/platform/qcom/vidc/resources.h b/drivers/media/platform/qcom/vidc/resources.h new file mode 100644 index 000000000000..9f2afad78a9f --- /dev/null +++ b/drivers/media/platform/qcom/vidc/resources.h @@ -0,0 +1,46 @@ +/* + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved. + * Copyright (C) 2016 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ +#ifndef __VIDC_RESOURCES_H__ +#define __VIDC_RESOURCES_H__ + +#define VIDC_CLKS_NUM_MAX 7 + +struct freq_tbl { + unsigned int load; + unsigned long freq; +}; + +struct reg_val { + u32 reg; + u32 value; +}; + +struct vidc_resources { + u64 dma_mask; + const struct freq_tbl *freq_tbl; + unsigned int freq_tbl_size; + const struct reg_val *reg_tbl; + unsigned int reg_tbl_size; + const char *clks[VIDC_CLKS_NUM_MAX]; + unsigned int clks_num; + unsigned int hfi_version; + u32 max_load; + unsigned int vmem_id; + u32 vmem_size; + u32 vmem_addr; +}; + +extern const struct vidc_resources msm8916_res; +#endif
This adds core part of the vidc driver common helper functions used by encoder and decoder specific files. - core.c has implemented the platform dirver methods, file operations and v4l2 registration. - helpers.c has implemented common helper functions for buffer management, vb2_ops and functions for format propagation. - int_bufs.c implements functions for allocating and freeing buffers for internal usage. The buffer parameters describing internal buffers depends on current format, resolution and codec. - load.c consists functions for calculation of current load of the hardware. Depending on the count of instances and resolutions it selects the best clock rate for the video core. - mem.c has two functions for memory allocation, currently those functions are used for internal buffers and to allocate the shared memory for communication with firmware via HFI (Host Firmware Interface) interface commands. - resources.c exports a structure describing the details specific to platform and SoC. Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> --- drivers/media/platform/qcom/vidc/core.c | 548 +++++++++++++++++++++++++++ drivers/media/platform/qcom/vidc/core.h | 196 ++++++++++ drivers/media/platform/qcom/vidc/helpers.c | 394 +++++++++++++++++++ drivers/media/platform/qcom/vidc/helpers.h | 43 +++ drivers/media/platform/qcom/vidc/int_bufs.c | 325 ++++++++++++++++ drivers/media/platform/qcom/vidc/int_bufs.h | 23 ++ drivers/media/platform/qcom/vidc/load.c | 104 +++++ drivers/media/platform/qcom/vidc/load.h | 22 ++ drivers/media/platform/qcom/vidc/mem.c | 64 ++++ drivers/media/platform/qcom/vidc/mem.h | 32 ++ drivers/media/platform/qcom/vidc/resources.c | 46 +++ drivers/media/platform/qcom/vidc/resources.h | 46 +++ 12 files changed, 1843 insertions(+) create mode 100644 drivers/media/platform/qcom/vidc/core.c create mode 100644 drivers/media/platform/qcom/vidc/core.h create mode 100644 drivers/media/platform/qcom/vidc/helpers.c create mode 100644 drivers/media/platform/qcom/vidc/helpers.h create mode 100644 drivers/media/platform/qcom/vidc/int_bufs.c create mode 100644 drivers/media/platform/qcom/vidc/int_bufs.h create mode 100644 drivers/media/platform/qcom/vidc/load.c create mode 100644 drivers/media/platform/qcom/vidc/load.h create mode 100644 drivers/media/platform/qcom/vidc/mem.c create mode 100644 drivers/media/platform/qcom/vidc/mem.h create mode 100644 drivers/media/platform/qcom/vidc/resources.c create mode 100644 drivers/media/platform/qcom/vidc/resources.h