Re: [PATCH 1/6] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING
Hi Akinobu, On Wed, Apr 11, 2018 at 01:37:03AM +0900, Akinobu Mita wrote: > 2018-04-09 15:58 GMT+09:00 jacopo mondi : > > Hello Akinobu, > > thank you for the patch. > > > > On which platform have you tested the series (just curious) ? > > I use Zynq-7000 development board with Xilinx Video IP driver and > custom video pipeline design based on the example reference project. > That's interesting! I was just wondering if there were other development boards around that ship with this sensor... > > On Sun, Apr 08, 2018 at 12:48:05AM +0900, Akinobu Mita wrote: > >> The ov772x driver only works when the i2c controller have > >> I2C_FUNC_PROTOCOL_MANGLING. However, many i2c controller drivers don't > >> support it. > >> > >> The reason that the ov772x requires I2C_FUNC_PROTOCOL_MANGLING is that > >> it doesn't support repeated starts. > >> > >> This change adds an alternative method for reading from ov772x register > >> which uses two separated i2c messages for the i2c controllers without > >> I2C_FUNC_PROTOCOL_MANGLING. > > > > Actually, and please correct me if I'm wrong, what I see here is that > > an i2c_master_send+i2c_master_recv sequence is equivalent to a mangled > > smbus transaction: > > > > i2c_smbus_read_byte_data | I2C_CLIENT_SCCB: > > S Addr Wr [A] Comm [A] P S Addr Rd [A] [Data] NA P > > > > i2c_master_send() + i2c_master_recv(): > > S Addr Wr [A] Data [A] P > > S Addr Rd [A] [Data] NA P > > > > I wonder if it is not worth to ditch the existing read() function in > > favour of your new proposed one completely. > > > > I have tested it on the Migo-R board where I have an ov772x installed > > and it works fine. > > I'll replace the read() function to the new implementation in the next > version of this patchset. Although handling in the I2C core is fascinating, > I feel the area of influence is a bit large. > Yep, i2c is huge and complex but fascinating, and the SCCB use case should probably be handled with a master_send+master_read instead of relying on the i2c adapter ability to send an additional stop bit in between an smbus transaction, as Laurent suggested. I wonder why it has not been implemented this way from day 1. I'm sure there are reasons :) I'll wait for v2. Thanks j > > Although I would like to have a confirmation this is fine by people > > how has seen more i2c adapters in action than me :) > > > > Thanks > >j > > > >> > >> Cc: Jacopo Mondi > >> Cc: Laurent Pinchart > >> Cc: Hans Verkuil > >> Cc: Sakari Ailus > >> Cc: Mauro Carvalho Chehab > >> Signed-off-by: Akinobu Mita > >> --- > >> drivers/media/i2c/ov772x.c | 42 +- > >> 1 file changed, 33 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > >> index b62860c..283ae2c 100644 > >> --- a/drivers/media/i2c/ov772x.c > >> +++ b/drivers/media/i2c/ov772x.c > >> @@ -424,6 +424,7 @@ struct ov772x_priv { > >> /* band_filter = COM8[5] ? 256 - BDBASE : 0 */ > >> unsigned shortband_filter; > >> unsigned int fps; > >> + int (*reg_read)(struct i2c_client *client, u8 addr); > >> }; > >> > >> /* > >> @@ -542,11 +543,34 @@ static struct ov772x_priv *to_ov772x(struct > >> v4l2_subdev *sd) > >> return container_of(sd, struct ov772x_priv, subdev); > >> } > >> > >> -static inline int ov772x_read(struct i2c_client *client, u8 addr) > >> +static int ov772x_read(struct i2c_client *client, u8 addr) > >> +{ > >> + struct v4l2_subdev *sd = i2c_get_clientdata(client); > >> + struct ov772x_priv *priv = to_ov772x(sd); > >> + > >> + return priv->reg_read(client, addr); > >> +} > >> + > >> +static int ov772x_reg_read(struct i2c_client *client, u8 addr) > >> { > >> return i2c_smbus_read_byte_data(client, addr); > >> } > >> > >> +static int ov772x_reg_read_fallback(struct i2c_client *client, u8 addr) > >> +{ > >> + int ret; > >> + u8 val; > >> + > >> + ret = i2c_master_send(client, &addr, 1); > >> + if (ret < 0) > >> + return ret; > >> + ret = i2c_master_recv(client, &val, 1); > >> + if (ret < 0) > >> + return ret; > >> + > >> + return val; > >> +} > >> + > >> static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 > >> value) > >> { > >> return i2c_smbus_write_byte_data(client, addr, value); > >> @@ -1255,20 +1279,20 @@ static int ov772x_probe(struct i2c_client *client, > >> return -EINVAL; > >> } > >> > >> - if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA | > >> - I2C_FUNC_PROTOCOL_MANGLING)) { > >> - dev_err(&adapter->dev, > >> - "I2C-Adapter doesn't support SMBUS_BYTE_DATA or > >> PROTOCOL_MANGLING\n"); > >> - return -EIO; > >> - } > >> - client->flags |= I2C_CLIENT_SCCB; > >> - > >> priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERN
Re: [RFCv11 PATCH 14/29] v4l2-ctrls: add core request support
Hi, On Mon, 2018-04-09 at 16:20 +0200, Hans Verkuil wrote: > From: Hans Verkuil > > Integrate the request support. This adds the > v4l2_ctrl_request_complete > and v4l2_ctrl_request_setup functions to complete a request and (as a > helper function) to apply a request to the hardware. > > It takes care of queuing requests and correctly chaining control > values > in the request queue. > > Note that when a request is marked completed it will copy control > values > to the internal request state. This can be optimized in the future > since > this is sub-optimal when dealing with large compound and/or array > controls. > > For the initial 'stateless codec' use-case the current implementation > is > sufficient. See one comment about a missing unlock below. > Signed-off-by: Hans Verkuil > --- > drivers/media/v4l2-core/v4l2-ctrls.c | 325 > ++- > include/media/v4l2-ctrls.h | 23 +++ > 2 files changed, 342 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c > b/drivers/media/v4l2-core/v4l2-ctrls.c > index da4cc1485dc4..6e2c5e24734f 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -1647,6 +1647,13 @@ static int new_to_user(struct v4l2_ext_control > *c, > return ptr_to_user(c, ctrl, ctrl->p_new); > } > > +/* Helper function: copy the request value back to the caller */ > +static int req_to_user(struct v4l2_ext_control *c, > +struct v4l2_ctrl_ref *ref) > +{ > + return ptr_to_user(c, ref->ctrl, ref->p_req); > +} > + > /* Helper function: copy the initial control value back to the caller > */ > static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl > *ctrl) > { > @@ -1766,6 +1773,26 @@ static void cur_to_new(struct v4l2_ctrl *ctrl) > ptr_to_ptr(ctrl, ctrl->p_cur, ctrl->p_new); > } > > +/* Copy the new value to the request value */ > +static void new_to_req(struct v4l2_ctrl_ref *ref) > +{ > + if (!ref) > + return; > + ptr_to_ptr(ref->ctrl, ref->ctrl->p_new, ref->p_req); > + ref->req = ref; > +} > + > +/* Copy the request value to the new value */ > +static void req_to_new(struct v4l2_ctrl_ref *ref) > +{ > + if (!ref) > + return; > + if (ref->req) > + ptr_to_ptr(ref->ctrl, ref->req->p_req, ref->ctrl- > >p_new); > + else > + ptr_to_ptr(ref->ctrl, ref->ctrl->p_cur, ref->ctrl- > >p_new); > +} > + > /* Return non-zero if one or more of the controls in the cluster has > a new > value that differs from the current value. */ > static int cluster_changed(struct v4l2_ctrl *master) > @@ -1875,6 +1902,9 @@ int v4l2_ctrl_handler_init_class(struct > v4l2_ctrl_handler *hdl, > lockdep_set_class_and_name(hdl->lock, key, name); > INIT_LIST_HEAD(&hdl->ctrls); > INIT_LIST_HEAD(&hdl->ctrl_refs); > + INIT_LIST_HEAD(&hdl->requests); > + INIT_LIST_HEAD(&hdl->requests_queued); > + hdl->request_is_queued = false; > hdl->nr_of_buckets = 1 + nr_of_controls_hint / 8; > hdl->buckets = kvmalloc_array(hdl->nr_of_buckets, > sizeof(hdl->buckets[0]), > @@ -1895,6 +1925,14 @@ void v4l2_ctrl_handler_free(struct > v4l2_ctrl_handler *hdl) > if (hdl == NULL || hdl->buckets == NULL) > return; > > + if (!hdl->req_obj.req && !list_empty(&hdl->requests)) { > + struct v4l2_ctrl_handler *req, *next_req; > + > + list_for_each_entry_safe(req, next_req, &hdl- > >requests, requests) { > + media_request_object_unbind(&req->req_obj); > + media_request_object_put(&req->req_obj); > + } > + } > mutex_lock(hdl->lock); > /* Free all nodes */ > list_for_each_entry_safe(ref, next_ref, &hdl->ctrl_refs, > node) { > @@ -2816,6 +2854,126 @@ int v4l2_querymenu(struct v4l2_ctrl_handler > *hdl, struct v4l2_querymenu *qm) > } > EXPORT_SYMBOL(v4l2_querymenu); > > +static int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl, > +const struct v4l2_ctrl_handler > *from) > +{ > + struct v4l2_ctrl_ref *ref; > + int err; > + > + if (WARN_ON(!hdl || hdl == from)) > + return -EINVAL; > + > + if (hdl->error) > + return hdl->error; > + > + WARN_ON(hdl->lock != &hdl->_lock); > + > + mutex_lock(from->lock); > + list_for_each_entry(ref, &from->ctrl_refs, node) { > + struct v4l2_ctrl *ctrl = ref->ctrl; > + struct v4l2_ctrl_ref *new_ref; > + > + /* Skip refs inherited from other devices */ > + if (ref->from_other_dev) > + continue; > + /* And buttons */ > + if (ctrl->type == V4L2_CTRL_TYPE_BUTTON) > + continue; > + err = handler_new_ref(hdl, ctrl, &new_ref, false, > true); > + if (err) { > +
Re: [RFCv11 PATCH 14/29] v4l2-ctrls: add core request support
Hi Hans, On Mon, Apr 9, 2018 at 11:20 PM Hans Verkuil wrote: > From: Hans Verkuil > Integrate the request support. This adds the v4l2_ctrl_request_complete > and v4l2_ctrl_request_setup functions to complete a request and (as a > helper function) to apply a request to the hardware. Please see my comments inline. [snip] > +static int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl, > + const struct v4l2_ctrl_handler *from) > +{ > + struct v4l2_ctrl_ref *ref; > + int err; > + > + if (WARN_ON(!hdl || hdl == from)) > + return -EINVAL; > + > + if (hdl->error) > + return hdl->error; > + > + WARN_ON(hdl->lock != &hdl->_lock); > + > + mutex_lock(from->lock); > + list_for_each_entry(ref, &from->ctrl_refs, node) { > + struct v4l2_ctrl *ctrl = ref->ctrl; > + struct v4l2_ctrl_ref *new_ref; > + > + /* Skip refs inherited from other devices */ > + if (ref->from_other_dev) > + continue; > + /* And buttons */ > + if (ctrl->type == V4L2_CTRL_TYPE_BUTTON) > + continue; > + err = handler_new_ref(hdl, ctrl, &new_ref, false, true); > + if (err) { > + printk("%s: handler_new_ref on control %x (%s) returned %d\n", __func__, ctrl->id, ctrl->name, err); Perhaps pr_err()? > + err = 0; > + continue; Hmm, is it really fine to ignore the error and continue here? > + } > + if (err) > + break; > + } > + mutex_unlock(from->lock); > + return err; > +} > + > +static void v4l2_ctrl_request_queue(struct media_request_object *obj) > +{ > + struct v4l2_ctrl_handler *hdl = > + container_of(obj, struct v4l2_ctrl_handler, req_obj); > + struct v4l2_ctrl_handler *main_hdl = obj->priv; > + struct v4l2_ctrl_handler *prev = NULL; > + struct v4l2_ctrl_ref *ref_hdl, *ref_prev = NULL; > + > + if (list_empty(&main_hdl->requests_queued)) > + goto queue; > + > + prev = list_last_entry(&main_hdl->requests_queued, > + struct v4l2_ctrl_handler, requests_queued); > + mutex_lock(prev->lock); > + ref_prev = list_first_entry(&prev->ctrl_refs, > + struct v4l2_ctrl_ref, node); > + list_for_each_entry(ref_hdl, &hdl->ctrl_refs, node) { > + if (ref_hdl->req) > + continue; > + while (ref_prev->ctrl->id < ref_hdl->ctrl->id) > + ref_prev = list_next_entry(ref_prev, node); Is this really safe? The only stop condition here is the control id. Perhaps the code below could be better? list_for_each_entry_from(ref_prev, &prev->ctrl_refs, node) if (ref_prev->ctrl->id >= ref_hdl->ctrl->id) break; > + if (WARN_ON(ref_prev->ctrl->id != ref_hdl->ctrl->id)) > + break; > + ref_hdl->req = ref_prev->req; > + } > + mutex_unlock(prev->lock); > +queue: > + list_add_tail(&hdl->requests_queued, &main_hdl->requests_queued); > + hdl->request_is_queued = true; > +} > + [snip] > +void v4l2_ctrl_request_complete(struct media_request *req, > + struct v4l2_ctrl_handler *main_hdl) > +{ > + struct media_request_object *obj; > + struct v4l2_ctrl_handler *hdl; > + struct v4l2_ctrl_ref *ref; > + > + if (!req || !main_hdl) > + return; Can this happen normally? Perhaps WARN_ON() would make sense? [snip] > +void v4l2_ctrl_request_setup(struct media_request *req, > +struct v4l2_ctrl_handler *main_hdl) > +{ > + struct media_request_object *obj; > + struct v4l2_ctrl_handler *hdl; > + struct v4l2_ctrl_ref *ref; > + > + if (!req || !main_hdl) Can this happen normally? Perhaps WARN_ON() would make sense? > + return; > + > + obj = media_request_object_find(req, &req_ops, main_hdl); > + if (!obj) > + return; > + if (obj->completed) { > + media_request_object_put(obj); > + return; > + } > + hdl = container_of(obj, struct v4l2_ctrl_handler, req_obj); > + > + mutex_lock(hdl->lock); > + > + list_for_each_entry(ref, &hdl->ctrl_refs, node) > + ref->done = false; > + > + list_for_each_entry(ref, &hdl->ctrl_refs, node) { > + struct v4l2_ctrl *ctrl = ref->ctrl; > + struct v4l2_ctrl *master = ctrl->cluster[0]; > + bool have_new_data = false; > + int i; > + > + /* Skip if this control was already handled by a cluster. */ > + /* Skip button controls and read-only controls. */ > +
a 4.16 kernel with Debian 9.4 "stretch" causes a log explosion
Hi Laurent, Not sure whether that's a kernel or a user-space problem, but UVC related anyway. I've got a UVC 1.5 (!) Logitech camera, that used to work fine with earlier kernels. I now installed "media 4.16" and saw, that the kernel log was filling with messages like uvcvideo: Failed to query (GET_MIN) UVC control 2 on unit 1: -32 (exp. 1). The expected /dev/video[01] nodes were not created correctly, and the hard-drive was getting full very quickly. The latter was happening because the the /var/log/uvcdynctrl-udev.log file was growing. A truncated sample is attached. At its bottom you see messages [libwebcam] Warning: The driver behind device video0 has a slightly buggy implementation of the V4L2_CTRL_FLAG_NEXT_CTRL flag. It does not return the next higher control ID if a control query fails. A workaround has been enabled. repeating, which continues even if the camera is unplugged. The kernel is the head of the master branch of git://linuxtv.org/media_tree.git Just figured out this commit From: Edgar Thier Date: Thu, 12 Oct 2017 03:54:17 -0400 Subject: [PATCH] media: uvcvideo: Apply flags from device to actual properties as the culprit. Without it everything is back to normal. Thanks Guennadi == uvcdynctrl script version 0.3 running from '/lib/udev/uvcdynctrl' == uvcdynctrl script version 0.3 running from '/lib/udev/uvcdynctrl' Triggered at Wed Apr 11 09:08:26 CEST 2018 ACTION='add' DEVLINKS='/dev/v4l/by-path/pci-:00:14.0-usb-0:1:1.0-video-index0 /dev/v4l/by-id/usb-046d_Logitech_Webcam_C930e_816415EE-video-index0' DEVNAME='/dev/video0' DEVPATH='/devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0/video4linux/video0' ID_BUS='usb' ID_FOR_SEAT='video4linux-pci-_00_14_0-usb-0_1_1_0' ID_MODEL='Logitech_Webcam_C930e' ID_MODEL_ENC='Logitech\x20Webcam\x20C930e' ID_MODEL_ID='0843' ID_PATH='pci-:00:14.0-usb-0:1:1.0' ID_PATH_TAG='pci-_00_14_0-usb-0_1_1_0' ID_REVISION='0013' ID_SERIAL='046d_Logitech_Webcam_C930e_816415EE' ID_SERIAL_SHORT='816415EE' ID_TYPE='video' ID_USB_DRIVER='uvcvideo' ID_USB_INTERFACES=':0e0100:0e0200:010100:010200:' ID_USB_INTERFACE_NUM='00' ID_V4L_CAPABILITIES=':capture:' ID_V4L_PRODUCT='Logitech Webcam C930e' ID_V4L_VERSION='2' ID_VENDOR='046d' ID_VENDOR_ENC='046d' ID_VENDOR_ID='046d' IFS=' ' MAJOR='81' MINOR='0' OPTIND='1' PATH='/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin' PPID='853' PS1='# ' PS2='> ' PS4='+ ' PWD='/' SEQNUM='1895' SUBSYSTEM='video4linux' TAGS=':uaccess:seat:' USEC_INITIALIZED='175470181' debug='1' logfile='/var/log/uvcdynctrl-udev.log' uvcdynctrlpath='uvcdynctrl' version='0.3' Triggered at Wed Apr 11 09:08:26 CEST 2018 VID of new device: '046d' PID of new device: '0843' Executing command: 'uvcdynctrl -d /dev/video0 --addctrl=046d:0843' ACTION='add' DEVLINKS='/dev/v4l/by-id/usb-046d_Logitech_Webcam_C930e_816415EE-video-index1 /dev/v4l/by-path/pci-:00:14.0-usb-0:1:1.0-video-index1' DEVNAME='/dev/video1' DEVPATH='/devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0/video4linux/video1' ID_BUS='usb' ID_FOR_SEAT='video4linux-pci-_00_14_0-usb-0_1_1_0' ID_MODEL='Logitech_Webcam_C930e' ID_MODEL_ENC='Logitech\x20Webcam\x20C930e' ID_MODEL_ID='0843' ID_PATH='pci-:00:14.0-usb-0:1:1.0' ID_PATH_TAG='pci-_00_14_0-usb-0_1_1_0' ID_REVISION='0013' ID_SERIAL='046d_Logitech_Webcam_C930e_816415EE' ID_SERIAL_SHORT='816415EE' ID_TYPE='video' ID_USB_DRIVER='uvcvideo' ID_USB_INTERFACES=':0e0100:0e0200:010100:010200:' ID_USB_INTERFACE_NUM='00' ID_V4L_CAPABILITIES=':capture:' ID_V4L_PRODUCT='Logitech Webcam C930e' ID_V4L_VERSION='2' ID_VENDOR='046d' ID_VENDOR_ENC='046d' ID_VENDOR_ID='046d' IFS=' ' MAJOR='81' MINOR='1' OPTIND='1' PATH='/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin' PPID='858' PS1='# ' PS2='> ' PS4='+ ' PWD='/' SEQNUM='1896' SUBSYSTEM='video4linux' TAGS=':uaccess:seat:' USEC_INITIALIZED='175470183' debug='1' logfile='/var/log/uvcdynctrl-udev.log' uvcdynctrlpath='uvcdynctrl' version='0.3' VID of new device: '046d' PID of new device: '0843' Executing command: 'uvcdynctrl -d /dev/video1 --addctrl=046d:0843' [libwebcam] Warning: The driver behind device video0 has a slightly buggy implementation of the V4L2_CTRL_FLAG_NEXT_CTRL flag. It does not return the next higher control ID if a control query fails. A workaround has been enabled. [libwebcam] Warning: The driver behind device video0 has a slightly buggy implementation of the V4L2_CTRL_FLAG_NEXT_CTRL flag. It does not return the next higher control ID if a control query fails. A workaround has been enabled. [libwebcam] Warning: The driver behind device video0 has a slightly buggy implementation of the V4L2_CTRL_FLAG_NEXT_CTRL flag. It does not return the next higher control ID if a control query fails. A workaround has been enabled. [libwebcam] Warning: The driver behind device video
sensor driver - v4l2 - MEDIA_BUS_FMT
Hi All, We are trying develop a sensor driver code for hdmi2csi adapter. Reguired data format is RGB888. But in media format header file, we could see three macros related to RGB888. Hardware connection is mipi csi2. #define MEDIA_BUS_FMT_RGB888_1X24 0x100a #define MEDIA_BUS_FMT_RGB888_2X12_BE 0x100b #define MEDIA_BUS_FMT_RGB888_2X12_LE 0x100c How to decide whether we go for RGB888_1X24 or RGB888_2X12 macros. Thanks & Regards - Asad
Re: [RFCv11 PATCH 03/29] media-request: allocate media requests
Em Tue, 10 Apr 2018 14:14:30 +0300 Sakari Ailus escreveu: > > > /** > > > @@ -88,6 +96,8 @@ struct media_device_ops { > > > * @disable_source: Disable Source Handler function pointer > > > * > > > * @ops: Operation handler callbacks > > > + * @req_lock:Serialise access to requests > > > + * @req_queue_mutex: Serialise validating and queueing requests > > > > IMHO, this would better describe it: > > Serialise validate and queue requests > > > > Yet, IMO, it doesn't let it clear when the spin lock should be > > used and when the mutex should be used. > > > > I mean, what of them protect what variable? > > It might not be obvious, but the purpose of this mutex is to prevent > queueing multiple requests simultaneously in order to serialise access to > the top of the queue. It is not obvious at all. The purpose of a lock is to prevent multiple acesses to the content of a data structure. >From what I see, here we have a single structure with two locks. To make it worse, you're introducing first the lock and then, on patch 04/29, the actual structs to be protected. Well, a house with two doors is only closed if both doors are closed. The same happens with a data structure protected by two locks. It is only locked if both locks are locked. So, every time I see two locks meant to protect the same struct, it sounds poorly designed or wrong. There's one exception, though: if you have an independent guest house and a main house, you could have two locks at the same place, one for the main house and another one for the guest house, but in this case, you should clearly tag with lock protects each house. So, in this case, two new locks that are being proposed to be added at for struct media_device. So, I would be expecting a comment like: @req_lock: serialize access to &struct media_request @req_queue_mutex: serialize access to &struct media_request_object Clearly stating what data structures each lock protects. That was my concern when I pointed it. After looking at the entire patchset, what I saw was a non-consistent locking model. It sounded to me that the original design to be: 1) req_queue_mutex was designed to protect struct media_request instances; 2) req_lock was designed to protect just one field inside struct media_request (the state field). There is a big issue on that: As state is part of media_request, assuming that the data struct won't disappear while in use (with is warranted by kref), before changing its value and touching other fields at req, the code should be locking both req_queue_mutex and req_lock, but I didn't see that behavior on several places. Also, I noticed several locking inconsistencies, as, on several places, the content of a media_request instance and/or its state was accessed/altered without either locks. > How about this instead: > > Serialise access to accessing device state on the tail of the > request queue. It still doesn't mention what struct each of the new locks protect. IMHO, this patch should either be bound with patch 04/29 or come after that, and explicitly mention what data is protected by each lock. Thanks, Mauro
Re: sensor driver - v4l2 - MEDIA_BUS_FMT
On Wed, Apr 11, 2018 at 03:33:50PM +0530, asadpt iqroot wrote: > Hi All, > > We are trying develop a sensor driver code for hdmi2csi adapter. > Reguired data format is RGB888. But in media format header file, we > could see three macros related to RGB888. Hardware connection is mipi > csi2. > > #define MEDIA_BUS_FMT_RGB888_1X24 0x100a > #define MEDIA_BUS_FMT_RGB888_2X12_BE 0x100b > #define MEDIA_BUS_FMT_RGB888_2X12_LE 0x100c > > How to decide whether we go for RGB888_1X24 or RGB888_2X12 macros. Originally when support was added for serial busses, we did not create new formats for these busses but instead re-used the ones intended for parallel busses. Please use the single-sample variant of the format. -- Sakari Ailus e-mail: sakari.ai...@iki.fi
[PATCH] media: ddbridge: better handle optional spin locks at the code
Currently, ddbridge produces 4 warnings on sparse: drivers/media/pci/ddbridge/ddbridge-core.c:495:9: warning: context imbalance in 'ddb_output_start' - different lock contexts for basic block drivers/media/pci/ddbridge/ddbridge-core.c:510:9: warning: context imbalance in 'ddb_output_stop' - different lock contexts for basic block drivers/media/pci/ddbridge/ddbridge-core.c:525:9: warning: context imbalance in 'ddb_input_stop' - different lock contexts for basic block drivers/media/pci/ddbridge/ddbridge-core.c:560:9: warning: context imbalance in 'ddb_input_start' - different lock contexts for basic block Those are all false positives, but they result from the fact that there could potentially have some troubles at the locking schema, because the lock depends on a var (output->dma). I discussed that in priv with Sparse author and with the current maintainer. Both believe that sparse is doing the right thing, and that the proper fix would be to change the code to make it clearer that, when spin_lock_irq() is called, spin_unlock_irq() will be also called. That help not only static analyzers to better understand the code, but also humans that could need to take a look at the code. It was also pointed that gcc would likely be smart enough to optimize the code and produce the same result. I double checked: indeed, the size of the driver didn't change after this patch. Signed-off-by: Mauro Carvalho Chehab --- drivers/media/pci/ddbridge/ddbridge-core.c | 43 -- 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c b/drivers/media/pci/ddbridge/ddbridge-core.c index 4a2819d3e225..080e2189ca7f 100644 --- a/drivers/media/pci/ddbridge/ddbridge-core.c +++ b/drivers/media/pci/ddbridge/ddbridge-core.c @@ -458,13 +458,12 @@ static void calc_con(struct ddb_output *output, u32 *con, u32 *con2, u32 flags) *con2 = (nco << 16) | gap; } -static void ddb_output_start(struct ddb_output *output) +static void __ddb_output_start(struct ddb_output *output) { struct ddb *dev = output->port->dev; u32 con = 0x11c, con2 = 0; if (output->dma) { - spin_lock_irq(&output->dma->lock); output->dma->cbuf = 0; output->dma->coff = 0; output->dma->stat = 0; @@ -492,9 +491,18 @@ static void ddb_output_start(struct ddb_output *output) ddbwritel(dev, con | 1, TS_CONTROL(output)); - if (output->dma) { + if (output->dma) output->dma->running = 1; +} + +static void ddb_output_start(struct ddb_output *output) +{ + if (output->dma) { + spin_lock_irq(&output->dma->lock); + __ddb_output_start(output); spin_unlock_irq(&output->dma->lock); + } else { + __ddb_output_start(output); } } @@ -502,15 +510,13 @@ static void ddb_output_stop(struct ddb_output *output) { struct ddb *dev = output->port->dev; - if (output->dma) - spin_lock_irq(&output->dma->lock); - - ddbwritel(dev, 0, TS_CONTROL(output)); - if (output->dma) { + spin_lock_irq(&output->dma->lock); ddbwritel(dev, 0, DMA_BUFFER_CONTROL(output->dma)); output->dma->running = 0; spin_unlock_irq(&output->dma->lock); + } else { + ddbwritel(dev, 0, TS_CONTROL(output)); } } @@ -519,22 +525,21 @@ static void ddb_input_stop(struct ddb_input *input) struct ddb *dev = input->port->dev; u32 tag = DDB_LINK_TAG(input->port->lnr); - if (input->dma) - spin_lock_irq(&input->dma->lock); - ddbwritel(dev, 0, tag | TS_CONTROL(input)); if (input->dma) { + spin_lock_irq(&input->dma->lock); ddbwritel(dev, 0, DMA_BUFFER_CONTROL(input->dma)); input->dma->running = 0; spin_unlock_irq(&input->dma->lock); + } else { + ddbwritel(dev, 0, tag | TS_CONTROL(input)); } } -static void ddb_input_start(struct ddb_input *input) +static void __ddb_input_start(struct ddb_input *input) { struct ddb *dev = input->port->dev; if (input->dma) { - spin_lock_irq(&input->dma->lock); input->dma->cbuf = 0; input->dma->coff = 0; input->dma->stat = 0; @@ -557,9 +562,19 @@ static void ddb_input_start(struct ddb_input *input) if (input->port->type == DDB_TUNER_DUMMY) ddbwritel(dev, 0x000fff01, TS_CONTROL2(input)); - if (input->dma) { + if (input->dma) input->dma->running = 1; +} + +static void ddb_input_start(struct ddb_input *input) +{ + + if (input->dma) { + spin_lock_irq(&input->dma->lock); + __ddb_input_start(input); spin_unlock_irq(&input->dma->lock); +
Re: [PATCH v7 2/2] uvcvideo: handle control pipe protocol STALLs
Hi Laurent, First just answers to your questions: On Sat, 7 Apr 2018, Laurent Pinchart wrote: > Hi Guennadi, > > Thank you for the patch. > > On Friday, 23 March 2018 11:24:01 EEST Laurent Pinchart wrote: > > From: Guennadi Liakhovetski > > > > When a command ends up in a STALL on the control pipe, use the Request > > Error Code control to provide a more precise error information to the > > user. > > This is the kind of change that I believe is completely right, but that > nonetheless worries me. All the years I've spent working with UVC webcams > taught me that lots of cameras have buggy firmware, and that any change in > how > the host driver issues requests can have dire consequences for users. This is > especially true when the host driver issues a request that was never issued > before. > > The UVC specification also doesn't clearly tell whether the request error > code > control is mandatory for a device to implement. My interpretation of the > specification is that it is, but it would have been nice for the > specification > to be explicit on this topic. Have you encountered devices that don't > implement this control ? No, I haven't. But I haven't explicitly tested too many either. This patch would only issue that control if a STALL condition is detected, and normally that doesn't happen. > This being said, I don't claim that would be a reason not to use the request > error code control as proposed by this patch, but I would feel less worried > if > I knew whether the Windows driver used that control as well. If it does > there's a high chance that cameras will implement it correctly, while if it > doesn't we will most certainly hit bugs with several cameras. I was thus > wondering if in the course of your UVC developments you would have happened > to > find out whether this control is used by Windows. No, sorry, I never tried to analyse the behaviour of the Windows UVC driver. > I would also like to know a bit more about the purpose of this patch. Logging > the error code is certainly useful for diagnosis purpose. Have you also found > it useful to report different errors back to userspace ? If so, could you > explain your use cases ? Yes, with this patch the user-space can with certainty identify the reason of a stall, specifically you would know, when the camera is in a "not ready" state. With the previous patch in this series the driver shouldn't be sending a second SET_CUR command to the same control, before the first one has completed, but on some cameras different controls can also be interrelated. In such a case trying to set a different control, while a previous one is still being processed, can also cause a STALL with a "Not ready" error state. > > Signed-off-by: Guennadi Liakhovetski > > --- > > drivers/media/usb/uvc/uvc_video.c | 59 > > 1 file changed, 53 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c > > b/drivers/media/usb/uvc/uvc_video.c index aa0082fe5833..eb9e04a59427 100644 > > --- a/drivers/media/usb/uvc/uvc_video.c > > +++ b/drivers/media/usb/uvc/uvc_video.c > > @@ -34,15 +34,59 @@ static int __uvc_query_ctrl(struct uvc_device *dev, u8 > > query, u8 unit, u8 intfnum, u8 cs, void *data, u16 size, > > int timeout) > > { > > - u8 type = USB_TYPE_CLASS | USB_RECIP_INTERFACE; > > + u8 type = USB_TYPE_CLASS | USB_RECIP_INTERFACE, tmp, error; > > Would you mind declaring one variable per line to match the style of the rest > of the driver ? In fact I would, but well... ;-) > > unsigned int pipe; > > + int ret; > > > > pipe = (query & 0x80) ? usb_rcvctrlpipe(dev->udev, 0) > > : usb_sndctrlpipe(dev->udev, 0); > > > > type |= (query & 0x80) ? USB_DIR_IN : USB_DIR_OUT; > > > > - return usb_control_msg(dev->udev, pipe, query, type, cs << 8, > > + ret = usb_control_msg(dev->udev, pipe, query, type, cs << 8, > > unit << 8 | intfnum, data, size, timeout); > > + > > Nitpicking, you can remove the blank line here. > > > + if (ret != -EPIPE) > > + return ret; > > + > > + tmp = *(u8 *)data; > > + > > + pipe = usb_rcvctrlpipe(dev->udev, 0); > > + type = USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN; > > + ret = usb_control_msg(dev->udev, pipe, UVC_GET_CUR, type, > > + UVC_VC_REQUEST_ERROR_CODE_CONTROL << 8, > > + unit << 8 | intfnum, data, 1, timeout); > > Unless I'm mistaken the wIndex value should be "Zero and Interface" according > to both the UVC 1.1 and UVC 1.5 specifications. This should thus be > > ret = usb_control_msg(dev->udev, pipe, UVC_GET_CUR, type, > UVC_VC_REQUEST_ERROR_CODE_CONTROL << 8, > intfnum, data, 1, timeout); Hm, the 1.5 spec says: This read-only control indicates the status of each host-initiated request to a Termi
Re: [RFCv11 PATCH 04/29] media-request: core request support
Hi Mauro, On Tue, Apr 10, 2018 at 11:51:43AM -0300, Mauro Carvalho Chehab wrote: > Em Tue, 10 Apr 2018 15:32:34 +0300 > Sakari Ailus escreveu: > > > Hi Mauro and Hans, > > > > On Tue, Apr 10, 2018 at 07:32:06AM -0300, Mauro Carvalho Chehab wrote: > > ... > > > > +static void media_request_release(struct kref *kref) > > > > +{ > > > > + struct media_request *req = > > > > + container_of(kref, struct media_request, kref); > > > > + struct media_device *mdev = req->mdev; > > > > + unsigned long flags; > > > > + > > > > + dev_dbg(mdev->dev, "request: release %s\n", req->debug_str); > > > > + > > > > + spin_lock_irqsave(&req->lock, flags); > > > > + req->state = MEDIA_REQUEST_STATE_CLEANING; > > > > + spin_unlock_irqrestore(&req->lock, flags); > > > > + > > > > + media_request_clean(req); > > > > + > > > > + if (mdev->ops->req_free) > > > > + mdev->ops->req_free(req); > > > > + else > > > > + kfree(req); > > > > > > Without looking at req_free() implementation, I would actually prefer > > > to always do a kfree(req) here. So, a req_free() function would only > > > free "extra" allocations, and not the request itself. e. g.: > > > > > > ... > > > if (mdev->ops->req_free) > > > mdev->ops->req_free(req); > > > > > > kfree(req); > > > } > > > > The idea is that you can embed the request object in a driver specific > > struct. The drivers can store information related to that request rather > > easily that way, without requiring to be aware of two objects with > > references pointing to each other. I rather prefer the current > > implementation. It same pattern is actually used on videobuf2 buffers. > > Ok, then document it so. > > Btw, one of the things it is missing is a kAPI documentation patch, > describing things like that. I agree. Documentation will need to be added and improved. > > > > > ... > > > > > > +static unsigned int media_request_poll(struct file *filp, > > > > + struct poll_table_struct *wait) > > > > +{ > > > > + struct media_request *req = filp->private_data; > > > > + unsigned long flags; > > > > + enum media_request_state state; > > > > + > > > > + if (!(poll_requested_events(wait) & POLLPRI)) > > > > + return 0; > > > > + > > > > + spin_lock_irqsave(&req->lock, flags); > > > > + state = req->state; > > > > + spin_unlock_irqrestore(&req->lock, flags); > > > > > > IMO, it would be better to use an atomic var for state, having a > > > lockless access to it. > > > > The lock serialises access to the whole request, not just to its state. > > From what I understood at the code is that the spin lock > is used *sometimes* to protect just the state. I didn't > see it used to protect the hole data struct. > > Instead, the mutex is used for that purpose, but, again, > it is *sometimes* used, but on several parts, neither the > spin lock nor the mutex is used. > > It should be noticed that the data, itself, should be protected > by *either* mutex or spin lock. > > > While it doesn't matter here as you're just reading the state, writing it > > would still require taking the lock. Using atomic_t might suggest > > otherwise, and could end up being a source of bugs. > > There are already a lot of bugs at the locking, from what I noticed. > > We should do it right: it should use *just one* kind of memory > protection for struct media_request. On the places where it is > safe to just read the status without locking, atomic_t() should > be used. Where it doesn't, probably the entire code should be > protected by the lock. If done that way, it needs to be documented because it isn't obvious. Keeping the code simple helps a lot; I don't see the core code growing a lot so this looks good. > > > > > > > > + > > > > + if (state == MEDIA_REQUEST_STATE_COMPLETE) > > > > + return POLLPRI; > > > > + if (state == MEDIA_REQUEST_STATE_IDLE) > > > > + return POLLERR; > > > > + > > > > + poll_wait(filp, &req->poll_wait, wait); > > > > + return 0; > > > > +} > > > > + > > > > +static long media_request_ioctl(struct file *filp, unsigned int cmd, > > > > + unsigned long __arg) > > > > +{ > > > > + return -ENOIOCTLCMD; > > > > +} > > > > + > > > > +static const struct file_operations request_fops = { > > > > + .owner = THIS_MODULE, > > > > + .poll = media_request_poll, > > > > + .unlocked_ioctl = media_request_ioctl, > > > > + .release = media_request_close, > > > > +}; > > > > + > > > > int media_request_alloc(struct media_device *mdev, > > > > struct media_request_alloc *alloc) > > > > { > > > > - return -ENOMEM; > > > > + struct media_request *req; > > > > + struct file *filp; > > > > + char comm[TASK_COMM_LEN]; > > > > +
[PATCH 2/5] dma-buf: add optional invalidate_mappings callback v3
Each importer can now provide an invalidate_mappings callback. This allows the exporter to provide the mappings without the need to pin the backing store. v2: don't try to invalidate mappings when the callback is NULL, lock the reservation obj while using the attachments, add helper to set the callback v3: move flag for invalidation support into the DMA-buf, use new attach_info structure to set the callback v4: use importer_priv field instead of mangling exporter priv. Signed-off-by: Christian König --- drivers/dma-buf/dma-buf.c | 44 include/linux/dma-buf.h | 36 ++-- 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 4b46982c6d9c..ffdaab10e2f2 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -565,6 +565,8 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info attach->dev = info->dev; attach->dmabuf = dmabuf; + attach->importer_priv = info->importer_priv; + attach->invalidate = info->invalidate; mutex_lock(&dmabuf->lock); @@ -573,7 +575,9 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info if (ret) goto err_attach; } + reservation_object_lock(dmabuf->resv, NULL); list_add(&attach->node, &dmabuf->attachments); + reservation_object_unlock(dmabuf->resv); mutex_unlock(&dmabuf->lock); return attach; @@ -599,7 +603,9 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) return; mutex_lock(&dmabuf->lock); + reservation_object_lock(dmabuf->resv, NULL); list_del(&attach->node); + reservation_object_unlock(dmabuf->resv); if (dmabuf->ops->detach) dmabuf->ops->detach(dmabuf, attach); @@ -633,10 +639,23 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, if (WARN_ON(!attach || !attach->dmabuf)) return ERR_PTR(-EINVAL); + /* +* Mapping a DMA-buf can trigger its invalidation, prevent sending this +* event to the caller by temporary removing this attachment from the +* list. +*/ + if (attach->invalidate) { + reservation_object_assert_held(attach->dmabuf->resv); + list_del(&attach->node); + } + sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); if (!sg_table) sg_table = ERR_PTR(-ENOMEM); + if (attach->invalidate) + list_add(&attach->node, &attach->dmabuf->attachments); + return sg_table; } EXPORT_SYMBOL_GPL(dma_buf_map_attachment); @@ -657,6 +676,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, { might_sleep(); + if (attach->invalidate) + reservation_object_assert_held(attach->dmabuf->resv); + if (WARN_ON(!attach || !attach->dmabuf || !sg_table)) return; @@ -665,6 +687,26 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); +/** + * dma_buf_invalidate_mappings - invalidate all mappings of this dma_buf + * + * @dmabuf:[in]buffer which mappings should be invalidated + * + * Informs all attachmenst that they need to destroy and recreated all their + * mappings. + */ +void dma_buf_invalidate_mappings(struct dma_buf *dmabuf) +{ + struct dma_buf_attachment *attach; + + reservation_object_assert_held(dmabuf->resv); + + list_for_each_entry(attach, &dmabuf->attachments, node) + if (attach->invalidate) + attach->invalidate(attach); +} +EXPORT_SYMBOL_GPL(dma_buf_invalidate_mappings); + /** * DOC: cpu access * @@ -1122,10 +1164,12 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) seq_puts(s, "\tAttached Devices:\n"); attach_count = 0; + reservation_object_lock(buf_obj->resv, NULL); list_for_each_entry(attach_obj, &buf_obj->attachments, node) { seq_printf(s, "\t%s\n", dev_name(attach_obj->dev)); attach_count++; } + reservation_object_unlock(buf_obj->resv); seq_printf(s, "Total %d devices attached\n\n", attach_count); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 414b4dde5eb7..566503dd2b4f 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -270,6 +270,8 @@ struct dma_buf_ops { * @poll: for userspace poll support * @cb_excl: for userspace poll support * @cb_shared: for userspace poll support + * @invalidation_supported: True when the exporter supports unpinned operation + *
[PATCH 1/5] dma-buf: use parameter structure for dma_buf_attach
Move the parameters into a structure to make it simpler to extend it in follow up patches. This also adds the importer private as parameter so that we can directly work with a completely filled in attachment structure. Signed-off-by: Christian König --- drivers/dma-buf/dma-buf.c | 15 --- drivers/gpu/drm/armada/armada_gem.c | 6 +- drivers/gpu/drm/drm_prime.c | 6 +- drivers/gpu/drm/i915/i915_gem_dmabuf.c| 6 +- drivers/gpu/drm/tegra/gem.c | 6 +- drivers/gpu/drm/udl/udl_dmabuf.c | 6 +- drivers/media/common/videobuf2/videobuf2-dma-contig.c | 6 +- drivers/media/common/videobuf2/videobuf2-dma-sg.c | 6 +- drivers/staging/media/tegra-vde/tegra-vde.c | 6 +- include/linux/dma-buf.h | 17 +++-- 10 files changed, 63 insertions(+), 17 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index d78d5fc173dc..4b46982c6d9c 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -534,8 +534,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put); /** * dma_buf_attach - Add the device to dma_buf's attachments list; optionally, * calls attach() of dma_buf_ops to allow device-specific attach functionality - * @dmabuf:[in]buffer to attach device to. - * @dev: [in]device to be attached. + * @info: [in]holds all the attach related information provided + * by the importer. see &struct dma_buf_attach_info + * for further details. * * Returns struct dma_buf_attachment pointer for this attachment. Attachments * must be cleaned up by calling dma_buf_detach(). @@ -549,26 +550,26 @@ EXPORT_SYMBOL_GPL(dma_buf_put); * accessible to @dev, and cannot be moved to a more suitable place. This is * indicated with the error code -EBUSY. */ -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, - struct device *dev) +struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info) { + struct dma_buf *dmabuf = info->dmabuf; struct dma_buf_attachment *attach; int ret; - if (WARN_ON(!dmabuf || !dev)) + if (WARN_ON(!dmabuf || !info->dev)) return ERR_PTR(-EINVAL); attach = kzalloc(sizeof(*attach), GFP_KERNEL); if (!attach) return ERR_PTR(-ENOMEM); - attach->dev = dev; + attach->dev = info->dev; attach->dmabuf = dmabuf; mutex_lock(&dmabuf->lock); if (dmabuf->ops->attach) { - ret = dmabuf->ops->attach(dmabuf, dev, attach); + ret = dmabuf->ops->attach(dmabuf, info->dev, attach); if (ret) goto err_attach; } diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index a97f509743a5..f4d1c11f57ea 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -514,6 +514,10 @@ armada_gem_prime_export(struct drm_device *dev, struct drm_gem_object *obj, struct drm_gem_object * armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf) { + struct dma_buf_attach_info attach_info = { + .dev = dev->dev, + .dmabuf = buf + }; struct dma_buf_attachment *attach; struct armada_gem_object *dobj; @@ -529,7 +533,7 @@ armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf) } } - attach = dma_buf_attach(buf, dev->dev); + attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) return ERR_CAST(attach); diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 7856a9b3f8a8..4da242de51c2 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -707,6 +707,10 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, struct dma_buf *dma_buf, struct device *attach_dev) { + struct dma_buf_attach_info attach_info = { + .dev = attach_dev, + .dmabuf = dma_buf + }; struct dma_buf_attachment *attach; struct sg_table *sgt; struct drm_gem_object *obj; @@ -727,7 +731,7 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, if (!dev->driver->gem_prime_import_sg_table) return ERR_PTR(-EINVAL); - attach = dma_buf_attach(dma_buf, attach_dev); + attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) return ERR_CAST(attach); diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index 69a7aec49e84..7b737a883106 100644 --- a
[PATCH 3/5] drm/ttm: remove the backing store if no placement is given
Pipeline removal of the BOs backing store when the placement is given during validation. Signed-off-by: Christian König --- drivers/gpu/drm/ttm/ttm_bo.c | 12 1 file changed, 12 insertions(+) diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 98e06f8bf23b..17e821f01d0a 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -1078,6 +1078,18 @@ int ttm_bo_validate(struct ttm_buffer_object *bo, uint32_t new_flags; reservation_object_assert_held(bo->resv); + + /* +* Remove the backing store if no placement is given. +*/ + if (!placement->num_placement && !placement->num_busy_placement) { + ret = ttm_bo_pipeline_gutting(bo); + if (ret) + return ret; + + return ttm_tt_create(bo, false); + } + /* * Check whether we need to move buffer. */ -- 2.14.1
[PATCH 4/5] drm/amdgpu: add independent DMA-buf export v2
Instead of relying on the DRM functions just implement our own export functions. This adds support for taking care of unpinned DMA-buf. v2: fix unintended recursion, remove debugging leftovers Signed-off-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 - drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 1 - drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 5 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 108 ++--- 4 files changed, 72 insertions(+), 43 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 2babfad1fd7f..3e1727251edc 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -371,7 +371,6 @@ int amdgpu_gem_object_open(struct drm_gem_object *obj, void amdgpu_gem_object_close(struct drm_gem_object *obj, struct drm_file *file_priv); unsigned long amdgpu_gem_timeout(uint64_t timeout_ns); -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object *obj); struct drm_gem_object * amdgpu_gem_prime_import_sg_table(struct drm_device *dev, struct dma_buf_attachment *attach, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 0b19482b36b8..6107b845d8a8 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -890,7 +890,6 @@ static struct drm_driver kms_driver = { .gem_prime_export = amdgpu_gem_prime_export, .gem_prime_import = amdgpu_gem_prime_import, .gem_prime_res_obj = amdgpu_gem_prime_res_obj, - .gem_prime_get_sg_table = amdgpu_gem_prime_get_sg_table, .gem_prime_import_sg_table = amdgpu_gem_prime_import_sg_table, .gem_prime_vmap = amdgpu_gem_prime_vmap, .gem_prime_vunmap = amdgpu_gem_prime_vunmap, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index 9e23d6f6f3f3..15bb48cebbc4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -31,6 +31,7 @@ */ #include #include +#include #include #include #include @@ -953,6 +954,10 @@ void amdgpu_bo_move_notify(struct ttm_buffer_object *bo, amdgpu_bo_kunmap(abo); + if (abo->gem_base.dma_buf && !abo->gem_base.import_attach && + bo->mem.mem_type != TTM_PL_SYSTEM) + dma_buf_invalidate_mappings(abo->gem_base.dma_buf); + /* remember the eviction */ if (evict) atomic64_inc(&adev->num_evictions); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c index 4b584cb75bf4..7fef95f0fed1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c @@ -32,14 +32,6 @@ static const struct dma_buf_ops amdgpu_dmabuf_ops; -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object *obj) -{ - struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); - int npages = bo->tbo.num_pages; - - return drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages); -} - void *amdgpu_gem_prime_vmap(struct drm_gem_object *obj) { struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); @@ -126,23 +118,22 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev, return ERR_PTR(ret); } -static int amdgpu_gem_map_attach(struct dma_buf *dma_buf, -struct device *target_dev, -struct dma_buf_attachment *attach) +static struct sg_table * +amdgpu_gem_map_dma_buf(struct dma_buf_attachment *attach, + enum dma_data_direction dir) { + struct dma_buf *dma_buf = attach->dmabuf; struct drm_gem_object *obj = dma_buf->priv; struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); + struct sg_table *sgt; long r; - r = drm_gem_map_attach(dma_buf, target_dev, attach); - if (r) - return r; - - r = amdgpu_bo_reserve(bo, false); - if (unlikely(r != 0)) - goto error_detach; - + if (!attach->invalidate) { + r = amdgpu_bo_reserve(bo, false); + if (unlikely(r != 0)) + return ERR_PTR(r); + } if (attach->dev->driver != adev->dev->driver) { /* @@ -158,42 +149,77 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf, } } - /* pin buffer into GTT */ - r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT, NULL); - if (r) + if (!attach->invalidate) { + /* pin buffer into GTT */ + r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT, NULL); + if (r) + goto error_unreserve; + + } else { + /* move buffer into GTT */ + struct
[PATCH 5/5] drm/amdgpu: add independent DMA-buf import v3
Instead of relying on the DRM functions just implement our own import functions. This adds support for taking care of unpinned DMA-buf. v2: enable for all exporters, not just amdgpu, fix invalidation handling, lock reservation object while setting callback v3: change to new dma_buf attach interface Signed-off-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 72 ++- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 32 +++--- 2 files changed, 89 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c index 7fef95f0fed1..58dcfba0225a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c @@ -86,28 +86,24 @@ int amdgpu_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma return ret; } -struct drm_gem_object * -amdgpu_gem_prime_import_sg_table(struct drm_device *dev, -struct dma_buf_attachment *attach, -struct sg_table *sg) +static struct drm_gem_object * +amdgpu_gem_prime_create_obj(struct drm_device *dev, struct dma_buf *dma_buf) { - struct reservation_object *resv = attach->dmabuf->resv; + struct reservation_object *resv = dma_buf->resv; struct amdgpu_device *adev = dev->dev_private; struct amdgpu_bo *bo; int ret; ww_mutex_lock(&resv->lock, NULL); - ret = amdgpu_bo_create(adev, attach->dmabuf->size, PAGE_SIZE, + ret = amdgpu_bo_create(adev, dma_buf->size, PAGE_SIZE, AMDGPU_GEM_DOMAIN_CPU, 0, ttm_bo_type_sg, resv, &bo); if (ret) goto error; - bo->tbo.sg = sg; - bo->tbo.ttm->sg = sg; bo->allowed_domains = AMDGPU_GEM_DOMAIN_GTT; bo->preferred_domains = AMDGPU_GEM_DOMAIN_GTT; - if (attach->dmabuf->ops != &amdgpu_dmabuf_ops) + if (dma_buf->ops != &amdgpu_dmabuf_ops) bo->prime_shared_count = 1; ww_mutex_unlock(&resv->lock); @@ -118,6 +114,26 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev, return ERR_PTR(ret); } +struct drm_gem_object * +amdgpu_gem_prime_import_sg_table(struct drm_device *dev, +struct dma_buf_attachment *attach, +struct sg_table *sg) +{ + struct drm_gem_object *obj; + struct amdgpu_bo *bo; + + obj = amdgpu_gem_prime_create_obj(dev, attach->dmabuf); + if (IS_ERR(obj)) + return obj; + + bo = gem_to_amdgpu_bo(obj); + amdgpu_bo_reserve(bo, true); + bo->tbo.sg = sg; + bo->tbo.ttm->sg = sg; + amdgpu_bo_unreserve(bo); + return obj; +} + static struct sg_table * amdgpu_gem_map_dma_buf(struct dma_buf_attachment *attach, enum dma_data_direction dir) @@ -293,9 +309,29 @@ struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev, return buf; } +static void +amdgpu_gem_prime_invalidate_mappings(struct dma_buf_attachment *attach) +{ + struct ttm_operation_ctx ctx = { false, false }; + struct drm_gem_object *obj = attach->priv; + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); + struct ttm_placement placement = {}; + int r; + + r = ttm_bo_validate(&bo->tbo, &placement, &ctx); + if (r) + DRM_ERROR("Failed to unmap DMA-buf import (%d))\n", r); +} + struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf) { + struct dma_buf_attach_info attach_info = { + .dev = dev->dev, + .dmabuf = dma_buf, + .invalidate = amdgpu_gem_prime_invalidate_mappings + }; + struct dma_buf_attachment *attach; struct drm_gem_object *obj; if (dma_buf->ops == &amdgpu_dmabuf_ops) { @@ -310,5 +346,21 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev, } } - return drm_gem_prime_import(dev, dma_buf); + if (!dma_buf->invalidation_supported) + return drm_gem_prime_import(dev, dma_buf); + + obj = amdgpu_gem_prime_create_obj(dev, dma_buf); + if (IS_ERR(obj)) + return obj; + + attach_info.importer_priv = obj; + attach = dma_buf_attach(&attach_info); + if (IS_ERR(attach)) { + drm_gem_object_put(obj); + return ERR_CAST(attach); + } + + get_dma_buf(dma_buf); + obj->import_attach = attach; + return obj; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index ab73300e6c7f..2a8f328918cb 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -43,6 +43,7 @@ #include #include #include +#include #inclu
Re: [RFCv11 PATCH 04/29] media-request: core request support
Em Wed, 11 Apr 2018 16:21:16 +0300 Sakari Ailus escreveu: > > > > Btw, this is a very good reason why you should define the ioctl to > > > > have an integer argument instead of a struct with a __s32 field > > > > on it, as per my comment to patch 02/29: > > > > > > > > #define MEDIA_IOC_REQUEST_ALLOC _IOWR('|', 0x05, int) > > > > > > > > At 64 bit architectures, you're truncating the file descriptor! > > > > > > I'm not quite sure what do you mean. int is 32 bits on 64-bit systems as > > > well. > > > > Hmm.. you're right. I was thinking that it could be 64 bits on some > > archs like sparc64 (Tru64 C compiler declares it with 64 bits), but, > > according with: > > > > https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html > > > > This is not the case on gcc. > > Ok. The reasoning back then was that what "int" means varies across > compilers and languages. And the intent was to codify this to __s32 which > is what the kernel effectively uses. ... > The rest of the kernel uses int rather liberally in the uAPI so I'm not > sure in the end whether something desirable was achieved. Perhaps it'd be > good to go back to the original discussion to find out for sure. > > Still binaries compiled with Tru64 C compiler wouldn't work on Linux anyway > due to that difference. > > Well, I stop here for this begins to be off-topic. :-) Yes. Let's keep it as s32 as originally proposed. Just ignore my comments about that :-) > > > > > + get_task_comm(comm, current); > > > > > + snprintf(req->debug_str, sizeof(req->debug_str), "%s:%d", > > > > > + comm, fd); > > > > > > > > Not sure if it is a good idea to store the task that allocated > > > > the request. While it makes sense for the dev_dbg() below, it > > > > may not make sense anymore on other dev_dbg() you would be > > > > using it. > > > > > > The lifetime of the file handle roughly matches that of the request. It's > > > for debug only anyway. > > > > > > Better proposals are always welcome of course. But I think we should have > > > something here that helps debugging by meaningfully making the requests > > > identifiable from logs. > > > > What I meant to say is that one PID could be allocating the > > request, while some other one could be actually doing Q/DQ_BUF. > > On such scenario, the debug string could provide mislead prints. > > Um, yes, indeed it would no longer match the process. But the request is > still the same. That's actually a positive thing since it allows you to > identify the request. > > With a global ID space this was trivial; you could just print the request > ID and that was all that was ever needed. (I'm not proposing to consider > that though.) > IMO, a global ID number would work better than get_task_comm(). Just add a static int monotonic counter and use it for the debug purposes, e. g.: { static unsigned int req_count = 0; snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d", req_count++, fd); Ok, eventually, it will overflow, but, it will be unique within a reasonable timeframe to be good enough for debugging purposes. Thanks, Mauro
Re: [RFCv11 PATCH 27/29] vim2m: support requests
Hi, On Mon, 2018-04-09 at 16:20 +0200, Hans Verkuil wrote: > From: Hans Verkuil > > Add support for requests to vim2m. Depending on where STREAMON happens in the sequence, there is a possibility that v4l2_m2m_try_schedule is never called and thus that device_run never runs when submitting the request. More specifically, the m2m fashion of the STREAMON ioctl handler will normally call v4l2_m2m_try_schedule. Hence, there is no issue if it's called after submitting the request. On the other hand, if the request was not submitted when calling STREAMON (which is a valid use case), buffers are not available and v4l2_m2m_try_schedule won't schedule anything. Once the request is submitted, the internal plumbing will detect that STREAMON was requested but did not take effect, so it will call vb2_streamon. And of course, vb2_streamon has no provision for trying to schedule a m2m device run. One way to fix this is to call v4l2_m2m_try_schedule from a workqueue triggered in the driver's start_streaming qop. The same is also probably necessary in buf_queue and buf_prepare since those qops are not called from the m2m ioctl handler either. What do you think? > Signed-off-by: Hans Verkuil > --- > drivers/media/platform/vim2m.c | 25 + > 1 file changed, 25 insertions(+) > > diff --git a/drivers/media/platform/vim2m.c > b/drivers/media/platform/vim2m.c > index 9b18b32c255d..2dcf0ea85705 100644 > --- a/drivers/media/platform/vim2m.c > +++ b/drivers/media/platform/vim2m.c > @@ -387,8 +387,26 @@ static void device_run(void *priv) > src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx); > dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > > + /* Apply request if needed */ > + if (src_buf->vb2_buf.req_obj.req) > + v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req, > + &ctx->hdl); > + if (dst_buf->vb2_buf.req_obj.req && > + dst_buf->vb2_buf.req_obj.req != src_buf- > >vb2_buf.req_obj.req) > + v4l2_ctrl_request_setup(dst_buf->vb2_buf.req_obj.req, > + &ctx->hdl); > + > device_process(ctx, src_buf, dst_buf); > > + /* Complete request if needed */ > + if (src_buf->vb2_buf.req_obj.req) > + v4l2_ctrl_request_complete(src_buf- > >vb2_buf.req_obj.req, > + &ctx->hdl); > + if (dst_buf->vb2_buf.req_obj.req && > + dst_buf->vb2_buf.req_obj.req != src_buf- > >vb2_buf.req_obj.req) > + v4l2_ctrl_request_complete(dst_buf- > >vb2_buf.req_obj.req, > + &ctx->hdl); > + > /* Run a timer, which simulates a hardware irq */ > schedule_irq(dev, ctx->transtime); > } > @@ -823,6 +841,8 @@ static void vim2m_stop_streaming(struct vb2_queue > *q) > vbuf = v4l2_m2m_dst_buf_remove(ctx- > >fh.m2m_ctx); > if (vbuf == NULL) > return; > + v4l2_ctrl_request_complete(vbuf->vb2_buf.req_obj.req, > +&ctx->hdl); > spin_lock_irqsave(&ctx->dev->irqlock, flags); > v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); > spin_unlock_irqrestore(&ctx->dev->irqlock, flags); > @@ -1003,6 +1023,10 @@ static const struct v4l2_m2m_ops m2m_ops = { > .job_abort = job_abort, > }; > > +static const struct media_device_ops m2m_media_ops = { > + .req_queue = vb2_request_queue, > +}; > + > static int vim2m_probe(struct platform_device *pdev) > { > struct vim2m_dev *dev; > @@ -1027,6 +1051,7 @@ static int vim2m_probe(struct platform_device > *pdev) > dev->mdev.dev = &pdev->dev; > strlcpy(dev->mdev.model, "vim2m", sizeof(dev->mdev.model)); > media_device_init(&dev->mdev); > + dev->mdev.ops = &m2m_media_ops; > dev->v4l2_dev.mdev = &dev->mdev; > dev->pad[0].flags = MEDIA_PAD_FL_SINK; > dev->pad[1].flags = MEDIA_PAD_FL_SOURCE; -- Paul Kocialkowski, Bootlin (formerly Free Electrons) Embedded Linux and kernel engineering https://bootlin.com signature.asc Description: This is a digitally signed message part
Re: [RFCv11 PATCH 26/29] vim2m: use workqueue
Hi, On Mon, 2018-04-09 at 16:20 +0200, Hans Verkuil wrote: > From: Hans Verkuil > > v4l2_ctrl uses mutexes, so we can't setup a ctrl_handler in > interrupt context. Switch to a workqueue instead. See one comment below. > Signed-off-by: Hans Verkuil > --- > drivers/media/platform/vim2m.c | 15 +-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/vim2m.c > b/drivers/media/platform/vim2m.c > index ef970434af13..9b18b32c255d 100644 > --- a/drivers/media/platform/vim2m.c > +++ b/drivers/media/platform/vim2m.c > @@ -150,6 +150,7 @@ struct vim2m_dev { > spinlock_t irqlock; > > struct timer_list timer; > + struct work_struct work_run; Wouldn't it make more sense to move this to vim2m_ctx instead (since this is heavily m2m-specific)? > struct v4l2_m2m_dev *m2m_dev; > }; > @@ -392,9 +393,10 @@ static void device_run(void *priv) > schedule_irq(dev, ctx->transtime); > } > > -static void device_isr(struct timer_list *t) > +static void device_work(struct work_struct *w) > { > - struct vim2m_dev *vim2m_dev = from_timer(vim2m_dev, t, > timer); > + struct vim2m_dev *vim2m_dev = > + container_of(w, struct vim2m_dev, work_run); > struct vim2m_ctx *curr_ctx; > struct vb2_v4l2_buffer *src_vb, *dst_vb; > unsigned long flags; > @@ -426,6 +428,13 @@ static void device_isr(struct timer_list *t) > } > } > > +static void device_isr(struct timer_list *t) > +{ > + struct vim2m_dev *vim2m_dev = from_timer(vim2m_dev, t, > timer); > + > + schedule_work(&vim2m_dev->work_run); > +} > + > /* > * video ioctls > */ > @@ -806,6 +815,7 @@ static void vim2m_stop_streaming(struct vb2_queue > *q) > struct vb2_v4l2_buffer *vbuf; > unsigned long flags; > > + flush_scheduled_work(); > for (;;) { > if (V4L2_TYPE_IS_OUTPUT(q->type)) > vbuf = v4l2_m2m_src_buf_remove(ctx- > >fh.m2m_ctx); > @@ -1011,6 +1021,7 @@ static int vim2m_probe(struct platform_device > *pdev) > vfd = &dev->vfd; > vfd->lock = &dev->dev_mutex; > vfd->v4l2_dev = &dev->v4l2_dev; > + INIT_WORK(&dev->work_run, device_work); > > #ifdef CONFIG_MEDIA_CONTROLLER > dev->mdev.dev = &pdev->dev; -- Paul Kocialkowski, Bootlin (formerly Free Electrons) Embedded Linux and kernel engineering https://bootlin.com signature.asc Description: This is a digitally signed message part
Re: [RFCv11 PATCH 04/29] media-request: core request support
On Wed, Apr 11, 2018 at 10:49:35AM -0300, Mauro Carvalho Chehab wrote: > Em Wed, 11 Apr 2018 16:21:16 +0300 > Sakari Ailus escreveu: > > > > > > > Btw, this is a very good reason why you should define the ioctl to > > > > > have an integer argument instead of a struct with a __s32 field > > > > > on it, as per my comment to patch 02/29: > > > > > > > > > > #define MEDIA_IOC_REQUEST_ALLOC _IOWR('|', 0x05, int) > > > > > > > > > > At 64 bit architectures, you're truncating the file descriptor! > > > > > > > > I'm not quite sure what do you mean. int is 32 bits on 64-bit systems as > > > > well. > > > > > > Hmm.. you're right. I was thinking that it could be 64 bits on some > > > archs like sparc64 (Tru64 C compiler declares it with 64 bits), but, > > > according with: > > > > > > https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html > > > > > > This is not the case on gcc. > > > > Ok. The reasoning back then was that what "int" means varies across > > compilers and languages. And the intent was to codify this to __s32 which > > is what the kernel effectively uses. > > ... > > > The rest of the kernel uses int rather liberally in the uAPI so I'm not > > sure in the end whether something desirable was achieved. Perhaps it'd be > > good to go back to the original discussion to find out for sure. > > > > Still binaries compiled with Tru64 C compiler wouldn't work on Linux anyway > > due to that difference. > > > > Well, I stop here for this begins to be off-topic. :-) > > Yes. Let's keep it as s32 as originally proposed. Just ignore my comments > about that :-) > > > > > > > + get_task_comm(comm, current); > > > > > > + snprintf(req->debug_str, sizeof(req->debug_str), "%s:%d", > > > > > > +comm, fd); > > > > > > > > > > Not sure if it is a good idea to store the task that allocated > > > > > the request. While it makes sense for the dev_dbg() below, it > > > > > may not make sense anymore on other dev_dbg() you would be > > > > > using it. > > > > > > > > The lifetime of the file handle roughly matches that of the request. > > > > It's > > > > for debug only anyway. > > > > > > > > Better proposals are always welcome of course. But I think we should > > > > have > > > > something here that helps debugging by meaningfully making the requests > > > > identifiable from logs. > > > > > > What I meant to say is that one PID could be allocating the > > > request, while some other one could be actually doing Q/DQ_BUF. > > > On such scenario, the debug string could provide mislead prints. > > > > Um, yes, indeed it would no longer match the process. But the request is > > still the same. That's actually a positive thing since it allows you to > > identify the request. > > > > With a global ID space this was trivial; you could just print the request > > ID and that was all that was ever needed. (I'm not proposing to consider > > that though.) > > > > IMO, a global ID number would work better than get_task_comm(). > > Just add a static int monotonic counter and use it for the debug purposes, > e. g.: > > { > static unsigned int req_count = 0; > > snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d", > req_count++, fd); > > Ok, eventually, it will overflow, but, it will be unique within > a reasonable timeframe to be good enough for debugging purposes. Yes, but you can't figure out which process allocated it anymore, making associating kernel debug logs with user space process logs harder. How about process id + file handle? That still doesn't tell which process operated on the request though, but I'm not sure whether that's really a crucial piece of information. -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [RFCv11 PATCH 04/29] media-request: core request support
Em Wed, 11 Apr 2018 18:02:19 +0300 Sakari Ailus escreveu: > On Wed, Apr 11, 2018 at 10:49:35AM -0300, Mauro Carvalho Chehab wrote: > > Em Wed, 11 Apr 2018 16:21:16 +0300 > > Sakari Ailus escreveu: > > > > > > > > > > Btw, this is a very good reason why you should define the ioctl to > > > > > > have an integer argument instead of a struct with a __s32 field > > > > > > on it, as per my comment to patch 02/29: > > > > > > > > > > > > #define MEDIA_IOC_REQUEST_ALLOC _IOWR('|', 0x05, int) > > > > > > > > > > > > At 64 bit architectures, you're truncating the file descriptor! > > > > > > > > > > > > > > > > I'm not quite sure what do you mean. int is 32 bits on 64-bit systems > > > > > as > > > > > well. > > > > > > > > Hmm.. you're right. I was thinking that it could be 64 bits on some > > > > archs like sparc64 (Tru64 C compiler declares it with 64 bits), but, > > > > according with: > > > > > > > > https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html > > > > > > > > This is not the case on gcc. > > > > > > Ok. The reasoning back then was that what "int" means varies across > > > compilers and languages. And the intent was to codify this to __s32 which > > > is what the kernel effectively uses. > > > > ... > > > > > The rest of the kernel uses int rather liberally in the uAPI so I'm not > > > sure in the end whether something desirable was achieved. Perhaps it'd be > > > good to go back to the original discussion to find out for sure. > > > > > > Still binaries compiled with Tru64 C compiler wouldn't work on Linux > > > anyway > > > due to that difference. > > > > > > Well, I stop here for this begins to be off-topic. :-) > > > > Yes. Let's keep it as s32 as originally proposed. Just ignore my comments > > about that :-) > > > > > > > > > + get_task_comm(comm, current); > > > > > > > + snprintf(req->debug_str, sizeof(req->debug_str), "%s:%d", > > > > > > > + comm, fd); > > > > > > > > > > > > Not sure if it is a good idea to store the task that allocated > > > > > > the request. While it makes sense for the dev_dbg() below, it > > > > > > may not make sense anymore on other dev_dbg() you would be > > > > > > using it. > > > > > > > > > > The lifetime of the file handle roughly matches that of the request. > > > > > It's > > > > > for debug only anyway. > > > > > > > > > > Better proposals are always welcome of course. But I think we should > > > > > have > > > > > something here that helps debugging by meaningfully making the > > > > > requests > > > > > identifiable from logs. > > > > > > > > What I meant to say is that one PID could be allocating the > > > > request, while some other one could be actually doing Q/DQ_BUF. > > > > On such scenario, the debug string could provide mislead prints. > > > > > > Um, yes, indeed it would no longer match the process. But the request is > > > still the same. That's actually a positive thing since it allows you to > > > identify the request. > > > > > > With a global ID space this was trivial; you could just print the request > > > ID and that was all that was ever needed. (I'm not proposing to consider > > > that though.) > > > > > > > IMO, a global ID number would work better than get_task_comm(). > > > > Just add a static int monotonic counter and use it for the debug purposes, > > e. g.: > > > > { > > static unsigned int req_count = 0; > > > > snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d", > > req_count++, fd); > > > > Ok, eventually, it will overflow, but, it will be unique within > > a reasonable timeframe to be good enough for debugging purposes. > > Yes, but you can't figure out which process allocated it anymore, making > associating kernel debug logs with user space process logs harder. > > How about process id + file handle? That still doesn't tell which process > operated on the request though, but I'm not sure whether that's really a > crucial piece of information. You don't need that. With dev_dbg() - and other *_dbg() macros - you can enable process ID for all debug messages. Basically, if the user needs the PID, all it needs is to use "+pt", e. g. something like: echo "file drivers/media/* +pt" > /sys/kernel/debug/dynamic_debug/control [1] see: https://www.kernel.org/doc/html/v4.11/admin-guide/dynamic-debug-howto.html Thanks, Mauro
Smatch and sparse errors
Hi Hans/Jasmin, There is a regression with sparse and upstream Kernels, with also affect smatch. Due to that, both will produce hundreds of new errors on all places that directly or indirectly use min() or max(). Those were caused by those upstream patches: 3c8ba0d61d04 ("kernel.h: Retain constant expression output for max()/min()") e9092d0d9796 ("Fix subtle macro variable shadowing in min_not_zero()") There is already an upstream patch for hidding it: https://patchwork.kernel.org/patch/10334353/ While upstream doesn't fix it (either by applying this patch or with some other fixup patch), it should be applied on both sparse and smatch trees, in order to get rid of those false positives. I suggest applying it to the logic with does daily compilations, in order to avoid generating useless reports for smatch/sparse. Thanks, Mauro
Re: [RFCv11 PATCH 04/29] media-request: core request support
On Wed, Apr 11, 2018 at 12:17:27PM -0300, Mauro Carvalho Chehab wrote: > Em Wed, 11 Apr 2018 18:02:19 +0300 > Sakari Ailus escreveu: > > > On Wed, Apr 11, 2018 at 10:49:35AM -0300, Mauro Carvalho Chehab wrote: > > > Em Wed, 11 Apr 2018 16:21:16 +0300 > > > Sakari Ailus escreveu: > > > > > > > > > > > > > Btw, this is a very good reason why you should define the ioctl to > > > > > > > have an integer argument instead of a struct with a __s32 field > > > > > > > on it, as per my comment to patch 02/29: > > > > > > > > > > > > > > #define MEDIA_IOC_REQUEST_ALLOC _IOWR('|', 0x05, int) > > > > > > > > > > > > > > At 64 bit architectures, you're truncating the file descriptor! > > > > > > > > > > > > > > > > > > > I'm not quite sure what do you mean. int is 32 bits on 64-bit > > > > > > systems as > > > > > > well. > > > > > > > > > > Hmm.. you're right. I was thinking that it could be 64 bits on some > > > > > archs like sparc64 (Tru64 C compiler declares it with 64 bits), but, > > > > > according with: > > > > > > > > > > https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html > > > > > > > > > > This is not the case on gcc. > > > > > > > > Ok. The reasoning back then was that what "int" means varies across > > > > compilers and languages. And the intent was to codify this to __s32 > > > > which > > > > is what the kernel effectively uses. > > > > > > ... > > > > > > > The rest of the kernel uses int rather liberally in the uAPI so I'm not > > > > sure in the end whether something desirable was achieved. Perhaps it'd > > > > be > > > > good to go back to the original discussion to find out for sure. > > > > > > > > Still binaries compiled with Tru64 C compiler wouldn't work on Linux > > > > anyway > > > > due to that difference. > > > > > > > > Well, I stop here for this begins to be off-topic. :-) > > > > > > Yes. Let's keep it as s32 as originally proposed. Just ignore my comments > > > about that :-) > > > > > > > > > > > + get_task_comm(comm, current); > > > > > > > > + snprintf(req->debug_str, sizeof(req->debug_str), > > > > > > > > "%s:%d", > > > > > > > > +comm, fd); > > > > > > > > > > > > > > Not sure if it is a good idea to store the task that allocated > > > > > > > the request. While it makes sense for the dev_dbg() below, it > > > > > > > may not make sense anymore on other dev_dbg() you would be > > > > > > > using it. > > > > > > > > > > > > The lifetime of the file handle roughly matches that of the > > > > > > request. It's > > > > > > for debug only anyway. > > > > > > > > > > > > Better proposals are always welcome of course. But I think we > > > > > > should have > > > > > > something here that helps debugging by meaningfully making the > > > > > > requests > > > > > > identifiable from logs. > > > > > > > > > > What I meant to say is that one PID could be allocating the > > > > > request, while some other one could be actually doing Q/DQ_BUF. > > > > > On such scenario, the debug string could provide mislead prints. > > > > > > > > Um, yes, indeed it would no longer match the process. But the request is > > > > still the same. That's actually a positive thing since it allows you to > > > > identify the request. > > > > > > > > With a global ID space this was trivial; you could just print the > > > > request > > > > ID and that was all that was ever needed. (I'm not proposing to consider > > > > that though.) > > > > > > > > > > IMO, a global ID number would work better than get_task_comm(). > > > > > > Just add a static int monotonic counter and use it for the debug purposes, > > > e. g.: > > > > > > { > > > static unsigned int req_count = 0; > > > > > > snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d", > > > req_count++, fd); > > > > > > Ok, eventually, it will overflow, but, it will be unique within > > > a reasonable timeframe to be good enough for debugging purposes. > > > > Yes, but you can't figure out which process allocated it anymore, making > > associating kernel debug logs with user space process logs harder. > > > > How about process id + file handle? That still doesn't tell which process > > operated on the request though, but I'm not sure whether that's really a > > crucial piece of information. > > You don't need that. With dev_dbg() - and other *_dbg() macros - you can > enable process ID for all debug messages. With this, the problem again is that it does not uniquely identify the request: the request is the same request independently of which process would operate on it. Or whether it is being processed in an interrupt context. AFAICT, the allocator PID (or process name) + file handle are both required to match a request between user and kernel space logs. > > Basically, if the user needs the PID, all it needs is to use "+pt", > e. g. something like: > > echo "file drivers/media/* +pt" > > /sys/kernel/debu
Re: [PATCH] media: ddbridge: better handle optional spin locks at the code
Am Wed, 11 Apr 2018 08:03:37 -0400 schrieb Mauro Carvalho Chehab : > Currently, ddbridge produces 4 warnings on sparse: > drivers/media/pci/ddbridge/ddbridge-core.c:495:9: warning: context > imbalance in 'ddb_output_start' - different lock contexts for basic block > drivers/media/pci/ddbridge/ddbridge-core.c:510:9: warning: context > imbalance in 'ddb_output_stop' - different lock contexts for basic block > drivers/media/pci/ddbridge/ddbridge-core.c:525:9: warning: context > imbalance in 'ddb_input_stop' - different lock contexts for basic block > drivers/media/pci/ddbridge/ddbridge-core.c:560:9: warning: context > imbalance in 'ddb_input_start' - different lock contexts for basic block > > Those are all false positives, but they result from the fact that > there could potentially have some troubles at the locking schema, > because the lock depends on a var (output->dma). > > I discussed that in priv with Sparse author and with the current > maintainer. Both believe that sparse is doing the right thing, and > that the proper fix would be to change the code to make it clearer > that, when spin_lock_irq() is called, spin_unlock_irq() will be > also called. > > That help not only static analyzers to better understand the code, > but also humans that could need to take a look at the code. > > It was also pointed that gcc would likely be smart enough to > optimize the code and produce the same result. I double > checked: indeed, the size of the driver didn't change after > this patch. > > Signed-off-by: Mauro Carvalho Chehab > --- > drivers/media/pci/ddbridge/ddbridge-core.c | 43 > -- > 1 file changed, 29 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c > b/drivers/media/pci/ddbridge/ddbridge-core.c > index 4a2819d3e225..080e2189ca7f 100644 > --- a/drivers/media/pci/ddbridge/ddbridge-core.c > +++ b/drivers/media/pci/ddbridge/ddbridge-core.c > @@ -458,13 +458,12 @@ static void calc_con(struct ddb_output *output, u32 > *con, u32 *con2, u32 flags) > *con2 = (nco << 16) | gap; > } > > -static void ddb_output_start(struct ddb_output *output) > +static void __ddb_output_start(struct ddb_output *output) > { > struct ddb *dev = output->port->dev; > u32 con = 0x11c, con2 = 0; > > if (output->dma) { > - spin_lock_irq(&output->dma->lock); > output->dma->cbuf = 0; > output->dma->coff = 0; > output->dma->stat = 0; > @@ -492,9 +491,18 @@ static void ddb_output_start(struct ddb_output *output) > > ddbwritel(dev, con | 1, TS_CONTROL(output)); > > - if (output->dma) { > + if (output->dma) > output->dma->running = 1; > +} > + > +static void ddb_output_start(struct ddb_output *output) > +{ > + if (output->dma) { > + spin_lock_irq(&output->dma->lock); > + __ddb_output_start(output); > spin_unlock_irq(&output->dma->lock); > + } else { > + __ddb_output_start(output); > } > } This makes things look rather strange (at least to my eyes), especially when simply trying to satisfy automated checkers, which in this case is useless since both lock and unlock will always happen based on the same condition ([input|output]->dma != NULL). Though I agree having the locking inside a condition in it's current form isn't optimal, too, and I also already thought about this in the past. I'd rather try to fix this by checking for the dma ptrs at the beginning of the four functions and immediately return if the ptr is invalid. Though I don't know if this may cause side effects as there's data written to the regs pointed by the TS_CONTROL() macros even if there's no dma ptr present. I'd like to hear Ralph's opinion on this, and also like to have this changed (in whatever way) in the upstream (dddvb) repository, too. Please refrain from applying this patch until we agreed on a proper solution that works for everyone. Best regards, Daniel Scheller -- https://github.com/herrnst
Re: [RFCv11 PATCH 04/29] media-request: core request support
Em Wed, 11 Apr 2018 18:35:14 +0300 Sakari Ailus escreveu: > On Wed, Apr 11, 2018 at 12:17:27PM -0300, Mauro Carvalho Chehab wrote: > > Em Wed, 11 Apr 2018 18:02:19 +0300 > > Sakari Ailus escreveu: > > > > > On Wed, Apr 11, 2018 at 10:49:35AM -0300, Mauro Carvalho Chehab wrote: > > > > Em Wed, 11 Apr 2018 16:21:16 +0300 > > > > Sakari Ailus escreveu: > > > > > > > > > > > > > > > > Btw, this is a very good reason why you should define the ioctl > > > > > > > > to > > > > > > > > have an integer argument instead of a struct with a __s32 field > > > > > > > > on it, as per my comment to patch 02/29: > > > > > > > > > > > > > > > > #define MEDIA_IOC_REQUEST_ALLOC _IOWR('|', 0x05, int) > > > > > > > > > > > > > > > > At 64 bit architectures, you're truncating the file descriptor! > > > > > > > > > > > > > > > > > > > > > > I'm not quite sure what do you mean. int is 32 bits on 64-bit > > > > > > > systems as > > > > > > > well. > > > > > > > > > > > > Hmm.. you're right. I was thinking that it could be 64 bits on some > > > > > > archs like sparc64 (Tru64 C compiler declares it with 64 bits), but, > > > > > > according with: > > > > > > > > > > > > https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html > > > > > > > > > > > > This is not the case on gcc. > > > > > > > > > > Ok. The reasoning back then was that what "int" means varies across > > > > > compilers and languages. And the intent was to codify this to __s32 > > > > > which > > > > > is what the kernel effectively uses. > > > > > > > > ... > > > > > > > > > The rest of the kernel uses int rather liberally in the uAPI so I'm > > > > > not > > > > > sure in the end whether something desirable was achieved. Perhaps > > > > > it'd be > > > > > good to go back to the original discussion to find out for sure. > > > > > > > > > > Still binaries compiled with Tru64 C compiler wouldn't work on Linux > > > > > anyway > > > > > due to that difference. > > > > > > > > > > Well, I stop here for this begins to be off-topic. :-) > > > > > > > > Yes. Let's keep it as s32 as originally proposed. Just ignore my > > > > comments > > > > about that :-) > > > > > > > > > > > > > + get_task_comm(comm, current); > > > > > > > > > + snprintf(req->debug_str, sizeof(req->debug_str), > > > > > > > > > "%s:%d", > > > > > > > > > + comm, fd); > > > > > > > > > > > > > > > > Not sure if it is a good idea to store the task that allocated > > > > > > > > the request. While it makes sense for the dev_dbg() below, it > > > > > > > > may not make sense anymore on other dev_dbg() you would be > > > > > > > > using it. > > > > > > > > > > > > > > The lifetime of the file handle roughly matches that of the > > > > > > > request. It's > > > > > > > for debug only anyway. > > > > > > > > > > > > > > Better proposals are always welcome of course. But I think we > > > > > > > should have > > > > > > > something here that helps debugging by meaningfully making the > > > > > > > requests > > > > > > > identifiable from logs. > > > > > > > > > > > > What I meant to say is that one PID could be allocating the > > > > > > request, while some other one could be actually doing Q/DQ_BUF. > > > > > > On such scenario, the debug string could provide mislead prints. > > > > > > > > > > > > > > > > Um, yes, indeed it would no longer match the process. But the request > > > > > is > > > > > still the same. That's actually a positive thing since it allows you > > > > > to > > > > > identify the request. > > > > > > > > > > With a global ID space this was trivial; you could just print the > > > > > request > > > > > ID and that was all that was ever needed. (I'm not proposing to > > > > > consider > > > > > that though.) > > > > > > > > > > > > > IMO, a global ID number would work better than get_task_comm(). > > > > > > > > Just add a static int monotonic counter and use it for the debug > > > > purposes, > > > > e. g.: > > > > > > > > { > > > > static unsigned int req_count = 0; > > > > > > > > snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d", > > > > req_count++, fd); > > > > > > > > Ok, eventually, it will overflow, but, it will be unique within > > > > a reasonable timeframe to be good enough for debugging purposes. > > > > > > Yes, but you can't figure out which process allocated it anymore, making > > > associating kernel debug logs with user space process logs harder. > > > > > > How about process id + file handle? That still doesn't tell which process > > > operated on the request though, but I'm not sure whether that's really a > > > crucial piece of information. > > > > You don't need that. With dev_dbg() - and other *_dbg() macros - you can > > enable process ID for all debug messages. > > With this, the problem again is that it does not uniquely identify the > request: the reques
Re: a 4.16 kernel with Debian 9.4 "stretch" causes a log explosion
Hi Guennadi, On 11/04/18 10:56, Guennadi Liakhovetski wrote: > Hi Laurent, > > Not sure whether that's a kernel or a user-space problem, but UVC related > anyway. I've got a UVC 1.5 (!) Logitech camera, that used to work fine > with earlier kernels. I now installed "media 4.16" and saw, that the > kernel log was filling with messages like > > uvcvideo: Failed to query (GET_MIN) UVC control 2 on unit 1: -32 (exp. 1). > > The expected /dev/video[01] nodes were not created correctly, and the > hard-drive was getting full very quickly. The latter was happening because > the the /var/log/uvcdynctrl-udev.log file was growing. A truncated sample > is attached. At its bottom you see messages > > [libwebcam] Warning: The driver behind device video0 has a slightly buggy > implementation > of the V4L2_CTRL_FLAG_NEXT_CTRL flag. It does not return the next higher > control ID if a control query fails. A workaround has been enabled. > > repeating, which continues even if the camera is unplugged. The kernel is > the head of the master branch of git://linuxtv.org/media_tree.git > > Just figured out this commit > > From: Edgar Thier > Date: Thu, 12 Oct 2017 03:54:17 -0400 > Subject: [PATCH] media: uvcvideo: Apply flags from device to actual properties > > as the culprit. Without it everything is back to normal. I've already investigated and fixed this: Please apply: https://patchwork.kernel.org/patch/10299735/ You stated that this is showing up on a v4.16 kernel ... but as far as I'm aware - this feature shouldn't make it in until v4.17. Are you using linux-next or a media/master or such ? Regards Kieran > Thanks > Guennadi >
Re: a 4.16 kernel with Debian 9.4 "stretch" causes a log explosion
Hi Guennadi, On 11/04/18 17:17, Kieran Bingham wrote: > Hi Guennadi, > > On 11/04/18 10:56, Guennadi Liakhovetski wrote: >> Hi Laurent, >> >> Not sure whether that's a kernel or a user-space problem, but UVC related >> anyway. I've got a UVC 1.5 (!) Logitech camera, that used to work fine >> with earlier kernels. I now installed "media 4.16" and saw, that the >> kernel log was filling with messages like >> >> uvcvideo: Failed to query (GET_MIN) UVC control 2 on unit 1: -32 (exp. 1). >> >> The expected /dev/video[01] nodes were not created correctly, and the >> hard-drive was getting full very quickly. The latter was happening because >> the the /var/log/uvcdynctrl-udev.log file was growing. A truncated sample >> is attached. At its bottom you see messages >> >> [libwebcam] Warning: The driver behind device video0 has a slightly buggy >> implementation >> of the V4L2_CTRL_FLAG_NEXT_CTRL flag. It does not return the next higher >> control ID if a control query fails. A workaround has been enabled. >> >> repeating, which continues even if the camera is unplugged. The kernel is >> the head of the master branch of git://linuxtv.org/media_tree.git >> >> Just figured out this commit >> >> From: Edgar Thier >> Date: Thu, 12 Oct 2017 03:54:17 -0400 >> Subject: [PATCH] media: uvcvideo: Apply flags from device to actual >> properties >> >> as the culprit. Without it everything is back to normal. > > I've already investigated and fixed this: > > Please apply: > https://patchwork.kernel.org/patch/10299735/ > > You stated that this is showing up on a v4.16 kernel ... but as far as I'm > aware > - this feature shouldn't make it in until v4.17. Are you using linux-next or a > media/master or such ? Aha - never mind - I just re-read your message. I expect the fix to make it into the media mainline when Mauro pulls from Laurent? So I'm sure it will find its way soon. -- Cheers Kieran > > Regards > > Kieran > >> Thanks >> Guennadi >>
[GIT FIXES for 4.17] omap3isp IOMMU error handling fix
Hi Mauro, Here's a fix for the omap3isp driver's IOMMU error handling in probe. Please pull. The following changes since commit a95845ba184b854106972f5d8f50354c2d272c06: media: v4l2-core: fix size of devnode_nums[] bitarray (2018-04-05 06:41:30 -0400) are available in the git repository at: ssh://linuxtv.org/git/sailus/media_tree.git fixes-4.17-1 for you to fetch changes up to d4e0f478573131d424b7116cdb63f91e65164963: media: omap3isp: fix unbalanced dma_iommu_mapping (2018-04-11 18:07:37 +0300) Suman Anna (1): media: omap3isp: fix unbalanced dma_iommu_mapping drivers/media/platform/omap3isp/isp.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v2] media: omap3isp: fix unbalanced dma_iommu_mapping
On Mon, Mar 26, 2018 at 11:40:58AM -0500, Suman Anna wrote: > Hi Mauro, > > On 03/21/2018 05:26 AM, Laurent Pinchart wrote: > > Hi Suman, > > > > Thank you for the patch. > > > > On Wednesday, 14 March 2018 17:41:36 EET Suman Anna wrote: > >> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware > >> ARM DMA backend. The current code creates a dma_iommu_mapping and > >> attaches this to the ISP device, but never detaches the mapping in > >> either the probe failure paths or the driver remove path resulting > >> in an unbalanced mapping refcount and a memory leak. Fix this properly. > >> > >> Reported-by: Pavel Machek > >> Signed-off-by: Suman Anna > >> Acked-by: Sakari Ailus > > > > Reviewed-by: Laurent Pinchart > > I don't see this patch in -next yet, can you pick up this patch at your > earliest? Here: https://patchwork.linuxtv.org/patch/48599/> -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH] media: ddbridge: better handle optional spin locks at the code
Em Wed, 11 Apr 2018 18:03:15 +0200 Daniel Scheller escreveu: > Am Wed, 11 Apr 2018 08:03:37 -0400 > schrieb Mauro Carvalho Chehab : > > > Currently, ddbridge produces 4 warnings on sparse: > > drivers/media/pci/ddbridge/ddbridge-core.c:495:9: warning: context > > imbalance in 'ddb_output_start' - different lock contexts for basic block > > drivers/media/pci/ddbridge/ddbridge-core.c:510:9: warning: context > > imbalance in 'ddb_output_stop' - different lock contexts for basic block > > drivers/media/pci/ddbridge/ddbridge-core.c:525:9: warning: context > > imbalance in 'ddb_input_stop' - different lock contexts for basic block > > drivers/media/pci/ddbridge/ddbridge-core.c:560:9: warning: context > > imbalance in 'ddb_input_start' - different lock contexts for basic block > > > > Those are all false positives, but they result from the fact that > > there could potentially have some troubles at the locking schema, > > because the lock depends on a var (output->dma). > > > > I discussed that in priv with Sparse author and with the current > > maintainer. Both believe that sparse is doing the right thing, and > > that the proper fix would be to change the code to make it clearer > > that, when spin_lock_irq() is called, spin_unlock_irq() will be > > also called. > > > > That help not only static analyzers to better understand the code, > > but also humans that could need to take a look at the code. > > > > It was also pointed that gcc would likely be smart enough to > > optimize the code and produce the same result. I double > > checked: indeed, the size of the driver didn't change after > > this patch. > > > > Signed-off-by: Mauro Carvalho Chehab > > --- > > drivers/media/pci/ddbridge/ddbridge-core.c | 43 > > -- > > 1 file changed, 29 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c > > b/drivers/media/pci/ddbridge/ddbridge-core.c > > index 4a2819d3e225..080e2189ca7f 100644 > > --- a/drivers/media/pci/ddbridge/ddbridge-core.c > > +++ b/drivers/media/pci/ddbridge/ddbridge-core.c > > @@ -458,13 +458,12 @@ static void calc_con(struct ddb_output *output, u32 > > *con, u32 *con2, u32 flags) > > *con2 = (nco << 16) | gap; > > } > > > > -static void ddb_output_start(struct ddb_output *output) > > +static void __ddb_output_start(struct ddb_output *output) > > { > > struct ddb *dev = output->port->dev; > > u32 con = 0x11c, con2 = 0; > > > > if (output->dma) { > > - spin_lock_irq(&output->dma->lock); > > output->dma->cbuf = 0; > > output->dma->coff = 0; > > output->dma->stat = 0; > > @@ -492,9 +491,18 @@ static void ddb_output_start(struct ddb_output *output) > > > > ddbwritel(dev, con | 1, TS_CONTROL(output)); > > > > - if (output->dma) { > > + if (output->dma) > > output->dma->running = 1; > > +} > > + > > +static void ddb_output_start(struct ddb_output *output) > > +{ > > + if (output->dma) { > > + spin_lock_irq(&output->dma->lock); > > + __ddb_output_start(output); > > spin_unlock_irq(&output->dma->lock); > > + } else { > > + __ddb_output_start(output); > > } > > } > > This makes things look rather strange (at least to my eyes), especially > when simply trying to satisfy automated checkers, which in this case is > useless since both lock and unlock will always happen based on the same > condition ([input|output]->dma != NULL). Though I agree having the > locking inside a condition in it's current form isn't optimal, too, and > I also already thought about this in the past. > > I'd rather try to fix this by checking for the dma ptrs at the > beginning of the four functions and immediately return if the ptr is > invalid. Though I don't know if this may cause side effects as there's > data written to the regs pointed by the TS_CONTROL() macros even if > there's no dma ptr present. > > I'd like to hear Ralph's opinion on this, and also like to have this > changed (in whatever way) in the upstream (dddvb) repository, too. > > Please refrain from applying this patch until we agreed on a proper > solution that works for everyone. Yeah, sure. Btw, does ddbridge driver works without DMA? On a quick look, it seems that it is enabled all the times. > > Best regards, > Daniel Scheller Thanks, Mauro
Re: a 4.16 kernel with Debian 9.4 "stretch" causes a log explosion
Hi Kieran, On Wed, 11 Apr 2018, Kieran Bingham wrote: > Hi Guennadi, > > On 11/04/18 17:17, Kieran Bingham wrote: > > Hi Guennadi, > > > > On 11/04/18 10:56, Guennadi Liakhovetski wrote: > >> Hi Laurent, > >> > >> Not sure whether that's a kernel or a user-space problem, but UVC related > >> anyway. I've got a UVC 1.5 (!) Logitech camera, that used to work fine > >> with earlier kernels. I now installed "media 4.16" and saw, that the > >> kernel log was filling with messages like > >> > >> uvcvideo: Failed to query (GET_MIN) UVC control 2 on unit 1: -32 (exp. 1). > >> > >> The expected /dev/video[01] nodes were not created correctly, and the > >> hard-drive was getting full very quickly. The latter was happening because > >> the the /var/log/uvcdynctrl-udev.log file was growing. A truncated sample > >> is attached. At its bottom you see messages > >> > >> [libwebcam] Warning: The driver behind device video0 has a slightly buggy > >> implementation > >> of the V4L2_CTRL_FLAG_NEXT_CTRL flag. It does not return the next higher > >> control ID if a control query fails. A workaround has been enabled. > >> > >> repeating, which continues even if the camera is unplugged. The kernel is > >> the head of the master branch of git://linuxtv.org/media_tree.git > >> > >> Just figured out this commit > >> > >> From: Edgar Thier > >> Date: Thu, 12 Oct 2017 03:54:17 -0400 > >> Subject: [PATCH] media: uvcvideo: Apply flags from device to actual > >> properties > >> > >> as the culprit. Without it everything is back to normal. > > > > I've already investigated and fixed this: > > > > Please apply: > > https://patchwork.kernel.org/patch/10299735/ Great, thanks! That seems to fix my problem. Regards Guennadi > > > > You stated that this is showing up on a v4.16 kernel ... but as far as I'm > > aware > > - this feature shouldn't make it in until v4.17. Are you using linux-next > > or a > > media/master or such ? > > Aha - never mind - I just re-read your message. > > I expect the fix to make it into the media mainline when Mauro pulls from > Laurent? So I'm sure it will find its way soon. > > -- > Cheers > > Kieran > > > > > > Regards > > > > Kieran > > > >> Thanks > >> Guennadi > >> >
Re: [PATCH] media: ddbridge: better handle optional spin locks at the code
Am Wed, 11 Apr 2018 13:33:02 -0300 schrieb Mauro Carvalho Chehab : > Em Wed, 11 Apr 2018 18:03:15 +0200 > Daniel Scheller escreveu: > > > Am Wed, 11 Apr 2018 08:03:37 -0400 > > schrieb Mauro Carvalho Chehab : > > > > > Currently, ddbridge produces 4 warnings on sparse: > > > drivers/media/pci/ddbridge/ddbridge-core.c:495:9: warning: context > > > imbalance in 'ddb_output_start' - different lock contexts for basic block > > > drivers/media/pci/ddbridge/ddbridge-core.c:510:9: warning: context > > > imbalance in 'ddb_output_stop' - different lock contexts for basic block > > > drivers/media/pci/ddbridge/ddbridge-core.c:525:9: warning: context > > > imbalance in 'ddb_input_stop' - different lock contexts for basic block > > > drivers/media/pci/ddbridge/ddbridge-core.c:560:9: warning: context > > > imbalance in 'ddb_input_start' - different lock contexts for basic block > > > > > > Those are all false positives, but they result from the fact that > > > there could potentially have some troubles at the locking schema, > > > because the lock depends on a var (output->dma). > > > > > > I discussed that in priv with Sparse author and with the current > > > maintainer. Both believe that sparse is doing the right thing, and > > > that the proper fix would be to change the code to make it clearer > > > that, when spin_lock_irq() is called, spin_unlock_irq() will be > > > also called. > > > > > > That help not only static analyzers to better understand the code, > > > but also humans that could need to take a look at the code. > > > > > > It was also pointed that gcc would likely be smart enough to > > > optimize the code and produce the same result. I double > > > checked: indeed, the size of the driver didn't change after > > > this patch. > > > > > > Signed-off-by: Mauro Carvalho Chehab > > > --- > > > drivers/media/pci/ddbridge/ddbridge-core.c | 43 > > > -- > > > 1 file changed, 29 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c > > > b/drivers/media/pci/ddbridge/ddbridge-core.c > > > index 4a2819d3e225..080e2189ca7f 100644 > > > --- a/drivers/media/pci/ddbridge/ddbridge-core.c > > > +++ b/drivers/media/pci/ddbridge/ddbridge-core.c > > > @@ -458,13 +458,12 @@ static void calc_con(struct ddb_output *output, u32 > > > *con, u32 *con2, u32 flags) > > > *con2 = (nco << 16) | gap; > > > } > > > > > > -static void ddb_output_start(struct ddb_output *output) > > > +static void __ddb_output_start(struct ddb_output *output) > > > { > > > struct ddb *dev = output->port->dev; > > > u32 con = 0x11c, con2 = 0; > > > > > > if (output->dma) { > > > - spin_lock_irq(&output->dma->lock); > > > output->dma->cbuf = 0; > > > output->dma->coff = 0; > > > output->dma->stat = 0; > > > @@ -492,9 +491,18 @@ static void ddb_output_start(struct ddb_output > > > *output) > > > > > > ddbwritel(dev, con | 1, TS_CONTROL(output)); > > > > > > - if (output->dma) { > > > + if (output->dma) > > > output->dma->running = 1; > > > +} > > > + > > > +static void ddb_output_start(struct ddb_output *output) > > > +{ > > > + if (output->dma) { > > > + spin_lock_irq(&output->dma->lock); > > > + __ddb_output_start(output); > > > spin_unlock_irq(&output->dma->lock); > > > + } else { > > > + __ddb_output_start(output); > > > } > > > } > > > > This makes things look rather strange (at least to my eyes), especially > > when simply trying to satisfy automated checkers, which in this case is > > useless since both lock and unlock will always happen based on the same > > condition ([input|output]->dma != NULL). Though I agree having the > > locking inside a condition in it's current form isn't optimal, too, and > > I also already thought about this in the past. > > > > I'd rather try to fix this by checking for the dma ptrs at the > > beginning of the four functions and immediately return if the ptr is > > invalid. Though I don't know if this may cause side effects as there's > > data written to the regs pointed by the TS_CONTROL() macros even if > > there's no dma ptr present. > > > > I'd like to hear Ralph's opinion on this, and also like to have this > > changed (in whatever way) in the upstream (dddvb) repository, too. > > > > Please refrain from applying this patch until we agreed on a proper > > solution that works for everyone. > > Yeah, sure. > > Btw, does ddbridge driver works without DMA? On a quick look, it > seems that it is enabled all the times. DMA (and only this way of transportation) is used for all TS stream input/output from/to demods and CI adapters (and the modulator cards in the upstream driver) when driven by any of the PCIe bridges. After another quick glance, [in|out]put->dma should really always be set. In the end, they are pointers to "struct ddb_dma"'s to the fixed members of struct ddb, which is allocated when th
Re: a 4.16 kernel with Debian 9.4 "stretch" causes a log explosion
Hi Guennadi, On 11/04/18 18:06, Guennadi Liakhovetski wrote: Just figured out this commit From: Edgar Thier Date: Thu, 12 Oct 2017 03:54:17 -0400 Subject: [PATCH] media: uvcvideo: Apply flags from device to actual properties as the culprit. Without it everything is back to normal. >>> >>> I've already investigated and fixed this: >>> >>> Please apply: >>> https://patchwork.kernel.org/patch/10299735/ > > Great, thanks! That seems to fix my problem. Fantastic. I'm glad it helped. - Can I call that a Tested-by: ? :-D Regards Kieran
Re: [PATCH] media: ddbridge: better handle optional spin locks at the code
Mauro Carvalho Chehab writes: > Em Wed, 11 Apr 2018 18:03:15 +0200 > Daniel Scheller escreveu: > > > Am Wed, 11 Apr 2018 08:03:37 -0400 > > schrieb Mauro Carvalho Chehab : > > > > > Currently, ddbridge produces 4 warnings on sparse: > > > drivers/media/pci/ddbridge/ddbridge-core.c:495:9: warning: context > > > imbalance in 'ddb_output_start' - different lock contexts for basic block > > > drivers/media/pci/ddbridge/ddbridge-core.c:510:9: warning: context > > > imbalance in 'ddb_output_stop' - different lock contexts for basic block > > > drivers/media/pci/ddbridge/ddbridge-core.c:525:9: warning: context > > > imbalance in 'ddb_input_stop' - different lock contexts for basic block > > > drivers/media/pci/ddbridge/ddbridge-core.c:560:9: warning: context > > > imbalance in 'ddb_input_start' - different lock contexts for basic block > > > > > > Those are all false positives, but they result from the fact that > > > there could potentially have some troubles at the locking schema, > > > because the lock depends on a var (output->dma). > > > Yeah, there were false positives on other parts of the driver where it was obvious to anybody that the bad case cannot happen ... > > > I discussed that in priv with Sparse author and with the current > > > maintainer. Both believe that sparse is doing the right thing, and > > > that the proper fix would be to change the code to make it clearer > > > that, when spin_lock_irq() is called, spin_unlock_irq() will be > > > also called. > > > > > > That help not only static analyzers to better understand the code, > > > but also humans that could need to take a look at the code. > > > > > > It was also pointed that gcc would likely be smart enough to > > > optimize the code and produce the same result. I double > > > checked: indeed, the size of the driver didn't change after > > > this patch. > > > > > > Signed-off-by: Mauro Carvalho Chehab > > > --- > > > drivers/media/pci/ddbridge/ddbridge-core.c | 43 > > > -- > > > 1 file changed, 29 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c > > > b/drivers/media/pci/ddbridge/ddbridge-core.c > > > index 4a2819d3e225..080e2189ca7f 100644 > > > --- a/drivers/media/pci/ddbridge/ddbridge-core.c > > > +++ b/drivers/media/pci/ddbridge/ddbridge-core.c > > > @@ -458,13 +458,12 @@ static void calc_con(struct ddb_output *output, > > > u32 *con, u32 *con2, u32 flags) > > > *con2 = (nco << 16) | gap; > > > } > > > > > > -static void ddb_output_start(struct ddb_output *output) > > > +static void __ddb_output_start(struct ddb_output *output) > > > { > > > struct ddb *dev = output->port->dev; > > > u32 con = 0x11c, con2 = 0; > > > > > > if (output->dma) { > > > -spin_lock_irq(&output->dma->lock); > > > output->dma->cbuf = 0; > > > output->dma->coff = 0; > > > output->dma->stat = 0; > > > @@ -492,9 +491,18 @@ static void ddb_output_start(struct ddb_output > > > *output) > > > > > > ddbwritel(dev, con | 1, TS_CONTROL(output)); > > > > > > -if (output->dma) { > > > +if (output->dma) > > > output->dma->running = 1; > > > +} > > > + > > > +static void ddb_output_start(struct ddb_output *output) > > > +{ > > > +if (output->dma) { > > > +spin_lock_irq(&output->dma->lock); > > > +__ddb_output_start(output); > > > spin_unlock_irq(&output->dma->lock); > > > +} else { > > > +__ddb_output_start(output); > > > } > > > } What does it say if you do: struct ddb_dma *dma = output->dma; if (dma) lock; ... if (dma) unlock; ? If that does not work, what if you make dma const? > > > > This makes things look rather strange (at least to my eyes), especially > > when simply trying to satisfy automated checkers, which in this case is > > useless since both lock and unlock will always happen based on the same > > condition ([input|output]->dma != NULL). Though I agree having the > > locking inside a condition in it's current form isn't optimal, too, and > > I also already thought about this in the past. > > > > I'd rather try to fix this by checking for the dma ptrs at the > > beginning of the four functions and immediately return if the ptr is > > invalid. Though I don't know if this may cause side effects as there's > > data written to the regs pointed by the TS_CONTROL() macros even if > > there's no dma ptr present. TS_CONTROL only controls the output itself and does not need to have DMA. > > > > I'd like to hear Ralph's opinion on this, and also like to have this > > changed (in whatever way) in the upstream (dddvb) repository, too. > > > > Please refrain from applying this patch until we agreed on a
Re: [PATCH] media: uvcvideo: Prevent setting unavailable flags
Hi Kieran, Thanks for the patch, it fixed a problem I was having with media master, working with a Logitech UVC 1.5 camera. On Wed, 21 Mar 2018, Kieran Bingham wrote: > The addition of an extra operation to use the GET_INFO command > overwrites all existing flags from the uvc_ctrls table. This includes > setting all controls as supporting GET_MIN, GET_MAX, GET_RES, and > GET_DEF regardless of whether they do or not. > > Move the initialisation of these control capabilities directly to the > uvc_ctrl_fill_xu_info() call where they were originally located in that > use case, and ensure that the new functionality in uvc_ctrl_get_flags() > will only set flags based on their reported capability from the GET_INFO > call. > > Fixes: 859086ae3636 ("media: uvcvideo: Apply flags from device to actual > properties") > > Signed-off-by: Kieran Bingham Tested-by: Guennadi Liakhovetski Thanks Guennadi > --- > drivers/media/usb/uvc/uvc_ctrl.c | 17 + > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c > b/drivers/media/usb/uvc/uvc_ctrl.c > index 1daf444371be..4042cbdb721b 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1607,14 +1607,12 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev, > ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, >info->selector, data, 1); > if (!ret) > - info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX > - | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF > - | (data[0] & UVC_CONTROL_CAP_GET ? > -UVC_CTRL_FLAG_GET_CUR : 0) > - | (data[0] & UVC_CONTROL_CAP_SET ? > -UVC_CTRL_FLAG_SET_CUR : 0) > - | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? > -UVC_CTRL_FLAG_AUTO_UPDATE : 0); > + info->flags |= (data[0] & UVC_CONTROL_CAP_GET ? > + UVC_CTRL_FLAG_GET_CUR : 0) > + | (data[0] & UVC_CONTROL_CAP_SET ? > + UVC_CTRL_FLAG_SET_CUR : 0) > + | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? > + UVC_CTRL_FLAG_AUTO_UPDATE : 0); > > kfree(data); > return ret; > @@ -1689,6 +1687,9 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev, > > info->size = le16_to_cpup((__le16 *)data); > > + info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX > + | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF; > + > ret = uvc_ctrl_get_flags(dev, ctrl, info); > if (ret < 0) { > uvc_trace(UVC_TRACE_CONTROL, > -- > 2.7.4 >
Re: a 4.16 kernel with Debian 9.4 "stretch" causes a log explosion
On Wed, 11 Apr 2018, Kieran Bingham wrote: > Hi Guennadi, > > On 11/04/18 18:06, Guennadi Liakhovetski wrote: > > > > > Just figured out this commit > > From: Edgar Thier > Date: Thu, 12 Oct 2017 03:54:17 -0400 > Subject: [PATCH] media: uvcvideo: Apply flags from device to actual > properties > > as the culprit. Without it everything is back to normal. > >>> > >>> I've already investigated and fixed this: > >>> > >>> Please apply: > >>> https://patchwork.kernel.org/patch/10299735/ > > > > Great, thanks! That seems to fix my problem. > > Fantastic. I'm glad it helped. > > - Can I call that a Tested-by: ? :-D Now you officially can - I replied to the patch. Thanks Guennadi > > Regards > > Kieran >
Emulated formats do not allow to use highest FPS of some cameras
Hello, I'm using a Logitech Brio webcam which is able to capture video at 60+ FPS when using MJPEG input format at e.g. 1280x720 pixels, but only up to 30 FPS when using other formats such as YUYV. When using the cam with OBS Studio, the MJPEG input format is not currently supported by OBS so that I tried to choose one of the emulated formats. However, when using e.g. emulated YU12 or YV12 input formats, I can only choose up to 30 FPS. I think that the reason for this is that the automatic ranking of available input formats by the function "v4lconvert_get_rank" in "libv4lconvert.c" uses a heuristic to rank formats which disregards characteristics of a format such as the maximum available FPS. Indeed, if I lower the rank (in this case lower == better) manually for MJPEG in said function, I can choose 60 or even 90 FPS in OBS when using one of the emulated formats. Do you think a change of the heuristic used in "v4lconvert_get_rank" would be in order here, or do you have any other advice? Best Regards Sebastian
[PATCH] media: cx88: enable IR transmitter on HVR-1300
The HVR 1300 has a Z8F0811 IR device, which can do both IR transmit and receive. The transmit part was not probed. Signed-off-by: Sean Young --- drivers/media/pci/cx88/cx88-input.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/media/pci/cx88/cx88-input.c b/drivers/media/pci/cx88/cx88-input.c index 6f4e6923a91a..16233e837fcc 100644 --- a/drivers/media/pci/cx88/cx88-input.c +++ b/drivers/media/pci/cx88/cx88-input.c @@ -632,8 +632,9 @@ void cx88_i2c_init_ir(struct cx88_core *core) memset(&core->init_data, 0, sizeof(core->init_data)); if (*addrp == 0x71) { - /* Hauppauge XVR */ - core->init_data.name = "cx88 Hauppauge XVR remote"; + /* Hauppauge Z8F0811 */ + strlcpy(info.type, "ir_z8f0811_haup", I2C_NAME_SIZE); + core->init_data.name = core->board.name; core->init_data.ir_codes = RC_MAP_HAUPPAUGE; core->init_data.type = RC_PROTO_BIT_RC5 | RC_PROTO_BIT_RC6_MCE | RC_PROTO_BIT_RC6_6A_32; -- 2.14.3
drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1087:1-3: WARNING: PTR_ERR_OR_ZERO can be used
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 8837c70d531a1788f975c366c254a5cb973a5291 commit: 648a9576932a26e1c6a157b4c9345204de975957 media: vcodec: fix error return value from mtk_jpeg_clk_init() date: 7 days ago coccinelle warnings: (new ones prefixed by >>) >> drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1087:1-3: WARNING: >> PTR_ERR_OR_ZERO can be used Please review and possibly fold the followup patch. --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
[PATCH] media: vcodec: fix ptr_ret.cocci warnings
From: Fengguang Wu drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1087:1-3: WARNING: PTR_ERR_OR_ZERO can be used Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR Generated by: scripts/coccinelle/api/ptr_ret.cocci Fixes: 648a9576932a ("media: vcodec: fix error return value from mtk_jpeg_clk_init()") CC: Ryder Lee Signed-off-by: Fengguang Wu --- mtk_jpeg_core.c |5 + 1 file changed, 1 insertion(+), 4 deletions(-) --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c @@ -1084,10 +1084,7 @@ static int mtk_jpeg_clk_init(struct mtk_ return PTR_ERR(jpeg->clk_jdec); jpeg->clk_jdec_smi = devm_clk_get(jpeg->dev, "jpgdec-smi"); - if (IS_ERR(jpeg->clk_jdec_smi)) - return PTR_ERR(jpeg->clk_jdec_smi); - - return 0; + return PTR_ERR_OR_ZERO(jpeg->clk_jdec_smi); } static int mtk_jpeg_probe(struct platform_device *pdev)
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: Thu Apr 12 05:00:18 CEST 2018 media-tree git hash:b284d4d5a6785f8cd07eda2646a95782373cd01e media_build git hash: 541653bb52fcf7c34b8b83a4c8cc66b09c68ac37 v4l-utils git hash: 47d43b130dc6e9e0edc900759fb37649208371e4 gcc version:i686-linux-gcc (GCC) 7.3.0 sparse version: v0.5.2-rc1 smatch version: v0.5.0-4419-g3b5bf5c9 host hardware: x86_64 host os:4.14.0-3-amd64 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-arm64: OK linux-git-i686: 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.36.4-x86_64: ERRORS linux-2.6.37.6-i686: ERRORS linux-2.6.37.6-x86_64: ERRORS linux-2.6.38.8-i686: ERRORS linux-2.6.38.8-x86_64: ERRORS linux-2.6.39.4-i686: ERRORS linux-2.6.39.4-x86_64: ERRORS linux-3.0.101-i686: ERRORS linux-3.0.101-x86_64: ERRORS linux-3.1.10-i686: ERRORS linux-3.1.10-x86_64: ERRORS linux-3.2.101-i686: ERRORS linux-3.2.101-x86_64: ERRORS linux-3.3.8-i686: ERRORS linux-3.3.8-x86_64: ERRORS linux-3.4.113-i686: ERRORS linux-3.4.113-x86_64: ERRORS linux-3.5.7-i686: ERRORS linux-3.5.7-x86_64: ERRORS linux-3.6.11-i686: ERRORS linux-3.6.11-x86_64: ERRORS linux-3.7.10-i686: ERRORS linux-3.7.10-x86_64: ERRORS linux-3.8.13-i686: ERRORS linux-3.8.13-x86_64: ERRORS linux-3.9.11-i686: ERRORS linux-3.9.11-x86_64: ERRORS linux-3.10.108-i686: ERRORS linux-3.10.108-x86_64: ERRORS linux-3.11.10-i686: ERRORS linux-3.11.10-x86_64: ERRORS linux-3.12.74-i686: ERRORS linux-3.12.74-x86_64: ERRORS linux-3.13.11-i686: ERRORS linux-3.13.11-x86_64: ERRORS linux-3.14.79-i686: ERRORS linux-3.14.79-x86_64: ERRORS linux-3.15.10-i686: ERRORS linux-3.15.10-x86_64: ERRORS linux-3.16.56-i686: ERRORS linux-3.16.56-x86_64: ERRORS linux-3.17.8-i686: ERRORS linux-3.17.8-x86_64: ERRORS linux-3.18.102-i686: ERRORS linux-3.18.102-x86_64: ERRORS linux-3.19.8-i686: ERRORS linux-3.19.8-x86_64: ERRORS linux-4.0.9-i686: ERRORS linux-4.0.9-x86_64: ERRORS linux-4.1.51-i686: ERRORS linux-4.1.51-x86_64: ERRORS linux-4.2.8-i686: ERRORS linux-4.2.8-x86_64: ERRORS linux-4.3.6-i686: ERRORS linux-4.3.6-x86_64: ERRORS linux-4.4.109-i686: ERRORS linux-4.4.109-x86_64: ERRORS linux-4.5.7-i686: ERRORS linux-4.5.7-x86_64: ERRORS linux-4.6.7-i686: ERRORS linux-4.6.7-x86_64: ERRORS linux-4.7.10-i686: ERRORS linux-4.7.10-x86_64: ERRORS linux-4.8.17-i686: ERRORS linux-4.8.17-x86_64: ERRORS linux-4.9.91-i686: ERRORS linux-4.9.91-x86_64: ERRORS linux-4.14.31-i686: WARNINGS linux-4.14.31-x86_64: WARNINGS linux-4.15.14-i686: WARNINGS linux-4.15.14-x86_64: WARNINGS linux-4.16-i686: WARNINGS linux-4.16-x86_64: WARNINGS apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html