Re: [PATCH v2 08/17] media: v4l2-ioctl.h: convert debug macros into enum and document
Hi Mauro, On Monday, 18 December 2017 17:13:26 EET Mauro Carvalho Chehab wrote: > Em Fri, 13 Oct 2017 15:38:11 +0300 Laurent Pinchart escreveu: > > On Thursday, 28 September 2017 00:46:51 EEST Mauro Carvalho Chehab wrote: > >> Currently, there's no way to document #define foo > >> with kernel-doc. So, convert it to an enum, and document. > > > > The documentation seems fine to me (except for one comment below). > > However, converting macros to an enum just to work around a defect of the > > documentation system doesn't seem like a good idea to me. I'd rather find > > a way to document macros. > > I agree that this limitation should be fixed. > > Yet, in this specific case where we have an "array" of defines, all > associated to the same field (even being a bitmask), and assuming that > we would add a way for kernel-doc to parse this kind of defines > (not sure how easy/doable would be), then, in order to respect the > way kernel-doc markup is, the documentation for those macros would be: > > > /** > * define: Just log the ioctl name + error code > */ > #define V4L2_DEV_DEBUG_IOCTL 0x01 > /** > * define: Log the ioctl name arguments + error code > */ > #define V4L2_DEV_DEBUG_IOCTL_ARG 0x02 > /** > * define: Log the file operations open, release, mmap and get_unmapped_area > */ > #define V4L2_DEV_DEBUG_FOP0x04 > /** > * define: Log the read and write file operations and the VIDIOC_(D)QBUF > ioctls */ > #define V4L2_DEV_DEBUG_STREAMING 0x08 > > IMHO, this is a way easier to read/understand by humans, and a way more > coincise: > > /** > * enum v4l2_debug_flags - Device debug flags to be used with the video > *device debug attribute > * > * @V4L2_DEV_DEBUG_IOCTL: Just log the ioctl name + error code. > * @V4L2_DEV_DEBUG_IOCTL_ARG: Log the ioctl name arguments + error code. > * @V4L2_DEV_DEBUG_FOP: Log the file operations and open, > release, > *mmap and get_unmapped_area syscalls. > * @V4L2_DEV_DEBUG_STREAMING: Log the read and write syscalls and > *:c:ref:`VIDIOC_[Q|DQ]BUFF ` ioctls. > */ > > It also underlines the aspect that those names are grouped altogether. > > So, IMHO, the main reason to place them inside an enum and document > as such is that it looks a way better for humans to read. As we're talking about extending kerneldoc to document macros, we're free to decide on a format that would make it easy and clear. Based on your above example, we could write it /** * define v4l2_debug_flags - Device debug flags to be used with the video * device debug attribute * * @V4L2_DEV_DEBUG_IOCTL: Just log the ioctl name + error code. * @V4L2_DEV_DEBUG_IOCTL_ARG: Log the ioctl name arguments + error code. * @V4L2_DEV_DEBUG_FOP: Log the file operations and open, release, * mmap and get_unmapped_area syscalls. * @V4L2_DEV_DEBUG_STREAMING: Log the read and write syscalls and * :c:ref:`VIDIOC_[Q|DQ]BUFF ` ioctls. */ That would be simple, clear, and wouldn't require a code change to workaround a limitation of the documentation system. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 15/17] media: v4l2-ctrls: document nested members of structs
Hi Mauro,
Thank you for the patch.
On Thursday, 28 September 2017 00:46:58 EEST Mauro Carvalho Chehab wrote:
> There are a few nested members at v4l2-ctrls.h. Now that
> kernel-doc supports, document them.
>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---
> include/media/v4l2-ctrls.h | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index dacfe54057f8..ca05f0f49bc5 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -147,7 +147,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl
> *ctrl, void *priv); * @type_ops: The control type ops.
> * @id: The control ID.
> * @name:The control name.
> - * @type:The control type.
> + * @type:The control type, as defined by v4l2_ctrl_type.
Why do you need this ? The field is an enum v4l2_ctrl_type, Sphinx should
generate the proper link already.
> * @minimum: The control's minimum value.
> * @maximum: The control's maximum value.
> * @default_value: The control's default value.
> @@ -166,8 +166,15 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl
> *ctrl, void *priv);
> * empty strings ("") correspond to non-existing menu items (this
> * is in addition to the menu_skip_mask above). The last entry
> * must be NULL.
> + * Used only if the @type is %V4L2_CTRL_TYPE_MENU.
> + * @qmenu_int: A 64-bit integer array for with integer menu items.
> + * The size of array must be equal to the menu size, e. g.:
> + * :math:`ceil(\frac{maximum - minimum}{step}) + 1`.
> + * Used only if the @type is %V4L2_CTRL_TYPE_INTEGER_MENU.
> * @flags: The control's flags.
> - * @cur: The control's current value.
> + * @cur: Struct to store data about the current value.
s/Struct/Structure/
s/data about the current value/the current value/
> + * @cur.val: The control's current value, if the @type is represented via
> + * a u32 integer (see v4l2_ctrl_type).
> * @val: The control's new s32 value.
> * @priv:The control's private pointer. For use by the driver. It is
> * untouched by the control framework. Note that this pointer is
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 14/17] media: v4l2-async: better describe match union at async match struct
Hi Mauro,
Thank you for the patch.
On Thursday, 28 September 2017 00:46:57 EEST Mauro Carvalho Chehab wrote:
> Now that kernel-doc handles nested unions, better document the
> match union at struct v4l2_async_subdev.
>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---
> include/media/v4l2-async.h | 35 ---
> 1 file changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index e66a3521596f..62c2d572ec23 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -46,10 +46,39 @@ enum v4l2_async_match_type {
> /**
> * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge
> *
> - * @match_type: type of match that will be used
> - * @match: union of per-bus type matching data sets
> + * @match_type:
> + * type of match that will be used
> + * @match:
> + * union of per-bus type matching data sets
The lines don't exceed the 80 columnes limit, you can keep them as-is.
> + * @match.fwnode:
> + * pointer to fwnode_handle to be matched.
> + * Used if @match_type is %V4L2_ASYNC_MATCH_FWNODE.
> + * @match.device_name:
> + * string containing the device name to be matched.
> + * Used if @match_type is %V4L2_ASYNC_MATCH_DEVNAME.
> + * @match.i2c:
> + * embedded struct with I2C parameters to be matched.
> + * Both @match.i2c.adapter_id and @match.i2c.address
> + * should be matched.
> + * Used if @match_type is %V4L2_ASYNC_MATCH_I2C.
Do you really need to document this ? Isn't it enough to document
@match.i2c.adapter_id and @match.i2c.address ?
> + * @match.i2c.adapter_id:
> + * I2C adapter ID to be matched.
> + * Used if @match_type is %V4L2_ASYNC_MATCH_I2C.
> + * @match.i2c.address:
> + * I2C address to be matched.
> + * Used if @match_type is %V4L2_ASYNC_MATCH_I2C.
> + * @match.custom:
> + * Driver-specific match criteria.
> + * Used if @match_type is %V4L2_ASYNC_MATCH_CUSTOM.
Same here.
> + * @match.custom.match:
> + * Driver-specific match function to be used if
> + * %V4L2_ASYNC_MATCH_CUSTOM.
> + * @match.custom.priv:
> + * Driver-specific private struct with match parameters
> + * to be used if %V4L2_ASYNC_MATCH_CUSTOM.
> * @list:used to link struct v4l2_async_subdev objects, waiting to be
> - * probed, to a notifier->waiting list
> + * probed, to a notifier->waiting list.
> + * Not to be used by drivers.
> */
> struct v4l2_async_subdev {
> enum v4l2_async_match_type match_type;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 12/17] media: v4l2-fwnode.h: better describe bus union at fwnode endpoint struct
Hi Mauro,
Thank you for the patch.
On Thursday, 28 September 2017 00:46:55 EEST Mauro Carvalho Chehab wrote:
> Now that kernel-doc handles nested unions, better document the
> bus union at struct v4l2_fwnode_endpoint.
>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---
> include/media/v4l2-fwnode.h | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 7adec9851d9e..5f4716f967d0 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -79,7 +79,18 @@ struct v4l2_fwnode_bus_mipi_csi1 {
> * struct v4l2_fwnode_endpoint - the endpoint data structure
> * @base: fwnode endpoint of the v4l2_fwnode
> * @bus_type: bus type
> - * @bus: bus configuration data structure
> + * @bus: union with bus configuration data structure
> + * @bus.parallel: pointer for v4l2_fwnode_bus_parallel.
> + * Used if the bus is parallel.
> + * @bus.mipi_csi1: pointer for v4l2_fwnode_bus_mipi_csi1.
> + * Used if the bus is Mobile Industry Processor
> + * Interface's Camera Serial Interface version 1
> + * (MIPI CSI1) or Standard Mobile Imaging Architecture's
> + * Compact Camera Port 2 (SMIA CCP2).
> + * @bus.mipi_csi2: pointer for v4l2_fwnode_bus_mipi_csi2.
> + * Used if the bus is Mobile Industry Processor
> + * Interface's Camera Serial Interface version 2
> + * (MIPI CSI2).
These are not pointers.
> * @link_frequencies: array of supported link frequencies
> * @nr_of_link_frequencies: number of elements in link_frequenccies array
> */
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/17] media: v4l2-ioctl.h: convert debug macros into enum and document
Hi Mauro,
Thank you for the patch.
On Thursday, 28 September 2017 00:46:51 EEST Mauro Carvalho Chehab wrote:
> Currently, there's no way to document #define foo
> with kernel-doc. So, convert it to an enum, and document.
The documentation seems fine to me (except for one comment below). However,
converting macros to an enum just to work around a defect of the documentation
system doesn't seem like a good idea to me. I'd rather find a way to document
macros.
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---
> include/media/v4l2-ioctl.h | 33 +++--
> 1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index bd5312118013..136e2cffcf9e 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -588,20 +588,25 @@ struct v4l2_ioctl_ops {
> };
>
>
> -/* v4l debugging and diagnostics */
> -
> -/* Device debug flags to be used with the video device debug attribute */
> -
> -/* Just log the ioctl name + error code */
> -#define V4L2_DEV_DEBUG_IOCTL 0x01
> -/* Log the ioctl name arguments + error code */
> -#define V4L2_DEV_DEBUG_IOCTL_ARG 0x02
> -/* Log the file operations open, release, mmap and get_unmapped_area */
> -#define V4L2_DEV_DEBUG_FOP 0x04
> -/* Log the read and write file operations and the VIDIOC_(D)QBUF ioctls */
> -#define V4L2_DEV_DEBUG_STREAMING 0x08
> -/* Log poll() */
> -#define V4L2_DEV_DEBUG_POLL 0x10
> +/**
> + * enum v4l2_debug_flags - Device debug flags to be used with the video
> + * device debug attribute
> + *
> + * @V4L2_DEV_DEBUG_IOCTL:Just log the ioctl name + error code.
> + * @V4L2_DEV_DEBUG_IOCTL_ARG:Log the ioctl name arguments + error
> code.
> + * @V4L2_DEV_DEBUG_FOP: Log the file operations and open,
> release,
> + * mmap and get_unmapped_area syscalls.
> + * @V4L2_DEV_DEBUG_STREAMING:Log the read and write syscalls and
> + * :c:ref:`VIDIOC_[Q|DQ]BUFF ` ioctls.
s/BUFF/BUF.
A regexp would use VIDIOC_(Q|DQ)BUF. You can also write VIDIOC_{QBUF,DQBUF}
which seems clearer to me.
> + * @V4L2_DEV_DEBUG_POLL: Log poll syscalls.
> + */
> +enum v4l2_debug_flags {
> + V4L2_DEV_DEBUG_IOCTL= 0x01,
> + V4L2_DEV_DEBUG_IOCTL_ARG= 0x02,
> + V4L2_DEV_DEBUG_FOP = 0x04,
> + V4L2_DEV_DEBUG_STREAMING= 0x08,
> + V4L2_DEV_DEBUG_POLL = 0x10,
> +};
>
> /* Video standard functions */
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 06/17] media: v4l2-dv-timings.h: convert comment into kernel-doc markup
Hi Mauro,
Thank you for the patch.
On Thursday, 28 September 2017 00:46:49 EEST Mauro Carvalho Chehab wrote:
> The can_reduce_fps() is already documented, but it is not
> using the kernel-doc markup. Convert it, in order to generate
> documentation from it.
>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---
> include/media/v4l2-dv-timings.h | 14 --
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/include/media/v4l2-dv-timings.h
> b/include/media/v4l2-dv-timings.h index 61a18893e004..c0855887ad87 100644
> --- a/include/media/v4l2-dv-timings.h
> +++ b/include/media/v4l2-dv-timings.h
> @@ -203,13 +203,15 @@ struct v4l2_fract v4l2_calc_aspect_ratio(u8
> hor_landscape, u8 vert_portrait); */
> struct v4l2_fract v4l2_dv_timings_aspect_ratio(const struct v4l2_dv_timings
> *t);
>
> -/*
> - * reduce_fps - check if conditions for reduced fps are true.
> - * bt - v4l2 timing structure
> +/**
> + * can_reduce_fps - check if conditions for reduced fps are true.
> + * @bt: v4l2 timing structure
> + *
> * For different timings reduced fps is allowed if following conditions
While at it, s/following conditions/the following conditions/
> - * are met -
> - * For CVT timings: if reduced blanking v2 (vsync == 8) is true.
> - * For CEA861 timings: if V4L2_DV_FL_CAN_REDUCE_FPS flag is true.
> + * are met:
> + *
> + * - For CVT timings: if reduced blanking v2 (vsync == 8) is true.
> + * - For CEA861 timings: if %V4L2_DV_FL_CAN_REDUCE_FPS flag is true.
> */
> static inline bool can_reduce_fps(struct v4l2_bt_timings *bt)
> {
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 05/17] media: v4l2-device.h: document ancillary macros
d against v4l2_subdev->grp_id
> + * group ID to be matched. Use 0 to match them all.
> + * @o: name of the element at v4l2_subdev_ops that contains @f.
> + * Each element there groups a set of callbacks functions.
> + * @f: callback function that will be called if @cond matches.
> + * The callback functions are defined in groups, according to
> + * each element at v4l2_subdev_ops.
> + * @args...: arguments for @f.
> + *
> + * Return:
> + *
> + * If the callback returns an error other than 0 or ``-ENOIOCTLCMD``
> + * for any subdevice, then abort and return with that error code.
> + *
> + * Note: subdevs cannot be added or deleted while walking at
> + * the subdevs list.
> */
> #define v4l2_device_mask_call_until_err(v4l2_dev, grpmsk, o, f, args...) \
> ({ \
> @@ -312,9 +463,19 @@ static inline void v4l2_subdev_notify(struct
> v4l2_subdev *sd, ##args); \
> })
>
> -/*
> - * Does any subdev with matching grpid (or all if grpid == 0) has the given
> - * op?
> +
> +/**
> + * v4l2_device_has_op - checks if any subdev with matching grpid has a
> + * given ops.
> + *
> + * @v4l2_dev: pointer to v4l2_device
> + * @grpid: v4l2_subdev->grp_id group ID to match.
> + * Use 0 to match them all.
> + * @o: name of the element at v4l2_subdev_ops that contains @f.
> + * Each element there groups a set of callbacks functions.
> + * @f: callback function that will be called if @cond matches.
> + * The callback functions are defined in groups, according to
> + * each element at v4l2_subdev_ops.
> */
> #define v4l2_device_has_op(v4l2_dev, grpid, o, f) \
> ({ \
> @@ -331,9 +492,18 @@ static inline void v4l2_subdev_notify(struct
> v4l2_subdev *sd, __result;
> \
> })
>
> -/*
> - * Does any subdev with matching grpmsk (or all if grpmsk == 0) has the
> given
> - * op?
> +/**
> + * v4l2_device_mask_has_op - checks if any subdev with matching group
> + * mask has a given ops.
> + *
> + * @v4l2_dev: pointer to v4l2_device
> + * @grpmsk: bitmask to be checked against v4l2_subdev->grp_id
> + * group ID to be matched. Use 0 to match them all.
> + * @o: name of the element at v4l2_subdev_ops that contains @f.
> + * Each element there groups a set of callbacks functions.
> + * @f: callback function that will be called if @cond matches.
> + * The callback functions are defined in groups, according to
> + * each element at v4l2_subdev_ops.
> */
> #define v4l2_device_mask_has_op(v4l2_dev, grpmsk, o, f)
> \
> ({ \
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/17] media: v4l2-common.h: document ancillary functions
emaining ioctls/structs should be removed as well, but
> they
> * are still used in tuner-simple.c (TUNER_SET_CONFIG) and cx18/ivtv
> (RESET).
> * To remove these ioctls some more cleanup is needed in those modules.
> + *
> + * It doesn't make much sense on documenting them, as what we really want
> is
> + * to get rid of them.
> */
>
> /* s_config */
> @@ -243,17 +272,72 @@ struct v4l2_priv_tun_config {
>
> /* Miscellaneous helper functions */
>
> -void v4l_bound_align_image(unsigned int *w, unsigned int wmin,
> +/**
> + * v4l_bound_align_image - adjust video dimensions according to
> + * a given criteria.
s/a given criteria/the given constraints/
> + *
> + * @width: pointer to width that will be adjusted if needed.
> + * @wmin:minimum width.
> + * @wmax:maximum width.
> + * @walign: least significant bit on width.
> + * @height: pointer to height that will be adjusted if needed.
> + * @hmin:minimum height.
> + * @hmax:maximum height.
> + * @halign: least significant bit on width.
> + * @salign: least significant bit for the image size (e. g.
> + * :math:`width * height`).
> + *
> + * Bound an image to have @width between @wmin and @wmax, and @height
> between
> + * @hmin and @hmax, inclusive.
s/Bound/Bind/ ? I think "Clip" would be better actually.
> + *
> + * Additionally, the @width will be a multiple of :math:`2^{walign}`,
> + * the @height will be a multiple of :math:`2^{halign}`, and the overall
> + * size :math:`width * height` will be a multiple of :math:`2^{salign}`.
> + *
> + * .. note::
> + *
> + *#. The image may be shrunk or enlarged to fit the alignment
> constraints.
It's not the image that will be shrunk or enlarged, but the clipping
rectangle.
> + *#. @wmax must not be smaller than @wmin.
> + *#. @hmax must not be smaller than @hmin.
> + *#. The alignments must not be so high there are no possible image
> + * sizes within the allowed bounds.
> + *#. @wmin and @hmin must be at least 1 (don't use 0).
> + *#. For @walign, @halign and @salign, if you don't care about a
> certain
> + * alignment, specify ``0``, as :math:`2^0 = 1` and one byte
> alignment
> + * is equivalent to no alignment.
> + *#. If you only want to adjust downward, specify a maximum that's the
> + * same as the initial value.
> + */
> +void v4l_bound_align_image(unsigned int *width, unsigned int wmin,
> unsigned int wmax, unsigned int walign,
> -unsigned int *h, unsigned int hmin,
> +unsigned int *height, unsigned int hmin,
> unsigned int hmax, unsigned int halign,
> unsigned int salign);
>
> +/**
> + * v4l2_find_nearest_format - find the nearest format size among a discrete
> + * set of resolutions.
> + *
> + * @sizes: array with a pointer to & struct v4l2_frmsize_discrete image
> sizes.
> + * @num_sizes: size of @sizes array.
I'd say "length" instead of "size", or "number of elements in the @sizes
array". Size could be interpreted as the array size in bytes.
> + * @width: desired width.
> + * @height: desired heigth.
> + *
> + * Finds the closest resolution to minimize the width and height
> differences
> + * between what userspace requested and the supported resolutions.
This function isn't restricted to be used to handle userspace sizes, I
wouldn't mention userspace here.
> + */
> const struct v4l2_frmsize_discrete
> *v4l2_find_nearest_format(const struct v4l2_frmsize_discrete *sizes,
> const size_t num_sizes,
> s32 width, s32 height);
>
> +/**
> + * v4l2_get_timestamp - ancillary routine to get a timestamp to be used
> when
> + * filling streaming metadata. Internally, it uses ktime_get_ts(), +
> * with is the recommended way to get it.
s/with/which/
> + *
> + * @tv: pointer to timeval to be filled.
> + */
> void v4l2_get_timestamp(struct timeval *tv);
>
> #endif /* V4L2_COMMON_H_ */
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 02/17] media: v4l2-common: get rid of v4l2_routing dead struct
Hi Mauro,
Thank you for the patch.
On Thursday, 28 September 2017 00:46:45 EEST Mauro Carvalho Chehab wrote:
> This struct is not used anymore. Get rid of it and update
> the documentation about what should still be converted.
>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
Acked-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> ---
> include/media/v4l2-common.h | 14 +-
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index aac8b7b6e691..7dbecbe3009c 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -224,10 +224,11 @@ void v4l2_spi_subdev_init(struct v4l2_subdev *sd,
> struct spi_device *spi,
>
> /*
> -
> */
>
> -/* Note: these remaining ioctls/structs should be removed as well, but they
> are - still used in tuner-simple.c (TUNER_SET_CONFIG), cx18/ivtv (RESET)
> and - v4l2-int-device.h (v4l2_routing). To remove these ioctls some more
> cleanup - is needed in those modules. */
> +/*
> + * FIXME: these remaining ioctls/structs should be removed as well, but
> they + * are still used in tuner-simple.c (TUNER_SET_CONFIG) and cx18/ivtv
> (RESET). + * To remove these ioctls some more cleanup is needed in those
> modules. + */
>
> /* s_config */
> struct v4l2_priv_tun_config {
> @@ -238,11 +239,6 @@ struct v4l2_priv_tun_config {
>
> #define VIDIOC_INT_RESET _IOW ('d', 102, u32)
>
> -struct v4l2_routing {
> - u32 input;
> - u32 output;
> -};
> -
> /*
> -----
> */
>
> /* Miscellaneous helper functions */
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/17] media: v4l2-common: get rid of struct v4l2_discrete_probe
Hi Mauro,
Thank you for the patch.
On Thursday, 28 September 2017 00:46:46 EEST Mauro Carvalho Chehab wrote:
> This struct is there just two store two arguments of
> v4l2_find_nearest_format(). The other two arguments are passed
> as parameter.
>
> IMHO, there isn't much sense on doing that, and that will just
> add one more struct to document ;)
>
> So, let's get rid of the struct, passing the parameters directly.
>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---
> drivers/media/platform/vivid/vivid-vid-cap.c | 9 +++--
> drivers/media/v4l2-core/v4l2-common.c| 13 +++--
> include/media/v4l2-common.h | 12
> 3 files changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/platform/vivid/vivid-vid-cap.c
> b/drivers/media/platform/vivid/vivid-vid-cap.c index
> 01419455e545..0fbbcde19f0d 100644
> --- a/drivers/media/platform/vivid/vivid-vid-cap.c
> +++ b/drivers/media/platform/vivid/vivid-vid-cap.c
> @@ -93,11 +93,6 @@ static const struct v4l2_fract
> webcam_intervals[VIVID_WEBCAM_IVALS] = { { 1, 60 },
> };
>
> -static const struct v4l2_discrete_probe webcam_probe = {
> - webcam_sizes,
> - VIVID_WEBCAM_SIZES
> -};
> -
> static int vid_cap_queue_setup(struct vb2_queue *vq,
> unsigned *nbuffers, unsigned *nplanes,
> unsigned sizes[], struct device *alloc_devs[])
> @@ -578,7 +573,9 @@ int vivid_try_fmt_vid_cap(struct file *file, void *priv,
> mp->field = vivid_field_cap(dev, mp->field);
> if (vivid_is_webcam(dev)) {
> const struct v4l2_frmsize_discrete *sz =
> - v4l2_find_nearest_format(_probe, mp->width,
> mp->height);
> + v4l2_find_nearest_format(webcam_sizes,
> + VIVID_WEBCAM_SIZES,
> + mp->width, mp->height);
>
> w = sz->width;
> h = sz->height;
> diff --git a/drivers/media/v4l2-core/v4l2-common.c
> b/drivers/media/v4l2-core/v4l2-common.c index a5ea1f517291..fb9a2a3c1072
> 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -371,18 +371,19 @@ void v4l_bound_align_image(u32 *w, unsigned int wmin,
> unsigned int wmax, }
> EXPORT_SYMBOL_GPL(v4l_bound_align_image);
>
> -const struct v4l2_frmsize_discrete *v4l2_find_nearest_format(
> - const struct v4l2_discrete_probe *probe,
> - s32 width, s32 height)
> +const struct v4l2_frmsize_discrete
> +*v4l2_find_nearest_format(const struct v4l2_frmsize_discrete *sizes,
> + const size_t num_sizes,
> + s32 width, s32 height)
> {
> int i;
> u32 error, min_error = UINT_MAX;
> const struct v4l2_frmsize_discrete *size, *best = NULL;
>
> - if (!probe)
> - return best;
> + if (!sizes)
> + return NULL;
>
> - for (i = 0, size = probe->sizes; i < probe->num_sizes; i++, size++) {
> + for (i = 0, size = sizes; i < num_sizes; i++, size++) {
> error = abs(size->width - width) + abs(size->height - height);
> if (error < min_error) {
> min_error = error;
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 7dbecbe3009c..7ae7840df068 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -249,14 +249,10 @@ void v4l_bound_align_image(unsigned int *w, unsigned
> int wmin, unsigned int hmax, unsigned int halign,
> unsigned int salign);
>
> -struct v4l2_discrete_probe {
> - const struct v4l2_frmsize_discrete *sizes;
> - int num_sizes;
> -};
> -
> -const struct v4l2_frmsize_discrete *v4l2_find_nearest_format(
> - const struct v4l2_discrete_probe *probe,
> - s32 width, s32 height);
> +const struct v4l2_frmsize_discrete
> +*v4l2_find_nearest_format(const struct v4l2_frmsize_discrete *sizes,
> + const size_t num_sizes,
No need for a const keyword.
> + s32 width, s32 height);
>
> void v4l2_get_timestamp(struct timeval *tv);
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] media: open.rst: document devnode-centric and mc-centric types
Hi Mauro, On Friday, 25 August 2017 16:38:23 EEST Mauro Carvalho Chehab wrote: > Em Fri, 25 Aug 2017 16:11:15 +0300 Laurent Pinchart escreveu: > > On Friday, 25 August 2017 12:40:05 EEST Mauro Carvalho Chehab wrote: > >> From: "mche...@s-opensource.com" <mche...@s-opensource.com> > >> > >> When we added support for omap3, back in 2010, we added a new > >> type of V4L2 devices that aren't fully controlled via the V4L2 > >> device node. Yet, we never made it clear, at the V4L2 spec, > >> about the differences between both types. > > > > Nitpicking (and there will be lots of nitpicking in this review as I think > > it's very important to get this piece of the documentation right down to > > details): > > > Sure. > > > s/at the V4L2 spec/in the V4L2 spec/ > > > > "make it clear about" doesn't sound good to me. How about the following ? > > > > "Yet, we have never clearly documented in the V4L2 specification the > > differences between the two types." > > > >> Let's document them with the current implementation. > > > > s/with the/based on the/ > > OK. > > >> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > >> --- > >> > >> Documentation/media/uapi/v4l/open.rst | 50 + > >> 1 file changed, 50 insertions(+) > >> > >> diff --git a/Documentation/media/uapi/v4l/open.rst > >> b/Documentation/media/uapi/v4l/open.rst index afd116edb40d..a72d142897c0 > >> 100644 > >> --- a/Documentation/media/uapi/v4l/open.rst > >> +++ b/Documentation/media/uapi/v4l/open.rst > >> @@ -6,6 +6,56 @@ > >> Opening and Closing Devices > >> *** > >> > >> +Types of V4L2 hardware control > >> +== > >> + > >> +V4L2 devices are usually complex: they are implemented via a main > >> driver and > >> +often several additional drivers. The main driver always exposes one or > >> +more **V4L2 device** devnodes (see :ref:`v4l2_device_naming`). > > > > First of all, as stated in a previous e-mail, I think we should start by > > defining the terms we are going to use. > > Well, we can add a Glossary at the spec, but this is a huge work, > as we'll need to seek for other terms and to adjust all references > to use the Glossary wording. This is a separate work than what this > patch proposes to solve. > > Yet, I agree that ambiguity should be solved. I believe that the > second version of this patch series address it. As discussed over IRC we don't need a full glossary. We can start with definitions of the terms we use here, and enrich it incrementally in the future. > > The word "device" is ambiguous, it can mean > > > > - device node > > > > The /dev device node through which a character- or block-based API is > > exposed through userspace. > > > > The term "device node" is commonly used in the kernel, I think we can use > > it as-is without ambiguity. > > Yes. IMO, we should use: > > - "device node" when it may refer to any device node, > including mc and subdev; > - "V4L2 device node" when it refers only to the devices that are now > described at the "V4L2 Device Node Naming" section (see patch 1 of the > new patch series). And "V4L2 subdev device node" and "MC device node" when we want to refer specifically to the /dev/v4l-subdev* and /dev/media* device nodes ? > > - kernel struct device > > > > An instance of the kernel struct device. Kernel devices are used to > > represent hardware devices (and are then embedded in bus-specific device > > structure such as platform_device, pci_device or i2c_client) and logical > > devices (and are then embedded in class-spcific device structures such as > > struct video_device or struct net_device). > > > > For now I believe this isn't relevant to the V4L2 userspace API > > discussion, so we can leave it out. > > Yes. Let's leave it out. > > > - hardware device > > > > The hardware counterpart of the first category of the kernel struct > > device, a hardware (and thus physical) device such as an SoC IP core or > > an I2C chip. > > > > We could call this "hardware device" or "hardware component". Other > > proposals are welcome. > > as proposed by Hans, on the newer version, we're just using "hardware". Hardware is very generic too. A PC
Re: [PATCH 2/3] media: videodev2: add a flag for vdev-centric devices
Hi Mauro,
On Friday, 25 August 2017 13:06:32 EEST Mauro Carvalho Chehab wrote:
> Em Fri, 25 Aug 2017 11:44:27 +0200 Hans Verkuil escreveu:
> > On 08/25/2017 11:40 AM, Mauro Carvalho Chehab wrote:
> > > From: Mauro Carvalho Chehab <mche...@osg.samsung.com>
> > >
> > > As both vdev-centric and mc-centric devices may implement the
> > > same APIs, we need a flag to allow userspace to distinguish
> > > between them.
> > >
> > > Signed-off-by: Mauro Carvalho Chehab <mche...@osg.samsung.com>
> > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> > > ---
> > >
> > > Documentation/media/uapi/v4l/open.rst| 6 ++
> > > Documentation/media/uapi/v4l/vidioc-querycap.rst | 4
> > > include/uapi/linux/videodev2.h | 2 ++
> > > 3 files changed, 12 insertions(+)
> > >
> > > diff --git a/Documentation/media/uapi/v4l/open.rst
> > > b/Documentation/media/uapi/v4l/open.rst index
> > > a72d142897c0..eb3f0ec57edb 100644
> > > --- a/Documentation/media/uapi/v4l/open.rst
> > > +++ b/Documentation/media/uapi/v4l/open.rst
> > > @@ -33,6 +33,12 @@ For **vdev-centric** control, the device and their
> > > corresponding hardware> >
> > > pipelines are controlled via the **V4L2 device** node. They may
> > > optionally
> > > expose via the :ref:`media controller API `.
> > >
> > > +.. note::
> > > +
> > > + **vdev-centric** devices should report V4L2_VDEV_CENTERED
> >
> > You mean CENTRIC, not CENTERED.
>
> Yeah, true. I'll fix it.
>
> > But I would change this to MC_CENTRIC: the vast majority of drivers are
> > VDEV centric, so it makes a lot more sense to keep that as the default
> > and only set the cap for MC-centric drivers.
>
> I actually focused it on what an userspace application would do.
>
> An specialized application for a given hardware will likely just
> ignore whatever flag is added, and use vdev, mc and subdev APIs
> as it pleases. So, those applications don't need any flag at all.
>
> However, a generic application needs a flag to allow them to check
> if a given hardware can be controlled by the traditional way
> to control the device (e. g. if it accepts vdev-centric type of
> hardware control).
>
> It is an old desire (since when MC was designed) to allow that
> generic V4L2 apps to also work with MC-centric hardware somehow.
>
> At the moment we add that (either in Kernelspace, as proposed for
> iMX6 [1] or via libv4l), a mc-centric hardware can also be
> vdev-centric.
>
> [1] one alternative proposed for iMX6 driver, would be to enable
> vdev-centric control only for hardware with a single camera
> slot, like those cheap RPi3-camera compatible hardware, by
> using some info at the DT.
DT isn't the right place for this, it should describe the hardware, not how it
gets exposed by the kernel to userspace. It could be up to each device driver
to decide, based on the complexity of the hardware as defined in DT, whether
to expose a vdev-centric or MC-centric API, but I wouldn't recommend that as
it would drasticly increase the complexity of the driver.
> >> + :c:type:`v4l2_capability` flag (see :ref:`VIDIOC_QUERYCAP`).
> >> +
> >> +
> >> For **MC-centric** control, before using the V4L2 device, it is
> >> required to set the hardware pipelines via the
> >> :ref:`media controller API `. For those devices, the
> >> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst
> >> b/Documentation/media/uapi/v4l/vidioc-querycap.rst index
> >> 12e0d9a63cd8..4856821b7608 100644
> >> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
> >> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> >> @@ -252,6 +252,10 @@ specification the ioctl returns an ``EINVAL`` error
> >> code.
> >> * - ``V4L2_CAP_TOUCH``
> >>- 0x1000
> >>- This is a touch device.
> >> +* - ``V4L2_VDEV_CENTERED``
> >> + - 0x2000
> >> + - This is controlled via V4L2 device nodes (radio, video, vbi,
> >> +sdr
> >> * - ``V4L2_CAP_DEVICE_CAPS``
> >>- 0x8000
> >>- The driver fills the ``device_caps`` field. This capability can
> >>
> >> diff --git a/include/uapi/linux/videodev2.h
> >> b/include/uapi/linux/videodev2.h index 45cf7359822c..d89090d99042
> >> 100644
> >> --- a/include/uapi/linux/videodev2.h
> >> +++ b/include/uapi/linux/videodev2.h
> >> @@ -460,6 +460,8 @@ struct v4l2_capability {
> >>
> >> #define V4L2_CAP_TOUCH 0x1000 /* Is a touch
> >> device */
> >>
> >> +#define V4L2_CAP_VDEV_CENTERED 0x2000 /* V4L2 Device is
> >> controlled via V4L2 device devnode */ +
> >>
> >> #define V4L2_CAP_DEVICE_CAPS0x8000 /* sets device
> >> capabilities field */
> >>
> >> /*
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] media: open.rst: document devnode-centric and mc-centric types
between vdev-centric and MC- centric devices. For instance the previous paragraph that explains that V4L2 originally had a single type of device control that we now call vdev-centric doesn't really help understanding what a vdev-centric device is. It would be clearer in my opinion to explain the two kind of devices with an associated use case, without referring to the history. > +For **vdev-centric** control, the device and their corresponding hardware > +pipelines are controlled via the **V4L2 device** node. They may optionally > +expose via the :ref:`media controller API `. > + > +For **MC-centric** control, before using the V4L2 device, it is required to > +set the hardware pipelines via the > +:ref:`media controller API `. For those devices, the > +sub-devices' configuration can be controlled via the > +:ref:`sub-device API `, with creates one device node per sub > device. > + > +In summary, for **MC-centric** devices: > + > +- The **V4L2 device** node is responsible for controlling the streaming > + features; > +- The **media controller device** is responsible to setup the pipelines; > +- The **V4L2 sub-devices** are responsible for sub-device > + specific settings. How about a summary of vdev-centric devices too ? Or, possibly, no summary at all ? The need to summarize 5 short paragraphs probably indicates that those paragraphis are not clear enough :-) I can try to help by proposing enhancements to the documentation once we agree on the glossary above. > +.. note:: > + > + A **vdev-centric** may optionally expose V4L2 sub-devices via > + :ref:`sub-device API `. In that case, it has to implement > + the :ref:`media controller API ` as well. The separation between vdev-centric and MC-centric devices is quite clear. If we allow a vdev-centric device to expose subdev nodes we will open the door to all kind of hybrid implementations that have no clear definition today. It will be very important in that case to document in details what is allowed and what isn't, and how the video device nodes and subdev device nodes interact with each other. I prefer not giving a green light to such implementations until we have to, and I also prefer discussing this topic separately. It will require a fair amount of work to document (and thus first agree on), and there's no need to block the rest of this patch until we complete that work. For those reasons I would like to explicitly disallow those hybrid devices in this patch, and start discussing the use cases we have for them, and how they would operate. > +.. _v4l2_device_naming: > > Device Naming > = -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2] media: open.rst: document devnode-centric and mc-centric types
Hi Hans, On Friday, 25 August 2017 11:59:40 EEST Hans Verkuil wrote: > On 24/08/17 14:07, Mauro Carvalho Chehab wrote: > > From: "mche...@s-opensource.com" <mche...@s-opensource.com> > > > > When we added support for omap3, back in 2010, we added a new > > type of V4L2 devices that aren't fully controlled via the V4L2 > > device node. Yet, we never made it clear, at the V4L2 spec, > > about the differences between both types. > > > > Let's document them with the current implementation. > > > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > > Signed-off-by: Mauro Carvalho Chehab <mche...@osg.samsung.com> > > --- > > > > Documentation/media/uapi/v4l/open.rst | 47 ++ > > 1 file changed, 47 insertions(+) > > > > diff --git a/Documentation/media/uapi/v4l/open.rst > > b/Documentation/media/uapi/v4l/open.rst index afd116edb40d..cf522d9bb53c > > 100644 > > --- a/Documentation/media/uapi/v4l/open.rst > > +++ b/Documentation/media/uapi/v4l/open.rst > > @@ -6,6 +6,53 @@ > > > > Opening and Closing Devices > > *** > > > > +Types of V4L2 device control > > I don't like calling this 'device control'. Mostly because the word 'device' > can mean almost anything and is very overused. > > How about "hardware control"? The word device is used for different purposes that make the text unclear in my opinion. We have at least three different kinds of devices: - device node - kernel struct device (fortunately not relevant to the V4L2 API discussion) - hardware counterpart of the kernel struct device (SoC IP core, I2C chip, ...) - group of hardware devices that together make a larger user-facing functional device (for instance the SoC ISP IP cores and external camera sensors together make a camera device) We need different terms for those different concepts, and we need to be very consistent in our usage of those terms. I believe we should also define them formally at the beginning of the documentation to avoid confusion. [snip] -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 5/6] drm: bridge: dw-hdmi: Add Documentation on supported input formats
Hi Neil, Thank you for the patch. On Monday 03 Apr 2017 16:42:37 Neil Armstrong wrote: > This patch adds a new DRM documentation entry and links to the input > format table added in the dw_hdmi header. > > Reviewed-by: Archit Taneja <arch...@codeaurora.org> > Signed-off-by: Neil Armstrong <narmstr...@baylibre.com> Acked-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > --- > Documentation/gpu/bridge/dw-hdmi.rst | 15 +++ > Documentation/gpu/index.rst | 1 + > 2 files changed, 16 insertions(+) > create mode 100644 Documentation/gpu/bridge/dw-hdmi.rst > > diff --git a/Documentation/gpu/bridge/dw-hdmi.rst > b/Documentation/gpu/bridge/dw-hdmi.rst new file mode 100644 > index 000..486faad > --- /dev/null > +++ b/Documentation/gpu/bridge/dw-hdmi.rst > @@ -0,0 +1,15 @@ > +=== > + drm/bridge/dw-hdmi Synopsys DesignWare HDMI Controller > +=== > + > +Synopsys DesignWare HDMI Controller > +=== > + > +This section covers everything related to the Synopsys DesignWare HDMI > +Controller implemented as a DRM bridge. > + > +Supported Input Formats and Encodings > +- > + > +.. kernel-doc:: include/drm/bridge/dw_hdmi.h > + :doc: Supported input formats and encodings > diff --git a/Documentation/gpu/index.rst b/Documentation/gpu/index.rst > index e998ee0..d81c6ff 100644 > --- a/Documentation/gpu/index.rst > +++ b/Documentation/gpu/index.rst > @@ -15,6 +15,7 @@ Linux GPU Driver Developer's Guide > vc4 > vga-switcheroo > vgaarbiter > + bridge/dw-hdmi > todo > > .. only:: subproject and html -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 4/6] drm: bridge: dw-hdmi: Switch to V4L bus format and encodings
Hi Neil, Thank you for the patch. On Monday 03 Apr 2017 16:42:36 Neil Armstrong wrote: > Some display pipelines can only provide non-RBG input pixels to the HDMI TX > Controller, this patch takes the pixel format from the plat_data if > provided. The commit message doesn't seem to match the subject line. > Reviewed-by: Jose Abreu <joab...@synopsys.com> > Reviewed-by: Archit Taneja <arch...@codeaurora.org> > Signed-off-by: Neil Armstrong <narmstr...@baylibre.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 326 +++ > include/drm/bridge/dw_hdmi.h | 63 ++ > 2 files changed, 294 insertions(+), 95 deletions(-) [snip] > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > index bcceee8..45c2c15 100644 > --- a/include/drm/bridge/dw_hdmi.h > +++ b/include/drm/bridge/dw_hdmi.h > @@ -14,6 +14,67 @@ > > struct dw_hdmi; > > +/** > + * DOC: Supported input formats and encodings > + * > + * Depending on the Hardware configuration of the Controller IP, it > supports > + * a subset of the following input formats and encodings on it's internal > + * 48bit bus. > + * s/it's/its/ [snip] Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com> -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] documentation: media: Add documentation for new RGB and YUV bus formats
Hi Neil, Thank you for the patch. On Monday 03 Apr 2017 16:42:35 Neil Armstrong wrote: > Add documentation for added Bus Formats to describe RGB and YUV formats used > as input to the Synopsys DesignWare HDMI TX Controller. > > Acked-by: Hans Verkuil <hans.verk...@cisco.com> > Reviewed-by: Archit Taneja <arch...@codeaurora.org> > Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > Signed-off-by: Neil Armstrong <narmstr...@baylibre.com> > --- > Documentation/media/uapi/v4l/subdev-formats.rst | 960 - > 1 file changed, 959 insertions(+), 1 deletion(-) > > diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst > b/Documentation/media/uapi/v4l/subdev-formats.rst index d6152c9..4032d97 > 100644 > --- a/Documentation/media/uapi/v4l/subdev-formats.rst > +++ b/Documentation/media/uapi/v4l/subdev-formats.rst > @@ -1258,6 +1258,319 @@ The following tables list existing packed RGB > formats. - b\ :sub:`2` >- b\ :sub:`1` >- b\ :sub:`0` > +* .. _MEDIA-BUS-FMT-RGB101010-1X30: > + > + - MEDIA_BUS_FMT_RGB101010_1X30 > + - 0x1018 > + - > + - 0 > + - 0 > + - r\ :sub:`9` > + - r\ :sub:`8` > + - r\ :sub:`7` > + - r\ :sub:`6` > + - r\ :sub:`5` > + - r\ :sub:`4` > + - r\ :sub:`3` > + - r\ :sub:`2` > + - r\ :sub:`1` > + - r\ :sub:`0` > + - g\ :sub:`9` > + - g\ :sub:`8` > + - g\ :sub:`7` > + - g\ :sub:`6` > + - g\ :sub:`5` > + - g\ :sub:`4` > + - g\ :sub:`3` > + - g\ :sub:`2` > + - g\ :sub:`1` > + - g\ :sub:`0` > + - b\ :sub:`9` > + - b\ :sub:`8` > + - b\ :sub:`7` > + - b\ :sub:`6` > + - b\ :sub:`5` > + - b\ :sub:`4` > + - b\ :sub:`3` > + - b\ :sub:`2` > + - b\ :sub:`1` > + - b\ :sub:`0` > + > +.. raw:: latex > + > +\endgroup > + > + > +The following table list existing packed 36bit wide RGB formats. s/list/lists/ Same comment for the other tables. Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com> [snip] -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
Hi Alan,
On Thursday 30 Mar 2017 11:55:18 Alan Stern wrote:
> On Thu, 30 Mar 2017, Mauro Carvalho Chehab wrote:
> > Em Thu, 30 Mar 2017 10:26:32 -0400 (EDT) Alan Stern escreveu:
> >> On Thu, 30 Mar 2017, Oliver Neukum wrote:
> >>>> Btw, I'm a lot more concerned about USB storage drivers. When I was
> >>>> discussing about this issue at the #raspberrypi-devel IRC channel,
> >>>> someone complained that, after switching from the RPi downstream
> >>>> Kernel to upstream, his USB data storage got corrupted. Well, if the
> >>>> USB storage drivers also assume that the buffer can be continuous,
> >>>> that can corrupt data.
> >>>
> >>> They do assume that.
> >>
> >> Wait a minute. Where does that assumption occur?
> >>
> >> And exactly what is the assumption? Mauro wrote "the buffer can be
> >> continuous", but that is certainly not what he meant.
> >
> > What I meant to say is that drivers like the uvcdriver (and maybe network
> > and usb-storage drivers) may allocate a big buffer and get data there on
> > some random order, e. g.:
> >
> > int get_from_buf_pos(char *buf, int pos, int size)
> > {
> > /* or an equivalent call to usb_submit_urb() */
> > usb_control_msg(..., buf + pos, size, ...);
> > }
> >
> > some_function ()
> > {
> > ...
> >
> > chr *buf = kzalloc(4, GFP_KERNEL);
> >
> > /*
> > * Access the bytes at the array on a random order, with random size,
> > * Like:
> > */
> > get_from_buf_pos(buf, 2, 2);/* should read 0x56, 0x78 */
> > get_from_buf_pos(buf, 0, 2);/* should read 0x12, 0x34 */
> >
> > /*
> > * the expected value for the buffer would be:
> > * { 0x12, 0x34, 0x56, 0x78 }
> > */
> >
> > E. g. they assume that the transfer URB can work with any arbitrary
> > pointer and size, without needing of pre-align them.
> >
> > This doesn't work with HCD drivers like dwc2, as each USB_IN operation
> > will actually write 4 bytes to the buffer.
> >
> > So, what happens, instead, is that each data transfer will get four
> > bytes. Due to a hack inside dwc2, with checks if the transfer_buffer
> > is DWORD aligned. So, the first transfer will do what's expected: it will
> > read 4 bytes to a temporary buffer, allocated inside the driver,
> > copying just two bytes to buf. So, after the first read, the
> >
> > buffer content will be:
> > buf = { 0x00, x00, 0x56, 0x78 }
> >
> > But, on the second read, it won't be using any temporary
> > buffer. So, instead of reading a 16-bits word (0x5678),
> > it will actually read 32 bits, with 16-bits with some random value,
> >
> > causing a buffer overflow. E. g. buffer content will now be:
> > buf = { 0x12, x34, 0xde, 0xad }
> >
> > In other words, the second transfer corrupted the data from the
> > first transfer.
>
> I'm pretty sure that usb-storage does not do this, at least, not when
> operating in its normal Bulk-Only-Transport mode. It never tries to
> read the results of an earlier transfer after carrying out a later
> transfer to any part of the same buffer.
The uvcvideo driver does something similar. Given the size of the transfer a
bounce buffer shouldn't affect performances. Handling this in the USB core
sounds like the best solution to me.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
Hi Mauro, On Thursday 30 Mar 2017 07:28:00 Mauro Carvalho Chehab wrote: > Em Thu, 30 Mar 2017 12:34:32 +0300 Laurent Pinchart escreveu: > > On Thursday 30 Mar 2017 10:11:31 Oliver Neukum wrote: > >> Am Donnerstag, den 30.03.2017, 01:15 +0300 schrieb Laurent Pinchart: > >>>> + may also override PAD bytes at the end of the > >>>> ``transfer_buffer``, up to the > >>>> + size of the CPU word. > >>> > >>> "May" is quite weak here. If some host controller drivers require > >>> buffers to be aligned, then it's an API requirement, and all buffers > >>> must be aligned. I'm not even sure I would mention that some host > >>> drivers require it, I think we should just state that the API requires > >>> buffers to be aligned. > >> > >> That effectively changes the API. Many network drivers are written with > >> the assumption that any contiguous buffer is valid. In fact you could > >> argue that those drivers are buggy and must use bounce buffers in those > >> cases. > > Blaming the dwc2 driver was my first approach, but such patch got nacked ;) > > Btw, the dwc2 driver has a routine that creates a temporary buffer if the > buffer pointer is not DWORD aligned. My first approach were to add > a logic there to also use the temporary buffer if the buffer size is > not DWORD aligned: > https://patchwork.linuxtv.org/patch/40093/ > > While debugging this issue, I saw *a lot* of network-generated URB > traffic from RPi3 Ethernet port drivers that were using non-aligned > buffers and were subject to the temporary buffer conversion. > > My understanding here is that having a temporary bounce buffer sucks, > as the performance and latency are affected. So, I see the value of > adding this constraint to the API, pushing the task of getting > aligned buffers to the USB drivers, This could however degrade performances when the HCD can handle unaligned buffers. > but you're right: that means a lot of work, as all USB drivers should be > reviewed. If we decide in the end to push the constraint on the USB device driver side, then the dwc2 HCD driver should return an error when the buffer isn't correctly aligned. > Btw, I'm a lot more concerned about USB storage drivers. When I was > discussing about this issue at the #raspberrypi-devel IRC channel, > someone complained that, after switching from the RPi downstream Kernel > to upstream, his USB data storage got corrupted. Well, if the USB > storage drivers also assume that the buffer can be continuous, > that can corrupt data. > > That's why I think that being verbose here is a good idea. > > I'll rework on this patch to put more emphasis about this issue. > > >> So we need to include the full story here. > > > > I personally don't care much about whose side is responsible for handling > > the alignment constraints, but I want it to be documented before "fixing" > > any USB driver. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
Hi Mauro, On Wednesday 29 Mar 2017 22:06:33 Mauro Carvalho Chehab wrote: > Em Thu, 30 Mar 2017 01:15:27 +0300 Laurent Pinchart escreveu: > > On Wednesday 29 Mar 2017 15:54:21 Mauro Carvalho Chehab wrote: > > > Several host controllers, commonly found on ARM, like dwc2, > > > require buffers that are CPU-word aligned for they to work. > > > > > > Failing to do that will cause random troubles at the caller > > > drivers, causing them to fail. > > > > > > Document it. > > > > > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > > > --- > > > > > > Documentation/driver-api/usb/URB.rst | 18 ++ > > > drivers/usb/core/message.c | 15 +++ > > > include/linux/usb.h | 18 ++ > > > 3 files changed, 51 insertions(+) > > > > > > diff --git a/Documentation/driver-api/usb/URB.rst > > > b/Documentation/driver-api/usb/URB.rst index d9ea6a3996e7..b83b557e9891 > > > 100644 > > > --- a/Documentation/driver-api/usb/URB.rst > > > +++ b/Documentation/driver-api/usb/URB.rst > > > @@ -274,6 +274,24 @@ If you specify your own start frame, make sure it's > > > several frames in advance of the current frame. You might want this > > > model > > > if you're synchronizing ISO data with some other event stream. > > > > > > +.. note:: > > > + > > > + Several host drivers require that the ``transfer_buffer`` to be > > > aligned > > > + with the CPU word size (e. g. DWORD for 32 bits, QDWORD for 64 > > > bits). > > > > Is it the CPU word size or the DMA transfer size ? I assume the latter, > > and I wouldn't be surprised if the alignment requirement was 32-bit on at > > least some of the 64-bit platforms. > > Yeah, it is actually the DMA transfer size. Yet, worse case scenario is that > the DMA transfer size to be 64 bits on 64 bits CPU. > > > > + It is up to USB drivers should ensure that they'll only pass buffers > > > + with such alignments. > > > + > > > + Please also notice that, due to such restriction, the host driver > > > > s/notice/note/ (and below as well) ? > > OK. > > > > + may also override PAD bytes at the end of the ``transfer_buffer``, > > > up to the > > > + size of the CPU word. > > > > "May" is quite weak here. If some host controller drivers require buffers > > to be aligned, then it's an API requirement, and all buffers must be > > aligned. I'm not even sure I would mention that some host drivers require > > it, I think we should just state that the API requires buffers to be > > aligned. > > What I'm trying to say here is that, on a 32-bits system, if the driver do > a USB_DIR_IN transfer using some code similar to: > > size = 4; > buffer = kmalloc(size, GFP_KERNEL); > > usb_control_msg(udev, pipe, req, type, val, idx, buffer + 2, 2, timeout); > usb_control_msg(udev, pipe, req, type, val, idx, buffer, size, timeout); > > Drivers like dwc2 will mess with the buffer. > > The first transfer will actually work, due to a workaround inside the > driver that will create a temporary DWORD-aligned buffer, avoiding it > to go past the buffer. > > However, the second transfer will destroy the data received from the > first usb_control_msg(), as it will write 4 bytes at the buffer. > > Not all drivers would do that, though. > > Please notice that, as kmalloc will always return a CPU-aligned buffer, > if the client do something like: > > size = 2; > buffer = kmalloc(size, GFP_KERNEL); > > usb_control_msg(udev, pipe, req, type, val, idx, buffer, 2, timeout); > > What happens there is that the DMA engine will still write 4 bytes at > the buffer, but the 2 bytes that go past the end of buffer will be > written on a memory that will never be used. I understand that, but stating that host controller drivers "may" do this won't help much. If they *may*, all USB device drivers *must* align buffers correctly. That's the part that needs to be documented. Let's not confuse developers by only stating that something may happened, let's be clear and tell what they must do. > > > + Please notice that ancillary routines that transfer URBs, like > > > + usb_control_msg() also have such restriction. > > > + > > > + Such word alignment condition is normally ensured if the buffer is > > > + allocated with kmalloc(), but this may not be the case if the
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
Hi Oliver, On Thursday 30 Mar 2017 10:11:31 Oliver Neukum wrote: > Am Donnerstag, den 30.03.2017, 01:15 +0300 schrieb Laurent Pinchart: > > > + may also override PAD bytes at the end of the ``transfer_buffer``, > > > up to the > > > + size of the CPU word. > > > > "May" is quite weak here. If some host controller drivers require buffers > > to be aligned, then it's an API requirement, and all buffers must be > > aligned. I'm not even sure I would mention that some host drivers require > > it, I think we should just state that the API requires buffers to be > > aligned. > > That effectively changes the API. Many network drivers are written with > the assumption that any contiguous buffer is valid. In fact you could > argue that those drivers are buggy and must use bounce buffers in those > cases. > > So we need to include the full story here. I personally don't care much about whose side is responsible for handling the alignment constraints, but I want it to be documented before "fixing" any USB driver. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
usb_control_msg() also have such restriction. > + * > + * Such word alignment condition is normally ensured if the buffer is > + * allocated with kmalloc(), but this may not be the case if the driver > + * allocates a bigger buffer and point to a random place inside it. > + * Couldn't we avoid three copies of the same text ? The chance they will get out-of-sync is quite high. > * Initialization: > * > * All URBs submitted must initialize the dev, pipe, transfer_flags (may be -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drm/doc: diagram for mode objects and properties
Hi Daniel,
Thank you for the patch.
On Wednesday 01 Mar 2017 09:27:14 Daniel Vetter wrote:
> Resulted in confusion a few times in the past.
>
> v2: Spelling fix (Eric).
>
> Cc: Eric Anholt <e...@anholt.net>
> Acked-by: Eric Anholt <e...@anholt.net>
> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Cc: Manasi Navare <manasi.d.nav...@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch>
Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> ---
> Documentation/gpu/drm-kms.rst | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 17a4cd5b14fd..a504d9ee4d94 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -161,6 +161,28 @@ KMS Core Structures and Functions
> Modeset Base Object Abstraction
> ===
>
> +.. kernel-render:: DOT
> + :alt: Mode Objects and Properties
> + :caption: Mode Objects and Properties
> +
> + digraph {
> + node [shape=box]
> +
> + "drm_property A" -> "drm_mode_object A"
> + "drm_property A" -> "drm_mode_object B"
> + "drm_property B" -> "drm_mode_object A"
> + }
> +
> +The base structure for all KMS objects is :c:type:`struct drm_mode_object
> +`. One of the base services it provides is tracking
> properties, +which are especially important for the atomic IOCTL (see
> `Atomic Mode +Setting`_). The somewhat surprising part here is that
> properties are not +directly instantiated on each object, but free-standing
> mode objects themselves, +represented by :c:type:`struct drm_property
> `, which only specify +the type and value range of a
> property. Any given property can be attached +multiple times to different
> objects using :c:func:`drm_object_attach_property()
> +`.
> +
> .. kernel-doc:: include/drm/drm_mode_object.h
>
> :internal:
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] docs-rst: automatically convert Graphviz and SVG images
Hi Daniel and Markus, On Thursday 02 Mar 2017 16:11:08 Daniel Vetter wrote: > On Thu, Mar 02, 2017 at 03:58:36PM +0100, Markus Heiser wrote: > > Am 02.03.2017 um 15:14 schrieb Laurent Pinchart: > > > On Thursday 02 Mar 2017 14:54:32 Daniel Vetter wrote: > > >> On Thu, Mar 2, 2017 at 1:26 PM, Laurent Pinchart wrote: > > >>> Hi Daniel, > > >>> > > >>> Thank you for the patch. > > >>> > > >>> With this applied, I get > > >>> > > >>> make[1]: Entering directory '/home/laurent/src/iob/renesas/linux64' > > >>> > > >>> SPHINX htmldocs --> > > >>> file:///home/laurent/src/iob/renesas/linux64/Documentation/output > > >>> PARSE > > >>> > > >>>include/uapi/linux/videodev2.h > > >>> > > >>> Running Sphinx v1.3.1 > > >>> > > >>> Extension error: > > >>> Could not import extension kfigure (exception: cannot import name > > >>> patches) > > >>> make[2]: *** > > >>> [/home/laurent/src/iob/renesas/linux/Documentation/Makefile.sphinx:70: > > >>> htmldocs] Error 1 make[1]: *** > > >>> [/home/laurent/src/iob/renesas/linux/Makefile:1453: htmldocs] Error 2 > > >>> make[1]: Leaving directory '/home/laurent/src/iob/renesas/linux64' > > >>> make: > > >>> *** [Makefile:152: sub-make] Error 2 > > >>> > > >>> sphinx.directive.patches got introduced in Sphinx 1.4. If you want to > > >>> bump > > >>> the minimum required version I think a notice is needed. > > >> > > >> Ugh. But this also goes completely over my head, no idea whether we > > >> must require sphinx 1.4 (it was released Mar 28, 2016), or whether > > >> there's some way to work around this ... Halp? > > > > > > I'm not a Sphinx expert so I don't know, but what I can tell is that > > > copying the patches.py from Sphinx 1.4 to Documentation/sphinx/ and > > > modifying kfigure.py to import it from there fixes the build. There's > > > thus no extra depencency on Sphinx 1.4 (or newer). > > > > > > I'm not sure we want to set a precedent by copying part of the Sphinx > > > source code to the kernel tree (or inlining the single small function > > > that the module provides), and I'll let someone more knowledgeable than > > > me decide how to proceed. > > > > Aargh ... we need virtualenv! For interim something like the following > > might help. In file Documentation/sphinx/kfigure.py edit the imports > > > > ... > > from docutils.parsers.rst.directives import images > > > > try: > > from sphinx.directives.patches import Figure > > > > except ImportError: > > Figure = images.Figure > > > > ... > > > > And fix the class definition, so it use 'Figure' and no > > longer 'patch.Figure':: > > > > ... > > -class KernelFigure(patches.Figure): > > +class KernelFigure(Figure): > > ... > > > > Sorry that I have not yet the time to send you a decent and tested > > patch. Do you like to test my suggestion? / thanks! > > I'll give it a shot at implementing it, but I can't (easily at least) test > on sphinx 1.3. I can, and it works. Tested-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com> Thank you for the quick fix Markus. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] docs-rst: automatically convert Graphviz and SVG images
Hi Daniel,
Thank you for the patch.
On Thursday 02 Mar 2017 16:16:34 Daniel Vetter wrote:
> From: Markus Heiser <markus.hei...@darmarit.de>
>
> This patch brings scalable figure, image handling and a concept to
> embed *render* markups:
>
> * DOT (http://www.graphviz.org)
> * SVG
>
> For image handling use the 'image' replacement::
>
> .. kernel-image:: svg_image.svg
>
>:alt:simple SVG image
>
> For figure handling use the 'figure' replacement::
>
> .. kernel-figure:: svg_image.svg
>
>:alt:simple SVG image
>
>SVG image example
>
> Embed *render* markups (or languages) like Graphviz's **DOT** is
> provided by the *render* directive.::
>
> .. kernel-render:: DOT
>
> :alt: foobar digraph
> :caption: Embedded **DOT** (Graphviz) code.
>
> digraph foo {
> "bar" -> "baz";
> }
>
> The *render* directive is a concept to integrate *render* markups and
> languages, yet supported markups:
>
> * DOT: render embedded Graphviz's **DOT**
> * SVG: render embedded Scalable Vector Graphics (**SVG**)
>
> v2: s/DOC/DOT/ in a few places (by Daniel).
>
> v3: Simplify stuff a bit (by Daniel):
>
> - Remove path detection and setup/check code for that. In
> Documentation/media/Makefile we already simply use these tools,
> better to have one consolidated check if we want/need one. Also
> remove the convertsvg support, we require ImageMagick's convert
> already in the doc build, no need for a 2nd fallback.
>
> - Use sphinx for depency tracking, remove hand-rolled version.
>
> - Forward stderr from dot and convert, otherwise debugging issues with
> the diagrams is impossible.
>
> v3: Only sphinx 1.4 (released in Mar 2016) has patches.Figure.
> Implement Markus suggestion for backwards compatability with earlier
> releases. Laurent reported this, running sphinx 1.3. Solution entirely
> untested.
Tested-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
However, you might want to explicitly check the Sphinx version, as done in
Documentation/conf.py:
import sphinx
# Get Sphinx version
major, minor, patch = map(int, sphinx.__version__.split("."))
if major == 1 and minor > 3:
...
This will make it clearer what version we depend on, and easier to remove dead
code when we'll bump the minimum version requirements.
> Cc: Jonathan Corbet <cor...@lwn.net>
> Cc: linux-doc@vger.kernel.org
> Cc: Jani Nikula <jani.nik...@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mche...@s-opensource.com>
> Cc: Markus Heiser <markus.hei...@darmarit.de>
> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Signed-off-by: Markus Heiser <markus.hei...@darmarit.de> (v1)
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
> ---
> Documentation/conf.py | 2 +-
> Documentation/doc-guide/hello.dot | 3 +
> Documentation/doc-guide/sphinx.rst| 90 ++-
> Documentation/doc-guide/svg_image.svg | 10 +
> Documentation/process/changes.rst | 7 +-
> Documentation/sphinx/kfigure.py | 444 ++
> 6 files changed, 550 insertions(+), 6 deletions(-)
> create mode 100644 Documentation/doc-guide/hello.dot
> create mode 100644 Documentation/doc-guide/svg_image.svg
> create mode 100644 Documentation/sphinx/kfigure.py
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drm/doc: atomic overview, with graph
Hi Daniel,
Thank you for the patch.
On Thursday 02 Mar 2017 08:19:58 Daniel Vetter wrote:
> I want to split up a few more things and document some details better
> (like how exactly to subclass drm_atomic_state). And maybe also split
> up the helpers a bit per-topic, but this should be a ok-ish start for
> better atomic overview.
>
> v2: Spelling and clarifications (Eric).
>
> v3: Implement suggestion from Gabriel to fix the graph.
>
> Cc: Gabriel Krisman Bertazi <kris...@collabora.co.uk>
> Acked-by: Eric Anholt <e...@anholt.net>
> Cc: Eric Anholt <e...@anholt.net>
> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Cc: Harry Wentland <harry.wentl...@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
> ---
> Documentation/gpu/drm-kms-helpers.rst | 2 +
> Documentation/gpu/drm-kms.rst | 84 +++-
> 2 files changed, 85 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/gpu/drm-kms-helpers.rst
> b/Documentation/gpu/drm-kms-helpers.rst index 050ebe81d256..ac53c0b893f6
> 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -42,6 +42,8 @@ Modeset Helper Reference for Common Vtables
> .. kernel-doc:: include/drm/drm_modeset_helper_vtables.h
>
> :internal:
> +.. _drm_atomic_helper:
> +
> Atomic Modeset Helper Functions Reference
> =
>
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index a504d9ee4d94..eb9d29865c41 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -189,8 +189,90 @@ multiple times to different objects using
> :c:func:`drm_object_attach_property() .. kernel-doc::
> drivers/gpu/drm/drm_mode_object.c
>
> :export:
> +Atomic Mode Setting
> +===
> +
> +
> +.. kernel-render:: DOT
> + :alt: Mode Objects and Properties
> + :caption: Mode Objects and Properties
> +
> + digraph {
> + node [shape=box]
> +
> + subgraph cluster_state {
> + style=dashed
> + label="Free-standing state"
> +
> + "drm_atomic_state" -> "duplicated drm_plane_state A"
> + "drm_atomic_state" -> "duplicated drm_plane_state B"
> + "drm_atomic_state" -> "duplicated drm_crtc_state"
> + "drm_atomic_state" -> "duplicated drm_connector_state"
> + "drm_atomic_state" -> "duplicated driver private state"
> + }
> +
> + subgraph cluster_current {
> + style=dashed
> + label="Current state"
> +
> + "drm_device" -> "drm_plane A"
> + "drm_device" -> "drm_plane B"
> + "drm_device" -> "drm_crtc"
> + "drm_device" -> "drm_connector"
> + "drm_device" -> "driver private object"
> +
> + "drm_plane A" -> "drm_plane_state A"
> + "drm_plane B" -> "drm_plane_state B"
> + "drm_crtc" -> "drm_crtc_state"
> + "drm_connector" -> "drm_connector_state"
> + "driver private object" -> "driver private state"
> + }
> +
> + "drm_atomic_state" -> "drm_device" [label="atomic_commit"]
> + "duplicated drm_plane_state A" -> "drm_device"[style=invis]
> + }
> +
> +Atomic provides transactional modeset (including planes) updates, but a
> +bit differently from the usual transactional approach of try-commit and
> +rollback:
> +
> +- Firstly, no hardware changes are allowed when the commit would fail. This
> + allows us to implement the DRM_MODE_ATOMIC_TEST_ONLY mode, which allows
> + userspace to explore whether certain configurations would work or not. +
> +- This would still allow setting and rollback of just the software state,
> + simplifying conversion of existing drivers. But auditing drivers for
> + correctness of the atomic_check code becomes really hard with that:
> Rolling
> + back changes in data structures all over the place is hard to get right.
> +
> +- Lastly, for backwards compatibility and to support all use-cases, atomic
s/backwards/backward/ (see my comment to patch 3/6, and not that the margin is
now slightly larger :-))
> + updates need to be incremental and be able to execute in parallel.
> Hardware
> + doesn't always allow it, but
Re: [PATCH 2/6] docs-rst: automatically convert Graphviz and SVG images
Hi Daniel, On Thursday 02 Mar 2017 14:54:32 Daniel Vetter wrote: > On Thu, Mar 2, 2017 at 1:26 PM, Laurent Pinchart wrote: > > Hi Daniel, > > > > Thank you for the patch. > > > > With this applied, I get > > > > make[1]: Entering directory '/home/laurent/src/iob/renesas/linux64' > > > > SPHINX htmldocs --> > > file:///home/laurent/src/iob/renesas/linux64/Documentation/output PARSE > > include/uapi/linux/videodev2.h > > > > Running Sphinx v1.3.1 > > > > Extension error: > > Could not import extension kfigure (exception: cannot import name patches) > > make[2]: *** > > [/home/laurent/src/iob/renesas/linux/Documentation/Makefile.sphinx:70: > > htmldocs] Error 1 make[1]: *** > > [/home/laurent/src/iob/renesas/linux/Makefile:1453: htmldocs] Error 2 > > make[1]: Leaving directory '/home/laurent/src/iob/renesas/linux64' make: > > *** [Makefile:152: sub-make] Error 2 > > > > sphinx.directive.patches got introduced in Sphinx 1.4. If you want to bump > > the minimum required version I think a notice is needed. > > Ugh. But this also goes completely over my head, no idea whether we > must require sphinx 1.4 (it was released Mar 28, 2016), or whether > there's some way to work around this ... Halp? I'm not a Sphinx expert so I don't know, but what I can tell is that copying the patches.py from Sphinx 1.4 to Documentation/sphinx/ and modifying kfigure.py to import it from there fixes the build. There's thus no extra depencency on Sphinx 1.4 (or newer). I'm not sure we want to set a precedent by copying part of the Sphinx source code to the kernel tree (or inlining the single small function that the module provides), and I'll let someone more knowledgeable than me decide how to proceed. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] docs-rst: automatically convert Graphviz and SVG images
Hi Daniel,
Thank you for the patch.
With this applied, I get
make[1]: Entering directory '/home/laurent/src/iob/renesas/linux64'
SPHINX htmldocs -->
file:///home/laurent/src/iob/renesas/linux64/Documentation/output
PARSE include/uapi/linux/videodev2.h
Running Sphinx v1.3.1
Extension error:
Could not import extension kfigure (exception: cannot import name patches)
make[2]: ***
[/home/laurent/src/iob/renesas/linux/Documentation/Makefile.sphinx:70:
htmldocs] Error 1
make[1]: *** [/home/laurent/src/iob/renesas/linux/Makefile:1453: htmldocs]
Error 2
make[1]: Leaving directory '/home/laurent/src/iob/renesas/linux64'
make: *** [Makefile:152: sub-make] Error 2
sphinx.directive.patches got introduced in Sphinx 1.4. If you want to bump the
minimum required version I think a notice is needed.
On Tuesday 28 Feb 2017 18:13:15 Daniel Vetter wrote:
> From: Markus Heiser <markus.hei...@darmarit.de>
>
> This patch brings scalable figure, image handling and a concept to
> embed *render* markups:
>
> * DOT (http://www.graphviz.org)
> * SVG
>
> For image handling use the 'image' replacement::
>
> .. kernel-image:: svg_image.svg
>
>:alt:simple SVG image
>
> For figure handling use the 'figure' replacement::
>
> .. kernel-figure:: svg_image.svg
>
>:alt:simple SVG image
>
>SVG image example
>
> Embed *render* markups (or languages) like Graphviz's **DOT** is
> provided by the *render* directive.::
>
> .. kernel-render:: DOT
>
> :alt: foobar digraph
> :caption: Embedded **DOT** (Graphviz) code.
>
> digraph foo {
> "bar" -> "baz";
> }
>
> The *render* directive is a concept to integrate *render* markups and
> languages, yet supported markups:
>
> * DOT: render embedded Graphviz's **DOT**
> * SVG: render embedded Scalable Vector Graphics (**SVG**)
>
> v2: s/DOC/DOT/ in a few places (by Daniel).
>
> v3: Simplify stuff a bit (by Daniel):
>
> - Remove path detection and setup/check code for that. In
> Documentation/media/Makefile we already simply use these tools,
> better to have one consolidated check if we want/need one. Also
> remove the convertsvg support, we require ImageMagick's convert
> already in the doc build, no need for a 2nd fallback.
>
> - Use sphinx for depency tracking, remove hand-rolled version.
>
> - Forward stderr from dot and convert, otherwise debugging issues with
> the diagrams is impossible.
>
> Cc: Jonathan Corbet <cor...@lwn.net>
> Cc: linux-doc@vger.kernel.org
> Cc: Jani Nikula <jani.nik...@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mche...@s-opensource.com>
> Signed-off-by: Markus Heiser <markus.hei...@darmarit.de> (v1)
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
> ---
> Documentation/conf.py | 2 +-
> Documentation/doc-guide/hello.dot | 3 +
> Documentation/doc-guide/sphinx.rst| 90 ++-
> Documentation/doc-guide/svg_image.svg | 10 +
> Documentation/process/changes.rst | 7 +-
> Documentation/sphinx/kfigure.py | 442 ++++++
> 6 files changed, 548 insertions(+), 6 deletions(-)
> create mode 100644 Documentation/doc-guide/hello.dot
> create mode 100644 Documentation/doc-guide/svg_image.svg
> create mode 100644 Documentation/sphinx/kfigure.py
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/47] [media] v4l2-dev.rst: fix a broken c domain reference
Hi Mauro, Thank you for the patch. On Thursday 08 Sep 2016 09:03:44 Mauro Carvalho Chehab wrote: > The "struct" were inside the reference, causing it to break. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> Acked-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > --- > Documentation/media/kapi/v4l2-dev.rst | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/media/kapi/v4l2-dev.rst > b/Documentation/media/kapi/v4l2-dev.rst index 5782be725334..0a3b4503a89f > 100644 > --- a/Documentation/media/kapi/v4l2-dev.rst > +++ b/Documentation/media/kapi/v4l2-dev.rst > > @@ -56,7 +56,7 @@ You should also set these fields of :c:type:`video_device`: >:c:type:`video_device`->vfl_dir fields are used to disable ops that do >:not > >match the type/dir combination. E.g. VBI ops are disabled for non-VBI > nodes, and output ops are disabled for a capture device. This makes it > possible to - provide just one :c:type:`v4l2_ioctl_ops struct` for both > vbi and + provide just one :c:type:`v4l2_ioctl_ops` struct for both vbi > and video nodes. > > - :c:type:`video_device`->lock: leave to ``NULL`` if you want to do all the -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANN] Media documentation converted to ReST markup language
Hi Mauro, On Wednesday 13 Jul 2016 11:11:43 Mauro Carvalho Chehab wrote: > Em Sat, 09 Jul 2016 20:10:21 +0300 Laurent Pinchart escreveu: > > The other one is related, the table of contents in the main page of each > > section > > (https://mchehab.fedorapeople.org/media_API_book/linux_tv/media/v4l/v4l2.h > > tml for instance) only shows the first level entries. We have a full table > > of contents now, and that's very practical to quickly search for the > > information we need without requiring many clicks (or actually any click > > at all). How can we keep that feature ? > > It is not hard to change the level of entries, although I really hated the > DocBook template that creates multi-depth TOCs everywhere, as it is very > messy to see those big indexes in the middle of the book. > > What I did was to add *one* full contents index (actually, up to level 5) > at the first page of the book: > https://linuxtv.org/downloads/v4l-dvb-apis-new/media/media_uapi.html > > and kept the other ones with depth 1. Thank you, that's exactly what I had in mind. > > By the way, the "Video for Linux API" section (and the other sibling > > sections) are child nodes of the "Introduction" section. That feels quite > > odd. > > This was fixed already. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANN] Media documentation converted to ReST markup language
Hi Mauro, On Friday 08 Jul 2016 10:34:20 Mauro Carvalho Chehab wrote: > As commented on the patch series I just submitted, we finished the > conversion of the Media uAPI book from DocBook to ReST. > > For now, I'm placing the new documentation, after parsed by Sphinx, at this > place: > https://mchehab.fedorapeople.org/media_API_book/ > > There are some instructions there about how to use Sphinx too, with can be > useful for the ones writing patches. Those are part of the docs-next that > will be sent to Kernel 4.8, thanks to Jani Nikula an Jonathan Corbet. > > The media docbook itself is located at: > https://mchehab.fedorapeople.org/media_API_book/linux_tv/index.html > > And the patches are already at the media tree, under the "docs-next" > branch: > https://git.linuxtv.org/media_tree.git/log/?h=docs-next > > If you find anything inconsistent, wrong or incomplete, feel free to > submit patches to it. My plan is to merge this branch on Kernel 4.8-rc1 > and then remove the Documentation/DocBook/media stuff from the Kernel. > > PS.: I'll soon be adding one extra patch there renaming the media > directory. "linux_tv" is not the best name for the media contents, > but, on the other hand, having a "media/media" directory also doesn't > make sense. So, I need to think for a better name before doing the > change. Pehaps I'll go for: > Documentation/media - for all media documentation, were we > should also store things that are now under > /video4linux and under /dvb; > > and: > Documentation/media/uapi - for the above book that were just > converted from DocBook. The layout looks fresh and new, that's nice. I've noticed two pain points though. One of them is that the left-hand side navigation table requires Javascript. I wonder if there would be away to expand it fully, or even remove it, when Javascript is disabled. The other one is related, the table of contents in the main page of each section (https://mchehab.fedorapeople.org/media_API_book/linux_tv/media/v4l/v4l2.html for instance) only shows the first level entries. We have a full table of contents now, and that's very practical to quickly search for the information we need without requiring many clicks (or actually any click at all). How can we keep that feature ? By the way, the "Video for Linux API" section (and the other sibling sections) are child nodes of the "Introduction" section. That feels quite odd. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
