[PATCH] media: staging/imx: Checking the right variable in vdic_get_ipu_resources()
We recently changed this error handling around but missed this error pointer check. We're testing "priv->vdi_in_ch_n" instead of "ch" so the error handling can't be triggered. Fixes: 0b2e9e7947e7 ("media: staging/imx: remove confusing IS_ERR_OR_NULL usage") Signed-off-by: Dan Carpenter diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c index 433474d58e3e..ed356844cdf6 100644 --- a/drivers/staging/media/imx/imx-media-vdic.c +++ b/drivers/staging/media/imx/imx-media-vdic.c @@ -177,7 +177,7 @@ static int vdic_get_ipu_resources(struct vdic_priv *priv) priv->vdi_in_ch = ch; ch = ipu_idmac_get(priv->ipu, IPUV3_CHANNEL_MEM_VDI_NEXT); - if (IS_ERR(priv->vdi_in_ch_n)) { + if (IS_ERR(ch)) { err_chan = IPUV3_CHANNEL_MEM_VDI_NEXT; ret = PTR_ERR(ch); goto out_err_chan;
Re: [RFC PATCH 6/9] media: vb2: add support for requests in QBUF ioctl
On Fri, Jan 12, 2018 at 8:37 PM, Hans Verkuil wrote: > On 12/15/17 08:56, Alexandre Courbot wrote: >> Support the request argument of the QBUF ioctl. >> >> Signed-off-by: Alexandre Courbot >> --- >> drivers/media/v4l2-core/v4l2-ioctl.c | 93 >> +++- >> 1 file changed, 92 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c >> b/drivers/media/v4l2-core/v4l2-ioctl.c >> index 8d041247e97f..28f9c368563e 100644 >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >> @@ -29,6 +29,7 @@ >> #include >> #include >> #include >> +#include >> >> #include >> >> @@ -965,6 +966,81 @@ static int check_fmt(struct file *file, enum >> v4l2_buf_type type) >> return -EINVAL; >> } >> >> +/* >> + * Validate that a given request can be used during an ioctl. >> + * >> + * When using the request API, request file descriptors must be matched >> against >> + * the actual request object. User-space can pass any file descriptor, so we >> + * need to make sure the call is valid before going further. >> + * >> + * This function looks up the request and associated data and performs the >> + * following sanity checks: >> + * >> + * * Make sure that the entity supports requests, >> + * * Make sure that the entity belongs to the media_device managing the >> passed >> + * request, >> + * * Make sure that the entity data (if any) is associated to the current >> file >> + * handler. >> + * >> + * This function returns a pointer to the valid request, or and error code >> in >> + * case of failure. When successful, a reference to the request is acquired >> and >> + * must be properly released. >> + */ >> +#ifdef CONFIG_MEDIA_CONTROLLER >> +static struct media_request * >> +check_request(int request, struct file *file, void *fh) >> +{ >> + struct media_request *req = NULL; >> + struct video_device *vfd = video_devdata(file); >> + struct v4l2_fh *vfh = >> + test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL; >> + struct media_entity *entity = &vfd->entity; >> + const struct media_entity *ent; >> + struct media_request_entity_data *data; >> + bool found = false; >> + >> + if (!entity) >> + return ERR_PTR(-EINVAL); >> + >> + /* Check that the entity supports requests */ >> + if (!entity->req_ops) >> + return ERR_PTR(-ENOTSUPP); >> + >> + req = media_request_get_from_fd(request); > > You can get the media_device from vfd->v4l2_dev->mdev. So it is much easier > to just pass the media_device as an argument to media_request_get_from_fd()... > >> + if (!req) >> + return ERR_PTR(-EINVAL); >> + >> + /* Validate that the entity belongs to the media_device managing >> + * the request queue */ >> + media_device_for_each_entity(ent, req->queue->mdev) { >> + if (entity == ent) { >> + found = true; >> + break; >> + } >> + } >> + if (!found) { >> + media_request_put(req); >> + return ERR_PTR(-EINVAL); >> + } > > ...and then you don't need to do this ^^^ extra validation check. Ah right, all you need to do is check that req->queue->mdev == vfd->v4l2_dev->mdev and you can get rid of this whole block. I don't think we can do that in media_request_get_from_fd() though, since it is called from other places where (IIUC) we don't have access to the media_device. > >> + >> + /* Validate that the entity's data belongs to the correct fh */ >> + data = media_request_get_entity_data(req, entity, vfh); >> + if (IS_ERR(data)) { >> + media_request_put(req); >> + return ERR_PTR(PTR_ERR(data)); >> + } > > This assumes that each filehandle has its own state. That's true for codecs, > but not for most (all?) other devices. There the state is per device instance. > > I'm not sure if we have a unique identifying mark for such drivers. The > closest > is checking if fh->m2m_ctx is non-NULL, but I don't know if all drivers with > per-filehandle state use that field. An alternative might be to check if > fh->ctrl_handler is non-NULL. But again, I'm not sure if that's a 100% valid > check. I think the current code already takes that case into account: if the device does not uses v4l2_fh, then the fh argument passed to media_request_get_entity_data() will be null, and so will be data->fh.
Re: [RFC PATCH 0/9] media: base request API support
Hi Hans, On Fri, Jan 12, 2018 at 8:45 PM, Hans Verkuil wrote: > Hi Alexandre, > > On 12/15/17 08:56, Alexandre Courbot wrote: >> Here is a new attempt at the request API, following the UAPI we agreed on in >> Prague. Hopefully this can be used as the basis to move forward. >> >> This series only introduces the very basics of how requests work: allocate a >> request, queue buffers to it, queue the request itself, wait for it to >> complete, >> reuse it. It does *not* yet use Hans' work with controls setting. I have >> preferred to submit it this way for now as it allows us to concentrate on the >> basic request/buffer flow, which was harder to get properly than I initially >> thought. I still have a gut feeling that it can be improved, with less >> back-and- >> forth into drivers. >> >> Plugging in controls support should not be too hard a task (basically just >> apply >> the saved controls when the request starts), and I am looking at it now. >> >> The resulting vim2m driver can be successfully used with requests, and my >> tests >> so far have been successful. >> >> There are still some rougher edges: >> >> * locking is currently quite coarse-grained >> * too many #ifdef CONFIG_MEDIA_CONTROLLER in the code, as the request API >> depends on it - I plan to craft the headers so that it becomes unnecessary. >> As it is, some of the code will probably not even compile if >> CONFIG_MEDIA_CONTROLLER is not set >> >> But all in all I think the request flow should be clear and easy to review, >> and >> the possibility of custom queue and entity support implementations should >> give >> us the flexibility we need to support more specific use-cases (I expect the >> generic implementations to be sufficient most of the time though). >> >> A very simple test program exercising this API is available here (don't >> forget >> to adapt the /dev/media0 hardcoding): >> https://gist.github.com/Gnurou/dbc3776ed97ea7d4ce6041ea15eb0438 >> >> Looking forward to your feedback and comments! > > I think this will become more interesting when the control code is in. Definitely. > The main thing I've noticed with this patch series is that it is very codec > oriented. Which in some ways is OK (after all, that's the first type of HW > that we want to support), but the vb2 code in particular should be more > generic. I don't want to expand too much into use-cases I do not master ; doing so would be speculating about how the API will be used. But feel free to point out where you think my focus on the codec use-case is not future-proof. > I would also recommend that you start preparing documentation patches: we > can review that and make sure all the corner-cases are correctly documented. > > The public API changes are (I think) fairly limited, but the devil is in > the details, so getting that reviewed early on will help you later. Yeah, I now regret to have submitted this series without documentation. Won't do that mistake again. > It's a bit unfortunate that the fence patch series is also making vb2 changes, > but I hope that will be merged fairly soon so you can develop on top of that > series. The fence series may actually make things easier. The vb2 code of this series is a bit confusing, and fences add a few extra constraints that should make things more predictable. So I am looking forward to being able to work on top of it.
Re: [RFC PATCH 4/9] videodev2.h: Add request field to v4l2_buffer
On Fri, Jan 12, 2018 at 7:22 PM, Hans Verkuil wrote: > On 12/15/17 08:56, Alexandre Courbot wrote: >> From: Hans Verkuil >> >> When queuing buffers allow for passing the request ID that >> should be associated with this buffer. >> >> Signed-off-by: Hans Verkuil >> [acour...@chromium.org: make request ID 32-bit] >> Signed-off-by: Alexandre Courbot >> --- >> drivers/media/usb/cpia2/cpia2_v4l.c | 2 +- >> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 7 --- >> drivers/media/v4l2-core/v4l2-ioctl.c | 4 ++-- >> drivers/media/v4l2-core/videobuf2-v4l2.c | 3 ++- >> include/media/videobuf2-v4l2.h| 2 ++ >> include/uapi/linux/videodev2.h| 3 ++- >> 6 files changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c >> b/drivers/media/usb/cpia2/cpia2_v4l.c >> index 3dedd83f0b19..7217dde95a8a 100644 >> --- a/drivers/media/usb/cpia2/cpia2_v4l.c >> +++ b/drivers/media/usb/cpia2/cpia2_v4l.c >> @@ -948,7 +948,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, >> struct v4l2_buffer *buf) >> buf->sequence = cam->buffers[buf->index].seq; >> buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer; >> buf->length = cam->frame_size; >> - buf->reserved2 = 0; >> + buf->request = 0; >> buf->reserved = 0; >> memset(&buf->timecode, 0, sizeof(buf->timecode)); >> >> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> index 821f2aa299ae..94f07c3b0b53 100644 >> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> @@ -370,7 +370,7 @@ struct v4l2_buffer32 { >> __s32 fd; >> } m; >> __u32 length; >> - __u32 reserved2; >> + __u32 request; >> __u32 reserved; >> }; >> >> @@ -438,7 +438,8 @@ static int get_v4l2_buffer32(struct v4l2_buffer *kp, >> struct v4l2_buffer32 __user >> get_user(kp->type, &up->type) || >> get_user(kp->flags, &up->flags) || >> get_user(kp->memory, &up->memory) || >> - get_user(kp->length, &up->length)) >> + get_user(kp->length, &up->length) || >> + get_user(kp->request, &up->request)) >> return -EFAULT; >> >> if (V4L2_TYPE_IS_OUTPUT(kp->type)) >> @@ -533,7 +534,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, >> struct v4l2_buffer32 __user >> put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) || >> copy_to_user(&up->timecode, &kp->timecode, sizeof(struct >> v4l2_timecode)) || >> put_user(kp->sequence, &up->sequence) || >> - put_user(kp->reserved2, &up->reserved2) || >> + put_user(kp->request, &up->request) || >> put_user(kp->reserved, &up->reserved) || >> put_user(kp->length, &up->length)) >> return -EFAULT; >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c >> b/drivers/media/v4l2-core/v4l2-ioctl.c >> index ec4ecd5aa8bf..8d041247e97f 100644 >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >> @@ -437,13 +437,13 @@ static void v4l_print_buffer(const void *arg, bool >> write_only) >> const struct v4l2_plane *plane; >> int i; >> >> - pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, flags=0x%08x, >> field=%s, sequence=%d, memory=%s", >> + pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, request=%u, >> flags=0x%08x, field=%s, sequence=%d, memory=%s", >> p->timestamp.tv_sec / 3600, >> (int)(p->timestamp.tv_sec / 60) % 60, >> (int)(p->timestamp.tv_sec % 60), >> (long)p->timestamp.tv_usec, >> p->index, >> - prt_names(p->type, v4l2_type_names), >> + prt_names(p->type, v4l2_type_names), p->request, >> p->flags, prt_names(p->field, v4l2_field_names), >> p->sequence, prt_names(p->memory, v4l2_memory_names)); >> >> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c >> b/drivers/media/v4l2-core/videobuf2-v4l2.c >> index 0c0669976bdc..bde7b8a3a303 100644 >> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c >> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c >> @@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, >> void *pb) >> b->timestamp = ns_to_timeval(vb->timestamp); >> b->timecode = vbuf->timecode; >> b->sequence = vbuf->sequence; >> - b->reserved2 = 0; >> + b->request = vbuf->request; >> b->reserved = 0; >> >> if (q->is_multiplanar) { >> @@ -320,6 +320,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, >> } >> vb->timestamp = 0; >> vbuf->sequence = 0; >> +
Re: [RFC PATCH 5/9] media: vb2: add support for requests
On Fri, Jan 12, 2018 at 7:49 PM, Hans Verkuil wrote: > On 12/15/17 08:56, Alexandre Courbot wrote: >> Add throttling support for buffers when requests are in use on a given >> queue. Buffers associated to a request are kept into the vb2 queue until >> the request becomes active, at which point all the buffers are passed to >> the driver. The queue can also signal that is has processed all of a >> request's buffers. >> >> Also add support for the request parameter when handling the QBUF ioctl. >> >> Signed-off-by: Alexandre Courbot >> --- >> drivers/media/v4l2-core/videobuf2-core.c | 59 >> >> drivers/media/v4l2-core/videobuf2-v4l2.c | 29 +++- >> include/media/videobuf2-core.h | 25 +- >> 3 files changed, 104 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c >> b/drivers/media/v4l2-core/videobuf2-core.c >> index cb115ba6a1d2..c01038b7962a 100644 >> --- a/drivers/media/v4l2-core/videobuf2-core.c >> +++ b/drivers/media/v4l2-core/videobuf2-core.c >> @@ -898,6 +898,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum >> vb2_buffer_state state) >> state != VB2_BUF_STATE_REQUEUEING)) >> state = VB2_BUF_STATE_ERROR; >> >> + WARN_ON(vb->request != q->cur_req); > > What's the reason for this WARN_ON? It's not immediately obvious to me. This is a safeguard against driver bugs: a buffer should not complete unless it is part of the request being currently processed. > >> + >> #ifdef CONFIG_VIDEO_ADV_DEBUG >> /* >>* Although this is not a callback, it still does have to balance >> @@ -920,6 +922,13 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum >> vb2_buffer_state state) >> /* Add the buffer to the done buffers list */ >> list_add_tail(&vb->done_entry, &q->done_list); >> vb->state = state; >> + >> + if (q->cur_req) { >> + WARN_ON(q->req_buf_cnt < 1); >> + >> + if (--q->req_buf_cnt == 0) >> + q->cur_req = NULL; >> + } >> } >> atomic_dec(&q->owned_by_drv_count); >> spin_unlock_irqrestore(&q->done_lock, flags); >> @@ -1298,6 +1307,16 @@ int vb2_core_prepare_buf(struct vb2_queue *q, >> unsigned int index, void *pb) >> } >> EXPORT_SYMBOL_GPL(vb2_core_prepare_buf); >> >> +static void vb2_queue_enqueue_current_buffers(struct vb2_queue *q) >> +{ >> + struct vb2_buffer *vb; >> + >> + list_for_each_entry(vb, &q->queued_list, queued_entry) { >> + if (vb->request == q->cur_req) >> + __enqueue_in_driver(vb); >> + } >> +} > > I think this will clash big time with the v4l2 fence patch series... Indeed, but on the other hand I was not a big fan of going through the whole list. :) So I welcome the extra throttling introduced by the fence series. > >> + >> /** >> * vb2_start_streaming() - Attempt to start streaming. >> * @q: videobuf2 queue >> @@ -1318,8 +1337,7 @@ static int vb2_start_streaming(struct vb2_queue *q) >>* If any buffers were queued before streamon, >>* we can now pass them to driver for processing. >>*/ >> - list_for_each_entry(vb, &q->queued_list, queued_entry) >> - __enqueue_in_driver(vb); >> + vb2_queue_enqueue_current_buffers(q); >> >> /* Tell the driver to start streaming */ >> q->start_streaming_called = 1; >> @@ -1361,7 +1379,8 @@ static int vb2_start_streaming(struct vb2_queue *q) >> return ret; >> } >> >> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb) >> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, >> + struct media_request *req, void *pb) >> { >> struct vb2_buffer *vb; >> int ret; >> @@ -1392,6 +1411,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int >> index, void *pb) >> q->queued_count++; >> q->waiting_for_buffers = false; >> vb->state = VB2_BUF_STATE_QUEUED; >> + vb->request = req; >> >> if (pb) >> call_void_bufop(q, copy_timestamp, vb, pb); >> @@ -1401,8 +1421,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int >> index, void *pb) >> /* >>* If already streaming, give the buffer to driver for processing. >>* If not, the buffer will be given to driver on next streamon. >> + * >> + * If using the request API, the buffer will be given to the driver >> + * when the request becomes active. >>*/ >> - if (q->start_streaming_called) >> + if (q->start_streaming_called && !req) >> __enqueue_in_driver(vb); >> >> /* Fill buffer information for the userspace */ >> @@ -1427,6 +1450,28 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int >> index, void *pb) >> } >> EXPORT_SYMBOL_GPL(vb2_core_qbuf); >> >> +void vb2_queue_start_request(struct vb2_queue *q, struct media_request *req)
Re: [RFC PATCH 0/9] media: base request API support
On 01/15/2018 09:24 AM, Alexandre Courbot wrote: > Hi Hans, > > On Fri, Jan 12, 2018 at 8:45 PM, Hans Verkuil wrote: >> Hi Alexandre, >> >> On 12/15/17 08:56, Alexandre Courbot wrote: >>> Here is a new attempt at the request API, following the UAPI we agreed on in >>> Prague. Hopefully this can be used as the basis to move forward. >>> >>> This series only introduces the very basics of how requests work: allocate a >>> request, queue buffers to it, queue the request itself, wait for it to >>> complete, >>> reuse it. It does *not* yet use Hans' work with controls setting. I have >>> preferred to submit it this way for now as it allows us to concentrate on >>> the >>> basic request/buffer flow, which was harder to get properly than I initially >>> thought. I still have a gut feeling that it can be improved, with less >>> back-and- >>> forth into drivers. >>> >>> Plugging in controls support should not be too hard a task (basically just >>> apply >>> the saved controls when the request starts), and I am looking at it now. >>> >>> The resulting vim2m driver can be successfully used with requests, and my >>> tests >>> so far have been successful. >>> >>> There are still some rougher edges: >>> >>> * locking is currently quite coarse-grained >>> * too many #ifdef CONFIG_MEDIA_CONTROLLER in the code, as the request API >>> depends on it - I plan to craft the headers so that it becomes >>> unnecessary. >>> As it is, some of the code will probably not even compile if >>> CONFIG_MEDIA_CONTROLLER is not set >>> >>> But all in all I think the request flow should be clear and easy to review, >>> and >>> the possibility of custom queue and entity support implementations should >>> give >>> us the flexibility we need to support more specific use-cases (I expect the >>> generic implementations to be sufficient most of the time though). >>> >>> A very simple test program exercising this API is available here (don't >>> forget >>> to adapt the /dev/media0 hardcoding): >>> https://gist.github.com/Gnurou/dbc3776ed97ea7d4ce6041ea15eb0438 >>> >>> Looking forward to your feedback and comments! >> >> I think this will become more interesting when the control code is in. > > Definitely. > >> The main thing I've noticed with this patch series is that it is very codec >> oriented. Which in some ways is OK (after all, that's the first type of HW >> that we want to support), but the vb2 code in particular should be more >> generic. > > I don't want to expand too much into use-cases I do not master ; doing > so would be speculating about how the API will be used. But feel free > to point out where you think my focus on the codec use-case is not > future-proof. The vb2 framework is a core component and it should function properly with the request API for all drivers using it. Easiest would be to test this using vivid and vimc. The vb2 code is relatively simple: all it has to do is keep track of requests and buffers and queue them up to the driver at the right time. It doesn't really need to know if the driver is a codec or something else, it's all the same to the framework. Regards, Hans >> I would also recommend that you start preparing documentation patches: we >> can review that and make sure all the corner-cases are correctly documented. >> >> The public API changes are (I think) fairly limited, but the devil is in >> the details, so getting that reviewed early on will help you later. > > Yeah, I now regret to have submitted this series without > documentation. Won't do that mistake again. > >> It's a bit unfortunate that the fence patch series is also making vb2 >> changes, >> but I hope that will be merged fairly soon so you can develop on top of that >> series. > > The fence series may actually make things easier. The vb2 code of this > series is a bit confusing, and fences add a few extra constraints that > should make things more predictable. So I am looking forward to being > able to work on top of it. >
Re: [RFC PATCH 5/9] media: vb2: add support for requests
On 01/15/2018 09:24 AM, Alexandre Courbot wrote: > On Fri, Jan 12, 2018 at 7:49 PM, Hans Verkuil wrote: >> On 12/15/17 08:56, Alexandre Courbot wrote: >>> Add throttling support for buffers when requests are in use on a given >>> queue. Buffers associated to a request are kept into the vb2 queue until >>> the request becomes active, at which point all the buffers are passed to >>> the driver. The queue can also signal that is has processed all of a >>> request's buffers. >>> >>> Also add support for the request parameter when handling the QBUF ioctl. >>> >>> Signed-off-by: Alexandre Courbot >>> --- >>> drivers/media/v4l2-core/videobuf2-core.c | 59 >>> >>> drivers/media/v4l2-core/videobuf2-v4l2.c | 29 +++- >>> include/media/videobuf2-core.h | 25 +- >>> 3 files changed, 104 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c >>> b/drivers/media/v4l2-core/videobuf2-core.c >>> index cb115ba6a1d2..c01038b7962a 100644 >>> --- a/drivers/media/v4l2-core/videobuf2-core.c >>> +++ b/drivers/media/v4l2-core/videobuf2-core.c >>> @@ -898,6 +898,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum >>> vb2_buffer_state state) >>> state != VB2_BUF_STATE_REQUEUEING)) >>> state = VB2_BUF_STATE_ERROR; >>> >>> + WARN_ON(vb->request != q->cur_req); >> >> What's the reason for this WARN_ON? It's not immediately obvious to me. > > This is a safeguard against driver bugs: a buffer should not complete > unless it is part of the request being currently processed. > >> >>> + >>> #ifdef CONFIG_VIDEO_ADV_DEBUG >>> /* >>>* Although this is not a callback, it still does have to balance >>> @@ -920,6 +922,13 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum >>> vb2_buffer_state state) >>> /* Add the buffer to the done buffers list */ >>> list_add_tail(&vb->done_entry, &q->done_list); >>> vb->state = state; >>> + >>> + if (q->cur_req) { >>> + WARN_ON(q->req_buf_cnt < 1); >>> + >>> + if (--q->req_buf_cnt == 0) >>> + q->cur_req = NULL; >>> + } >>> } >>> atomic_dec(&q->owned_by_drv_count); >>> spin_unlock_irqrestore(&q->done_lock, flags); >>> @@ -1298,6 +1307,16 @@ int vb2_core_prepare_buf(struct vb2_queue *q, >>> unsigned int index, void *pb) >>> } >>> EXPORT_SYMBOL_GPL(vb2_core_prepare_buf); >>> >>> +static void vb2_queue_enqueue_current_buffers(struct vb2_queue *q) >>> +{ >>> + struct vb2_buffer *vb; >>> + >>> + list_for_each_entry(vb, &q->queued_list, queued_entry) { >>> + if (vb->request == q->cur_req) >>> + __enqueue_in_driver(vb); >>> + } >>> +} >> >> I think this will clash big time with the v4l2 fence patch series... > > Indeed, but on the other hand I was not a big fan of going through the > whole list. :) So I welcome the extra throttling introduced by the > fence series. There is only throttling if fences are used by userspace. Otherwise there is no change. > >> >>> + >>> /** >>> * vb2_start_streaming() - Attempt to start streaming. >>> * @q: videobuf2 queue >>> @@ -1318,8 +1337,7 @@ static int vb2_start_streaming(struct vb2_queue *q) >>>* If any buffers were queued before streamon, >>>* we can now pass them to driver for processing. >>>*/ >>> - list_for_each_entry(vb, &q->queued_list, queued_entry) >>> - __enqueue_in_driver(vb); >>> + vb2_queue_enqueue_current_buffers(q); >>> >>> /* Tell the driver to start streaming */ >>> q->start_streaming_called = 1; >>> @@ -1361,7 +1379,8 @@ static int vb2_start_streaming(struct vb2_queue *q) >>> return ret; >>> } >>> >>> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb) >>> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, >>> + struct media_request *req, void *pb) >>> { >>> struct vb2_buffer *vb; >>> int ret; >>> @@ -1392,6 +1411,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int >>> index, void *pb) >>> q->queued_count++; >>> q->waiting_for_buffers = false; >>> vb->state = VB2_BUF_STATE_QUEUED; >>> + vb->request = req; >>> >>> if (pb) >>> call_void_bufop(q, copy_timestamp, vb, pb); >>> @@ -1401,8 +1421,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int >>> index, void *pb) >>> /* >>>* If already streaming, give the buffer to driver for processing. >>>* If not, the buffer will be given to driver on next streamon. >>> + * >>> + * If using the request API, the buffer will be given to the driver >>> + * when the request becomes active. >>>*/ >>> - if (q->start_streaming_called) >>> + if (q->start_streaming_called && !req) >>> __enqueue_in_driver(vb); >>> >>> /
Re: [RFC PATCH 6/9] media: vb2: add support for requests in QBUF ioctl
On 01/15/2018 09:24 AM, Alexandre Courbot wrote: > On Fri, Jan 12, 2018 at 8:37 PM, Hans Verkuil wrote: >> On 12/15/17 08:56, Alexandre Courbot wrote: >>> Support the request argument of the QBUF ioctl. >>> >>> Signed-off-by: Alexandre Courbot >>> --- >>> drivers/media/v4l2-core/v4l2-ioctl.c | 93 >>> +++- >>> 1 file changed, 92 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c >>> b/drivers/media/v4l2-core/v4l2-ioctl.c >>> index 8d041247e97f..28f9c368563e 100644 >>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >>> @@ -29,6 +29,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> #include >>> >>> @@ -965,6 +966,81 @@ static int check_fmt(struct file *file, enum >>> v4l2_buf_type type) >>> return -EINVAL; >>> } >>> >>> +/* >>> + * Validate that a given request can be used during an ioctl. >>> + * >>> + * When using the request API, request file descriptors must be matched >>> against >>> + * the actual request object. User-space can pass any file descriptor, so >>> we >>> + * need to make sure the call is valid before going further. >>> + * >>> + * This function looks up the request and associated data and performs the >>> + * following sanity checks: >>> + * >>> + * * Make sure that the entity supports requests, >>> + * * Make sure that the entity belongs to the media_device managing the >>> passed >>> + * request, >>> + * * Make sure that the entity data (if any) is associated to the current >>> file >>> + * handler. >>> + * >>> + * This function returns a pointer to the valid request, or and error code >>> in >>> + * case of failure. When successful, a reference to the request is >>> acquired and >>> + * must be properly released. >>> + */ >>> +#ifdef CONFIG_MEDIA_CONTROLLER >>> +static struct media_request * >>> +check_request(int request, struct file *file, void *fh) >>> +{ >>> + struct media_request *req = NULL; >>> + struct video_device *vfd = video_devdata(file); >>> + struct v4l2_fh *vfh = >>> + test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL; >>> + struct media_entity *entity = &vfd->entity; >>> + const struct media_entity *ent; >>> + struct media_request_entity_data *data; >>> + bool found = false; >>> + >>> + if (!entity) >>> + return ERR_PTR(-EINVAL); >>> + >>> + /* Check that the entity supports requests */ >>> + if (!entity->req_ops) >>> + return ERR_PTR(-ENOTSUPP); >>> + >>> + req = media_request_get_from_fd(request); >> >> You can get the media_device from vfd->v4l2_dev->mdev. So it is much easier >> to just pass the media_device as an argument to >> media_request_get_from_fd()... >> >>> + if (!req) >>> + return ERR_PTR(-EINVAL); >>> + >>> + /* Validate that the entity belongs to the media_device managing >>> + * the request queue */ >>> + media_device_for_each_entity(ent, req->queue->mdev) { >>> + if (entity == ent) { >>> + found = true; >>> + break; >>> + } >>> + } >>> + if (!found) { >>> + media_request_put(req); >>> + return ERR_PTR(-EINVAL); >>> + } >> >> ...and then you don't need to do this ^^^ extra validation check. > > Ah right, all you need to do is check that req->queue->mdev == > vfd->v4l2_dev->mdev and you can get rid of this whole block. Correct. I don't > think we can do that in media_request_get_from_fd() though, since it > is called from other places where (IIUC) we don't have access to the > media_device. Yes, sorry about that. Ignore my comment about media_request_get_from_fd(). I misunderstood what that function did. > >> >>> + >>> + /* Validate that the entity's data belongs to the correct fh */ >>> + data = media_request_get_entity_data(req, entity, vfh); >>> + if (IS_ERR(data)) { >>> + media_request_put(req); >>> + return ERR_PTR(PTR_ERR(data)); >>> + } >> >> This assumes that each filehandle has its own state. That's true for codecs, >> but not for most (all?) other devices. There the state is per device >> instance. >> >> I'm not sure if we have a unique identifying mark for such drivers. The >> closest >> is checking if fh->m2m_ctx is non-NULL, but I don't know if all drivers with >> per-filehandle state use that field. An alternative might be to check if >> fh->ctrl_handler is non-NULL. But again, I'm not sure if that's a 100% valid >> check. > > I think the current code already takes that case into account: if the > device does not uses v4l2_fh, then the fh argument passed to > media_request_get_entity_data() will be null, and so will be data->fh. > Using v4l2_fh doesn't mean it has per-filehandle state. Most drivers only have global state and they still use v4l2_fh. This is why you should also look at vivid and vimc for the request API o
Re: [PATCH] dma-buf/sw_sync: fix document of sw_sync_create_fence_data
On Mon, Jan 15, 2018 at 11:47:59AM +0800, Shawn Guo wrote: > The structure should really be sw_sync_create_fence_data rather than > sw_sync_ioctl_create_fence which is the function name. > > Signed-off-by: Shawn Guo Applied, thanks for your patch. -Daniel > --- > drivers/dma-buf/sw_sync.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c > index 24f83f9eeaed..7779bdbd18d1 100644 > --- a/drivers/dma-buf/sw_sync.c > +++ b/drivers/dma-buf/sw_sync.c > @@ -43,14 +43,14 @@ > * timelines. > * > * Fences can be created with SW_SYNC_IOC_CREATE_FENCE ioctl with struct > - * sw_sync_ioctl_create_fence as parameter. > + * sw_sync_create_fence_data as parameter. > * > * To increment the timeline counter, SW_SYNC_IOC_INC ioctl should be used > * with the increment as u32. This will update the last signaled value > * from the timeline and signal any fence that has a seqno smaller or equal > * to it. > * > - * struct sw_sync_ioctl_create_fence > + * struct sw_sync_create_fence_data > * @value: the seqno to initialise the fence with > * @name:the name of the new sync point > * @fence: return the fd of the new sync_file with the created fence > -- > 1.9.1 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH 0/5] new driver for Ahanix D.Vine 5 IR/VFD
This is a newer driver for this device. It originally supported by the lirc_sasem.c staging driver, which was removed in kernel v4.12. Here a some more information about the hardware and my attempts to understand it: http://www.mess.org/2018/01/17/Ahanix-D-Vine-5-IR-VFD-module/ Sean Young (5): auxdisplay: charlcd: no need to call charlcd_gotoxy() if nothing changes auxdisplay: charlcd: add flush function auxdisplay: charlcd: add escape sequence for brightness on NEC µPD16314 media: rc: add keymap for Dign Remote media: rc: new driver for Sasem Remote Controller VFD/IR MAINTAINERS| 6 + drivers/auxdisplay/charlcd.c | 33 - drivers/media/rc/Kconfig | 16 ++ drivers/media/rc/Makefile | 1 + drivers/media/rc/keymaps/Makefile | 1 + drivers/media/rc/keymaps/rc-dign.c | 70 + drivers/media/rc/sasem_ir.c| 297 + include/media/rc-map.h | 1 + include/misc/charlcd.h | 1 + 9 files changed, 421 insertions(+), 5 deletions(-) create mode 100644 drivers/media/rc/keymaps/rc-dign.c create mode 100644 drivers/media/rc/sasem_ir.c -- 2.14.3
[PATCH 1/5] auxdisplay: charlcd: no need to call charlcd_gotoxy() if nothing changes
If the line extends beyond the width to the screen, nothing changes. The existing code will call charlcd_gotoxy every time for this case. Signed-off-by: Sean Young --- drivers/auxdisplay/charlcd.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c index 642afd88870b..45ec5ce697c4 100644 --- a/drivers/auxdisplay/charlcd.c +++ b/drivers/auxdisplay/charlcd.c @@ -192,10 +192,11 @@ static void charlcd_print(struct charlcd *lcd, char c) c = lcd->char_conv[(unsigned char)c]; lcd->ops->write_data(lcd, c); priv->addr.x++; + + /* prevents the cursor from wrapping onto the next line */ + if (priv->addr.x == lcd->bwidth) + charlcd_gotoxy(lcd); } - /* prevents the cursor from wrapping onto the next line */ - if (priv->addr.x == lcd->bwidth) - charlcd_gotoxy(lcd); } static void charlcd_clear_fast(struct charlcd *lcd) -- 2.14.3
[PATCH 3/5] auxdisplay: charlcd: add escape sequence for brightness on NEC µPD16314
The NEC µPD16314 can alter the the brightness of the LCD. Make it possible to set this via escape sequence Y0 - Y3. B and R were already taken, so I picked Y for luminance. Signed-off-by: Sean Young --- drivers/auxdisplay/charlcd.c | 20 ++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c index a16c72779722..7a671ad959d1 100644 --- a/drivers/auxdisplay/charlcd.c +++ b/drivers/auxdisplay/charlcd.c @@ -39,6 +39,8 @@ #define LCD_FLAG_F 0x0020 /* Large font mode */ #define LCD_FLAG_N 0x0040 /* 2-rows mode */ #define LCD_FLAG_L 0x0080 /* Backlight enabled */ +#define LCD_BRIGHTNESS_MASK0x0300 /* Brightness */ +#define LCD_BRIGHTNESS_SHIFT 8 /* LCD commands */ #define LCD_CMD_DISPLAY_CLEAR 0x01/* Clear entire display */ @@ -490,6 +492,17 @@ static inline int handle_lcd_special_code(struct charlcd *lcd) charlcd_gotoxy(lcd); processed = 1; break; + case 'Y': /* brightness (luma) */ + switch (esc[1]) { + case '0': /* 25% */ + case '1': /* 50% */ + case '2': /* 75% */ + case '3': /* 100% */ + priv->flags = (priv->flags & ~(LCD_BRIGHTNESS_MASK)) | + (('3' - esc[1]) << LCD_BRIGHTNESS_SHIFT); + processed = 1; + break; + } } /* TODO: This indent party here got ugly, clean it! */ @@ -507,12 +520,15 @@ static inline int handle_lcd_special_code(struct charlcd *lcd) ((priv->flags & LCD_FLAG_C) ? LCD_CMD_CURSOR_ON : 0) | ((priv->flags & LCD_FLAG_B) ? LCD_CMD_BLINK_ON : 0)); /* check whether one of F,N flags was changed */ - else if ((oldflags ^ priv->flags) & (LCD_FLAG_F | LCD_FLAG_N)) + else if ((oldflags ^ priv->flags) & (LCD_FLAG_F | LCD_FLAG_N | +LCD_BRIGHTNESS_MASK)) lcd->ops->write_cmd(lcd, LCD_CMD_FUNCTION_SET | ((lcd->ifwidth == 8) ? LCD_CMD_DATA_LEN_8BITS : 0) | ((priv->flags & LCD_FLAG_F) ? LCD_CMD_FONT_5X10_DOTS : 0) | - ((priv->flags & LCD_FLAG_N) ? LCD_CMD_TWO_LINES : 0)); + ((priv->flags & LCD_FLAG_N) ? LCD_CMD_TWO_LINES : 0) | + ((priv->flags & LCD_BRIGHTNESS_MASK) >> + LCD_BRIGHTNESS_SHIFT)); /* check whether L flag was changed */ else if ((oldflags ^ priv->flags) & LCD_FLAG_L) charlcd_backlight(lcd, !!(priv->flags & LCD_FLAG_L)); -- 2.14.3
[PATCH 5/5] media: rc: new driver for Sasem Remote Controller VFD/IR
This device is built into the Ahanix D.Vine 5 HTPC case. It has an LCD device, and an IR receiver. The LCD can be controlled via the charlcd driver. Unfortunately the device does not seem to provide a method for accessing the character generator ram. Signed-off-by: Sean Young --- MAINTAINERS | 6 + drivers/media/rc/Kconfig| 16 +++ drivers/media/rc/Makefile | 1 + drivers/media/rc/sasem_ir.c | 297 4 files changed, 320 insertions(+) create mode 100644 drivers/media/rc/sasem_ir.c diff --git a/MAINTAINERS b/MAINTAINERS index 58797b83dd8d..a06801032ee3 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12066,6 +12066,12 @@ F: drivers/phy/samsung/phy-s5pv210-usb2.c F: drivers/phy/samsung/phy-samsung-usb2.c F: drivers/phy/samsung/phy-samsung-usb2.h +SASEM REMOTE CONTROLLER +M: Sean Young +L: linux-media@vger.kernel.org +S: Maintained +F: drivers/media/rc/sasem_ir.c + SC1200 WDT DRIVER M: Zwane Mwaikambo S: Maintained diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig index 7919f4a36ad2..bffa39e06a68 100644 --- a/drivers/media/rc/Kconfig +++ b/drivers/media/rc/Kconfig @@ -457,6 +457,22 @@ config IR_SERIAL To compile this driver as a module, choose M here: the module will be called serial-ir. +config IR_SASEM + tristate "Sasem Remote Controller" + depends on USB_ARCH_HAS_HCD + depends on RC_CORE + select USB + select CHARLCD + ---help--- + Driver for the Sasem OnAir Remocon-V or Dign HV5 HTPC IR/VFD Module + + The LCD can be controlled via the charlcd driver. Unfortunately the + device does not seem to provide a method for accessing the + character generator ram. + + To compile this driver as a module, choose M here: the module will + be called sesam_ir. + config IR_SERIAL_TRANSMITTER bool "Serial Port Transmitter" default y diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile index e098e127b26a..9b474c8b49dc 100644 --- a/drivers/media/rc/Makefile +++ b/drivers/media/rc/Makefile @@ -41,6 +41,7 @@ obj-$(CONFIG_IR_TTUSBIR) += ttusbir.o obj-$(CONFIG_RC_ST) += st_rc.o obj-$(CONFIG_IR_SUNXI) += sunxi-cir.o obj-$(CONFIG_IR_IMG) += img-ir/ +obj-$(CONFIG_IR_SASEM) += sasem_ir.o obj-$(CONFIG_IR_SERIAL) += serial_ir.o obj-$(CONFIG_IR_SIR) += sir_ir.o obj-$(CONFIG_IR_MTK) += mtk-cir.o diff --git a/drivers/media/rc/sasem_ir.c b/drivers/media/rc/sasem_ir.c new file mode 100644 index ..33d3b8bdb56d --- /dev/null +++ b/drivers/media/rc/sasem_ir.c @@ -0,0 +1,297 @@ +// SPDX-License-Identifier: GPL-2.0+ +// +// Copyright (C) 2018 Sean Young + +#include +#include +#include +#include + +#include + +struct sasem { + struct device *dev; + struct urb *ir_urb; + struct urb *vfd_urb; + struct rc_dev *rcdev; + struct completion completion; + u8 ir_buf[8]; + u8 vfd_buf[8]; + unsigned int offset; + char phys[64]; +}; + +static void sasem_ir_rx(struct urb *urb) +{ + struct sasem *sasem = urb->context; + enum rc_proto proto; + u32 code; + int ret; + + switch (urb->status) { + case 0: + dev_dbg(sasem->dev, "data: %*ph", 8, sasem->ir_buf); + /* +* Note that sanyo and jvc protocols are also supported, +* but the scancode seems garbled. More testing needed. +*/ + switch (sasem->ir_buf[0]) { + case 0xc: + code = ir_nec_bytes_to_scancode(sasem->ir_buf[1], + sasem->ir_buf[2], sasem->ir_buf[3], + sasem->ir_buf[4], &proto); + rc_keydown(sasem->rcdev, proto, code, 0); + break; + case 0x8: + rc_repeat(sasem->rcdev); + break; + } + break; + case -ECONNRESET: + case -ENOENT: + case -ESHUTDOWN: + usb_unlink_urb(urb); + return; + case -EPIPE: + default: + dev_dbg(sasem->dev, "error: urb status = %d", urb->status); + break; + } + + ret = usb_submit_urb(urb, GFP_ATOMIC); + if (ret && ret != -ENODEV) + dev_warn(sasem->dev, "failed to resubmit urb: %d", ret); +} + +static void sasem_vfd_complete(struct urb *urb) +{ + struct sasem *sasem = urb->context; + + if (urb->status) + dev_info(sasem->dev, "error: vfd urb status = %d", urb->status); + + complete(&sasem->completion); +} + +static int sasem_vfd_send(struct sasem *sasem) +{ + int ret; + + reinit_completion(&sasem->completion); + + ret = usb_submit_urb(sasem->vfd_urb, GFP_KERNEL); + if (ret) + return ret; + + if (wait_for_c
[PATCH 4/5] media: rc: add keymap for Dign Remote
This is the remote which comes with the Ahanix D.Vine 5 HTPC case. Signed-off-by: Sean Young --- drivers/media/rc/keymaps/Makefile | 1 + drivers/media/rc/keymaps/rc-dign.c | 70 ++ include/media/rc-map.h | 1 + 3 files changed, 72 insertions(+) create mode 100644 drivers/media/rc/keymaps/rc-dign.c diff --git a/drivers/media/rc/keymaps/Makefile b/drivers/media/rc/keymaps/Makefile index 50b319355edf..5a7aebe12285 100644 --- a/drivers/media/rc/keymaps/Makefile +++ b/drivers/media/rc/keymaps/Makefile @@ -29,6 +29,7 @@ obj-$(CONFIG_RC_MAP) += rc-adstech-dvb-t-pci.o \ rc-dib0700-rc5.o \ rc-digitalnow-tinytwin.o \ rc-digittrade.o \ + rc-dign.o \ rc-dm1105-nec.o \ rc-dntv-live-dvb-t.o \ rc-dntv-live-dvbt-pro.o \ diff --git a/drivers/media/rc/keymaps/rc-dign.c b/drivers/media/rc/keymaps/rc-dign.c new file mode 100644 index ..a70bb5f89278 --- /dev/null +++ b/drivers/media/rc/keymaps/rc-dign.c @@ -0,0 +1,70 @@ +// SPDX-License-Identifier: GPL-2.0+ +// +// Keymap for Dign Remote for Ahanix D.Vine 5 Case +// +// Copyright (C) 2018 by Sean Young + +#include +#include + +static struct rc_map_table dign[] = { + { 0x8000, KEY_POWER }, + { 0x8002, KEY_EJECTCD }, + { 0x8008, KEY_1 }, + { 0x8009, KEY_2 }, + { 0x800a, KEY_3 }, + { 0x8010, KEY_4 }, + { 0x8011, KEY_5 }, + { 0x8012, KEY_6 }, + { 0x8018, KEY_7 }, + { 0x8019, KEY_8 }, + { 0x801a, KEY_9 }, + + { 0x8040, KEY_ENTER }, /* Start/Windows */ + { 0x8041, KEY_0 }, + { 0x8006, KEY_MENU }, + + { 0x800f, KEY_VOLUMEDOWN }, + { 0x800e, KEY_VOLUMEUP }, + + { 0x8005, KEY_ESC }, + { 0x8007, KEY_CLOSE }, + { 0x800b, KEY_UP }, + { 0x8003, KEY_LEFT }, + { 0x801b, KEY_RIGHT }, + { 0x8043, KEY_DOWN }, + { 0x8013, KEY_ENTER }, + + { 0x800c, KEY_PREVIOUSSONG }, + { 0x8001, KEY_NEXTSONG }, + + { 0x800d, KEY_MUTE }, + { 0x8004, KEY_FRAMEFORWARD }, /* Step */ + { 0x8049, KEY_PLAYPAUSE }, + { 0x8048, KEY_STOP }, +}; + +static struct rc_map_list dign_map = { + .map = { + .scan = dign, + .size = ARRAY_SIZE(dign), + .rc_proto = RC_PROTO_NEC, + .name = RC_MAP_DIGN, + } +}; + +static int __init init_rc_map_dign(void) +{ + return rc_map_register(&dign_map); +} + +static void __exit exit_rc_map_dign(void) +{ + rc_map_unregister(&dign_map); +} + +module_init(init_rc_map_dign) +module_exit(exit_rc_map_dign) + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Sean Young "); diff --git a/include/media/rc-map.h b/include/media/rc-map.h index 7046734b3895..1c0016af45a1 100644 --- a/include/media/rc-map.h +++ b/include/media/rc-map.h @@ -185,6 +185,7 @@ struct rc_map *rc_map_get(const char *name); #define RC_MAP_DIB0700_RC5_TABLE "rc-dib0700-rc5" #define RC_MAP_DIGITALNOW_TINYTWIN "rc-digitalnow-tinytwin" #define RC_MAP_DIGITTRADE"rc-digittrade" +#define RC_MAP_DIGN "rc-dign" #define RC_MAP_DM1105_NEC"rc-dm1105-nec" #define RC_MAP_DNTV_LIVE_DVBT_PRO"rc-dntv-live-dvbt-pro" #define RC_MAP_DNTV_LIVE_DVB_T "rc-dntv-live-dvb-t" -- 2.14.3
[PATCH 2/5] auxdisplay: charlcd: add flush function
The Sasem Remote Controller has an LCD, which is connnected via usb. Multiple write reg or write data commands can be combined into one usb packet. The latency of usb is such that if we send commands one by one, we get very obvious tearing on the LCD. By adding a flush function, we can buffer all commands until either the usb packet is full or the lcd changes are complete. Signed-off-by: Sean Young --- drivers/auxdisplay/charlcd.c | 6 ++ include/misc/charlcd.h | 1 + 2 files changed, 7 insertions(+) diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c index 45ec5ce697c4..a16c72779722 100644 --- a/drivers/auxdisplay/charlcd.c +++ b/drivers/auxdisplay/charlcd.c @@ -642,6 +642,9 @@ static ssize_t charlcd_write(struct file *file, const char __user *buf, charlcd_write_char(the_charlcd, c); } + if (the_charlcd->ops->flush) + the_charlcd->ops->flush(the_charlcd); + return tmp - buf; } @@ -703,6 +706,9 @@ static void charlcd_puts(struct charlcd *lcd, const char *s) charlcd_write_char(lcd, *tmp); } + + if (lcd->ops->flush) + lcd->ops->flush(lcd); } /* initialize the LCD driver */ diff --git a/include/misc/charlcd.h b/include/misc/charlcd.h index 23f61850f363..ff8fd456018e 100644 --- a/include/misc/charlcd.h +++ b/include/misc/charlcd.h @@ -32,6 +32,7 @@ struct charlcd_ops { void (*write_cmd_raw4)(struct charlcd *lcd, int cmd); /* 4-bit only */ void (*clear_fast)(struct charlcd *lcd); void (*backlight)(struct charlcd *lcd, int on); + void (*flush)(struct charlcd *lcd); }; struct charlcd *charlcd_alloc(unsigned int drvdata_size); -- 2.14.3
Re: MT9M131 on I.MX6DL CSI color issue
Hello Anatolij, many thanks for explaining. It changed something at least - see below. On 12.01.2018 11:06, Anatolij Gustschin wrote: > On Fri, 12 Jan 2018 10:58:40 +0100 > Anatolij Gustschin ag...@denx.de wrote: > ... > I forgot the videoconvert, sorry. Try > > gst-launch v4l2src device=/dev/video4 num-buffers=1 ! \ > filesink location=frame.raw > > gst-launch filesrc num-buffers=1 Lotion=frame.raw ! \ > videoparse format=5 width=1280 height=1024 framerate=25/1 ! \ > videoconvert ! jpegenc ! filesink location=capture1.jpeg Capturing like this the colors turn a little bit less psychedelic green and purple. Looks like this: http://www.kernelconcepts.de/~florian/capture2.jpeg The dark area is in fact a very bright one. So maybe the format I read from the sensor is not exactly what it is supposed to be or similar... Greetings Florian -- The dream of yesterday Florian Boor is the hope of todayTel: +49 271-771091-15 and the reality of tomorrow.Fax: +49 271-338857-29 [Robert Hutchings Goddard, 1904]florian.b...@kernelconcepts.de http://www.kernelconcepts.de/en kernel concepts GmbH Hauptstraße 16 D-57074 Siegen Geschäftsführer: Ole Reinhardt HR Siegen, HR B 9613
Re: [PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers
2018-01-15 Alexandre Courbot : > On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan wrote: > > From: Gustavo Padovan > > > > Explicit synchronization benefits a lot from ordered queues, they fit > > better in a pipeline with DRM for example so create a opt-in way for > > drivers notify videobuf2 that the queue is unordered. > > > > Drivers don't need implement it if the queue is ordered. > > This is going to make user-space believe that *all* vb2 drivers use > ordered queues by default, at least until non-ordered drivers catch up > with this change. Wouldn't it be less dangerous to do the opposite > (make queues non-ordered by default)? The rational behind this decision was because most formats/drivers are ordered so only a small amount of drivers need to changed. I think this was proposed by Hans on the Media Summit. I understand your concern. My question is how dangerous will it be. If you are building a product you will make the changes in the driver if they are not there yet, or if it is a distribution you'd never know which driver/format you are using so you should be prepared for everything. AFAIK all Capture drivers are ordered and that is where I think fences is most useful. Gustavo
Re: [PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers
On 01/15/2018 01:01 PM, Gustavo Padovan wrote: > 2018-01-15 Alexandre Courbot : > >> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan wrote: >>> From: Gustavo Padovan >>> >>> Explicit synchronization benefits a lot from ordered queues, they fit >>> better in a pipeline with DRM for example so create a opt-in way for >>> drivers notify videobuf2 that the queue is unordered. >>> >>> Drivers don't need implement it if the queue is ordered. >> >> This is going to make user-space believe that *all* vb2 drivers use >> ordered queues by default, at least until non-ordered drivers catch up >> with this change. Wouldn't it be less dangerous to do the opposite >> (make queues non-ordered by default)? > > The rational behind this decision was because most formats/drivers are > ordered so only a small amount of drivers need to changed. I think this > was proposed by Hans on the Media Summit. > > I understand your concern. My question is how dangerous will it be. If > you are building a product you will make the changes in the driver if > they are not there yet, or if it is a distribution you'd never know > which driver/format you are using so you should be prepared for > everything. > > AFAIK all Capture drivers are ordered and that is where I think fences > is most useful. Right. What could be done is to mark all codec drivers as unordered initially ask the driver authors to verify this. All capture drivers using vb2 and not using REQUEUE are ordered. One thing we haven't looked at is what to do with drivers that do not use vb2. Those won't support fences, but how will userspace know that fences are not supported? I'm not sure what the best method is for that. I am leaning towards a new capability since this has to be advertised clearly. Regards, Hans
Re: [PATCH v6 4/6] media: i2c: Add TDA1997x HDMI receiver driver
On 12/28/2017 09:09 PM, Tim Harvey wrote: > Add support for the TDA1997x HDMI receivers. > > Cc: Hans Verkuil > Signed-off-by: Tim Harvey > --- > v6: > - fix return on regulator enablei in tda1997x_set_power() (Fabio) > - replace copyright with SPDX tag (Philippe) > - fix colorspace handling (Hans) > > v5: > - uppercase string constants > - use v4l2_hdmi_rx_coloriemtry to fill format > - fix V4L2_CID_DV_RX_RGB_RANGE > - fix interlaced mode format > > v4: > - move include/dt-bindings/media/tda1997x.h to bindings patch > - fix typos > - fix default quant range for VGA > - fix quant range handling and conv matrix > - add additional standards and capabilities to timings_cap > > v3: > - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros > - fixed missing break > - use only hdmi_infoframe_log for infoframe logging > - simplify tda1997x_s_stream error handling > - add delayed work proc to handle hotplug enable/disable > - fix set_edid (disable HPD before writing, enable after) > - remove enabling edid by default > - initialize timings > - take quant range into account in colorspace conversion > - remove vendor/product tracking (we provide this in log_status via > infoframes) > - add v4l_controls > - add more detail to log_status > - calculate vhref generator timings > - timing detection fixes (rounding errors, hswidth errors) > - rename configure_input/configure_conv functions > > v2: > - implement dv timings enum/cap > - remove deprecated g_mbus_config op > - fix dv_query_timings > - add EDID get/set handling > - remove max-pixel-rate support > - add audio codec DAI support > - change audio bindings > --- > drivers/media/i2c/Kconfig|9 + > drivers/media/i2c/Makefile |1 + > drivers/media/i2c/tda1997x.c | 3476 > ++ > include/media/i2c/tda1997x.h | 41 + > 4 files changed, 3527 insertions(+) > create mode 100644 drivers/media/i2c/tda1997x.c > create mode 100644 include/media/i2c/tda1997x.h > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 3c6d642..abf24b9 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -56,6 +56,15 @@ config VIDEO_TDA9840 > To compile this driver as a module, choose M here: the > module will be called tda9840. > > +config VIDEO_TDA1997X > + tristate "NXP TDA1997x HDMI receiver" > + depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API > + ---help--- > + V4L2 subdevice driver for the NXP TDA1997x HDMI receivers. > + > + To compile this driver as a module, choose M here: the > + module will be called tda1997x. > + > config VIDEO_TEA6415C > tristate "Philips TEA6415C audio processor" > depends on I2C > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index 548a9ef..adfcae9 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -13,6 +13,7 @@ obj-$(CONFIG_VIDEO_TVAUDIO) += tvaudio.o > obj-$(CONFIG_VIDEO_TDA7432) += tda7432.o > obj-$(CONFIG_VIDEO_SAA6588) += saa6588.o > obj-$(CONFIG_VIDEO_TDA9840) += tda9840.o > +obj-$(CONFIG_VIDEO_TDA1997X) += tda1997x.o > obj-$(CONFIG_VIDEO_TEA6415C) += tea6415c.o > obj-$(CONFIG_VIDEO_TEA6420) += tea6420.o > obj-$(CONFIG_VIDEO_SAA7110) += saa7110.o > diff --git a/drivers/media/i2c/tda1997x.c b/drivers/media/i2c/tda1997x.c > new file mode 100644 > index 000..0993c13 > --- /dev/null > +++ b/drivers/media/i2c/tda1997x.c > @@ -0,0 +1,3476 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2017 Gateworks Corporation > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > + > +#include > + > +/* debug level */ > +static int debug; > +module_param(debug, int, 0644); > +MODULE_PARM_DESC(debug, "debug level (0-2)"); > + > +/* Page 0x00 - General Control */ > +#define REG_VERSION 0x > +#define REG_INPUT_SEL0x0001 > +#define REG_SVC_MODE 0x0002 > +#define REG_HPD_MAN_CTRL 0x0003 > +#define REG_RT_MAN_CTRL 0x0004 > +#define REG_STANDBY_SOFT_RST 0x000A > +#define REG_HDMI_SOFT_RST0x000B > +#define REG_HDMI_INFO_RST0x000C > +#define REG_INT_FLG_CLR_TOP 0x000E > +#define REG_INT_FLG_CLR_SUS 0x000F > +#define REG_INT_FLG_CLR_DDC 0x0010 > +#define REG_INT_FLG_CLR_RATE 0x0011 > +#define REG_INT_FLG_CLR_MODE 0x0012 > +#define REG_INT_FLG_CLR_INFO 0x0013 > +#define REG_INT_FLG_CLR_AUDIO0x0014 > +#define REG_INT_FLG_CLR_HDCP 0x0015 > +#define REG_INT_FLG_CLR_AFE 0x0016 > +#define REG_INT_MASK_TOP 0x0017 > +#define REG_INT_MASK_SUS 0x0018 > +#define REG_INT_MASK_DDC 0x0019 > +#define REG_INT_MASK_RATE0x001A > +#define REG_INT_MASK_MODE0x001B > +#define R
Re: [PATCH v2] vsp1: fix video output on R8A77970
Hi Sergei, Thank you for the patch. On Tuesday, 26 December 2017 23:14:12 EET Sergei Shtylyov wrote: > Laurent has added support for the VSP2-D found on R-Car V3M (R8A77970) but I'm not sure there's a need to state my name in the commit message. > the video output that VSP2-D sends to DU has a greenish garbage-like line Why does the text in your patches (commit message, comments, ...) sometime have double spaces between words ? > repeated every 8 or so screen rows. Is it every "8 or so" rows, or exactly every 8 rows ? > It turns out that V3M has a teeny LIF register (at least it's documented!) > that you need to set to some kind of a magic value for the LIF to work > correctly... > > Based on the original (and large) patch by Daisuke Matsushita > . What else is in the big patch ? Is it available somewhere ? > Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and > VSP2-D instances") > Signed-off-by: Sergei Shtylyov > > --- > This patch is against the 'media_tree.git' repo's 'master' branch. > > Changes in version 2: > - added a comment before the V3M SoC check; > - fixed indetation in that check; > - reformatted the patch description. > > drivers/media/platform/vsp1/vsp1_lif.c | 12 > drivers/media/platform/vsp1/vsp1_regs.h |5 + > 2 files changed, 17 insertions(+) > > Index: media_tree/drivers/media/platform/vsp1/vsp1_lif.c > === > --- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c > +++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c > @@ -155,6 +155,18 @@ static void lif_configure(struct vsp1_en > (obth << VI6_LIF_CTRL_OBTH_SHIFT) | > (format->code == 0 ? VI6_LIF_CTRL_CFMT : 0) | > VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN); > + > + /* > + * R-Car V3M has the buffer attribute register you absolutely need > + * to write kinda magic value to for the LIF to work correctly... > + */ I'm not sure about the "kinda" magic value. 1536 is very likely a buffer size. How about the following text ? /* * On V3M the LBA register has to be set to a non-default value to * guarantee proper operation (otherwise artifacts may appear on the * output). The value required by the datasheet is not documented but * is likely a buffer size or threshold. */ The commit message should also be updated to feel a bit less magic. > + if ((entity->vsp1->version & > + (VI6_IP_VERSION_MODEL_MASK | VI6_IP_VERSION_SOC_MASK)) == > + (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) { > + vsp1_lif_write(lif, dl, VI6_LIF_LBA, > +VI6_LIF_LBA_LBA0 | > +(1536 << VI6_LIF_LBA_LBA1_SHIFT)); > + } > } The datasheet documents the register as being present on both V3M and M3-W (and the test I've just run on H3 shows that the register is present there as well). Should we program it on M3-W or leave it to the default value that should be what is recommended by the datasheet for that SoC ? > static const struct vsp1_entity_operations lif_entity_ops = { > Index: media_tree/drivers/media/platform/vsp1/vsp1_regs.h > === > --- media_tree.orig/drivers/media/platform/vsp1/vsp1_regs.h > +++ media_tree/drivers/media/platform/vsp1/vsp1_regs.h > @@ -693,6 +693,11 @@ > #define VI6_LIF_CSBTH_LBTH_MASK (0x7ff << 0) > #define VI6_LIF_CSBTH_LBTH_SHIFT 0 > > +#define VI6_LIF_LBA 0x3b0c > +#define VI6_LIF_LBA_LBA0 (1 << 31) > +#define VI6_LIF_LBA_LBA1_MASK(0xfff << 16) > +#define VI6_LIF_LBA_LBA1_SHIFT 16 > + > /* > * Security Control Registers > */ -- Regards, Laurent Pinchart
Re: MT9M131 on I.MX6DL CSI color issue
Hi Florian, On Fri, 2018-01-12 at 01:16 +0100, Florian Boor wrote: > Hello all, > > I have a Phytec VM-009 camera based on MT9M131 connected to CSI0 of a I.MX6DL > based board running mainline 4.13.0 + custom devicetree. Its using the > parallel > interface, 8 bit bus width on pins 12 to 19. > > Basically it works pretty well apart from the really strange colors. I guess > its > some YUV vs. RGB issue or similar. Here [1] is an example generated with the > following command. > > gst-launch v4l2src device=/dev/video4 num-buffers=1 ! jpegenc ! filesink > location=capture1.jpeg > > Apart from the colors everything is fine. > I'm pretty sure I have not seen such an effect before - what might be wrong > here? > > The current setup looks like this: > > IF=UYVY2X8 > GEOM="1280x1024" > media-ctl -l "'mt9m111 2-0048':0 -> 'ipu1_csi0_mux':4[1]" > media-ctl -l "'ipu1_csi0_mux':5 -> 'ipu1_csi0':0[1]" > media-ctl -l "'ipu1_csi0':2 -> 'ipu1_csi0 capture':0[1]" > > media-ctl -d /dev/media0 -v -V "'ipu1_csi0':2 [fmt:${IF}/${GEOM} field:none]" > media-ctl -d /dev/media0 -v -V "'ipu1_csi0 capture':0 [fmt:${IF}/${GEOM} > field:none]" > media-ctl -d /dev/media0 -v -V "'ipu1_csi0_mux':4 [fmt:${IF}/${GEOM} field: > none]" > media-ctl -d /dev/media0 -v -V "'ipu1_csi0_mux':5 [fmt:${IF}/${GEOM} field: > none]" > media-ctl -d /dev/media0 -v -V "'mt9m111 2-0048':0 [fmt:${IF}/${GEOM} field: > none]" media-ctl propagates video formats downstream, can you try reversing the order? Also, while the external format is UYVY2X8, internally the IPU only supports AYUV32, so the last call should be media-ctl -d /dev/media0 -v -V "'ipu1_csi0':2 [fmt:AYUV32/${GEOM} field:none]" not that it should make a difference. And setting a format on 'ipu1_csi0 capture' is not necessary. The new picture looks a little like there is 10-bit sensor data and only the lower 8-bit arrive in memory, given the number of wraparounds. Can you show the output of "media-ctl -p" (or "media-ctl --get-v4l2" for each pad in the pipeline)? media-ctl --get-v4l2 "'mt9m111 2-0048':0" media-ctl --get-v4l2 "'ipu1_csi0_mux':4" media-ctl --get-v4l2 "'ipu1_csi0_mux':5" media-ctl --get-v4l2 "'ipu1_csi0':0" media-ctl --get-v4l2 "'ipu1_csi0':2" regards Philipp
Re: MT9M131 on I.MX6DL CSI color issue
Hi Philipp, On 15.01.2018 13:49, Philipp Zabel wrote: > media-ctl propagates video formats downstream, can you try reversing the > order? I did but it does not make a difference. > Also, while the external format is UYVY2X8, internally the IPU only > supports AYUV32, so the last call should be > > media-ctl -d /dev/media0 -v -V "'ipu1_csi0':2 [fmt:AYUV32/${GEOM} field:none]" > not that it should make a visible difference. > And setting a format on 'ipu1_csi0 capture' is not necessary. Changed this as well. What I do now is the following: SF="UYVY2X8" IF="AYUV32" GEOM="1280x1024" media-ctl -r media-ctl -l "'mt9m111 2-0048':0 -> 'ipu1_csi0_mux':4[1]" media-ctl -l "'ipu1_csi0_mux':5 -> 'ipu1_csi0':0[1]" media-ctl -l "'ipu1_csi0':2 -> 'ipu1_csi0 capture':0[1]" media-ctl -d /dev/media0 -v -V "'mt9m111 2-0048':0 [fmt:${SF}/${GEOM} field: none]" media-ctl -d /dev/media0 -v -V "'ipu1_csi0_mux':4 [fmt:${SF}/${GEOM} field: none]" media-ctl -d /dev/media0 -v -V "'ipu1_csi0_mux':5 [fmt:${SF}/${GEOM} field: none]" media-ctl -d /dev/media0 -v -V "'ipu1_csi0':2 [fmt:${IF}/${GEOM} field:none]" > The new picture looks a little like there is 10-bit sensor data and only > the lower 8-bit arrive in memory, given the number of wraparounds. I will take a look at the sensor configuration. Maybe there is some issue or a difference among all th MT9M1x1 semsors the driver does not support. > Can you show the output of "media-ctl -p" (or "media-ctl --get-v4l2" for > each pad in the pipeline)? > media-ctl --get-v4l2 "'mt9m111 2-0048':0" [fmt:UYVY2X8/1280x1024 field:none] crop.bounds:(26,8)/1280x1024 crop:(26,8)/1280x1024] > media-ctl --get-v4l2 "'ipu1_csi0_mux':4" [fmt:UYVY2X8/1280x1024 field:none] > media-ctl --get-v4l2 "'ipu1_csi0_mux':5" [fmt:UYVY2X8/1280x1024 field:none] > media-ctl --get-v4l2 "'ipu1_csi0':0" [fmt:UYVY2X8/1280x1024 field:none crop.bounds:(0,0)/1280x1024 crop:(0,0)/1280x1024 compose.bounds:(0,0)/1280x1024 compose:(0,0)/1280x1024] > media-ctl --get-v4l2 "'ipu1_csi0':2" [fmt:AYUV32/1280x1024 field:none] I uploaded the complete topology output from media-ctrl -p as well [1]. Greetings Florian [1] http://www.kernelconcepts.de/~florian/media-ctl-topology.txt -- The dream of yesterday Florian Boor is the hope of todayTel: +49 271-771091-15 and the reality of tomorrow.Fax: +49 271-338857-29 [Robert Hutchings Goddard, 1904]florian.b...@kernelconcepts.de http://www.kernelconcepts.de/en kernel concepts GmbH Hauptstraße 16 D-57074 Siegen Geschäftsführer: Ole Reinhardt HR Siegen, HR B 9613
Re: [PATCH 3/4] tsi108_eth: use dma API properly
On Wed, Jan 10, 2018 at 10:09:20PM +0200, Andy Shevchenko wrote: > > + struct platform_device *pdev; > > Do you really need platform_defice reference? > > Perhaps > > struct device *hdev; // hardware device > > > data->hdev = &pdev->dev; > > Another idea > > dev->dev.parent = &pdev->dev; > > No new member needed. Maybe. But what I've done is the simplest change in a long obsolete driver that I don't understand at all. I'd rather keep it simple.
Re: [PATCH v2] vsp1: fix video output on R8A77970
Hello! On 01/15/2018 03:51 PM, Laurent Pinchart wrote: On Tuesday, 26 December 2017 23:14:12 EET Sergei Shtylyov wrote: Laurent has added support for the VSP2-D found on R-Car V3M (R8A77970) but I'm not sure there's a need to state my name in the commit message. You were the author of the patch this one has in the Fixes: tag, so I thought that was appropriate. I can remove that if you don't want you name mentioned... the video output that VSP2-D sends to DU has a greenish garbage-like line Why does the text in your patches (commit message, comments, ...) sometime have double spaces between words ? The text looks more pleasant (at least to me). Can remove them if you want... repeated every 8 or so screen rows. Is it every "8 or so" rows, or exactly every 8 rows ? You really want me to count the pixels? It turns out that V3M has a teeny LIF register (at least it's documented!) that you need to set to some kind of a magic value for the LIF to work correctly... Based on the original (and large) patch by Daisuke Matsushita . What else is in the big patch ? Is it available somewhere ? Assorted changes gathered together only because they all bring the support for R8A77970. If you're really curious, here you are: https://github.com/CogentEmbedded/meta-rcar/blob/v2.12.0/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0030-arm64-renesas-r8a7797-Add-Renesas-R8A7797-SoC-suppor.patch Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and VSP2-D instances") Signed-off-by: Sergei Shtylyov --- This patch is against the 'media_tree.git' repo's 'master' branch. Changes in version 2: - added a comment before the V3M SoC check; - fixed indetation in that check; - reformatted the patch description. drivers/media/platform/vsp1/vsp1_lif.c | 12 drivers/media/platform/vsp1/vsp1_regs.h |5 + 2 files changed, 17 insertions(+) Index: media_tree/drivers/media/platform/vsp1/vsp1_lif.c === --- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c +++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c @@ -155,6 +155,18 @@ static void lif_configure(struct vsp1_en (obth << VI6_LIF_CTRL_OBTH_SHIFT) | (format->code == 0 ? VI6_LIF_CTRL_CFMT : 0) | VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN); + + /* +* R-Car V3M has the buffer attribute register you absolutely need +* to write kinda magic value to for the LIF to work correctly... +*/ I'm not sure about the "kinda" magic value. 1536 is very likely a buffer size. Well, that's only guessing. The manual doesn't say anything about what the number is. How about the following text ? /* * On V3M the LBA register has to be set to a non-default value to I'd then spell its full name, LIF0 Buffer Attribute register. * guarantee proper operation (otherwise artifacts may appear on the * output). The value required by the datasheet is not documented but * is likely a buffer size or threshold. */ > OK, can change that (modulo the name). The commit message should also be updated to feel a bit less magic. I'll see about it. + if ((entity->vsp1->version & +(VI6_IP_VERSION_MODEL_MASK | VI6_IP_VERSION_SOC_MASK)) == + (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) { + vsp1_lif_write(lif, dl, VI6_LIF_LBA, + VI6_LIF_LBA_LBA0 | + (1536 << VI6_LIF_LBA_LBA1_SHIFT)); + } } The datasheet documents the register as being present on both V3M and M3-W (and the test I've just run on H3 shows that the register is present there as well). Should we program it on M3-W or leave it to the default value that should be what is recommended by the datasheet for that SoC ? If the default value matches what's recommended by the manual, then I'd leave the register alone. But my task was R8A77970 support only anyway... [..] MBR, Sergei
Re: [PATCH] media: staging/imx: Checking the right variable in vdic_get_ipu_resources()
Acked-by: Steve Longerbeam On 01/15/2018 12:11 AM, Dan Carpenter wrote: We recently changed this error handling around but missed this error pointer check. We're testing "priv->vdi_in_ch_n" instead of "ch" so the error handling can't be triggered. Fixes: 0b2e9e7947e7 ("media: staging/imx: remove confusing IS_ERR_OR_NULL usage") Signed-off-by: Dan Carpenter diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c index 433474d58e3e..ed356844cdf6 100644 --- a/drivers/staging/media/imx/imx-media-vdic.c +++ b/drivers/staging/media/imx/imx-media-vdic.c @@ -177,7 +177,7 @@ static int vdic_get_ipu_resources(struct vdic_priv *priv) priv->vdi_in_ch = ch; ch = ipu_idmac_get(priv->ipu, IPUV3_CHANNEL_MEM_VDI_NEXT); - if (IS_ERR(priv->vdi_in_ch_n)) { + if (IS_ERR(ch)) { err_chan = IPUV3_CHANNEL_MEM_VDI_NEXT; ret = PTR_ERR(ch); goto out_err_chan;
[PATCH v5 0/9] vsp1: TLB optimisation and DL caching
Each display list currently allocates an area of DMA memory to store register settings for the VSP1 to process. Each of these allocations adds pressure to the IPMMU TLB entries. We can reduce the pressure by pre-allocating larger areas and dividing the area across multiple bodies represented as a pool. With this reconfiguration of bodies, we can adapt the configuration code to separate out constant hardware configuration and cache it for re-use. -- The patches provided in this series can be found at: git://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git tags/vsp1/tlb-optimise/v5 This series is temporarily based on the renesas-drivers-2018-01-09-v4.15-rc7 release tag, until changes for the DRM and UIF make it upstream. Changelog: -- v5: - Rebased on to renesas-drivers-2018-01-09-v4.15-rc7 to fix conflicts with DRM and UIF updates on VSP1 driver v4: - Rebased to v4.14 * v4l: vsp1: Use reference counting for bodies - Fix up reference handling comments * v4l: vsp1: Provide a body pool - Provide comment explaining extra allocation on body pool highlighting area for optimisation later. * v4l: vsp1: Refactor display list configure operations - Fix up comment to describe yuv_mode caching rather than format * vsp1: Adapt entities to configure into a body - Rename vsp1_dl_list_get_body() to vsp1_dl_list_get_body0() * v4l: vsp1: Move video configuration to a cached dlb - Adjust pipe configured flag to be reset on resume rather than suspend - rename dl_child, dl_next Testing: The VSP unit tests have been run on this patch set with the following results: - vsp-unit-test-.sh Test Conditions: Platform Renesas Salvator-X 2nd version board based on r8a7795 ES2.0+ Kernel release4.15.0-rc7-arm64-renesas-9-gfa58cc7e4788 yavta /usr/bin/yavta convert /usr/bin/convert compare /usr/bin/compare killall /usr/bin/killall raw2rgbpnm/usr/bin/raw2rgbpnm - vsp-unit-test-0001.sh Testing WPF packing in RGB332: pass Testing WPF packing in ARGB555: pass Testing WPF packing in XRGB555: pass Testing WPF packing in RGB565: pass Testing WPF packing in BGR24: pass Testing WPF packing in RGB24: pass Testing WPF packing in ABGR32: pass Testing WPF packing in ARGB32: pass Testing WPF packing in XBGR32: pass Testing WPF packing in XRGB32: pass - vsp-unit-test-0002.sh Testing WPF packing in NV12M: pass Testing WPF packing in NV16M: pass Testing WPF packing in NV21M: pass Testing WPF packing in NV61M: pass Testing WPF packing in UYVY: pass Testing WPF packing in VYUY: skip Testing WPF packing in YUV420M: pass Testing WPF packing in YUV422M: pass Testing WPF packing in YUV444M: pass Testing WPF packing in YVU420M: pass Testing WPF packing in YVU422M: pass Testing WPF packing in YVU444M: pass Testing WPF packing in YUYV: pass Testing WPF packing in YVYU: pass - vsp-unit-test-0003.sh Testing scaling from 640x640 to 640x480 in RGB24: pass Testing scaling from 1024x768 to 640x480 in RGB24: pass Testing scaling from 640x480 to 1024x768 in RGB24: pass Testing scaling from 640x640 to 640x480 in YUV444M: pass Testing scaling from 1024x768 to 640x480 in YUV444M: pass Testing scaling from 640x480 to 1024x768 in YUV444M: pass - vsp-unit-test-0004.sh Testing histogram in RGB24: pass Testing histogram in YUV444M: pass - vsp-unit-test-0005.sh Testing RPF.0: pass Testing RPF.1: pass Testing RPF.2: pass Testing RPF.3: pass Testing RPF.4: pass - vsp-unit-test-0006.sh Testing invalid pipeline with no RPF: pass Testing invalid pipeline with no WPF: pass - vsp-unit-test-0007.sh Testing BRU in RGB24 with 1 inputs: pass Testing BRU in RGB24 with 2 inputs: pass Testing BRU in RGB24 with 3 inputs: pass Testing BRU in RGB24 with 4 inputs: pass Testing BRU in RGB24 with 5 inputs: pass Testing BRU in YUV444M with 1 inputs: pass Testing BRU in YUV444M with 2 inputs: pass Testing BRU in YUV444M with 3 inputs: pass Testing BRU in YUV444M with 4 inputs: pass Testing BRU in YUV444M with 5 inputs: pass - vsp-unit-test-0008.sh Test requires unavailable feature set `bru rpf.0 uds wpf.0': skipped - vsp-unit-test-0009.sh Test requires unavailable feature set `rpf.0 wpf.0 wpf.1': skipped - vsp-unit-test-0010.sh Testing CLU in RGB24 with zero configuration: pass Testing CLU in RGB24 with identity configuration: pass Testing CLU in RGB24 with wave configuration: pass Testing CLU in YUV444M with zero configuration: pass Testing CLU in YUV444M with identity configuration: pass Testing CLU in YUV444M with wave configuration: pass Testing LUT in RGB24 with zero configuration: pass Testing LUT in RGB24 with identity configuration: pass Testing LUT in RGB24 with gamma configuration: pass Testing LUT in YUV444M with zero configuration: pass Testing LUT in YUV444M with identity configuration: pass Testing LUT in YUV444M with gamma configuration: pass - vsp-unit-test-0011.sh Testing hflip=0 vflip=0 rotate=0: pass Testing hflip=1 vflip=0
[PATCH v5 3/9] v4l: vsp1: Provide a body pool
Each display list allocates a body to store register values in a dma accessible buffer from a dma_alloc_wc() allocation. Each of these results in an entry in the TLB, and a large number of display list allocations adds pressure to this resource. Reduce TLB pressure on the IPMMUs by allocating multiple display list bodies in a single allocation, and providing these to the display list through a 'body pool'. A pool can be allocated by the display list manager or entities which require their own body allocations. Signed-off-by: Kieran Bingham --- v4: - Provide comment explaining extra allocation on body pool highlighting area for optimisation later. v3: - s/fragment/body/, s/fragments/bodies/ - qty -> num_bodies - indentation fix - s/vsp1_dl_body_pool_{alloc,free}/vsp1_dl_body_pool_{create,destroy}/' - Add kerneldoc to non-static functions v2: - assign dlb->dma correctly --- drivers/media/platform/vsp1/vsp1_dl.c | 163 +++- drivers/media/platform/vsp1/vsp1_dl.h | 8 +- 2 files changed, 171 insertions(+) diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c index ecc3659a7884..4e71792d183c 100644 --- a/drivers/media/platform/vsp1/vsp1_dl.c +++ b/drivers/media/platform/vsp1/vsp1_dl.c @@ -45,6 +45,8 @@ struct vsp1_dl_entry { /** * struct vsp1_dl_body - Display list body * @list: entry in the display list list of bodies + * @free: entry in the pool free body list + * @pool: pool to which this body belongs * @vsp1: the VSP1 device * @entries: array of entries * @dma: DMA address of the entries @@ -54,6 +56,9 @@ struct vsp1_dl_entry { */ struct vsp1_dl_body { struct list_head list; + struct list_head free; + + struct vsp1_dl_body_pool *pool; struct vsp1_device *vsp1; struct vsp1_dl_entry *entries; @@ -65,6 +70,30 @@ struct vsp1_dl_body { }; /** + * struct vsp1_dl_body_pool - display list body pool + * @dma: DMA address of the entries + * @size: size of the full DMA memory pool in bytes + * @mem: CPU memory pointer for the pool + * @bodies: Array of DLB structures for the pool + * @free: List of free DLB entries + * @lock: Protects the pool and free list + * @vsp1: the VSP1 device + */ +struct vsp1_dl_body_pool { + /* DMA allocation */ + dma_addr_t dma; + size_t size; + void *mem; + + /* Body management */ + struct vsp1_dl_body *bodies; + struct list_head free; + spinlock_t lock; + + struct vsp1_device *vsp1; +}; + +/** * struct vsp1_dl_list - Display list * @list: entry in the display list manager lists * @dlm: the display list manager @@ -105,6 +134,7 @@ enum vsp1_dl_mode { * @active: list currently being processed (loaded) by hardware * @queued: list queued to the hardware (written to the DL registers) * @pending: list waiting to be queued to the hardware + * @pool: body pool for the display list bodies * @gc_work: bodies garbage collector work struct * @gc_bodies: array of display list bodies waiting to be freed */ @@ -120,6 +150,8 @@ struct vsp1_dl_manager { struct vsp1_dl_list *queued; struct vsp1_dl_list *pending; + struct vsp1_dl_body_pool *pool; + struct work_struct gc_work; struct list_head gc_bodies; }; @@ -128,6 +160,137 @@ struct vsp1_dl_manager { * Display List Body Management */ +/** + * vsp1_dl_body_pool_create - Create a pool of bodies from a single allocation + * @vsp1: The VSP1 device + * @num_bodies: The quantity of bodies to allocate + * @num_entries: The maximum number of entries that the body can contain + * @extra_size: Extra allocation provided for the bodies + * + * Allocate a pool of display list bodies each with enough memory to contain the + * requested number of entries. + * + * Return a pointer to a pool on success or NULL if memory can't be allocated. + */ +struct vsp1_dl_body_pool * +vsp1_dl_body_pool_create(struct vsp1_device *vsp1, unsigned int num_bodies, +unsigned int num_entries, size_t extra_size) +{ + struct vsp1_dl_body_pool *pool; + size_t dlb_size; + unsigned int i; + + pool = kzalloc(sizeof(*pool), GFP_KERNEL); + if (!pool) + return NULL; + + pool->vsp1 = vsp1; + + /* +* Todo: 'extra_size' is only used by vsp1_dlm_create(), to allocate +* extra memory for the display list header. We need only one header per +* display list, not per display list body, thus this allocation is +* extraneous and should be reworked in the future. +*/ + dlb_size = num_entries * sizeof(struct vsp1_dl_entry) + extra_size; + pool->size = dlb_size * num_bodies; + + pool->bodies = kcalloc(num_bodies, sizeof(*pool->bodies), GFP_KERNEL); + if (!pool->bodies) { + kfree(pool); + return NULL; + } + + pool->mem = dma_alloc_wc(vsp1->bus_master, pool->size, &pool->dma, +
[PATCH v5 1/9] v4l: vsp1: Reword uses of 'fragment' as 'body'
Throughout the codebase, the term 'fragment' is used to represent a display list body. This term duplicates the 'body' which is already in use. The datasheet references these objects as a body, therefore replace all mentions of a fragment with a body, along with the corresponding pluralised terms. Signed-off-by: Kieran Bingham --- drivers/media/platform/vsp1/vsp1_clu.c | 10 +- drivers/media/platform/vsp1/vsp1_dl.c | 107 -- drivers/media/platform/vsp1/vsp1_dl.h | 14 +-- drivers/media/platform/vsp1/vsp1_lut.c | 8 +- 4 files changed, 69 insertions(+), 70 deletions(-) diff --git a/drivers/media/platform/vsp1/vsp1_clu.c b/drivers/media/platform/vsp1/vsp1_clu.c index bc931a3ab498..246dd595c978 100644 --- a/drivers/media/platform/vsp1/vsp1_clu.c +++ b/drivers/media/platform/vsp1/vsp1_clu.c @@ -47,19 +47,19 @@ static int clu_set_table(struct vsp1_clu *clu, struct v4l2_ctrl *ctrl) struct vsp1_dl_body *dlb; unsigned int i; - dlb = vsp1_dl_fragment_alloc(clu->entity.vsp1, 1 + 17 * 17 * 17); + dlb = vsp1_dl_body_alloc(clu->entity.vsp1, 1 + 17 * 17 * 17); if (!dlb) return -ENOMEM; - vsp1_dl_fragment_write(dlb, VI6_CLU_ADDR, 0); + vsp1_dl_body_write(dlb, VI6_CLU_ADDR, 0); for (i = 0; i < 17 * 17 * 17; ++i) - vsp1_dl_fragment_write(dlb, VI6_CLU_DATA, ctrl->p_new.p_u32[i]); + vsp1_dl_body_write(dlb, VI6_CLU_DATA, ctrl->p_new.p_u32[i]); spin_lock_irq(&clu->lock); swap(clu->clu, dlb); spin_unlock_irq(&clu->lock); - vsp1_dl_fragment_free(dlb); + vsp1_dl_body_free(dlb); return 0; } @@ -215,7 +215,7 @@ static void clu_configure(struct vsp1_entity *entity, spin_unlock_irqrestore(&clu->lock, flags); if (dlb) - vsp1_dl_list_add_fragment(dl, dlb); + vsp1_dl_list_add_body(dl, dlb); break; } } diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c index 4257451f1bd8..90e972a75c62 100644 --- a/drivers/media/platform/vsp1/vsp1_dl.c +++ b/drivers/media/platform/vsp1/vsp1_dl.c @@ -69,7 +69,7 @@ struct vsp1_dl_body { * @header: display list header, NULL for headerless lists * @dma: DMA address for the header * @body0: first display list body - * @fragments: list of extra display list bodies + * @bodies: list of extra display list bodies * @has_chain: if true, indicates that there's a partition chain * @chain: entry in the display list partition chain */ @@ -81,7 +81,7 @@ struct vsp1_dl_list { dma_addr_t dma; struct vsp1_dl_body body0; - struct list_head fragments; + struct list_head bodies; bool has_chain; struct list_head chain; @@ -98,13 +98,13 @@ enum vsp1_dl_mode { * @mode: display list operation mode (header or headerless) * @singleshot: execute the display list in single-shot mode * @vsp1: the VSP1 device - * @lock: protects the free, active, queued, pending and gc_fragments lists + * @lock: protects the free, active, queued, pending and gc_bodies lists * @free: array of all free display lists * @active: list currently being processed (loaded) by hardware * @queued: list queued to the hardware (written to the DL registers) * @pending: list waiting to be queued to the hardware - * @gc_work: fragments garbage collector work struct - * @gc_fragments: array of display list fragments waiting to be freed + * @gc_work: bodies garbage collector work struct + * @gc_bodies: array of display list bodies waiting to be freed */ struct vsp1_dl_manager { unsigned int index; @@ -119,7 +119,7 @@ struct vsp1_dl_manager { struct vsp1_dl_list *pending; struct work_struct gc_work; - struct list_head gc_fragments; + struct list_head gc_bodies; }; /* - @@ -157,17 +157,16 @@ static void vsp1_dl_body_cleanup(struct vsp1_dl_body *dlb) } /** - * vsp1_dl_fragment_alloc - Allocate a display list fragment + * vsp1_dl_body_alloc - Allocate a display list body * @vsp1: The VSP1 device - * @num_entries: The maximum number of entries that the fragment can contain + * @num_entries: The maximum number of entries that the body can contain * - * Allocate a display list fragment with enough memory to contain the requested + * Allocate a display list body with enough memory to contain the requested * number of entries. * - * Return a pointer to a fragment on success or NULL if memory can't be - * allocated. + * Return a pointer to a body on success or NULL if memory can't be allocated. */ -struct vsp1_dl_body *vsp1_dl_fragment_alloc(struct vsp1_device *vsp1, +struct vsp1_dl_body *vsp1_dl_body_alloc(struct vsp1_device *vsp1, unsigned int num_entries) { struct vsp1_dl_body *dlb; @@ -18
[PATCH v5 2/9] v4l: vsp1: Protect bodies against overflow
The body write function relies on the code never asking it to write more than the entries available in the list. Currently with each list body containing 256 entries, this is fine, but we can reduce this number greatly saving memory. In preparation of this add a level of protection to catch any buffer overflows. Signed-off-by: Kieran Bingham Reviewed-by: Laurent Pinchart --- v3: - adapt for new 'body' terminology - simplify WARN_ON macro usage --- drivers/media/platform/vsp1/vsp1_dl.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c index 90e972a75c62..ecc3659a7884 100644 --- a/drivers/media/platform/vsp1/vsp1_dl.c +++ b/drivers/media/platform/vsp1/vsp1_dl.c @@ -50,6 +50,7 @@ struct vsp1_dl_entry { * @dma: DMA address of the entries * @size: size of the DMA memory in bytes * @num_entries: number of stored entries + * @max_entries: number of entries available */ struct vsp1_dl_body { struct list_head list; @@ -60,6 +61,7 @@ struct vsp1_dl_body { size_t size; unsigned int num_entries; + unsigned int max_entries; }; /** @@ -139,6 +141,7 @@ static int vsp1_dl_body_init(struct vsp1_device *vsp1, dlb->vsp1 = vsp1; dlb->size = size; + dlb->max_entries = num_entries; dlb->entries = dma_alloc_wc(vsp1->bus_master, dlb->size, &dlb->dma, GFP_KERNEL); @@ -220,6 +223,10 @@ void vsp1_dl_body_free(struct vsp1_dl_body *dlb) */ void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32 reg, u32 data) { + if (WARN_ONCE(dlb->num_entries >= dlb->max_entries, + "DLB size exceeded (max %u)", dlb->max_entries)) + return; + dlb->entries[dlb->num_entries].addr = reg; dlb->entries[dlb->num_entries].data = data; dlb->num_entries++; -- git-series 0.9.1
[PATCH v5 8/9] v4l: vsp1: Move video configuration to a cached dlb
We are now able to configure a pipeline directly into a local display list body. Take advantage of this fact, and create a cacheable body to store the configuration of the pipeline in the video object. vsp1_video_pipeline_run() is now the last user of the pipe->dl object. Convert this function to use the cached video->config body and obtain a local display list reference. Attach the video->config body to the display list when needed before committing to hardware. The pipe object is marked as un-configured when resuming from a suspend. This ensures that when the hardware is reset - our cached configuration will be re-attached to the next committed DL. Signed-off-by: Kieran Bingham --- v3: - 's/fragment/body/', 's/fragments/bodies/' - video dlb cache allocation increased from 2 to 3 dlbs Our video DL usage now looks like the below output: dl->body0 contains our disposable runtime configuration. Max 41. dl_child->body0 is our partition specific configuration. Max 12. dl->bodies shows our constant configuration and LUTs. These two are LUT/CLU: * dl->bodies[x]->num_entries 256 / max 256 * dl->bodies[x]->num_entries 4914 / max 4914 Which shows that our 'constant' configuration cache is currently utilised to a maximum of 64 entries. trace-cmd report | \ grep max | sed 's/.*vsp1_dl_list_commit://g' | sort | uniq; dl->body0->num_entries 13 / max 128 dl->body0->num_entries 14 / max 128 dl->body0->num_entries 16 / max 128 dl->body0->num_entries 20 / max 128 dl->body0->num_entries 27 / max 128 dl->body0->num_entries 34 / max 128 dl->body0->num_entries 41 / max 128 dl_child->body0->num_entries 10 / max 128 dl_child->body0->num_entries 12 / max 128 dl->bodies[x]->num_entries 15 / max 128 dl->bodies[x]->num_entries 16 / max 128 dl->bodies[x]->num_entries 17 / max 128 dl->bodies[x]->num_entries 18 / max 128 dl->bodies[x]->num_entries 20 / max 128 dl->bodies[x]->num_entries 21 / max 128 dl->bodies[x]->num_entries 256 / max 256 dl->bodies[x]->num_entries 31 / max 128 dl->bodies[x]->num_entries 32 / max 128 dl->bodies[x]->num_entries 39 / max 128 dl->bodies[x]->num_entries 40 / max 128 dl->bodies[x]->num_entries 47 / max 128 dl->bodies[x]->num_entries 48 / max 128 dl->bodies[x]->num_entries 4914 / max 4914 dl->bodies[x]->num_entries 55 / max 128 dl->bodies[x]->num_entries 56 / max 128 dl->bodies[x]->num_entries 63 / max 128 dl->bodies[x]->num_entries 64 / max 128 v4: - Adjust pipe configured flag to be reset on resume rather than suspend - rename dl_child, dl_next --- drivers/media/platform/vsp1/vsp1_pipe.c | 7 +++- drivers/media/platform/vsp1/vsp1_pipe.h | 4 +- drivers/media/platform/vsp1/vsp1_video.c | 67 - drivers/media/platform/vsp1/vsp1_video.h | 2 +- 4 files changed, 54 insertions(+), 26 deletions(-) diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c b/drivers/media/platform/vsp1/vsp1_pipe.c index 5012643583b6..fa445b1a2e38 100644 --- a/drivers/media/platform/vsp1/vsp1_pipe.c +++ b/drivers/media/platform/vsp1/vsp1_pipe.c @@ -249,6 +249,7 @@ void vsp1_pipeline_run(struct vsp1_pipeline *pipe) vsp1_write(vsp1, VI6_CMD(pipe->output->entity.index), VI6_CMD_STRCMD); pipe->state = VSP1_PIPELINE_RUNNING; + pipe->configured = true; } pipe->buffers_ready = 0; @@ -470,6 +471,12 @@ void vsp1_pipelines_resume(struct vsp1_device *vsp1) continue; spin_lock_irqsave(&pipe->irqlock, flags); + /* +* The hardware may have been reset during a suspend and will +* need a full reconfiguration +*/ + pipe->configured = false; + if (vsp1_pipeline_ready(pipe)) vsp1_pipeline_run(pipe); spin_unlock_irqrestore(&pipe->irqlock, flags); diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h b/drivers/media/platform/vsp1/vsp1_pipe.h index 90d29492b9b9..e7ad6211b4d0 100644 --- a/drivers/media/platform/vsp1/vsp1_pipe.h +++ b/drivers/media/platform/vsp1/vsp1_pipe.h @@ -90,6 +90,7 @@ struct vsp1_partition { * @irqlock: protects the pipeline state * @state: current state * @wq: wait queue to wait for state change completion + * @configured: flag determining if the hardware has run since reset * @frame_end: frame end interrupt handler * @lock: protects the pipeline use count and stream count * @kref: pipeline reference count @@ -117,6 +118,7 @@ struct vsp1_pipeline { spinlock_t irqlock; enum vsp1_pipeline_state state; wait_queue_head_t wq; + bool configured; void (*frame_end)(struct vsp1_pipeline *pipe, bool completed); @@ -143,8 +145,6 @@ struct vsp1_pipeline { */ struct list_head entities; - struct vsp1_dl_list *dl; - unsigned int partitions; struct vsp1_partition *partition;
[PATCH v5 9/9] v4l: vsp1: Reduce display list body size
The display list originally allocated a body of 256 entries to store all of the register lists required for each frame. This has now been separated into fragments for constant stream setup, and runtime updates. Empirical testing shows that the body0 now uses a maximum of 41 registers for each frame, for both DRM and Video API pipelines thus a rounded 64 entries provides a suitable allocation. Signed-off-by: Kieran Bingham --- drivers/media/platform/vsp1/vsp1_dl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c index 1fc52496fc13..ce315821b60c 100644 --- a/drivers/media/platform/vsp1/vsp1_dl.c +++ b/drivers/media/platform/vsp1/vsp1_dl.c @@ -21,7 +21,7 @@ #include "vsp1.h" #include "vsp1_dl.h" -#define VSP1_DL_NUM_ENTRIES256 +#define VSP1_DL_NUM_ENTRIES64 #define VSP1_DLH_INT_ENABLE(1 << 1) #define VSP1_DLH_AUTO_START(1 << 0) -- git-series 0.9.1
[PATCH v5 4/9] v4l: vsp1: Convert display lists to use new body pool
Adapt the dl->body0 object to use an object from the body pool. This greatly reduces the pressure on the TLB for IPMMU use cases, as all of the lists use a single allocation for the main body. The CLU and LUT objects pre-allocate a pool containing three bodies, allowing a userspace update before the hardware has committed a previous set of tables. Bodies are no longer 'freed' in interrupt context, but instead released back to their respective pools. This allows us to remove the garbage collector in the DLM. Signed-off-by: Kieran Bingham --- v3: - 's/fragment/body', 's/fragments/bodies/' - CLU/LUT now allocate 3 bodies - vsp1_dl_list_fragments_free -> vsp1_dl_list_bodies_put v2: - Use dl->body0->max_entries to determine header offset, instead of the global constant VSP1_DL_NUM_ENTRIES which is incorrect. - squash updates for LUT, CLU, and fragment cleanup into single patch. (Not fully bisectable when separated) --- drivers/media/platform/vsp1/vsp1_clu.c | 27 ++- drivers/media/platform/vsp1/vsp1_clu.h | 1 +- drivers/media/platform/vsp1/vsp1_dl.c | 223 ++ drivers/media/platform/vsp1/vsp1_dl.h | 3 +- drivers/media/platform/vsp1/vsp1_lut.c | 27 ++- drivers/media/platform/vsp1/vsp1_lut.h | 1 +- 6 files changed, 101 insertions(+), 181 deletions(-) diff --git a/drivers/media/platform/vsp1/vsp1_clu.c b/drivers/media/platform/vsp1/vsp1_clu.c index 246dd595c978..a765d56c4118 100644 --- a/drivers/media/platform/vsp1/vsp1_clu.c +++ b/drivers/media/platform/vsp1/vsp1_clu.c @@ -23,6 +23,8 @@ #define CLU_MIN_SIZE 4U #define CLU_MAX_SIZE 8190U +#define CLU_SIZE (17 * 17 * 17) + /* - * Device Access */ @@ -47,19 +49,19 @@ static int clu_set_table(struct vsp1_clu *clu, struct v4l2_ctrl *ctrl) struct vsp1_dl_body *dlb; unsigned int i; - dlb = vsp1_dl_body_alloc(clu->entity.vsp1, 1 + 17 * 17 * 17); + dlb = vsp1_dl_body_get(clu->pool); if (!dlb) return -ENOMEM; vsp1_dl_body_write(dlb, VI6_CLU_ADDR, 0); - for (i = 0; i < 17 * 17 * 17; ++i) + for (i = 0; i < CLU_SIZE; ++i) vsp1_dl_body_write(dlb, VI6_CLU_DATA, ctrl->p_new.p_u32[i]); spin_lock_irq(&clu->lock); swap(clu->clu, dlb); spin_unlock_irq(&clu->lock); - vsp1_dl_body_free(dlb); + vsp1_dl_body_put(dlb); return 0; } @@ -220,8 +222,16 @@ static void clu_configure(struct vsp1_entity *entity, } } +static void clu_destroy(struct vsp1_entity *entity) +{ + struct vsp1_clu *clu = to_clu(&entity->subdev); + + vsp1_dl_body_pool_destroy(clu->pool); +} + static const struct vsp1_entity_operations clu_entity_ops = { .configure = clu_configure, + .destroy = clu_destroy, }; /* - @@ -247,6 +257,17 @@ struct vsp1_clu *vsp1_clu_create(struct vsp1_device *vsp1) if (ret < 0) return ERR_PTR(ret); + /* +* Pre-allocate a body pool, with 3 bodies allowing a userspace update +* before the hardware has committed a previous set of tables, handling +* both the queued and pending dl entries. One extra entry is added to +* the CLU_SIZE to allow for the VI6_CLU_ADDR header. +*/ + clu->pool = vsp1_dl_body_pool_create(clu->entity.vsp1, 3, CLU_SIZE + 1, +0); + if (!clu->pool) + return ERR_PTR(-ENOMEM); + /* Initialize the control handler. */ v4l2_ctrl_handler_init(&clu->ctrls, 2); v4l2_ctrl_new_custom(&clu->ctrls, &clu_table_control, NULL); diff --git a/drivers/media/platform/vsp1/vsp1_clu.h b/drivers/media/platform/vsp1/vsp1_clu.h index 036e0a2f1a42..fa3fe856725b 100644 --- a/drivers/media/platform/vsp1/vsp1_clu.h +++ b/drivers/media/platform/vsp1/vsp1_clu.h @@ -36,6 +36,7 @@ struct vsp1_clu { spinlock_t lock; unsigned int mode; struct vsp1_dl_body *clu; + struct vsp1_dl_body_pool *pool; }; static inline struct vsp1_clu *to_clu(struct v4l2_subdev *subdev) diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c index 4e71792d183c..961c3fd2ff1c 100644 --- a/drivers/media/platform/vsp1/vsp1_dl.c +++ b/drivers/media/platform/vsp1/vsp1_dl.c @@ -111,7 +111,7 @@ struct vsp1_dl_list { struct vsp1_dl_header *header; dma_addr_t dma; - struct vsp1_dl_body body0; + struct vsp1_dl_body *body0; struct list_head bodies; bool has_chain; @@ -135,8 +135,6 @@ enum vsp1_dl_mode { * @queued: list queued to the hardware (written to the DL registers) * @pending: list waiting to be queued to the hardware * @pool: body pool for the display list bodies - * @gc_work:
[PATCH v5 7/9] v4l: vsp1: Adapt entities to configure into a body
Currently the entities store their configurations into a display list. Adapt this such that the code can be configured into a body directly, allowing greater flexibility and control of the content. All users of vsp1_dl_list_write() are removed in this process, thus it too is removed. A helper, vsp1_dl_list_get_body0() is provided to access the internal body0 from the display list. Signed-off-by: Kieran Bingham --- v4: - Rename vsp1_dl_list_get_body() to vsp1_dl_list_get_body0() The similarities between vsp1_dl_list_get_body and vsp1_dl_list_body_get() were too close - body0 could be removed later when the default body is no longer needed. v5: - Support DRM/UIF changes --- drivers/media/platform/vsp1/vsp1_bru.c| 22 +-- drivers/media/platform/vsp1/vsp1_clu.c| 22 +-- drivers/media/platform/vsp1/vsp1_dl.c | 12 ++ drivers/media/platform/vsp1/vsp1_dl.h | 2 +- drivers/media/platform/vsp1/vsp1_drm.c| 24 +++- drivers/media/platform/vsp1/vsp1_entity.c | 16 drivers/media/platform/vsp1/vsp1_entity.h | 12 +++--- drivers/media/platform/vsp1/vsp1_hgo.c| 16 drivers/media/platform/vsp1/vsp1_hgt.c| 18 - drivers/media/platform/vsp1/vsp1_hsit.c | 10 ++--- drivers/media/platform/vsp1/vsp1_lif.c| 13 +++ drivers/media/platform/vsp1/vsp1_lut.c| 21 +-- drivers/media/platform/vsp1/vsp1_pipe.c | 4 +- drivers/media/platform/vsp1/vsp1_pipe.h | 3 +- drivers/media/platform/vsp1/vsp1_rpf.c| 47 +++- drivers/media/platform/vsp1/vsp1_sru.c| 14 +++ drivers/media/platform/vsp1/vsp1_uds.c| 24 ++-- drivers/media/platform/vsp1/vsp1_uds.h| 2 +- drivers/media/platform/vsp1/vsp1_uif.c| 18 - drivers/media/platform/vsp1/vsp1_video.c | 11 -- drivers/media/platform/vsp1/vsp1_wpf.c| 42 +++-- 21 files changed, 185 insertions(+), 168 deletions(-) diff --git a/drivers/media/platform/vsp1/vsp1_bru.c b/drivers/media/platform/vsp1/vsp1_bru.c index b9ff96f76b3e..60d449d7b135 100644 --- a/drivers/media/platform/vsp1/vsp1_bru.c +++ b/drivers/media/platform/vsp1/vsp1_bru.c @@ -30,10 +30,10 @@ * Device Access */ -static inline void vsp1_bru_write(struct vsp1_bru *bru, struct vsp1_dl_list *dl, - u32 reg, u32 data) +static inline void vsp1_bru_write(struct vsp1_bru *bru, + struct vsp1_dl_body *dlb, u32 reg, u32 data) { - vsp1_dl_list_write(dl, bru->base + reg, data); + vsp1_dl_body_write(dlb, bru->base + reg, data); } /* - @@ -287,7 +287,7 @@ static const struct v4l2_subdev_ops bru_ops = { static void bru_prepare(struct vsp1_entity *entity, struct vsp1_pipeline *pipe, - struct vsp1_dl_list *dl) + struct vsp1_dl_body *dlb) { struct vsp1_bru *bru = to_bru(&entity->subdev); struct v4l2_mbus_framefmt *format; @@ -309,7 +309,7 @@ static void bru_prepare(struct vsp1_entity *entity, * format at the pipeline output is premultiplied. */ flags = pipe->output ? pipe->output->format.flags : 0; - vsp1_bru_write(bru, dl, VI6_BRU_INCTRL, + vsp1_bru_write(bru, dlb, VI6_BRU_INCTRL, flags & V4L2_PIX_FMT_FLAG_PREMUL_ALPHA ? 0 : VI6_BRU_INCTRL_NRM); @@ -317,12 +317,12 @@ static void bru_prepare(struct vsp1_entity *entity, * Set the background position to cover the whole output image and * configure its color. */ - vsp1_bru_write(bru, dl, VI6_BRU_VIRRPF_SIZE, + vsp1_bru_write(bru, dlb, VI6_BRU_VIRRPF_SIZE, (format->width << VI6_BRU_VIRRPF_SIZE_HSIZE_SHIFT) | (format->height << VI6_BRU_VIRRPF_SIZE_VSIZE_SHIFT)); - vsp1_bru_write(bru, dl, VI6_BRU_VIRRPF_LOC, 0); + vsp1_bru_write(bru, dlb, VI6_BRU_VIRRPF_LOC, 0); - vsp1_bru_write(bru, dl, VI6_BRU_VIRRPF_COL, bru->bgcolor | + vsp1_bru_write(bru, dlb, VI6_BRU_VIRRPF_COL, bru->bgcolor | (0xff << VI6_BRU_VIRRPF_COL_A_SHIFT)); /* @@ -332,7 +332,7 @@ static void bru_prepare(struct vsp1_entity *entity, * unit. */ if (entity->type == VSP1_ENTITY_BRU) - vsp1_bru_write(bru, dl, VI6_BRU_ROP, + vsp1_bru_write(bru, dlb, VI6_BRU_ROP, VI6_BRU_ROP_DSTSEL_BRUIN(1) | VI6_BRU_ROP_CROP(VI6_ROP_NOP) | VI6_BRU_ROP_AROP(VI6_ROP_NOP)); @@ -374,7 +374,7 @@ static void bru_prepare(struct vsp1_entity *entity, if (!(entity->type == VSP1_ENTITY_BRU && i == 1)) ctrl |= VI6_BRU_CTRL_SRCSEL_BRUIN(i); - vsp1_bru_write(bru, dl, VI6_BRU_CTRL(i), ctrl); +
[PATCH v5 6/9] v4l: vsp1: Refactor display list configure operations
The entities provide a single .configure operation which configures the object into the target display list, based on the vsp1_entity_params selection. This restricts us to a single function prototype for both static configuration (the pre-stream INIT stage) and the dynamic runtime stages for both each frame - and each partition therein. Split the configure function into two parts, '.prepare()' and '.configure()', merging both the VSP1_ENTITY_PARAMS_RUNTIME and VSP1_ENTITY_PARAMS_PARTITION stages into a single call through the .configure(). The configuration for individual partitions is handled by passing the partition number to the configure call, and processing any runtime stage actions on the first partition only. Signed-off-by: Kieran Bingham --- drivers/media/platform/vsp1/vsp1_bru.c| 12 +- drivers/media/platform/vsp1/vsp1_clu.c| 42 +-- drivers/media/platform/vsp1/vsp1_dl.h | 1 +- drivers/media/platform/vsp1/vsp1_drm.c| 21 +-- drivers/media/platform/vsp1/vsp1_entity.c | 15 +- drivers/media/platform/vsp1/vsp1_entity.h | 27 +-- drivers/media/platform/vsp1/vsp1_hgo.c| 12 +- drivers/media/platform/vsp1/vsp1_hgt.c| 12 +- drivers/media/platform/vsp1/vsp1_hsit.c | 12 +- drivers/media/platform/vsp1/vsp1_lif.c| 12 +- drivers/media/platform/vsp1/vsp1_lut.c| 24 +- drivers/media/platform/vsp1/vsp1_rpf.c| 162 ++--- drivers/media/platform/vsp1/vsp1_sru.c| 12 +- drivers/media/platform/vsp1/vsp1_uds.c| 55 ++-- drivers/media/platform/vsp1/vsp1_uif.c| 20 +-- drivers/media/platform/vsp1/vsp1_video.c | 24 +-- drivers/media/platform/vsp1/vsp1_wpf.c| 297 --- 17 files changed, 368 insertions(+), 392 deletions(-) diff --git a/drivers/media/platform/vsp1/vsp1_bru.c b/drivers/media/platform/vsp1/vsp1_bru.c index e8fd2ae3b3eb..b9ff96f76b3e 100644 --- a/drivers/media/platform/vsp1/vsp1_bru.c +++ b/drivers/media/platform/vsp1/vsp1_bru.c @@ -285,19 +285,15 @@ static const struct v4l2_subdev_ops bru_ops = { * VSP1 Entity Operations */ -static void bru_configure(struct vsp1_entity *entity, - struct vsp1_pipeline *pipe, - struct vsp1_dl_list *dl, - enum vsp1_entity_params params) +static void bru_prepare(struct vsp1_entity *entity, + struct vsp1_pipeline *pipe, + struct vsp1_dl_list *dl) { struct vsp1_bru *bru = to_bru(&entity->subdev); struct v4l2_mbus_framefmt *format; unsigned int flags; unsigned int i; - if (params != VSP1_ENTITY_PARAMS_INIT) - return; - format = vsp1_entity_get_pad_format(&bru->entity, bru->entity.config, bru->entity.source_pad); @@ -404,7 +400,7 @@ static void bru_configure(struct vsp1_entity *entity, } static const struct vsp1_entity_operations bru_entity_ops = { - .configure = bru_configure, + .prepare = bru_prepare, }; /* - diff --git a/drivers/media/platform/vsp1/vsp1_clu.c b/drivers/media/platform/vsp1/vsp1_clu.c index 1142d004e238..006ad94bbe57 100644 --- a/drivers/media/platform/vsp1/vsp1_clu.c +++ b/drivers/media/platform/vsp1/vsp1_clu.c @@ -172,37 +172,36 @@ static const struct v4l2_subdev_ops clu_ops = { /* - * VSP1 Entity Operations */ +static void clu_prepare(struct vsp1_entity *entity, + struct vsp1_pipeline *pipe, + struct vsp1_dl_list *dl) +{ + struct vsp1_clu *clu = to_clu(&entity->subdev); + + /* +* The yuv_mode can't be changed during streaming. Cache it internally +* for future runtime configuration calls. +*/ + struct v4l2_mbus_framefmt *format; + + format = vsp1_entity_get_pad_format(&clu->entity, + clu->entity.config, + CLU_PAD_SINK); + clu->yuv_mode = format->code == MEDIA_BUS_FMT_AYUV8_1X32; +} static void clu_configure(struct vsp1_entity *entity, struct vsp1_pipeline *pipe, struct vsp1_dl_list *dl, - enum vsp1_entity_params params) + unsigned int partition) { struct vsp1_clu *clu = to_clu(&entity->subdev); struct vsp1_dl_body *dlb; unsigned long flags; u32 ctrl = VI6_CLU_CTRL_AAI | VI6_CLU_CTRL_MVS | VI6_CLU_CTRL_EN; - switch (params) { - case VSP1_ENTITY_PARAMS_INIT: { - /* -* The format can't be changed during streaming, only verify it -* at setup time and store the information internally for future -* runtime configuration calls. -*/ - struct v
[PATCH v5 5/9] v4l: vsp1: Use reference counting for bodies
Extend the display list body with a reference count, allowing bodies to be kept as long as a reference is maintained. This provides the ability to keep a cached copy of bodies which will not change, so that they can be re-applied to multiple display lists. Signed-off-by: Kieran Bingham --- This could be squashed into the body update code, but it's not a straightforward squash as the refcounts will affect both: v4l: vsp1: Provide a body pool and v4l: vsp1: Convert display lists to use new body pool therefore, I have kept this separate to prevent breaking bisectability of the vsp-tests. v3: - 's/fragment/body/' v4: - Fix up reference handling comments. --- drivers/media/platform/vsp1/vsp1_clu.c | 7 ++- drivers/media/platform/vsp1/vsp1_dl.c | 15 ++- drivers/media/platform/vsp1/vsp1_lut.c | 7 ++- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/drivers/media/platform/vsp1/vsp1_clu.c b/drivers/media/platform/vsp1/vsp1_clu.c index a765d56c4118..1142d004e238 100644 --- a/drivers/media/platform/vsp1/vsp1_clu.c +++ b/drivers/media/platform/vsp1/vsp1_clu.c @@ -216,8 +216,13 @@ static void clu_configure(struct vsp1_entity *entity, clu->clu = NULL; spin_unlock_irqrestore(&clu->lock, flags); - if (dlb) + if (dlb) { vsp1_dl_list_add_body(dl, dlb); + + /* release our local reference */ + vsp1_dl_body_put(dlb); + } + break; } } diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c index 961c3fd2ff1c..2784d3b48b02 100644 --- a/drivers/media/platform/vsp1/vsp1_dl.c +++ b/drivers/media/platform/vsp1/vsp1_dl.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -58,6 +59,8 @@ struct vsp1_dl_body { struct list_head list; struct list_head free; + refcount_t refcnt; + struct vsp1_dl_body_pool *pool; struct vsp1_device *vsp1; @@ -259,6 +262,7 @@ struct vsp1_dl_body *vsp1_dl_body_get(struct vsp1_dl_body_pool *pool) if (!list_empty(&pool->free)) { dlb = list_first_entry(&pool->free, struct vsp1_dl_body, free); list_del(&dlb->free); + refcount_set(&dlb->refcnt, 1); } spin_unlock_irqrestore(&pool->lock, flags); @@ -279,6 +283,9 @@ void vsp1_dl_body_put(struct vsp1_dl_body *dlb) if (!dlb) return; + if (!refcount_dec_and_test(&dlb->refcnt)) + return; + dlb->num_entries = 0; spin_lock_irqsave(&dlb->pool->lock, flags); @@ -465,7 +472,11 @@ void vsp1_dl_list_write(struct vsp1_dl_list *dl, u32 reg, u32 data) * in the order in which bodies are added. * * Adding a body to a display list passes ownership of the body to the list. The - * caller must not touch the body after this call. + * caller retains its reference to the fragment when adding it to the display + * list, but is not allowed to add new entries to the body. + * + * The reference must be explicitly released by a call to vsp1_dl_body_put() + * when the body isn't needed anymore. * * Additional bodies are only usable for display lists in header mode. * Attempting to add a body to a header-less display list will return an error. @@ -477,6 +488,8 @@ int vsp1_dl_list_add_body(struct vsp1_dl_list *dl, if (dl->dlm->mode != VSP1_DL_MODE_HEADER) return -EINVAL; + refcount_inc(&dlb->refcnt); + list_add_tail(&dlb->list, &dl->bodies); return 0; } diff --git a/drivers/media/platform/vsp1/vsp1_lut.c b/drivers/media/platform/vsp1/vsp1_lut.c index a9cf874312c1..7643f18b1ea6 100644 --- a/drivers/media/platform/vsp1/vsp1_lut.c +++ b/drivers/media/platform/vsp1/vsp1_lut.c @@ -172,8 +172,13 @@ static void lut_configure(struct vsp1_entity *entity, lut->lut = NULL; spin_unlock_irqrestore(&lut->lock, flags); - if (dlb) + if (dlb) { vsp1_dl_list_add_body(dl, dlb); + + /* release our local reference */ + vsp1_dl_body_put(dlb); + } + break; } } -- git-series 0.9.1
RE: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access
Hi, Tomasz, Thanks for the patch review. > -Original Message- > From: Tomasz Figa [mailto:tf...@chromium.org] > Sent: Friday, January 12, 2018 12:17 AM > To: Zhi, Yong > Cc: Linux Media Mailing List ; Sakari Ailus > ; Mani, Rajmohan > ; Cao, Bingbu > Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of- > bounds access > > On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi wrote: > > When dmabuf is used for BLOB type frame, the frame buffers allocated > > by gralloc will hold more pages than the valid frame data due to > > height alignment. > > > > In this case, the page numbers in sg list could exceed the FBPT upper > > limit value - max_lops(8)*1024 to cause crash. > > > > Limit the LOP access to the valid data length to avoid FBPT > > sub-entries overflow. > > > > Signed-off-by: Yong Zhi > > Signed-off-by: Cao Bing Bu > > --- > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > index 941caa987dab..949f43d206ad 100644 > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb) > > container_of(vb, struct cio2_buffer, vbb.vb2_buf); > > static const unsigned int entries_per_page = > > CIO2_PAGE_SIZE / sizeof(u32); > > - unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, > CIO2_PAGE_SIZE); > > - unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page); > > + unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, > > + CIO2_PAGE_SIZE) + 1; > > Why + 1? This would still overflow the buffer, wouldn't it? The "pages" variable is used to calculate lops which has one extra page at the end that points to dummy page. > > > + unsigned int lops = DIV_ROUND_UP(pages, entries_per_page); > > struct sg_table *sg; > > struct sg_page_iter sg_iter; > > int i, j; > > @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer > > *vb) > > > > i = j = 0; > > for_each_sg_page(sg->sgl, &sg_iter, sg->nents, 0) { > > + if (!pages--) > > + break; > > Or perhaps we should check here for (pages > 1)? This is so that the end of lop is set to the dummy_page. > > Best regards, > Tomasz
RE: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings
Hi, Tomasz, Thanks for reviewing the patch. > -Original Message- > From: Tomasz Figa [mailto:tf...@chromium.org] > Sent: Friday, January 12, 2018 12:19 AM > To: Zhi, Yong > Cc: Linux Media Mailing List ; Sakari Ailus > ; Mani, Rajmohan > ; Cao, Bingbu > Subject: Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state > warnings > > On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi wrote: > > cio2 driver should release buffer with QUEUED state when start_stream > > op failed, wrong buffer state will cause vb2 core throw a warning. > > > > Signed-off-by: Yong Zhi > > Signed-off-by: Cao Bing Bu > > --- > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 + > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > index 949f43d206ad..106d04306372 100644 > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > @@ -785,7 +785,8 @@ static irqreturn_t cio2_irq(int irq, void > > *cio2_ptr) > > > > / Videobuf2 interface / > > > > -static void cio2_vb2_return_all_buffers(struct cio2_queue *q) > > +static void cio2_vb2_return_all_buffers(struct cio2_queue *q, > > + enum vb2_buffer_state state) > > { > > unsigned int i; > > > > @@ -793,7 +794,7 @@ static void cio2_vb2_return_all_buffers(struct > cio2_queue *q) > > if (q->bufs[i]) { > > atomic_dec(&q->bufs_queued); > > vb2_buffer_done(&q->bufs[i]->vbb.vb2_buf, > > - VB2_BUF_STATE_ERROR); > > + state); > > nit: Does it really exceed 80 characters after folding into previous line? > Thanks for catching this, seems this patch was merged, may I fix it in future patch? > With the nit fixed: > Reviewed-by: Tomasz Figa > > Best regards, > Tomasz
Re: [PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers
2018-01-15 Hans Verkuil : > On 01/15/2018 01:01 PM, Gustavo Padovan wrote: > > 2018-01-15 Alexandre Courbot : > > > >> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan > >> wrote: > >>> From: Gustavo Padovan > >>> > >>> Explicit synchronization benefits a lot from ordered queues, they fit > >>> better in a pipeline with DRM for example so create a opt-in way for > >>> drivers notify videobuf2 that the queue is unordered. > >>> > >>> Drivers don't need implement it if the queue is ordered. > >> > >> This is going to make user-space believe that *all* vb2 drivers use > >> ordered queues by default, at least until non-ordered drivers catch up > >> with this change. Wouldn't it be less dangerous to do the opposite > >> (make queues non-ordered by default)? > > > > The rational behind this decision was because most formats/drivers are > > ordered so only a small amount of drivers need to changed. I think this > > was proposed by Hans on the Media Summit. > > > > I understand your concern. My question is how dangerous will it be. If > > you are building a product you will make the changes in the driver if > > they are not there yet, or if it is a distribution you'd never know > > which driver/format you are using so you should be prepared for > > everything. > > > > AFAIK all Capture drivers are ordered and that is where I think fences > > is most useful. > > Right. What could be done is to mark all codec drivers as unordered initially > ask the driver authors to verify this. All capture drivers using vb2 and not > using REQUEUE are ordered. That is a good way out. > > One thing we haven't looked at is what to do with drivers that do not use vb2. > Those won't support fences, but how will userspace know that fences are not > supported? I'm not sure what the best method is for that. > > I am leaning towards a new capability since this has to be advertised clearly. The capability flag makes sense to me, I'll incorporate it as part of my next patchset. Gustavo
Re: [RFT PATCH v3 0/6] Asynchronous UVC
Hi Kieran, On Fri, Jan 12, 2018 at 10:19 AM, Kieran Bingham wrote: > This series has been tested on both the ZED and BRIO cameras on arm64 > platforms, however due to the intrinsic changes in the driver I would like to > see it tested with other devices and other platforms, so I'd appreciate if > anyone can test this on a range of USB cameras. FWIW, Tested-by: Philipp Zabel with a Lite-On internal Laptop Webcam, Logitech C910 (USB2 isoc), Oculus Sensor (USB3 isoc), and Microsoft HoloLens Sensors (USB3 bulk). regards Philipp
[PATCH] [v4l-utils] buildsystem: Fix not reporting if libjpeg is not being used
Signed-off-by: Chris Mayo --- If configured --without-jpeg, currently see: compile time options summary Host OS: linux-gnu X11: yes GL : yes glu: yes libjpeg: configure.ac | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index dc1e9cbf5..cfbdffd99 100644 --- a/configure.ac +++ b/configure.ac @@ -195,7 +195,8 @@ AS_IF([test "x$with_jpeg" != xno], [have_jpeg=no AC_MSG_WARN(cannot find libjpeg (v6 or later required))])], [have_jpeg=no -AC_MSG_WARN(cannot find libjpeg)])]) +AC_MSG_WARN(cannot find libjpeg)])], + [have_jpeg=no]) AM_CONDITIONAL([HAVE_JPEG], [test x$have_jpeg = xyes]) -- 2.13.6
Re: [PATCH v2] vsp1: fix video output on R8A77970
Hi Sergei, On Monday, 15 January 2018 18:06:48 EET Sergei Shtylyov wrote: > On 01/15/2018 03:51 PM, Laurent Pinchart wrote: > > On Tuesday, 26 December 2017 23:14:12 EET Sergei Shtylyov wrote: > >> Laurent has added support for the VSP2-D found on R-Car V3M (R8A77970) > >> but > > > > I'm not sure there's a need to state my name in the commit message. > > You were the author of the patch this one has in the Fixes: tag, so I > thought that was appropriate. I can remove that if you don't want you name > mentioned... It's just that I thought it was more important to refer to the patch than to my name, developers reading the log will be more interested in the technical details than in my person :-) > >> the video output that VSP2-D sends to DU has a greenish garbage-like > >> line > > > > Why does the text in your patches (commit message, comments, ...) sometime > > have double spaces between words ? > > The text looks more pleasant (at least to me). Can remove them if you > want... I don't think we try to do typesetting in kernel code or commit messages, so I'd stick to single spaces if you don't mind. > >> repeated every 8 or so screen rows. > > > > Is it every "8 or so" rows, or exactly every 8 rows ? > > You really want me to count the pixels? I'd like to know a bit more about the systems, whether the green lines appear at the exact same interval, whether they bounce up and down or are always at the same place, ... > >> It turns out that V3M has a teeny LIF register (at least it's > >> documented!) that you need to set to some kind of a magic value for the > >> LIF to work correctly... > >> > >> Based on the original (and large) patch by Daisuke Matsushita > >> . > > > > What else is in the big patch ? Is it available somewhere ? > > Assorted changes gathered together only because they all bring the > support for R8A77970. If you're really curious, here you are: > > https://github.com/CogentEmbedded/meta-rcar/blob/v2.12.0/meta-rcar-gen3/reci > pes-kernel/linux/linux-renesas/0030-arm64-renesas-r8a7797-Add-Renesas-R8A779 > 7-SoC-suppor.patch Thank you. > >> Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and > >> VSP2-D instances") > >> Signed-off-by: Sergei Shtylyov > >> > >> --- > >> This patch is against the 'media_tree.git' repo's 'master' branch. > >> > >> Changes in version 2: > >> - added a comment before the V3M SoC check; > >> - fixed indetation in that check; > >> - reformatted the patch description. > >> > >> drivers/media/platform/vsp1/vsp1_lif.c | 12 > >> drivers/media/platform/vsp1/vsp1_regs.h |5 + > >> 2 files changed, 17 insertions(+) > >> > >> Index: media_tree/drivers/media/platform/vsp1/vsp1_lif.c > >> === > >> --- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c > >> +++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c > >> @@ -155,6 +155,18 @@ static void lif_configure(struct vsp1_en > >>(obth << VI6_LIF_CTRL_OBTH_SHIFT) | > >>(format->code == 0 ? VI6_LIF_CTRL_CFMT : 0) | > >>VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN); > >> + > >> + /* > >> + * R-Car V3M has the buffer attribute register you absolutely need > >> + * to write kinda magic value to for the LIF to work correctly... > >> + */ > > > > I'm not sure about the "kinda" magic value. 1536 is very likely a buffer > > size. > > Well, that's only guessing. The manual doesn't say anything about what the > number is. Yes it's just an educated guess. > > How about the following text ? > > > > /* > > > > * On V3M the LBA register has to be set to a non-default value to > > I'd then spell its full name, LIF0 Buffer Attribute register. Sounds good to me. > > * guarantee proper operation (otherwise artifacts may appear on the > > * output). The value required by the datasheet is not documented but > > * is likely a buffer size or threshold. > > */ > > OK, can change that (modulo the name). > > > The commit message should also be updated to feel a bit less magic. > > I'll see about it. > > >> + if ((entity->vsp1->version & > >> + (VI6_IP_VERSION_MODEL_MASK | VI6_IP_VERSION_SOC_MASK)) == > >> + (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) { > >> + vsp1_lif_write(lif, dl, VI6_LIF_LBA, > >> + VI6_LIF_LBA_LBA0 | > >> + (1536 << VI6_LIF_LBA_LBA1_SHIFT)); > >> + } > >> > >> } > > > > The datasheet documents the register as being present on both V3M and M3-W > > (and the test I've just run on H3 shows that the register is present there > > as well). Should we program it on M3-W or leave it to the default value > > that should be what is recommended by the datasheet for that SoC ? > > If the default value matches what's recommended by the manual, then I'd > leave the register alone. OK, I agree.
Re: [PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers
On Mon, Jan 15, 2018 at 9:01 PM, Gustavo Padovan wrote: > 2018-01-15 Alexandre Courbot : > >> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan wrote: >> > From: Gustavo Padovan >> > >> > Explicit synchronization benefits a lot from ordered queues, they fit >> > better in a pipeline with DRM for example so create a opt-in way for >> > drivers notify videobuf2 that the queue is unordered. >> > >> > Drivers don't need implement it if the queue is ordered. >> >> This is going to make user-space believe that *all* vb2 drivers use >> ordered queues by default, at least until non-ordered drivers catch up >> with this change. Wouldn't it be less dangerous to do the opposite >> (make queues non-ordered by default)? > > The rational behind this decision was because most formats/drivers are > ordered so only a small amount of drivers need to changed. I think this > was proposed by Hans on the Media Summit. As long as all concerned drivers are updated we should be on the safe side. At first I was surprised that we expose the ordering feature in a negative tense, but if the vast majority of devices are ordered this probably makes sense.
Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access
Hi Yong, On Tue, Jan 16, 2018 at 2:05 AM, Zhi, Yong wrote: > Hi, Tomasz, > > Thanks for the patch review. > >> -Original Message- >> From: Tomasz Figa [mailto:tf...@chromium.org] >> Sent: Friday, January 12, 2018 12:17 AM >> To: Zhi, Yong >> Cc: Linux Media Mailing List ; Sakari Ailus >> ; Mani, Rajmohan >> ; Cao, Bingbu >> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of- >> bounds access >> >> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi wrote: >> > When dmabuf is used for BLOB type frame, the frame buffers allocated >> > by gralloc will hold more pages than the valid frame data due to >> > height alignment. >> > >> > In this case, the page numbers in sg list could exceed the FBPT upper >> > limit value - max_lops(8)*1024 to cause crash. >> > >> > Limit the LOP access to the valid data length to avoid FBPT >> > sub-entries overflow. >> > >> > Signed-off-by: Yong Zhi >> > Signed-off-by: Cao Bing Bu >> > --- >> > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +-- >> > 1 file changed, 5 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c >> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c >> > index 941caa987dab..949f43d206ad 100644 >> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c >> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c >> > @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb) >> > container_of(vb, struct cio2_buffer, vbb.vb2_buf); >> > static const unsigned int entries_per_page = >> > CIO2_PAGE_SIZE / sizeof(u32); >> > - unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, >> CIO2_PAGE_SIZE); >> > - unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page); >> > + unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, >> > + CIO2_PAGE_SIZE) + 1; >> >> Why + 1? This would still overflow the buffer, wouldn't it? > > The "pages" variable is used to calculate lops which has one extra page at > the end that points to dummy page. > >> >> > + unsigned int lops = DIV_ROUND_UP(pages, entries_per_page); >> > struct sg_table *sg; >> > struct sg_page_iter sg_iter; >> > int i, j; >> > @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer >> > *vb) >> > >> > i = j = 0; >> > for_each_sg_page(sg->sgl, &sg_iter, sg->nents, 0) { >> > + if (!pages--) >> > + break; >> >> Or perhaps we should check here for (pages > 1)? > > This is so that the end of lop is set to the dummy_page. How about this simple example: vb->planes[0].length = 1023 * 4096 pages = 1023 + 1 = 1024 lops = 1 If sg->sgl includes more than 1023 pages, the for_each_sg_page() loop will iterate for pages from 1024 to 1 inclusive and ends up overflowing the dummy page to next lop (i == 1 and j == 0), but we only allocated 1 lop. Best regards, Tomasz
Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings
On Tue, Jan 16, 2018 at 2:07 AM, Zhi, Yong wrote: > Hi, Tomasz, > > Thanks for reviewing the patch. > >> -Original Message- >> From: Tomasz Figa [mailto:tf...@chromium.org] >> Sent: Friday, January 12, 2018 12:19 AM >> To: Zhi, Yong >> Cc: Linux Media Mailing List ; Sakari Ailus >> ; Mani, Rajmohan >> ; Cao, Bingbu >> Subject: Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state >> warnings >> >> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi wrote: >> > cio2 driver should release buffer with QUEUED state when start_stream >> > op failed, wrong buffer state will cause vb2 core throw a warning. >> > >> > Signed-off-by: Yong Zhi >> > Signed-off-by: Cao Bing Bu >> > --- >> > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 + >> > 1 file changed, 5 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c >> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c >> > index 949f43d206ad..106d04306372 100644 >> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c >> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c >> > @@ -785,7 +785,8 @@ static irqreturn_t cio2_irq(int irq, void >> > *cio2_ptr) >> > >> > / Videobuf2 interface / >> > >> > -static void cio2_vb2_return_all_buffers(struct cio2_queue *q) >> > +static void cio2_vb2_return_all_buffers(struct cio2_queue *q, >> > + enum vb2_buffer_state state) >> > { >> > unsigned int i; >> > >> > @@ -793,7 +794,7 @@ static void cio2_vb2_return_all_buffers(struct >> cio2_queue *q) >> > if (q->bufs[i]) { >> > atomic_dec(&q->bufs_queued); >> > vb2_buffer_done(&q->bufs[i]->vbb.vb2_buf, >> > - VB2_BUF_STATE_ERROR); >> > + state); >> >> nit: Does it really exceed 80 characters after folding into previous line? >> > > Thanks for catching this, seems this patch was merged, may I fix it in future > patch? It's just a nit that I was hoping to be fixed at patch applying time. Otherwise just never mind. Best regards, Tomasz
RE: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access
I think if set the pages as the DIV_ROUND_UP(vb->planes[0].length, CIO2_PAGE_SIZE) + 1, the ' if (!pages--)' in loop is not correct. should be 'if (!--pages)'. The last page from sg list is the last valid page. __ BRs, Cao, Bingbu > -Original Message- > From: Tomasz Figa [mailto:tf...@chromium.org] > Sent: Tuesday, January 16, 2018 10:40 AM > To: Zhi, Yong > Cc: Linux Media Mailing List ; Sakari Ailus > ; Mani, Rajmohan ; > Cao, Bingbu > Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out- > of-bounds access > > Hi Yong, > > On Tue, Jan 16, 2018 at 2:05 AM, Zhi, Yong wrote: > > Hi, Tomasz, > > > > Thanks for the patch review. > > > >> -Original Message- > >> From: Tomasz Figa [mailto:tf...@chromium.org] > >> Sent: Friday, January 12, 2018 12:17 AM > >> To: Zhi, Yong > >> Cc: Linux Media Mailing List ; Sakari > >> Ailus ; Mani, Rajmohan > >> ; Cao, Bingbu > >> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with > >> out-of- bounds access > >> > >> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi wrote: > >> > When dmabuf is used for BLOB type frame, the frame buffers > >> > allocated by gralloc will hold more pages than the valid frame data > >> > due to height alignment. > >> > > >> > In this case, the page numbers in sg list could exceed the FBPT > >> > upper limit value - max_lops(8)*1024 to cause crash. > >> > > >> > Limit the LOP access to the valid data length to avoid FBPT > >> > sub-entries overflow. > >> > > >> > Signed-off-by: Yong Zhi > >> > Signed-off-by: Cao Bing Bu > >> > --- > >> > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +-- > >> > 1 file changed, 5 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > >> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > >> > index 941caa987dab..949f43d206ad 100644 > >> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > >> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > >> > @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer > *vb) > >> > container_of(vb, struct cio2_buffer, vbb.vb2_buf); > >> > static const unsigned int entries_per_page = > >> > CIO2_PAGE_SIZE / sizeof(u32); > >> > - unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, > >> CIO2_PAGE_SIZE); > >> > - unsigned int lops = DIV_ROUND_UP(pages + 1, > entries_per_page); > >> > + unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, > >> > + CIO2_PAGE_SIZE) + 1; > >> > >> Why + 1? This would still overflow the buffer, wouldn't it? > > > > The "pages" variable is used to calculate lops which has one extra > page at the end that points to dummy page. > > > >> > >> > + unsigned int lops = DIV_ROUND_UP(pages, entries_per_page); > >> > struct sg_table *sg; > >> > struct sg_page_iter sg_iter; > >> > int i, j; > >> > @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer > >> > *vb) > >> > > >> > i = j = 0; > >> > for_each_sg_page(sg->sgl, &sg_iter, sg->nents, 0) { > >> > + if (!pages--) > >> > + break; > >> > >> Or perhaps we should check here for (pages > 1)? > > > > This is so that the end of lop is set to the dummy_page. > > How about this simple example: > > vb->planes[0].length = 1023 * 4096 > pages = 1023 + 1 = 1024 > lops = 1 > > If sg->sgl includes more than 1023 pages, the for_each_sg_page() loop > will iterate for pages from 1024 to 1 inclusive and ends up overflowing > the dummy page to next lop (i == 1 and j == 0), but we only allocated 1 > lop. > > Best regards, > Tomasz
Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access
Hi Bingbu, On Tue, Jan 16, 2018 at 1:05 PM, Cao, Bingbu wrote: > I think if set the pages as the DIV_ROUND_UP(vb->planes[0].length, > CIO2_PAGE_SIZE) + 1, the ' if (!pages--)' in loop is not correct. > should be 'if (!--pages)'. > The last page from sg list is the last valid page. > Yes, that's exactly what I meant. By the way, please avoid top-posting to mailing lists, it isn't well received by recipients. Best regards, Tomasz > > __ > BRs, > Cao, Bingbu > > > >> -Original Message- >> From: Tomasz Figa [mailto:tf...@chromium.org] >> Sent: Tuesday, January 16, 2018 10:40 AM >> To: Zhi, Yong >> Cc: Linux Media Mailing List ; Sakari Ailus >> ; Mani, Rajmohan ; >> Cao, Bingbu >> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out- >> of-bounds access >> >> Hi Yong, >> >> On Tue, Jan 16, 2018 at 2:05 AM, Zhi, Yong wrote: >> > Hi, Tomasz, >> > >> > Thanks for the patch review. >> > >> >> -Original Message- >> >> From: Tomasz Figa [mailto:tf...@chromium.org] >> >> Sent: Friday, January 12, 2018 12:17 AM >> >> To: Zhi, Yong >> >> Cc: Linux Media Mailing List ; Sakari >> >> Ailus ; Mani, Rajmohan >> >> ; Cao, Bingbu >> >> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with >> >> out-of- bounds access >> >> >> >> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi wrote: >> >> > When dmabuf is used for BLOB type frame, the frame buffers >> >> > allocated by gralloc will hold more pages than the valid frame data >> >> > due to height alignment. >> >> > >> >> > In this case, the page numbers in sg list could exceed the FBPT >> >> > upper limit value - max_lops(8)*1024 to cause crash. >> >> > >> >> > Limit the LOP access to the valid data length to avoid FBPT >> >> > sub-entries overflow. >> >> > >> >> > Signed-off-by: Yong Zhi >> >> > Signed-off-by: Cao Bing Bu >> >> > --- >> >> > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +-- >> >> > 1 file changed, 5 insertions(+), 2 deletions(-) >> >> > >> >> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c >> >> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c >> >> > index 941caa987dab..949f43d206ad 100644 >> >> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c >> >> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c >> >> > @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer >> *vb) >> >> > container_of(vb, struct cio2_buffer, vbb.vb2_buf); >> >> > static const unsigned int entries_per_page = >> >> > CIO2_PAGE_SIZE / sizeof(u32); >> >> > - unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, >> >> CIO2_PAGE_SIZE); >> >> > - unsigned int lops = DIV_ROUND_UP(pages + 1, >> entries_per_page); >> >> > + unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, >> >> > + CIO2_PAGE_SIZE) + 1; >> >> >> >> Why + 1? This would still overflow the buffer, wouldn't it? >> > >> > The "pages" variable is used to calculate lops which has one extra >> page at the end that points to dummy page. >> > >> >> >> >> > + unsigned int lops = DIV_ROUND_UP(pages, entries_per_page); >> >> > struct sg_table *sg; >> >> > struct sg_page_iter sg_iter; >> >> > int i, j; >> >> > @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer >> >> > *vb) >> >> > >> >> > i = j = 0; >> >> > for_each_sg_page(sg->sgl, &sg_iter, sg->nents, 0) { >> >> > + if (!pages--) >> >> > + break; >> >> >> >> Or perhaps we should check here for (pages > 1)? >> > >> > This is so that the end of lop is set to the dummy_page. >> >> How about this simple example: >> >> vb->planes[0].length = 1023 * 4096 >> pages = 1023 + 1 = 1024 >> lops = 1 >> >> If sg->sgl includes more than 1023 pages, the for_each_sg_page() loop >> will iterate for pages from 1024 to 1 inclusive and ends up overflowing >> the dummy page to next lop (i == 1 and j == 0), but we only allocated 1 >> lop. >> >> Best regards, >> Tomasz
cron job: media_tree daily build: ERRORS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Tue Jan 16 05:00:17 CET 2018 media-tree git hash:e3ee691dbf24096ea51b3200946b11d68ce75361 media_build git hash: 2bd1f1623fbadfdc1026712b3d55141ba164c403 v4l-utils git hash: 2c46e73aa82d913183eb4e83a8f2c9262718f2e2 gcc version:i686-linux-gcc (GCC) 7.1.0 sparse version: v0.5.0-3911-g6f737e1f smatch version: v0.5.0-3911-g6f737e1f host hardware: x86_64 host os:4.13.0-164 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-blackfin-bf561: OK linux-git-i686: OK linux-git-m32r: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK linux-2.6.36.4-i686: ERRORS linux-2.6.37.6-i686: ERRORS linux-2.6.38.8-i686: ERRORS linux-2.6.39.4-i686: ERRORS linux-3.0.60-i686: ERRORS linux-3.1.10-i686: ERRORS linux-3.2.37-i686: ERRORS linux-3.3.8-i686: ERRORS linux-3.4.27-i686: ERRORS linux-3.5.7-i686: ERRORS linux-3.6.11-i686: ERRORS linux-3.7.4-i686: ERRORS linux-3.8-i686: ERRORS linux-3.9.2-i686: WARNINGS linux-3.10.1-i686: WARNINGS linux-3.11.1-i686: ERRORS linux-3.12.67-i686: ERRORS linux-3.13.11-i686: ERRORS linux-3.14.9-i686: ERRORS linux-3.15.2-i686: ERRORS linux-3.16.7-i686: ERRORS linux-3.17.8-i686: ERRORS linux-3.18.7-i686: ERRORS linux-3.19-i686: ERRORS linux-4.0.9-i686: ERRORS linux-4.1.33-i686: ERRORS linux-4.2.8-i686: ERRORS linux-4.3.6-i686: ERRORS linux-4.4.22-i686: ERRORS linux-4.5.7-i686: ERRORS linux-4.6.7-i686: ERRORS linux-4.7.5-i686: ERRORS linux-4.8-i686: ERRORS linux-4.9.26-i686: ERRORS linux-4.10.14-i686: WARNINGS linux-4.11-i686: WARNINGS linux-4.12.1-i686: WARNINGS linux-4.13-i686: WARNINGS linux-4.14-i686: WARNINGS linux-2.6.36.4-x86_64: ERRORS linux-2.6.37.6-x86_64: ERRORS linux-2.6.38.8-x86_64: ERRORS linux-2.6.39.4-x86_64: ERRORS linux-3.0.60-x86_64: ERRORS linux-3.1.10-x86_64: ERRORS linux-3.2.37-x86_64: ERRORS linux-3.3.8-x86_64: ERRORS linux-3.4.27-x86_64: ERRORS linux-3.5.7-x86_64: ERRORS linux-3.6.11-x86_64: ERRORS linux-3.7.4-x86_64: ERRORS linux-3.8-x86_64: ERRORS linux-3.9.2-x86_64: WARNINGS linux-3.10.1-x86_64: WARNINGS linux-3.11.1-x86_64: ERRORS linux-3.12.67-x86_64: ERRORS linux-3.13.11-x86_64: ERRORS linux-3.14.9-x86_64: ERRORS linux-3.15.2-x86_64: ERRORS linux-3.16.7-x86_64: ERRORS linux-3.17.8-x86_64: ERRORS linux-3.18.7-x86_64: ERRORS linux-3.19-x86_64: ERRORS linux-4.0.9-x86_64: ERRORS linux-4.1.33-x86_64: ERRORS linux-4.2.8-x86_64: ERRORS linux-4.3.6-x86_64: ERRORS linux-4.4.22-x86_64: ERRORS linux-4.5.7-x86_64: ERRORS linux-4.6.7-x86_64: ERRORS linux-4.7.5-x86_64: ERRORS linux-4.8-x86_64: ERRORS linux-4.9.26-x86_64: ERRORS linux-4.10.14-x86_64: WARNINGS linux-4.11-x86_64: WARNINGS linux-4.12.1-x86_64: WARNINGS linux-4.13-x86_64: WARNINGS linux-4.14-x86_64: WARNINGS apps: OK spec-git: OK smatch: OK Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
Re: [PATCH 3/7] si2157: Add hybrid tuner support
Hello So the use case is to share single tuner with multiple demodulators? Why don't just register single tuner and pass that info to multiple demods? Antti On 01/12/2018 06:19 PM, Brad Love wrote: Add ability to share a tuner amongst demodulators. Addtional demods are attached using hybrid_tuner_instance_list. The changes are equivalent to moving all of probe to _attach. Results are backwards compatible with current usage. If the tuner is acquired via attach, then .release cleans state. if the tuner is an i2c driver, then .release is set to NULL, and .remove cleans remaining state. The following file contains a static si2157_attach: - drivers/media/pci/saa7164/saa7164-dvb.c The function name has been appended with _priv to appease the compiler. Signed-off-by: Brad Love --- drivers/media/pci/saa7164/saa7164-dvb.c | 11 +- drivers/media/tuners/si2157.c | 232 +++- drivers/media/tuners/si2157.h | 14 ++ drivers/media/tuners/si2157_priv.h | 5 + 4 files changed, 192 insertions(+), 70 deletions(-) diff --git a/drivers/media/pci/saa7164/saa7164-dvb.c b/drivers/media/pci/saa7164/saa7164-dvb.c index e76d3ba..9522c6c 100644 --- a/drivers/media/pci/saa7164/saa7164-dvb.c +++ b/drivers/media/pci/saa7164/saa7164-dvb.c @@ -110,8 +110,9 @@ static struct si2157_config hauppauge_hvr2255_tuner_config = { .if_port = 1, }; -static int si2157_attach(struct saa7164_port *port, struct i2c_adapter *adapter, - struct dvb_frontend *fe, u8 addr8bit, struct si2157_config *cfg) +static int si2157_attach_priv(struct saa7164_port *port, + struct i2c_adapter *adapter, struct dvb_frontend *fe, + u8 addr8bit, struct si2157_config *cfg) { struct i2c_board_info bi; struct i2c_client *tuner; @@ -624,11 +625,13 @@ int saa7164_dvb_register(struct saa7164_port *port) if (port->dvb.frontend != NULL) { if (port->nr == 0) { - si2157_attach(port, &dev->i2c_bus[0].i2c_adap, + si2157_attach_priv(port, + &dev->i2c_bus[0].i2c_adap, port->dvb.frontend, 0xc0, &hauppauge_hvr2255_tuner_config); } else { - si2157_attach(port, &dev->i2c_bus[1].i2c_adap, + si2157_attach_priv(port, + &dev->i2c_bus[1].i2c_adap, port->dvb.frontend, 0xc0, &hauppauge_hvr2255_tuner_config); } diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c index e35b1fa..9121361 100644 --- a/drivers/media/tuners/si2157.c +++ b/drivers/media/tuners/si2157.c @@ -18,6 +18,11 @@ static const struct dvb_tuner_ops si2157_ops; +static DEFINE_MUTEX(si2157_list_mutex); +static LIST_HEAD(hybrid_tuner_instance_list); + +/*-*/ + /* execute firmware command */ static int si2157_cmd_execute(struct i2c_client *client, struct si2157_cmd *cmd) { @@ -385,6 +390,31 @@ static int si2157_get_if_frequency(struct dvb_frontend *fe, u32 *frequency) return 0; } +static void si2157_release(struct dvb_frontend *fe) +{ + struct i2c_client *client = fe->tuner_priv; + struct si2157_dev *dev = i2c_get_clientdata(client); + + dev_dbg(&client->dev, "%s()\n", __func__); + + /* only do full cleanup on final instance */ + if (hybrid_tuner_report_instance_count(dev) == 1) { + /* stop statistics polling */ + cancel_delayed_work_sync(&dev->stat_work); +#ifdef CONFIG_MEDIA_CONTROLLER_DVB + if (dev->mdev) + media_device_unregister_entity(&dev->ent); +#endif + i2c_set_clientdata(client, NULL); + } + + mutex_lock(&si2157_list_mutex); + hybrid_tuner_release_state(dev); + mutex_unlock(&si2157_list_mutex); + + fe->tuner_priv = NULL; +} + static const struct dvb_tuner_ops si2157_ops = { .info = { .name = "Silicon Labs Si2141/Si2146/2147/2148/2157/2158", @@ -396,6 +426,7 @@ static const struct dvb_tuner_ops si2157_ops = { .sleep = si2157_sleep, .set_params = si2157_set_params, .get_if_frequency = si2157_get_if_frequency, + .release = si2157_release, }; static void si2157_stat_work(struct work_struct *work) @@ -431,72 +462,30 @@ static int si2157_probe(struct i2c_client *client, { struct si2157_config *cfg = client->dev.platform_data; struct dvb_frontend *fe = cfg->fe; - struct si2157_dev *dev; - struct si2157_cmd cmd; - int ret; - - dev = kzalloc(sizeof(*dev), GFP_KERNEL); - if (!dev) {
Re: [PATCH 4/7] si2168: Add ts bus coontrol, turn off bus on sleep
Hello And what is rationale here, is there some use case demod must be active and ts set to tristate (disabled)? Just put demod sleep when you don't use it. regards Antti On 01/12/2018 06:19 PM, Brad Love wrote: Includes a function to set TS MODE property os si2168. The function either disables the TS output bus, or sets mode to config option. When going to sleep the TS bus is turned off, this makes the driver compatible with multiple frontend usage. Signed-off-by: Brad Love --- drivers/media/dvb-frontends/si2168.c | 38 drivers/media/dvb-frontends/si2168.h | 1 + 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c index 539399d..429c03a 100644 --- a/drivers/media/dvb-frontends/si2168.c +++ b/drivers/media/dvb-frontends/si2168.c @@ -409,6 +409,30 @@ static int si2168_set_frontend(struct dvb_frontend *fe) return ret; } +static int si2168_ts_bus_ctrl(struct dvb_frontend *fe, int acquire) +{ + struct i2c_client *client = fe->demodulator_priv; + struct si2168_dev *dev = i2c_get_clientdata(client); + struct si2168_cmd cmd; + int ret = 0; + + dev_dbg(&client->dev, "%s acquire: %d\n", __func__, acquire); + + /* set TS_MODE property */ + memcpy(cmd.args, "\x14\x00\x01\x10\x10\x00", 6); + if (acquire) + cmd.args[4] |= dev->ts_mode; + else + cmd.args[4] |= SI2168_TS_TRISTATE; + if (dev->ts_clock_gapped) + cmd.args[4] |= 0x40; + cmd.wlen = 6; + cmd.rlen = 4; + ret = si2168_cmd_execute(client, &cmd); + + return ret; +} + static int si2168_init(struct dvb_frontend *fe) { struct i2c_client *client = fe->demodulator_priv; @@ -540,14 +564,7 @@ static int si2168_init(struct dvb_frontend *fe) dev->version >> 24 & 0xff, dev->version >> 16 & 0xff, dev->version >> 8 & 0xff, dev->version >> 0 & 0xff); - /* set ts mode */ - memcpy(cmd.args, "\x14\x00\x01\x10\x10\x00", 6); - cmd.args[4] |= dev->ts_mode; - if (dev->ts_clock_gapped) - cmd.args[4] |= 0x40; - cmd.wlen = 6; - cmd.rlen = 4; - ret = si2168_cmd_execute(client, &cmd); + ret = si2168_ts_bus_ctrl(fe, 1); if (ret) goto err; @@ -584,6 +601,9 @@ static int si2168_sleep(struct dvb_frontend *fe) dev->active = false; + /* tri-state data bus */ + si2168_ts_bus_ctrl(fe, 0); + /* Firmware B 4.0-11 or later loses warm state during sleep */ if (dev->version > ('B' << 24 | 4 << 16 | 0 << 8 | 11 << 0)) dev->warm = false; @@ -681,6 +701,8 @@ static const struct dvb_frontend_ops si2168_ops = { .init = si2168_init, .sleep = si2168_sleep, + .ts_bus_ctrl = si2168_ts_bus_ctrl, + .set_frontend = si2168_set_frontend, .read_status = si2168_read_status, diff --git a/drivers/media/dvb-frontends/si2168.h b/drivers/media/dvb-frontends/si2168.h index 3225d0c..f48f0fb 100644 --- a/drivers/media/dvb-frontends/si2168.h +++ b/drivers/media/dvb-frontends/si2168.h @@ -38,6 +38,7 @@ struct si2168_config { /* TS mode */ #define SI2168_TS_PARALLEL0x06 #define SI2168_TS_SERIAL 0x03 +#define SI2168_TS_TRISTATE 0x00 u8 ts_mode; /* TS clock inverted */ -- http://palosaari.fi/
Re: [PATCH 6/7] si2168: Announce frontend creation failure
hmmm, IIRC driver core even prints some error when driver probe fails? After that you could enable module debug logging to see more information. So I don't see point for that change. regards Antti On 01/12/2018 06:19 PM, Brad Love wrote: The driver outputs on success, but is silent on failure. Give one message that probe failed. Signed-off-by: Brad Love --- drivers/media/dvb-frontends/si2168.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c index 429c03a..c1a638c 100644 --- a/drivers/media/dvb-frontends/si2168.c +++ b/drivers/media/dvb-frontends/si2168.c @@ -810,7 +810,7 @@ static int si2168_probe(struct i2c_client *client, err_kfree: kfree(dev); err: - dev_dbg(&client->dev, "failed=%d\n", ret); + dev_warn(&client->dev, "probe failed = %d\n", ret); return ret; } -- http://palosaari.fi/