Re: [PATCH 22/24] drm/bridge: remove the .of_node member
Hi Peter,
I love your patch! Yet something to improve:
[auto build test ERROR on v4.17-rc2]
[also build test ERROR on next-20180426]
[cannot apply to drm/drm-next robclark/msm-next drm-exynos/exynos-drm/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Peter-Rosin/device-link-bridge-supplier-drm-device/20180428-135229
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
drivers/gpu//drm/rockchip/rockchip_lvds.c: In function 'rockchip_lvds_bind':
>> drivers/gpu//drm/rockchip/rockchip_lvds.c:381:24: error: 'struct drm_bridge'
>> has no member named 'of_node'
remote = lvds->bridge->of_node;
^~
vim +381 drivers/gpu//drm/rockchip/rockchip_lvds.c
34cc0aa25 Sandy Huang 2017-09-02 340
34cc0aa25 Sandy Huang 2017-09-02 341 static int rockchip_lvds_bind(struct
device *dev, struct device *master,
34cc0aa25 Sandy Huang 2017-09-02 342 void *data)
34cc0aa25 Sandy Huang 2017-09-02 343 {
34cc0aa25 Sandy Huang 2017-09-02 344 struct rockchip_lvds *lvds =
dev_get_drvdata(dev);
34cc0aa25 Sandy Huang 2017-09-02 345 struct drm_device *drm_dev = data;
34cc0aa25 Sandy Huang 2017-09-02 346 struct drm_encoder *encoder;
34cc0aa25 Sandy Huang 2017-09-02 347 struct drm_connector *connector;
34cc0aa25 Sandy Huang 2017-09-02 348 struct device_node *remote = NULL;
34cc0aa25 Sandy Huang 2017-09-02 349 struct device_node *port, *endpoint;
6bf2e0324 Sean Paul 2017-09-20 350 int ret = 0, child_count = 0;
34cc0aa25 Sandy Huang 2017-09-02 351 const char *name;
34cc0aa25 Sandy Huang 2017-09-02 352 u32 endpoint_id;
34cc0aa25 Sandy Huang 2017-09-02 353
34cc0aa25 Sandy Huang 2017-09-02 354 lvds->drm_dev = drm_dev;
34cc0aa25 Sandy Huang 2017-09-02 355 port =
of_graph_get_port_by_id(dev->of_node, 1);
34cc0aa25 Sandy Huang 2017-09-02 356 if (!port) {
34cc0aa25 Sandy Huang 2017-09-02 357 DRM_DEV_ERROR(dev,
34cc0aa25 Sandy Huang 2017-09-02 358 "can't found port
point, please init lvds panel port!\n");
34cc0aa25 Sandy Huang 2017-09-02 359 return -EINVAL;
34cc0aa25 Sandy Huang 2017-09-02 360 }
34cc0aa25 Sandy Huang 2017-09-02 361 for_each_child_of_node(port, endpoint) {
6bf2e0324 Sean Paul 2017-09-20 362 child_count++;
34cc0aa25 Sandy Huang 2017-09-02 363 of_property_read_u32(endpoint,
"reg", &endpoint_id);
34cc0aa25 Sandy Huang 2017-09-02 364 ret =
drm_of_find_panel_or_bridge(dev->of_node, 1, endpoint_id,
34cc0aa25 Sandy Huang 2017-09-02 365
&lvds->panel, &lvds->bridge);
34cc0aa25 Sandy Huang 2017-09-02 366 if (!ret)
34cc0aa25 Sandy Huang 2017-09-02 367 break;
34cc0aa25 Sandy Huang 2017-09-02 368 }
6bf2e0324 Sean Paul 2017-09-20 369 if (!child_count) {
6bf2e0324 Sean Paul 2017-09-20 370 DRM_DEV_ERROR(dev, "lvds port
does not have any children\n");
6bf2e0324 Sean Paul 2017-09-20 371 ret = -EINVAL;
6bf2e0324 Sean Paul 2017-09-20 372 goto err_put_port;
6bf2e0324 Sean Paul 2017-09-20 373 } else if (ret) {
34cc0aa25 Sandy Huang 2017-09-02 374 DRM_DEV_ERROR(dev, "failed to
find panel and bridge node\n");
34cc0aa25 Sandy Huang 2017-09-02 375 ret = -EPROBE_DEFER;
34cc0aa25 Sandy Huang 2017-09-02 376 goto err_put_port;
34cc0aa25 Sandy Huang 2017-09-02 377 }
34cc0aa25 Sandy Huang 2017-09-02 378 if (lvds->panel)
34cc0aa25 Sandy Huang 2017-09-02 379 remote =
lvds->panel->dev->of_node;
34cc0aa25 Sandy Huang 2017-09-02 380 else
34cc0aa25 Sandy Huang 2017-09-02 @381 remote = lvds->bridge->of_node;
34cc0aa25 Sandy Huang 2017-09-02 382 if
(of_property_read_string(dev->of_node, "rockchip,output", &name))
34cc0aa25 Sandy Huang 2017-09-02 383 /* default set it as output rgb
*/
34cc0aa25 Sandy Huang 2017-09-02 384 lvds->output =
DISPLAY_OUTPUT_RGB;
34cc0aa25 Sandy Huang 2017-09-02 385 else
34cc0aa25 Sandy Huang 2017-09-02 386 lvds->output =
lvds_name_to_output(name);
34cc0aa25 Sandy Huang 2017-09-02 387
34cc0aa25 Sandy Huang 2017-09-02 388 if (lvds->output < 0) {
34cc0aa25 Sandy Huang 2017-09-02 389 DRM_DEV_ERROR(dev, "invalid
output type [%s]\n", name);
34cc0aa25 Sandy Huang 2017-09-02 390 ret
Re: [PATCH 06/61] crypto: simplify getting .drvdata
On Thu, Apr 19, 2018 at 04:05:36PM +0200, Wolfram Sang wrote: > We should get drvdata from struct device directly. Going via > platform_device is an unneeded step back and forth. > > Signed-off-by: Wolfram Sang > --- > > Build tested only. buildbot is happy. Please apply individually. > > drivers/crypto/exynos-rng.c | 6 ++ > drivers/crypto/picoxcell_crypto.c | 6 ++ > 2 files changed, 4 insertions(+), 8 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] v4l: vsp1: Fix vsp1_regs.h license header
Hi Laurent, Thanks for your patch. On 2018-04-28 00:46:47 +0300, Laurent Pinchart wrote: > All source files of the vsp1 driver are licensed under the GPLv2+ except > for vsp1_regs.h which is licensed under GPLv2. This is caused by a bad > copy&paste that dates back from the initial version of the driver. Fix > it. > > Cc: Nobuhiro Iwamatsu > Cc: Kieran Bingham > Cc: Sergei Shtylyov > Cc: Niklas Söderlund > Cc: Wolfram Sang > Signed-off-by: Laurent Pinchart Acked-by: Niklas Söderlund -- Regards, Niklas Söderlund
Re: [PATCH] v4l: vsp1: Fix vsp1_regs.h license header
Hello! On 4/28/2018 12:46 AM, Laurent Pinchart wrote: All source files of the vsp1 driver are licensed under the GPLv2+ except for vsp1_regs.h which is licensed under GPLv2. This is caused by a bad copy&paste that dates back from the initial version of the driver. Fix it. Cc: Nobuhiro Iwamatsu Cc: Kieran Bingham Cc: Sergei Shtylyov Cc: Niklas Söderlund Cc: Wolfram Sang Signed-off-by: Laurent Pinchart [...] Acked-by: Sergei Shtylyov MBR, Sergei
Re: [PATCH v2 2/8] v4l: vsp1: Share the CLU, LIF and LUT set_fmt pad operation code
Hi Laurent,
very minor comments below
On Mon, Apr 23, 2018 at 01:34:24AM +0300, Laurent Pinchart wrote:
> The implementation of the set_fmt pad operation is identical in the
> three modules. Move it to a generic helper function.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/media/platform/vsp1/vsp1_clu.c| 65 +--
> drivers/media/platform/vsp1/vsp1_entity.c | 75
> +++
> drivers/media/platform/vsp1/vsp1_entity.h | 6 +++
> drivers/media/platform/vsp1/vsp1_lif.c| 65 +--
> drivers/media/platform/vsp1/vsp1_lut.c| 65 +--
> 5 files changed, 116 insertions(+), 160 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_clu.c
> b/drivers/media/platform/vsp1/vsp1_clu.c
> index 9626b6308585..96a448e1504c 100644
> --- a/drivers/media/platform/vsp1/vsp1_clu.c
> +++ b/drivers/media/platform/vsp1/vsp1_clu.c
> @@ -114,18 +114,18 @@ static const struct v4l2_ctrl_config clu_mode_control =
> {
> * V4L2 Subdevice Pad Operations
> */
>
> +static const unsigned int clu_codes[] = {
> + MEDIA_BUS_FMT_ARGB_1X32,
> + MEDIA_BUS_FMT_AHSV_1X32,
> + MEDIA_BUS_FMT_AYUV8_1X32,
> +};
> +
> static int clu_enum_mbus_code(struct v4l2_subdev *subdev,
> struct v4l2_subdev_pad_config *cfg,
> struct v4l2_subdev_mbus_code_enum *code)
> {
> - static const unsigned int codes[] = {
> - MEDIA_BUS_FMT_ARGB_1X32,
> - MEDIA_BUS_FMT_AHSV_1X32,
> - MEDIA_BUS_FMT_AYUV8_1X32,
> - };
> -
> - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes,
> - ARRAY_SIZE(codes));
> + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, clu_codes,
> + ARRAY_SIZE(clu_codes));
> }
>
> static int clu_enum_frame_size(struct v4l2_subdev *subdev,
> @@ -141,51 +141,10 @@ static int clu_set_format(struct v4l2_subdev *subdev,
> struct v4l2_subdev_pad_config *cfg,
> struct v4l2_subdev_format *fmt)
> {
> - struct vsp1_clu *clu = to_clu(subdev);
> - struct v4l2_subdev_pad_config *config;
> - struct v4l2_mbus_framefmt *format;
> - int ret = 0;
> -
> - mutex_lock(&clu->entity.lock);
> -
> - config = vsp1_entity_get_pad_config(&clu->entity, cfg, fmt->which);
> - if (!config) {
> - ret = -EINVAL;
> - goto done;
> - }
> -
> - /* Default to YUV if the requested format is not supported. */
> - if (fmt->format.code != MEDIA_BUS_FMT_ARGB_1X32 &&
> - fmt->format.code != MEDIA_BUS_FMT_AHSV_1X32 &&
> - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32)
> - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32;
The newly implemented vsp1_subdev_set_pad_format defaults to the first
clu_codes[] member (ARGB888_1x32), while here the code chose the AYUV8_1x32
format. Is it ok? Should you revers the clu_codes[] order?
> -
> - format = vsp1_entity_get_pad_format(&clu->entity, config, fmt->pad);
> -
> - if (fmt->pad == CLU_PAD_SOURCE) {
> - /* The CLU output format can't be modified. */
> - fmt->format = *format;
> - goto done;
> - }
> -
> - format->code = fmt->format.code;
> - format->width = clamp_t(unsigned int, fmt->format.width,
> - CLU_MIN_SIZE, CLU_MAX_SIZE);
> - format->height = clamp_t(unsigned int, fmt->format.height,
> - CLU_MIN_SIZE, CLU_MAX_SIZE);
> - format->field = V4L2_FIELD_NONE;
> - format->colorspace = V4L2_COLORSPACE_SRGB;
> -
> - fmt->format = *format;
> -
> - /* Propagate the format to the source pad. */
> - format = vsp1_entity_get_pad_format(&clu->entity, config,
> - CLU_PAD_SOURCE);
> - *format = fmt->format;
> -
> -done:
> - mutex_unlock(&clu->entity.lock);
> - return ret;
> + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, clu_codes,
> + ARRAY_SIZE(clu_codes),
> + CLU_MIN_SIZE, CLU_MIN_SIZE,
> + CLU_MAX_SIZE, CLU_MAX_SIZE);
> }
>
> /*
> -
> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c
> b/drivers/media/platform/vsp1/vsp1_entity.c
> index 72354caf5746..239df047efd0 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.c
> +++ b/drivers/media/platform/vsp1/vsp1_entity.c
> @@ -307,6 +307,81 @@ int vsp1_subdev_enum_frame_size(struct v4l2_subdev
> *subdev,
> return ret;
> }
>
> +/*
> + * vsp1_subdev_set_pad_format - Subdev pad set_fmt handler
> + * @subdev: V4L2 subdevice
> + * @cfg: V4L2 subdev pad configuration
> + * @fmt: V4L2 subdev format
> + * @codes: Array of supported media bus co
Re: [PATCH v2 3/8] v4l: vsp1: Reset the crop and compose rectangles in the set_fmt helper
Hi Laurent,
On Mon, Apr 23, 2018 at 01:34:25AM +0300, Laurent Pinchart wrote:
> To make vsp1_subdev_set_pad_format() usable by entities that support
> selection rectangles, we need to reset the crop and compose rectangles
> when setting the format on the sink pad. Do so and replace the custom
> set_fmt implementation of the histogram code by a call to
> vsp1_subdev_set_pad_format().
>
> Resetting the crop and compose rectangles for entities that don't
> support crop and compose has no adverse effect as the rectangles are
> ignored anyway.
>
> Signed-off-by: Laurent Pinchart
Reviewed-by: Jacopo Mondi
> ---
> drivers/media/platform/vsp1/vsp1_entity.c | 16 +
> drivers/media/platform/vsp1/vsp1_histo.c | 59
> +++
> 2 files changed, 20 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c
> b/drivers/media/platform/vsp1/vsp1_entity.c
> index 239df047efd0..181a583aecad 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.c
> +++ b/drivers/media/platform/vsp1/vsp1_entity.c
> @@ -335,6 +335,7 @@ int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev,
> struct vsp1_entity *entity = to_vsp1_entity(subdev);
> struct v4l2_subdev_pad_config *config;
> struct v4l2_mbus_framefmt *format;
> + struct v4l2_rect *selection;
> unsigned int i;
> int ret = 0;
>
> @@ -377,6 +378,21 @@ int vsp1_subdev_set_pad_format(struct v4l2_subdev
> *subdev,
> format = vsp1_entity_get_pad_format(entity, config, 1);
> *format = fmt->format;
>
> + /* Reset the crop and compose rectangles */
> + selection = vsp1_entity_get_pad_selection(entity, config, fmt->pad,
> + V4L2_SEL_TGT_CROP);
> + selection->left = 0;
> + selection->top = 0;
> + selection->width = format->width;
> + selection->height = format->height;
> +
> + selection = vsp1_entity_get_pad_selection(entity, config, fmt->pad,
> + V4L2_SEL_TGT_COMPOSE);
> + selection->left = 0;
> + selection->top = 0;
> + selection->width = format->width;
> + selection->height = format->height;
> +
> done:
> mutex_unlock(&entity->lock);
> return ret;
> diff --git a/drivers/media/platform/vsp1/vsp1_histo.c
> b/drivers/media/platform/vsp1/vsp1_histo.c
> index 029181c1fb61..5e15c8ff88d9 100644
> --- a/drivers/media/platform/vsp1/vsp1_histo.c
> +++ b/drivers/media/platform/vsp1/vsp1_histo.c
> @@ -389,65 +389,14 @@ static int histo_set_format(struct v4l2_subdev *subdev,
> struct v4l2_subdev_format *fmt)
> {
> struct vsp1_histogram *histo = subdev_to_histo(subdev);
> - struct v4l2_subdev_pad_config *config;
> - struct v4l2_mbus_framefmt *format;
> - struct v4l2_rect *selection;
> - unsigned int i;
> - int ret = 0;
>
> if (fmt->pad != HISTO_PAD_SINK)
> return histo_get_format(subdev, cfg, fmt);
>
> - mutex_lock(&histo->entity.lock);
> -
> - config = vsp1_entity_get_pad_config(&histo->entity, cfg, fmt->which);
> - if (!config) {
> - ret = -EINVAL;
> - goto done;
> - }
> -
> - /*
> - * Default to the first format if the requested format is not
> - * supported.
> - */
> - for (i = 0; i < histo->num_formats; ++i) {
> - if (fmt->format.code == histo->formats[i])
> - break;
> - }
> - if (i == histo->num_formats)
> - fmt->format.code = histo->formats[0];
> -
> - format = vsp1_entity_get_pad_format(&histo->entity, config, fmt->pad);
> -
> - format->code = fmt->format.code;
> - format->width = clamp_t(unsigned int, fmt->format.width,
> - HISTO_MIN_SIZE, HISTO_MAX_SIZE);
> - format->height = clamp_t(unsigned int, fmt->format.height,
> - HISTO_MIN_SIZE, HISTO_MAX_SIZE);
> - format->field = V4L2_FIELD_NONE;
> - format->colorspace = V4L2_COLORSPACE_SRGB;
> -
> - fmt->format = *format;
> -
> - /* Reset the crop and compose rectangles */
> - selection = vsp1_entity_get_pad_selection(&histo->entity, config,
> - fmt->pad, V4L2_SEL_TGT_CROP);
> - selection->left = 0;
> - selection->top = 0;
> - selection->width = format->width;
> - selection->height = format->height;
> -
> - selection = vsp1_entity_get_pad_selection(&histo->entity, config,
> - fmt->pad,
> - V4L2_SEL_TGT_COMPOSE);
> - selection->left = 0;
> - selection->top = 0;
> - selection->width = format->width;
> - selection->height = format->height;
> -
> -done:
> - mutex_unlock(&histo->entity.lock);
> - return ret;
> + return vsp1_subdev_set_pad_format(subdev, cfg, fmt,
> + histo->formats, histo->n
Re: [PATCH v2 5/8] v4l: vsp1: Extend the DU API to support CRC computation
Hi Laurent,
just one minor comment below
On Mon, Apr 23, 2018 at 01:34:27AM +0300, Laurent Pinchart wrote:
> Add a parameter (in the form of a structure to ease future API
> extensions) to the VSP atomic flush handler to pass CRC source
> configuration, and pass the CRC value to the completion callback.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 6 --
> drivers/media/platform/vsp1/vsp1_drm.c | 6 --
> drivers/media/platform/vsp1/vsp1_drm.h | 2 +-
> include/media/vsp1.h | 29 +++--
> 4 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index 2c260c33840b..bdcec201591f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -31,7 +31,7 @@
> #include "rcar_du_kms.h"
> #include "rcar_du_vsp.h"
>
> -static void rcar_du_vsp_complete(void *private, bool completed)
> +static void rcar_du_vsp_complete(void *private, bool completed, u32 crc)
> {
> struct rcar_du_crtc *crtc = private;
>
> @@ -102,7 +102,9 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
>
> void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
> {
> - vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe);
> + struct vsp1_du_atomic_pipe_config cfg = { { 0, } };
> +
> + vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
> }
>
> /* Keep the two tables in sync. */
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index 2b29a83dceb9..5fc31578f9b0 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -36,7 +36,7 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline
> *pipe,
> bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;
>
> if (drm_pipe->du_complete)
> - drm_pipe->du_complete(drm_pipe->du_private, complete);
> + drm_pipe->du_complete(drm_pipe->du_private, complete, 0);
>
> if (completion & VSP1_DL_FRAME_END_INTERNAL) {
> drm_pipe->force_brx_release = false;
> @@ -739,8 +739,10 @@ EXPORT_SYMBOL_GPL(vsp1_du_atomic_update);
> * vsp1_du_atomic_flush - Commit an atomic update
> * @dev: the VSP device
> * @pipe_index: the DRM pipeline index
> + * @cfg: atomic pipe configuration
> */
> -void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index)
> +void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
> + const struct vsp1_du_atomic_pipe_config *cfg)
> {
> struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.h
> b/drivers/media/platform/vsp1/vsp1_drm.h
> index f4af1b2b12d6..e5b88b28806c 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.h
> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> @@ -35,7 +35,7 @@ struct vsp1_drm_pipeline {
> wait_queue_head_t wait_queue;
>
> /* Frame synchronisation */
> - void (*du_complete)(void *, bool);
> + void (*du_complete)(void *data, bool completed, u32 crc);
> void *du_private;
> };
>
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index ff7ef894465d..ac63a9928a79 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -34,7 +34,7 @@ struct vsp1_du_lif_config {
> unsigned int width;
> unsigned int height;
>
> - void (*callback)(void *, bool);
> + void (*callback)(void *data, bool completed, u32 crc);
> void *callback_data;
> };
>
> @@ -61,11 +61,36 @@ struct vsp1_du_atomic_config {
> unsigned int zpos;
> };
>
> +/**
> + * enum vsp1_du_crc_source - Source used for CRC calculation
> + * @VSP1_DU_CRC_NONE: CRC calculation disabled
> + * @VSP_DU_CRC_PLANE: Perform CRC calculation on an input plane
> + * @VSP_DU_CRC_OUTPUT: Perform CRC calculation on the composed output
These two paramters are called VSP1_DU_CRC_* not VSP_DU_CRC_*
> + */
> +enum vsp1_du_crc_source {
> + VSP1_DU_CRC_NONE,
> + VSP1_DU_CRC_PLANE,
> + VSP1_DU_CRC_OUTPUT,
> +};
> +
> +/**
> + * struct vsp1_du_atomic_pipe_config - VSP atomic pipe configuration
> parameters
> + * @crc.source: source for CRC calculation
> + * @crc.index: index of the CRC source plane (when crc.source is set to
> plane)
> + */
> +struct vsp1_du_atomic_pipe_config {
> + struct {
> + enum vsp1_du_crc_source source;
> + unsigned int index;
> + } crc;
> +};
> +
> void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index);
> int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index,
> unsigned int rpf,
> const struct vsp1_du_atomic_config *cfg);
> -void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index);
> +void vsp1_du_a
Re: [PATCH v2 6/8] v4l: vsp1: Add support for the DISCOM entity
HI Laurent,
a few comments, mostly minor ones...
On Mon, Apr 23, 2018 at 01:34:28AM +0300, Laurent Pinchart wrote:
> The DISCOM calculates a CRC on a configurable window of the frame. It
> interfaces to the VSP through the UIF glue, hence the name used in the
> code.
>
> The module supports configuration of the CRC window through the crop
> rectangle on the ink pad of the corresponding entity. However, unlike
sink pad?
> the traditional V4L2 subdevice model, the crop rectangle does not
> influence the format on the source pad.
>
> Modeling the DISCOM as a sink-only entity would allow adhering to the
> V4L2 subdevice model at the expense of more complex code in the driver,
> as at the hardware level the UIF is handled as a sink+source entity. As
> the DISCOM is only present in R-Car Gen3 VSP-D and VSP-DL instances it
> is not exposed to userspace through V4L2 but controlled through the DU
> driver. We can thus change this model later if needed without fear of
> affecting userspace.
>
> Signed-off-by: Laurent Pinchart
> ---
> Changes since v1:
>
> - Don't return uninitialized value from uif_set_selection()
> ---
> drivers/media/platform/vsp1/Makefile | 2 +-
> drivers/media/platform/vsp1/vsp1.h| 4 +
> drivers/media/platform/vsp1/vsp1_drv.c| 20 +++
> drivers/media/platform/vsp1/vsp1_entity.c | 6 +
> drivers/media/platform/vsp1/vsp1_entity.h | 1 +
> drivers/media/platform/vsp1/vsp1_regs.h | 41 +
> drivers/media/platform/vsp1/vsp1_uif.c| 271
> ++
> drivers/media/platform/vsp1/vsp1_uif.h| 32
> 8 files changed, 376 insertions(+), 1 deletion(-)
> create mode 100644 drivers/media/platform/vsp1/vsp1_uif.c
> create mode 100644 drivers/media/platform/vsp1/vsp1_uif.h
>
> diff --git a/drivers/media/platform/vsp1/Makefile
> b/drivers/media/platform/vsp1/Makefile
> index 596775f932c0..4bb4dcbef7b5 100644
> --- a/drivers/media/platform/vsp1/Makefile
> +++ b/drivers/media/platform/vsp1/Makefile
> @@ -5,6 +5,6 @@ vsp1-y+= vsp1_rpf.o
> vsp1_rwpf.o vsp1_wpf.o
> vsp1-y += vsp1_clu.o vsp1_hsit.o
> vsp1_lut.o
> vsp1-y += vsp1_brx.o vsp1_sru.o
> vsp1_uds.o
> vsp1-y += vsp1_hgo.o vsp1_hgt.o
> vsp1_histo.o
> -vsp1-y += vsp1_lif.o
> +vsp1-y += vsp1_lif.o vsp1_uif.o
>
> obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1.o
> diff --git a/drivers/media/platform/vsp1/vsp1.h
> b/drivers/media/platform/vsp1/vsp1.h
> index 9cf4e1c4b036..33f632331474 100644
> --- a/drivers/media/platform/vsp1/vsp1.h
> +++ b/drivers/media/platform/vsp1/vsp1.h
> @@ -36,10 +36,12 @@ struct vsp1_lut;
> struct vsp1_rwpf;
> struct vsp1_sru;
> struct vsp1_uds;
> +struct vsp1_uif;
>
> #define VSP1_MAX_LIF 2
> #define VSP1_MAX_RPF 5
> #define VSP1_MAX_UDS 3
> +#define VSP1_MAX_UIF 2
> #define VSP1_MAX_WPF 4
>
> #define VSP1_HAS_LUT (1 << 1)
> @@ -60,6 +62,7 @@ struct vsp1_device_info {
> unsigned int lif_count;
> unsigned int rpf_count;
> unsigned int uds_count;
> + unsigned int uif_count;
> unsigned int wpf_count;
> unsigned int num_bru_inputs;
> bool uapi;
> @@ -86,6 +89,7 @@ struct vsp1_device {
> struct vsp1_rwpf *rpf[VSP1_MAX_RPF];
> struct vsp1_sru *sru;
> struct vsp1_uds *uds[VSP1_MAX_UDS];
> + struct vsp1_uif *uif[VSP1_MAX_UIF];
> struct vsp1_rwpf *wpf[VSP1_MAX_WPF];
>
> struct list_head entities;
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> b/drivers/media/platform/vsp1/vsp1_drv.c
> index 331a2e0af0d3..d29f9c4baebe 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -35,6 +35,7 @@
> #include "vsp1_rwpf.h"
> #include "vsp1_sru.h"
> #include "vsp1_uds.h"
> +#include "vsp1_uif.h"
> #include "vsp1_video.h"
>
> /*
> -
> @@ -409,6 +410,19 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
> list_add_tail(&uds->entity.list_dev, &vsp1->entities);
> }
>
> + for (i = 0; i < vsp1->info->uif_count; ++i) {
> + struct vsp1_uif *uif;
> +
> + uif = vsp1_uif_create(vsp1, i);
> + if (IS_ERR(uif)) {
> + ret = PTR_ERR(uif);
> + goto done;
> + }
> +
> + vsp1->uif[i] = uif;
> + list_add_tail(&uif->entity.list_dev, &vsp1->entities);
> + }
> +
> for (i = 0; i < vsp1->info->wpf_count; ++i) {
> struct vsp1_rwpf *wpf;
>
> @@ -513,6 +527,9 @@ static int vsp1_device_init(struct vsp1_device *vsp1)
> for (i = 0; i < vsp1->info->uds_count; ++i)
> vsp1_write(vsp1, VI6_DPR_UDS_ROUTE(i), VI6_DPR_NODE_UNUSED);
>
>
Re: [PATCH v2 7/8] v4l: vsp1: Integrate DISCOM in display pipeline
HI Laurent,
On Mon, Apr 23, 2018 at 01:34:29AM +0300, Laurent Pinchart wrote:
> The DISCOM is used to compute CRCs on display frames. Integrate it in
> the display pipeline at the output of the blending unit to process
> output frames.
>
> Computing CRCs on input frames is possible by positioning the DISCOM at
> a different point in the pipeline. This use case isn't supported at the
> moment and could be implemented by extending the API between the VSP1
> and DU drivers if needed.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/media/platform/vsp1/vsp1_drm.c | 115
> -
> drivers/media/platform/vsp1/vsp1_drm.h | 12
> 2 files changed, 124 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index 5fc31578f9b0..7864b43a90e1 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -22,6 +22,7 @@
> #include "vsp1_lif.h"
> #include "vsp1_pipe.h"
> #include "vsp1_rwpf.h"
> +#include "vsp1_uif.h"
>
> #define BRX_NAME(e) (e)->type == VSP1_ENTITY_BRU ? "BRU" : "BRS"
>
> @@ -35,8 +36,13 @@ static void vsp1_du_pipeline_frame_end(struct
> vsp1_pipeline *pipe,
> struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
> bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;
>
> - if (drm_pipe->du_complete)
> - drm_pipe->du_complete(drm_pipe->du_private, complete, 0);
> + if (drm_pipe->du_complete) {
> + struct vsp1_entity *uif = drm_pipe->uif;
> + u32 crc;
> +
> + crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
> + drm_pipe->du_complete(drm_pipe->du_private, complete, crc);
> + }
>
> if (completion & VSP1_DL_FRAME_END_INTERNAL) {
> drm_pipe->force_brx_release = false;
> @@ -48,10 +54,65 @@ static void vsp1_du_pipeline_frame_end(struct
> vsp1_pipeline *pipe,
> * Pipeline Configuration
> */
>
> +/*
> + * Insert the UIF in the pipeline between the prev and next entities. If no
> UIF
> + * is available connect the two entities directly.
> + */
> +static int vsp1_du_insert_uif(struct vsp1_device *vsp1,
> + struct vsp1_pipeline *pipe,
> + struct vsp1_entity *uif,
> + struct vsp1_entity *prev, unsigned int prev_pad,
> + struct vsp1_entity *next, unsigned int next_pad)
> +{
> + int ret;
> +
> + if (uif) {
> + struct v4l2_subdev_format format;
> +
> + prev->sink = uif;
> + prev->sink_pad = UIF_PAD_SINK;
> +
> + memset(&format, 0, sizeof(format));
> + format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + format.pad = prev_pad;
> +
> + ret = v4l2_subdev_call(&prev->subdev, pad, get_fmt, NULL,
> +&format);
> + if (ret < 0)
> + return ret;
> +
> + format.pad = UIF_PAD_SINK;
> +
> + ret = v4l2_subdev_call(&uif->subdev, pad, set_fmt, NULL,
> +&format);
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on UIF sink\n",
> + __func__, format.format.width, format.format.height,
> + format.format.code);
> +
> + /*
> + * The UIF doesn't mangle the format between its sink and
> + * source pads, so there is no need to retrieve the format on
> + * its source pad.
> + */
> +
> + uif->sink = next;
> + uif->sink_pad = next_pad;
> + } else {
> + prev->sink = next;
> + prev->sink_pad = next_pad;
Isn't the !uif case better handled in the caller? See below...
Or otherwise, if you prefer handling it here, shouldn't the
indentation be reduced with
if (!uif) {
prev->sink = next;
prev->sink_pad = next_pad;
return 0;
}
> + }
> +
> + return 0;
> +}
> +
> /* Setup one RPF and the connected BRx sink pad. */
> static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1,
> struct vsp1_pipeline *pipe,
> struct vsp1_rwpf *rpf,
> + struct vsp1_entity *uif,
> unsigned int brx_input)
> {
> struct v4l2_subdev_selection sel;
> @@ -122,6 +183,12 @@ static int vsp1_du_pipeline_setup_rpf(struct vsp1_device
> *vsp1,
> if (ret < 0)
> return ret;
>
> + /* Insert and configure the UIF if available. */
> + ret = vsp1_du_insert_uif(vsp1, pipe, uif, &rpf->entity, RWPF_PAD_SOURCE,
> + pipe->brx, brx_input);
> + if (ret < 0)
> + retur
Re: [PATCH v14 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver
Hi Niklas, apart from a small comment, as my comments on v13 have been clarified Reviewed-by: Jacopo Mondi On Thu, Apr 26, 2018 at 10:21:21PM +0200, Niklas Söderlund wrote: > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are > connected between the video sources and the video grabbers (VIN). > > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP. > > Signed-off-by: Niklas Söderlund > Reviewed-by: Hans Verkuil > Reviewed-by: Maxime Ripard > Reviewed-by: Laurent Pinchart > > --- > > * Changes since v13 > - Change return rcar_csi2_formats + i to return &rcar_csi2_formats[i]. > - Add define for PHCLM_STOPSTATECKL. > - Update spelling in comments. > - Update calculation in rcar_csi2_calc_phypll() according to > https://linuxtv.org/downloads/v4l-dvb-apis/kapi/csi2.html. The one > before v14 did not take into account that 2 bits per sample is > transmitted. > - Use Geert's suggestion of (1 << priv->lanes) - 1 instead of switch > statement to set correct number of lanes to enable. > - Change hex constants in hsfreqrange_m3w_h3es1[] to lower case to match > style of rest of file. > - Switch to %u instead of 0x%x when printing bus type. > - Switch to %u instead of %d for priv->lanes which is unsigned. > - Add MEDIA_BUS_FMT_YUYV8_1X16 to the list of supported formats in > rcar_csi2_formats[]. > - Fixed bps for MEDIA_BUS_FMT_YUYV10_2X10 to 20 and not 16. > - Set INTSTATE after PL-11 is confirmed to match flow chart in > datasheet. > - Change priv->notifier.subdevs == NULL to !priv->notifier.subdevs. > - Add Maxime's and laurent's tags. > --- > drivers/media/platform/rcar-vin/Kconfig | 12 + > drivers/media/platform/rcar-vin/Makefile| 1 + > drivers/media/platform/rcar-vin/rcar-csi2.c | 883 > 3 files changed, 896 insertions(+) > create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c > > diff --git a/drivers/media/platform/rcar-vin/Kconfig > b/drivers/media/platform/rcar-vin/Kconfig > index 8fa7ee468c63afb9..3dfeb91f8f186528 100644 > --- a/drivers/media/platform/rcar-vin/Kconfig > +++ b/drivers/media/platform/rcar-vin/Kconfig > @@ -1,3 +1,15 @@ > +config VIDEO_RCAR_CSI2 > + tristate "R-Car MIPI CSI-2 Receiver" > + depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF > + depends on ARCH_RENESAS || COMPILE_TEST > + select V4L2_FWNODE > + ---help--- > + Support for Renesas R-Car MIPI CSI-2 receiver. > + Supports R-Car Gen3 SoCs. > + > + To compile this driver as a module, choose M here: the > + module will be called rcar-csi2. > + > config VIDEO_RCAR_VIN > tristate "R-Car Video Input (VIN) Driver" > depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF && HAS_DMA && > MEDIA_CONTROLLER > diff --git a/drivers/media/platform/rcar-vin/Makefile > b/drivers/media/platform/rcar-vin/Makefile > index 48c5632c21dc060b..5ab803d3e7c1aa57 100644 > --- a/drivers/media/platform/rcar-vin/Makefile > +++ b/drivers/media/platform/rcar-vin/Makefile > @@ -1,3 +1,4 @@ > rcar-vin-objs = rcar-core.o rcar-dma.o rcar-v4l2.o > > +obj-$(CONFIG_VIDEO_RCAR_CSI2) += rcar-csi2.o > obj-$(CONFIG_VIDEO_RCAR_VIN) += rcar-vin.o > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c > b/drivers/media/platform/rcar-vin/rcar-csi2.c > new file mode 100644 > index ..49b29d5680f9d80b > --- /dev/null > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > @@ -0,0 +1,883 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Driver for Renesas R-Car MIPI CSI-2 Receiver > + * > + * Copyright (C) 2018 Renesas Electronics Corp. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > + > +/* Register offsets and bits */ > + > +/* Control Timing Select */ > +#define TREF_REG 0x00 > +#define TREF_TREFBIT(0) > + > +/* Software Reset */ > +#define SRST_REG 0x04 > +#define SRST_SRSTBIT(0) > + > +/* PHY Operation Control */ > +#define PHYCNT_REG 0x08 > +#define PHYCNT_SHUTDOWNZ BIT(17) > +#define PHYCNT_RSTZ BIT(16) > +#define PHYCNT_ENABLECLK BIT(4) > +#define PHYCNT_ENABLE_3 BIT(3) > +#define PHYCNT_ENABLE_2 BIT(2) > +#define PHYCNT_ENABLE_1 BIT(1) > +#define PHYCNT_ENABLE_0 BIT(0) > + > +/* Checksum Control */ > +#define CHKSUM_REG 0x0c > +#define CHKSUM_ECC_ENBIT(1) > +#define CHKSUM_CRC_ENBIT(0) > + > +/* > + * Channel Data Type Select > + * VCDT[0-15]: Channel 1 VCDT[16-31]: Channel 2 > + * VCDT2[0-15]: Channel 3 VCDT2[16-31]: Channel 4 > + */ > +#define VCDT_REG 0x1
Re: [PATCH v14 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver
Hi Jacopo,
Thanks for your feedback.
On 2018-04-28 13:28:27 +0200, Jacopo Mondi wrote:
> Hi Niklas,
>apart from a small comment, as my comments on v13 have been
>clarified
>
> Reviewed-by: Jacopo Mondi
Thanks!
[snip]
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > new file mode 100644
> > index ..49b29d5680f9d80b
> > --- /dev/null
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -0,0 +1,883 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for Renesas R-Car MIPI CSI-2 Receiver
> > + *
> > + * Copyright (C) 2018 Renesas Electronics Corp.
> > + */
[snip]
> > +MODULE_AUTHOR("Niklas Söderlund ");
> > +MODULE_DESCRIPTION("Renesas R-Car MIPI CSI-2 receiver");
> > +MODULE_LICENSE("GPL");
>
> This doesn't match the SPDX header that reports GPL-2.0
I'm now officially more confused then normal :-) I really tried to get
this right and the combination I use here
// SPDX-License-Identifier: GPL-2.0
MODULE_LICENSE("GPL");
Seems to be used all over the kernel, did some digging on the master
branch of the media tree from a few days ago and found 265 files with
this combination using this script:
count=0
for f in $(git grep -l "SPDX-License-Identifier: GPL-2.0$"); do
if grep -q 'MODULE_LICENSE("GPL")' $f; then
echo $f
grep SPDX-License-Identifier $f
grep MODULE_LICENSE $f;
count=$(($count + 1))
fi
done
echo "Count: $count"
I'm happy to post a new version of this series to make this right but
I'm afraid that I at this point know what right is. My intention is to
replace a licence text found in an old Renesas BSP which this work is
loosely based on:
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
So it's quiet clear it's GPL-2.0 and not GPL-2.0+ and AFIK what I have done
here is correct, please tell me why I'm wrong and how I can correct it :-)
--
Regards,
Niklas Söderlund
Re: [PATCH v14 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver
Hi Niklas,
On Sat, Apr 28, 2018 at 03:31:14PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2018-04-28 13:28:27 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >apart from a small comment, as my comments on v13 have been
> >clarified
> >
> > Reviewed-by: Jacopo Mondi
>
> Thanks!
>
> [snip]
>
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > new file mode 100644
> > > index ..49b29d5680f9d80b
> > > --- /dev/null
> > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > @@ -0,0 +1,883 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Driver for Renesas R-Car MIPI CSI-2 Receiver
> > > + *
> > > + * Copyright (C) 2018 Renesas Electronics Corp.
> > > + */
>
> [snip]
>
> > > +MODULE_AUTHOR("Niklas Söderlund ");
> > > +MODULE_DESCRIPTION("Renesas R-Car MIPI CSI-2 receiver");
> > > +MODULE_LICENSE("GPL");
> >
> > This doesn't match the SPDX header that reports GPL-2.0
>
> I'm now officially more confused then normal :-) I really tried to get
> this right and the combination I use here
>
> // SPDX-License-Identifier: GPL-2.0
> MODULE_LICENSE("GPL");
>
> Seems to be used all over the kernel, did some digging on the master
> branch of the media tree from a few days ago and found 265 files with
> this combination using this script:
>
> count=0
> for f in $(git grep -l "SPDX-License-Identifier: GPL-2.0$"); do
> if grep -q 'MODULE_LICENSE("GPL")' $f; then
> echo $f
> grep SPDX-License-Identifier $f
> grep MODULE_LICENSE $f;
> count=$(($count + 1))
> fi
> done
> echo "Count: $count"
>
> I'm happy to post a new version of this series to make this right but
> I'm afraid that I at this point know what right is. My intention is to
> replace a licence text found in an old Renesas BSP which this work is
> loosely based on:
>
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> * published by the Free Software Foundation.
>
> So it's quiet clear it's GPL-2.0 and not GPL-2.0+ and AFIK what I have done
> here is correct, please tell me why I'm wrong and how I can correct it :-)
>
I was just expecting to see "GPL v2" if the SPDX identifier reports
GPL-2.0
Maybe that's not even a thing, anyway, do not waste any time on this,
it a very minor nit.
Thank you
j
> --
> Regards,
> Niklas Söderlund
signature.asc
Description: PGP signature
Re: [PATCH v14 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver
Hi Niklas,
On Friday, 27 April 2018 02:28:32 EEST Niklas Söderlund wrote:
> On 2018-04-27 00:30:25 +0300, Laurent Pinchart wrote:
> > On Thursday, 26 April 2018 23:21:21 EEST Niklas Söderlund wrote:
> >> A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> >> supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
> >> connected between the video sources and the video grabbers (VIN).
> >>
> >> Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
> >>
> >> Signed-off-by: Niklas Söderlund
> >> Reviewed-by: Hans Verkuil
> >> Reviewed-by: Maxime Ripard
> >> Reviewed-by: Laurent Pinchart
> >>
> >> ---
> >>
> >> * Changes since v13
> >> - Change return rcar_csi2_formats + i to return &rcar_csi2_formats[i].
> >> - Add define for PHCLM_STOPSTATECKL.
> >> - Update spelling in comments.
> >> - Update calculation in rcar_csi2_calc_phypll() according to
> >> https://linuxtv.org/downloads/v4l-dvb-apis/kapi/csi2.html. The one
> >> before v14 did not take into account that 2 bits per sample is
> >> transmitted.
> >
> > Just one small comment about this, please see below.
> >
> >> - Use Geert's suggestion of (1 << priv->lanes) - 1 instead of switch
> >> statement to set correct number of lanes to enable.
> >> - Change hex constants in hsfreqrange_m3w_h3es1[] to lower case to match
> >> style of rest of file.
> >> - Switch to %u instead of 0x%x when printing bus type.
> >> - Switch to %u instead of %d for priv->lanes which is unsigned.
> >> - Add MEDIA_BUS_FMT_YUYV8_1X16 to the list of supported formats in
> >> rcar_csi2_formats[].
> >> - Fixed bps for MEDIA_BUS_FMT_YUYV10_2X10 to 20 and not 16.
> >> - Set INTSTATE after PL-11 is confirmed to match flow chart in
> >> datasheet.
> >> - Change priv->notifier.subdevs == NULL to !priv->notifier.subdevs.
> >> - Add Maxime's and laurent's tags.
> >> ---
> >>
> >> drivers/media/platform/rcar-vin/Kconfig | 12 +
> >> drivers/media/platform/rcar-vin/Makefile| 1 +
> >> drivers/media/platform/rcar-vin/rcar-csi2.c | 883
> >> 3 files changed, 896 insertions(+)
> >> create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c
> >
> > [snip]
> >
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644
> >> index ..49b29d5680f9d80b
> >> --- /dev/null
> > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >
> > [snip]
> >
> >> +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv, unsigned int
> >> bpp,
> >> + u32 *phypll)
> >> +{
> >> + const struct phypll_hsfreqrange *hsfreq;
> >> + struct v4l2_subdev *source;
> >> + struct v4l2_ctrl *ctrl;
> >> + u64 mbps;
> >> +
> >> + if (!priv->remote)
> >> + return -ENODEV;
> >> +
> >> + source = priv->remote;
> >> +
> >> + /* Read the pixel rate control from remote */
> >> + ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> >> + if (!ctrl) {
> >> + dev_err(priv->dev, "no pixel rate control in subdev %s\n",
> >> + source->name);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /*
> >> + * Calculate the phypll in mbps (from v4l2 documentation)
> >> + * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes)
> >> + */
> >> + mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> >> + do_div(mbps, priv->lanes * 200);
> >
> > pixel rate * bits per sample will give you the overall bit rate, which you
> > then divide by the number of lanes to get the bitrate per lane, and then
> > by 2 as D-PHY is a DDR PHY and transmits 2 bits per clock cycle. You then
> > end up with the link frequency, which is thus expressed in MHz, not in
> > Mbps. I would thus name the mbps variable freq, and rename the
> > phypll_hsfreqrange mbps field to freq (maybe with a small comment right
> > after the field to tell the value is expressed in MHz).
>
> I agree that freq would be a better name for what it represents. Never
> the less I prefer to stick with mbps as that is whats used in the
> datasheet. See Table '25.9 HSFREQRANGE Bit Set Values'.
>
> With this in mind if you still feel strongly about renaming it I could
> do so. But at lest for me it feels more useful to easily be able to map
> the variable to the datasheet tables :-)
After reading the datasheet I don't care too strongly about a rename, but I
now care about getting it right :-) The datasheet clearly specifies the
maximum data rate to be 1.5 Gbps / lane (section 25.1.1 Features), and figure
25.9 corroborates that and shows a corresponding frequency of 750 MHz. I thus
believe that the values in table 25.9 and 25.10 are indeed bitrates (in Mbps /
lane), so I think you shouldn't divide by 2 in the above formula.
> >> + for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> >> + if (hsfreq->mbps >= mbps)
> >> + break;
> >> +
> >> + if (!hsfreq->mbps) {
> >>
Re: [PATCH v2 2/8] v4l: vsp1: Share the CLU, LIF and LUT set_fmt pad operation code
Hi Jacopo,
On Saturday, 28 April 2018 12:50:48 EEST jacopo mondi wrote:
> Hi Laurent,
>very minor comments below
>
> On Mon, Apr 23, 2018 at 01:34:24AM +0300, Laurent Pinchart wrote:
> > The implementation of the set_fmt pad operation is identical in the
> > three modules. Move it to a generic helper function.
> >
> > Signed-off-by: Laurent Pinchart
> >
> > ---
> >
> > drivers/media/platform/vsp1/vsp1_clu.c| 65 +
> > drivers/media/platform/vsp1/vsp1_entity.c | 75 ++
> > drivers/media/platform/vsp1/vsp1_entity.h | 6 +++
> > drivers/media/platform/vsp1/vsp1_lif.c| 65 +
> > drivers/media/platform/vsp1/vsp1_lut.c| 65 +
> > 5 files changed, 116 insertions(+), 160 deletions(-)
> >
> > diff --git a/drivers/media/platform/vsp1/vsp1_clu.c
> > b/drivers/media/platform/vsp1/vsp1_clu.c index 9626b6308585..96a448e1504c
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_clu.c
> > +++ b/drivers/media/platform/vsp1/vsp1_clu.c
> > @@ -114,18 +114,18 @@ static const struct v4l2_ctrl_config
> > clu_mode_control = {
> > * V4L2 Subdevice Pad Operations
> > */
> >
> > +static const unsigned int clu_codes[] = {
> > + MEDIA_BUS_FMT_ARGB_1X32,
> > + MEDIA_BUS_FMT_AHSV_1X32,
> > + MEDIA_BUS_FMT_AYUV8_1X32,
> > +};
> > +
> > static int clu_enum_mbus_code(struct v4l2_subdev *subdev,
> > struct v4l2_subdev_pad_config *cfg,
> > struct v4l2_subdev_mbus_code_enum *code)
> > {
> > - static const unsigned int codes[] = {
> > - MEDIA_BUS_FMT_ARGB_1X32,
> > - MEDIA_BUS_FMT_AHSV_1X32,
> > - MEDIA_BUS_FMT_AYUV8_1X32,
> > - };
> > -
> > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes,
> > - ARRAY_SIZE(codes));
> > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, clu_codes,
> > + ARRAY_SIZE(clu_codes));
> > }
> >
> > static int clu_enum_frame_size(struct v4l2_subdev *subdev,
> > @@ -141,51 +141,10 @@ static int clu_set_format(struct v4l2_subdev
> > *subdev,
> > struct v4l2_subdev_pad_config *cfg,
> > struct v4l2_subdev_format *fmt)
> > {
> > - struct vsp1_clu *clu = to_clu(subdev);
> > - struct v4l2_subdev_pad_config *config;
> > - struct v4l2_mbus_framefmt *format;
> > - int ret = 0;
> > -
> > - mutex_lock(&clu->entity.lock);
> > -
> > - config = vsp1_entity_get_pad_config(&clu->entity, cfg, fmt->which);
> > - if (!config) {
> > - ret = -EINVAL;
> > - goto done;
> > - }
> > -
> > - /* Default to YUV if the requested format is not supported. */
> > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB_1X32 &&
> > - fmt->format.code != MEDIA_BUS_FMT_AHSV_1X32 &&
> > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32)
> > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32;
>
> The newly implemented vsp1_subdev_set_pad_format defaults to the first
> clu_codes[] member (ARGB888_1x32), while here the code chose the AYUV8_1x32
> format. Is it ok? Should you revers the clu_codes[] order?
But that would then change the order of the format enumeration.
I don't think it's a big deal, the change here will only affect the format
returned if userspace tries to pick a format that is not supported (and thus
not returned by the enumeration). This shouldn't happen in the first place,
and if it does, the driver has never guaranteed that a specific format would
be returned.
> > -
> > - format = vsp1_entity_get_pad_format(&clu->entity, config, fmt->pad);
> > -
> > - if (fmt->pad == CLU_PAD_SOURCE) {
> > - /* The CLU output format can't be modified. */
> > - fmt->format = *format;
> > - goto done;
> > - }
> > -
> > - format->code = fmt->format.code;
> > - format->width = clamp_t(unsigned int, fmt->format.width,
> > - CLU_MIN_SIZE, CLU_MAX_SIZE);
> > - format->height = clamp_t(unsigned int, fmt->format.height,
> > -CLU_MIN_SIZE, CLU_MAX_SIZE);
> > - format->field = V4L2_FIELD_NONE;
> > - format->colorspace = V4L2_COLORSPACE_SRGB;
> > -
> > - fmt->format = *format;
> > -
> > - /* Propagate the format to the source pad. */
> > - format = vsp1_entity_get_pad_format(&clu->entity, config,
> > - CLU_PAD_SOURCE);
> > - *format = fmt->format;
> > -
> > -done:
> > - mutex_unlock(&clu->entity.lock);
> > - return ret;
> > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, clu_codes,
> > + ARRAY_SIZE(clu_codes),
> > + CLU_MIN_SIZE, CLU_MIN_SIZE,
> > + CLU_MAX_SIZE, CLU_MAX_SIZE);
> > }
> >
> > /* --
> > diff --g
Re: [PATCH v2 5/8] v4l: vsp1: Extend the DU API to support CRC computation
Hi Jacopo,
On Saturday, 28 April 2018 13:03:16 EEST jacopo mondi wrote:
> Hi Laurent,
>just one minor comment below
>
> On Mon, Apr 23, 2018 at 01:34:27AM +0300, Laurent Pinchart wrote:
> > Add a parameter (in the form of a structure to ease future API
> > extensions) to the VSP atomic flush handler to pass CRC source
> > configuration, and pass the CRC value to the completion callback.
> >
> > Signed-off-by: Laurent Pinchart
> >
> > ---
> >
> > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 6 --
> > drivers/media/platform/vsp1/vsp1_drm.c | 6 --
> > drivers/media/platform/vsp1/vsp1_drm.h | 2 +-
> > include/media/vsp1.h | 29 +++--
> > 4 files changed, 36 insertions(+), 7 deletions(-)
[snip]
> > diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> > index ff7ef894465d..ac63a9928a79 100644
> > --- a/include/media/vsp1.h
> > +++ b/include/media/vsp1.h
[snip]
> > @@ -61,11 +61,36 @@ struct vsp1_du_atomic_config {
> > unsigned int zpos;
> > };
> >
> > +/**
> > + * enum vsp1_du_crc_source - Source used for CRC calculation
> > + * @VSP1_DU_CRC_NONE: CRC calculation disabled
> > + * @VSP_DU_CRC_PLANE: Perform CRC calculation on an input plane
> > + * @VSP_DU_CRC_OUTPUT: Perform CRC calculation on the composed output
>
> These two paramters are called VSP1_DU_CRC_* not VSP_DU_CRC_*
My bad. I've fixed this in my tree but will wait for other review comments
before posting a v3.
> > + */
> > +enum vsp1_du_crc_source {
> > + VSP1_DU_CRC_NONE,
> > + VSP1_DU_CRC_PLANE,
> > + VSP1_DU_CRC_OUTPUT,
> > +};
[snip]
--
Regards,
Laurent Pinchart
Re: [PATCH v2 6/8] v4l: vsp1: Add support for the DISCOM entity
Hi Jacopo,
On Saturday, 28 April 2018 13:40:02 EEST jacopo mondi wrote:
> HI Laurent,
>a few comments, mostly minor ones...
>
> On Mon, Apr 23, 2018 at 01:34:28AM +0300, Laurent Pinchart wrote:
> > The DISCOM calculates a CRC on a configurable window of the frame. It
> > interfaces to the VSP through the UIF glue, hence the name used in the
> > code.
> >
> > The module supports configuration of the CRC window through the crop
> > rectangle on the ink pad of the corresponding entity. However, unlike
>
> sink pad?
Oops. Consider it fixed.
> > the traditional V4L2 subdevice model, the crop rectangle does not
> > influence the format on the source pad.
> >
> > Modeling the DISCOM as a sink-only entity would allow adhering to the
> > V4L2 subdevice model at the expense of more complex code in the driver,
> > as at the hardware level the UIF is handled as a sink+source entity. As
> > the DISCOM is only present in R-Car Gen3 VSP-D and VSP-DL instances it
> > is not exposed to userspace through V4L2 but controlled through the DU
> > driver. We can thus change this model later if needed without fear of
> > affecting userspace.
> >
> > Signed-off-by: Laurent Pinchart
> >
> > ---
> > Changes since v1:
> >
> > - Don't return uninitialized value from uif_set_selection()
> > ---
> >
> > drivers/media/platform/vsp1/Makefile | 2 +-
> > drivers/media/platform/vsp1/vsp1.h| 4 +
> > drivers/media/platform/vsp1/vsp1_drv.c| 20 +++
> > drivers/media/platform/vsp1/vsp1_entity.c | 6 +
> > drivers/media/platform/vsp1/vsp1_entity.h | 1 +
> > drivers/media/platform/vsp1/vsp1_regs.h | 41 +
> > drivers/media/platform/vsp1/vsp1_uif.c| 271 +
> > drivers/media/platform/vsp1/vsp1_uif.h| 32
> > 8 files changed, 376 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/media/platform/vsp1/vsp1_uif.c
> > create mode 100644 drivers/media/platform/vsp1/vsp1_uif.h
[snip]
> > diff --git a/drivers/media/platform/vsp1/vsp1_uif.c
> > b/drivers/media/platform/vsp1/vsp1_uif.c new file mode 100644
> > index ..6de7e9c801ae
> > --- /dev/null
> > +++ b/drivers/media/platform/vsp1/vsp1_uif.c
> > @@ -0,0 +1,271 @@
[snip]
> > +static void uif_configure(struct vsp1_entity *entity,
> > + struct vsp1_pipeline *pipe,
> > + struct vsp1_dl_list *dl,
> > + enum vsp1_entity_params params)
> > +{
> > + struct vsp1_uif *uif = to_uif(&entity->subdev);
> > + const struct v4l2_rect *crop;
> > + unsigned int left;
> > + unsigned int width;
> > +
> > + /*
> > +* Per-partition configuration isn't needed as the DISCOM is used in
> > +* display pipelines only.
> > +*/
> > + if (params != VSP1_ENTITY_PARAMS_INIT)
> > + return;
> > +
> > + vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMPMR,
> > + VI6_UIF_DISCOM_DOCMPMR_SEL(9));
> > +
> > + crop = vsp1_entity_get_pad_selection(entity, entity->config,
> > +UIF_PAD_SINK, V4L2_SEL_TGT_CROP);
> > +
> > + /* On M3-W the horizontal coordinates are twice the register value. */
> > + if (uif->m3w_quirk) {
> > + left = crop->left / 2;
> > + width = crop->width / 2;
> > + } else {
> > + left = crop->left;
> > + width = crop->width;
> > + }
>
> I would write this as
>
> left = crop->left;
> width = crop->width;
> /* On M3-W the horizontal coordinates are twice the register value. */
> if (uif->m3w_quirk) {
> left /= 2;
> width /= 2;
> }
>
> But that's really up to you.
I prefer my style, but it looks like gcc 6.4.0 generates slightly better code
with your version (due to the fact that the crop->left value is converted to
unsigned before being divided by 2), so I'll go for it.
> > +
> > + vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSPXR, left);
> > + vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSPYR, crop->top);
> > + vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSZXR, width);
> > + vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSZYR, crop->height);
> > +
> > + vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMCR,
> > + VI6_UIF_DISCOM_DOCMCR_CMPR);
> > +}
> > +
> > +static const struct vsp1_entity_operations uif_entity_ops = {
> > + .configure = uif_configure,
> > +};
> > +
> > +/* --
> > + * Initialization and Cleanup
> > + */
> > +
> > +static const struct soc_device_attribute vsp1_r8a7796[] = {
> > + { .soc_id = "r8a7796" },
> > + { /* sentinel */ }
> > +};
> > +
> > +struct vsp1_uif *vsp1_uif_create(struct vsp1_device *vsp1, unsigned int
> > index) +{
> > + struct vsp1_uif *uif;
> > + char name[6];
> > + int ret;
> > +
> > + uif = devm_kzalloc(vsp1->dev, sizeof(*uif), GFP_KERNEL);
> > + if (uif == NULL)
>
> if (!uif)
>
> Otherwise checkpatch complains i
Re: [PATCH v2 7/8] v4l: vsp1: Integrate DISCOM in display pipeline
Hi Jacopo,
On Saturday, 28 April 2018 14:00:26 EEST jacopo mondi wrote:
> On Mon, Apr 23, 2018 at 01:34:29AM +0300, Laurent Pinchart wrote:
> > The DISCOM is used to compute CRCs on display frames. Integrate it in
> > the display pipeline at the output of the blending unit to process
> > output frames.
> >
> > Computing CRCs on input frames is possible by positioning the DISCOM at
> > a different point in the pipeline. This use case isn't supported at the
> > moment and could be implemented by extending the API between the VSP1
> > and DU drivers if needed.
> >
> > Signed-off-by: Laurent Pinchart
> >
> > ---
> >
> > drivers/media/platform/vsp1/vsp1_drm.c | 115 +++-
> > drivers/media/platform/vsp1/vsp1_drm.h | 12
> > 2 files changed, 124 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > b/drivers/media/platform/vsp1/vsp1_drm.c index 5fc31578f9b0..7864b43a90e1
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
[snip]
> > @@ -48,10 +54,65 @@ static void vsp1_du_pipeline_frame_end(struct
> > vsp1_pipeline *pipe,>
> > * Pipeline Configuration
> > */
> >
> > +/*
> > + * Insert the UIF in the pipeline between the prev and next entities. If
> > no UIF + * is available connect the two entities directly.
> > + */
> > +static int vsp1_du_insert_uif(struct vsp1_device *vsp1,
> > + struct vsp1_pipeline *pipe,
> > + struct vsp1_entity *uif,
> > + struct vsp1_entity *prev, unsigned int prev_pad,
> > + struct vsp1_entity *next, unsigned int next_pad)
> > +{
> > + int ret;
> > +
> > + if (uif) {
> > + struct v4l2_subdev_format format;
> > +
> > + prev->sink = uif;
> > + prev->sink_pad = UIF_PAD_SINK;
> > +
> > + memset(&format, 0, sizeof(format));
> > + format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > + format.pad = prev_pad;
> > +
> > + ret = v4l2_subdev_call(&prev->subdev, pad, get_fmt, NULL,
> > + &format);
> > + if (ret < 0)
> > + return ret;
> > +
> > + format.pad = UIF_PAD_SINK;
> > +
> > + ret = v4l2_subdev_call(&uif->subdev, pad, set_fmt, NULL,
> > + &format);
> > + if (ret < 0)
> > + return ret;
> > +
> > + dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on UIF sink\n",
> > + __func__, format.format.width, format.format.height,
> > + format.format.code);
> > +
> > + /*
> > +* The UIF doesn't mangle the format between its sink and
> > +* source pads, so there is no need to retrieve the format on
> > +* its source pad.
> > +*/
> > +
> > + uif->sink = next;
> > + uif->sink_pad = next_pad;
> > + } else {
> > + prev->sink = next;
> > + prev->sink_pad = next_pad;
>
> Isn't the !uif case better handled in the caller? See below...
> Or otherwise, if you prefer handling it here, shouldn't the
> indentation be reduced with
I don't think it's better handled in the caller, I prefer keeping the entities
linking code in a single place. I'll reduce the indentation.
> if (!uif) {
> prev->sink = next;
> prev->sink_pad = next_pad;
> return 0;
> }
>
> > + }
> > +
> > + return 0;
> > +}
[snip]
> > @@ -367,6 +441,31 @@ static int vsp1_du_pipeline_setup_inputs(struct
> > vsp1_device *vsp1,
> > }
> > }
> >
> > + /* Insert and configure the UIF at the BRx output if available. */
> > + uif = drm_pipe->crc.source == VSP1_DU_CRC_OUTPUT ? drm_pipe->uif :
NULL;
> > + if (uif)
> > + use_uif = true;
> > + ret = vsp1_du_insert_uif(vsp1, pipe, uif,
> > +pipe->brx, pipe->brx->source_pad,
> > +&pipe->output->entity, 0);
> > + if (ret < 0)
> > + dev_err(vsp1->dev, "%s: failed to setup UIF after %s\n",
> > + __func__, BRX_NAME(pipe->brx));
> > +
> > + /*
> > +* If the UIF is not in use schedule it for removal by setting its pipe
> > +* pointer to NULL, vsp1_du_pipeline_configure() will remove it from
the
> > +* hardware pipeline and from the pipeline's list of entities.
Otherwise
> > +* make sure it is present in the pipeline's list of entities if it
> > +* wasn't already.
> > +*/
> > + if (!use_uif) {
>
> ... here. If you don't use uif, don't call vspi1_du_insert_uif().
> True, you have to link entities explicitly here if there is not uif,
> but I may be missing where this was happening before this code was
> added.
>
> > + drm_pipe->uif->pipe = NULL;
> > + } else if (!drm_pipe->uif->pipe) {
> > + drm_pipe->uif->pipe
Re: [PATCH v2 2/8] v4l: vsp1: Share the CLU, LIF and LUT set_fmt pad operation code
Hi Laurent,
On 22/04/18 23:34, Laurent Pinchart wrote:
> The implementation of the set_fmt pad operation is identical in the
> three modules. Move it to a generic helper function.
>
> Signed-off-by: Laurent Pinchart
Only a minor pair of comments below regarding source/sink pad descriptions.
If it's not convenient/accurate to define these with an enum then don't worry
about it.
Otherwise,
Reviewed-by: Kieran Bingham
> ---
> drivers/media/platform/vsp1/vsp1_clu.c| 65 +--
> drivers/media/platform/vsp1/vsp1_entity.c | 75
> +++
> drivers/media/platform/vsp1/vsp1_entity.h | 6 +++
> drivers/media/platform/vsp1/vsp1_lif.c| 65 +--
> drivers/media/platform/vsp1/vsp1_lut.c| 65 +--
> 5 files changed, 116 insertions(+), 160 deletions(-)
That's a nice diffstat :-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_clu.c
> b/drivers/media/platform/vsp1/vsp1_clu.c
> index 9626b6308585..96a448e1504c 100644
> --- a/drivers/media/platform/vsp1/vsp1_clu.c
> +++ b/drivers/media/platform/vsp1/vsp1_clu.c
> @@ -114,18 +114,18 @@ static const struct v4l2_ctrl_config clu_mode_control =
> {
> * V4L2 Subdevice Pad Operations
> */
>
> +static const unsigned int clu_codes[] = {
> + MEDIA_BUS_FMT_ARGB_1X32,
> + MEDIA_BUS_FMT_AHSV_1X32,
> + MEDIA_BUS_FMT_AYUV8_1X32,
> +};
> +
> static int clu_enum_mbus_code(struct v4l2_subdev *subdev,
> struct v4l2_subdev_pad_config *cfg,
> struct v4l2_subdev_mbus_code_enum *code)
> {
> - static const unsigned int codes[] = {
> - MEDIA_BUS_FMT_ARGB_1X32,
> - MEDIA_BUS_FMT_AHSV_1X32,
> - MEDIA_BUS_FMT_AYUV8_1X32,
> - };
> -
> - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes,
> - ARRAY_SIZE(codes));
> + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, clu_codes,
> + ARRAY_SIZE(clu_codes));
> }
>
> static int clu_enum_frame_size(struct v4l2_subdev *subdev,
> @@ -141,51 +141,10 @@ static int clu_set_format(struct v4l2_subdev *subdev,
> struct v4l2_subdev_pad_config *cfg,
> struct v4l2_subdev_format *fmt)
> {
> - struct vsp1_clu *clu = to_clu(subdev);
> - struct v4l2_subdev_pad_config *config;
> - struct v4l2_mbus_framefmt *format;
> - int ret = 0;
> -
> - mutex_lock(&clu->entity.lock);
> -
> - config = vsp1_entity_get_pad_config(&clu->entity, cfg, fmt->which);
> - if (!config) {
> - ret = -EINVAL;
> - goto done;
> - }
> -
> - /* Default to YUV if the requested format is not supported. */
> - if (fmt->format.code != MEDIA_BUS_FMT_ARGB_1X32 &&
> - fmt->format.code != MEDIA_BUS_FMT_AHSV_1X32 &&
> - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32)
> - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32;
> -
> - format = vsp1_entity_get_pad_format(&clu->entity, config, fmt->pad);
> -
> - if (fmt->pad == CLU_PAD_SOURCE) {
> - /* The CLU output format can't be modified. */
> - fmt->format = *format;
> - goto done;
> - }
> -
> - format->code = fmt->format.code;
> - format->width = clamp_t(unsigned int, fmt->format.width,
> - CLU_MIN_SIZE, CLU_MAX_SIZE);
> - format->height = clamp_t(unsigned int, fmt->format.height,
> - CLU_MIN_SIZE, CLU_MAX_SIZE);
> - format->field = V4L2_FIELD_NONE;
> - format->colorspace = V4L2_COLORSPACE_SRGB;
> -
> - fmt->format = *format;
> -
> - /* Propagate the format to the source pad. */
> - format = vsp1_entity_get_pad_format(&clu->entity, config,
> - CLU_PAD_SOURCE);
> - *format = fmt->format;
> -
> -done:
> - mutex_unlock(&clu->entity.lock);
> - return ret;
> + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, clu_codes,
> + ARRAY_SIZE(clu_codes),
> + CLU_MIN_SIZE, CLU_MIN_SIZE,
> + CLU_MAX_SIZE, CLU_MAX_SIZE);
> }
>
> /*
> -
> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c
> b/drivers/media/platform/vsp1/vsp1_entity.c
> index 72354caf5746..239df047efd0 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.c
> +++ b/drivers/media/platform/vsp1/vsp1_entity.c
> @@ -307,6 +307,81 @@ int vsp1_subdev_enum_frame_size(struct v4l2_subdev
> *subdev,
> return ret;
> }
>
> +/*
> + * vsp1_subdev_set_pad_format - Subdev pad set_fmt handler
> + * @subdev: V4L2 subdevice
> + * @cfg: V4L2 subdev pad configuration
> + * @fmt: V4L2 subdev format
> + * @codes: Array of supported media bus codes
> + * @
Re: [PATCH v2 3/8] v4l: vsp1: Reset the crop and compose rectangles in the set_fmt helper
Hi Laurent,
On 22/04/18 23:34, Laurent Pinchart wrote:
> To make vsp1_subdev_set_pad_format() usable by entities that support
> selection rectangles, we need to reset the crop and compose rectangles
> when setting the format on the sink pad. Do so and replace the custom
> set_fmt implementation of the histogram code by a call to
> vsp1_subdev_set_pad_format().
>
> Resetting the crop and compose rectangles for entities that don't
> support crop and compose has no adverse effect as the rectangles are
> ignored anyway.
>
> Signed-off-by: Laurent Pinchart
This one looks fairly straight forward.
Reviewed-by: Kieran Bingham
> ---
> drivers/media/platform/vsp1/vsp1_entity.c | 16 +
> drivers/media/platform/vsp1/vsp1_histo.c | 59
> +++
> 2 files changed, 20 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c
> b/drivers/media/platform/vsp1/vsp1_entity.c
> index 239df047efd0..181a583aecad 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.c
> +++ b/drivers/media/platform/vsp1/vsp1_entity.c
> @@ -335,6 +335,7 @@ int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev,
> struct vsp1_entity *entity = to_vsp1_entity(subdev);
> struct v4l2_subdev_pad_config *config;
> struct v4l2_mbus_framefmt *format;
> + struct v4l2_rect *selection;
> unsigned int i;
> int ret = 0;
>
> @@ -377,6 +378,21 @@ int vsp1_subdev_set_pad_format(struct v4l2_subdev
> *subdev,
> format = vsp1_entity_get_pad_format(entity, config, 1);
> *format = fmt->format;
>
> + /* Reset the crop and compose rectangles */
> + selection = vsp1_entity_get_pad_selection(entity, config, fmt->pad,
> + V4L2_SEL_TGT_CROP);
> + selection->left = 0;
> + selection->top = 0;
> + selection->width = format->width;
> + selection->height = format->height;
> +
> + selection = vsp1_entity_get_pad_selection(entity, config, fmt->pad,
> + V4L2_SEL_TGT_COMPOSE);
> + selection->left = 0;
> + selection->top = 0;
> + selection->width = format->width;
> + selection->height = format->height;
> +
> done:
> mutex_unlock(&entity->lock);
> return ret;
> diff --git a/drivers/media/platform/vsp1/vsp1_histo.c
> b/drivers/media/platform/vsp1/vsp1_histo.c
> index 029181c1fb61..5e15c8ff88d9 100644
> --- a/drivers/media/platform/vsp1/vsp1_histo.c
> +++ b/drivers/media/platform/vsp1/vsp1_histo.c
> @@ -389,65 +389,14 @@ static int histo_set_format(struct v4l2_subdev *subdev,
> struct v4l2_subdev_format *fmt)
> {
> struct vsp1_histogram *histo = subdev_to_histo(subdev);
> - struct v4l2_subdev_pad_config *config;
> - struct v4l2_mbus_framefmt *format;
> - struct v4l2_rect *selection;
> - unsigned int i;
> - int ret = 0;
>
> if (fmt->pad != HISTO_PAD_SINK)
> return histo_get_format(subdev, cfg, fmt);
>
> - mutex_lock(&histo->entity.lock);
> -
> - config = vsp1_entity_get_pad_config(&histo->entity, cfg, fmt->which);
> - if (!config) {
> - ret = -EINVAL;
> - goto done;
> - }
> -
> - /*
> - * Default to the first format if the requested format is not
> - * supported.
> - */
> - for (i = 0; i < histo->num_formats; ++i) {
> - if (fmt->format.code == histo->formats[i])
> - break;
> - }
> - if (i == histo->num_formats)
> - fmt->format.code = histo->formats[0];
> -
> - format = vsp1_entity_get_pad_format(&histo->entity, config, fmt->pad);
> -
> - format->code = fmt->format.code;
> - format->width = clamp_t(unsigned int, fmt->format.width,
> - HISTO_MIN_SIZE, HISTO_MAX_SIZE);
> - format->height = clamp_t(unsigned int, fmt->format.height,
> - HISTO_MIN_SIZE, HISTO_MAX_SIZE);
> - format->field = V4L2_FIELD_NONE;
> - format->colorspace = V4L2_COLORSPACE_SRGB;
> -
> - fmt->format = *format;
> -
> - /* Reset the crop and compose rectangles */
> - selection = vsp1_entity_get_pad_selection(&histo->entity, config,
> - fmt->pad, V4L2_SEL_TGT_CROP);
> - selection->left = 0;
> - selection->top = 0;
> - selection->width = format->width;
> - selection->height = format->height;
> -
> - selection = vsp1_entity_get_pad_selection(&histo->entity, config,
> - fmt->pad,
> - V4L2_SEL_TGT_COMPOSE);
> - selection->left = 0;
> - selection->top = 0;
> - selection->width = format->width;
> - selection->height = format->height;
> -
> -done:
> - mutex_unlock(&histo->entity.lock);
> - return ret;
> + return vsp1_subdev_set_pad_format(subdev, cfg, fmt,
> +
Re: [PATCH v2 2/8] v4l: vsp1: Share the CLU, LIF and LUT set_fmt pad operation code
Hi Kieran,
On Saturday, 28 April 2018 20:16:11 EEST Kieran Bingham wrote:
> On 22/04/18 23:34, Laurent Pinchart wrote:
> > The implementation of the set_fmt pad operation is identical in the
> > three modules. Move it to a generic helper function.
> >
> > Signed-off-by: Laurent Pinchart
> >
>
> Only a minor pair of comments below regarding source/sink pad descriptions.
>
> If it's not convenient/accurate to define these with an enum then don't
> worry about it.
It's a good point. There are however other locations in vsp1_entity.c that
hardcode pad numbers, so I'll submit a patch on top of this series to fix them
all in one go.
> Otherwise,
>
> Reviewed-by: Kieran Bingham
>
> > ---
> >
> > drivers/media/platform/vsp1/vsp1_clu.c| 65 +---
> > drivers/media/platform/vsp1/vsp1_entity.c | 75 ++
> > drivers/media/platform/vsp1/vsp1_entity.h | 6 +++
> > drivers/media/platform/vsp1/vsp1_lif.c| 65 +
> > drivers/media/platform/vsp1/vsp1_lut.c| 65 +
> > 5 files changed, 116 insertions(+), 160 deletions(-)
>
> That's a nice diffstat :-)
>
> > diff --git a/drivers/media/platform/vsp1/vsp1_clu.c
> > b/drivers/media/platform/vsp1/vsp1_clu.c index 9626b6308585..96a448e1504c
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_clu.c
> > +++ b/drivers/media/platform/vsp1/vsp1_clu.c
> > @@ -114,18 +114,18 @@ static const struct v4l2_ctrl_config
> > clu_mode_control = {>
> > * V4L2 Subdevice Pad Operations
> > */
> >
> > +static const unsigned int clu_codes[] = {
> > + MEDIA_BUS_FMT_ARGB_1X32,
> > + MEDIA_BUS_FMT_AHSV_1X32,
> > + MEDIA_BUS_FMT_AYUV8_1X32,
> > +};
> > +
> >
> > static int clu_enum_mbus_code(struct v4l2_subdev *subdev,
> >
> > struct v4l2_subdev_pad_config *cfg,
> > struct v4l2_subdev_mbus_code_enum *code)
> >
> > {
> >
> > - static const unsigned int codes[] = {
> > - MEDIA_BUS_FMT_ARGB_1X32,
> > - MEDIA_BUS_FMT_AHSV_1X32,
> > - MEDIA_BUS_FMT_AYUV8_1X32,
> > - };
> > -
> > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes,
> > - ARRAY_SIZE(codes));
> > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, clu_codes,
> > + ARRAY_SIZE(clu_codes));
> >
> > }
> >
> > static int clu_enum_frame_size(struct v4l2_subdev *subdev,
> >
> > @@ -141,51 +141,10 @@ static int clu_set_format(struct v4l2_subdev
> > *subdev,
> >
> > struct v4l2_subdev_pad_config *cfg,
> > struct v4l2_subdev_format *fmt)
> >
> > {
> >
> > - struct vsp1_clu *clu = to_clu(subdev);
> > - struct v4l2_subdev_pad_config *config;
> > - struct v4l2_mbus_framefmt *format;
> > - int ret = 0;
> > -
> > - mutex_lock(&clu->entity.lock);
> > -
> > - config = vsp1_entity_get_pad_config(&clu->entity, cfg, fmt->which);
> > - if (!config) {
> > - ret = -EINVAL;
> > - goto done;
> > - }
> > -
> > - /* Default to YUV if the requested format is not supported. */
> > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB_1X32 &&
> > - fmt->format.code != MEDIA_BUS_FMT_AHSV_1X32 &&
> > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32)
> > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32;
> > -
> > - format = vsp1_entity_get_pad_format(&clu->entity, config, fmt->pad);
> > -
> > - if (fmt->pad == CLU_PAD_SOURCE) {
> > - /* The CLU output format can't be modified. */
> > - fmt->format = *format;
> > - goto done;
> > - }
> > -
> > - format->code = fmt->format.code;
> > - format->width = clamp_t(unsigned int, fmt->format.width,
> > - CLU_MIN_SIZE, CLU_MAX_SIZE);
> > - format->height = clamp_t(unsigned int, fmt->format.height,
> > -CLU_MIN_SIZE, CLU_MAX_SIZE);
> > - format->field = V4L2_FIELD_NONE;
> > - format->colorspace = V4L2_COLORSPACE_SRGB;
> > -
> > - fmt->format = *format;
> > -
> > - /* Propagate the format to the source pad. */
> > - format = vsp1_entity_get_pad_format(&clu->entity, config,
> > - CLU_PAD_SOURCE);
> > - *format = fmt->format;
> > -
> > -done:
> > - mutex_unlock(&clu->entity.lock);
> > - return ret;
> > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, clu_codes,
> > + ARRAY_SIZE(clu_codes),
> > + CLU_MIN_SIZE, CLU_MIN_SIZE,
> > + CLU_MAX_SIZE, CLU_MAX_SIZE);
> >
> > }
> >
> > /*
> >
> > ->
> > diff --git a/drivers/media/platform/vsp1/vsp1_entity.c
> > b/drivers/media/platform/vsp1/vsp1_entity.c index
> > 72354caf5746..239df047efd0 100644
> > --- a/drivers/media/platfor
Re: [PATCH v2 4/8] v4l: vsp1: Document the vsp1_du_atomic_config structure
Hi Laurent,
On 22/04/18 23:34, Laurent Pinchart wrote:
> The structure is used in the API that the VSP1 driver exposes to the DU
> driver. Documenting it is thus important.
>
> Signed-off-by: Laurent Pinchart
I look forward to being reminded to add a doc-line to this structure for my
interlaced support branch which added 'bool interlaced' to this structure before
it was documented, (thus doesn't add a doc string). Maybe me writing this
comment will remind me in the future when I rebase the interlaced patches ;-)
Reviewed-by: Kieran Bingham
> ---
> Changes since v1:
>
> - Fixed typo
> ---
> include/media/vsp1.h | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index 68a8abe4fac5..ff7ef894465d 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -41,6 +41,16 @@ struct vsp1_du_lif_config {
> int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
> const struct vsp1_du_lif_config *cfg);
>
> +/**
> + * struct vsp1_du_atomic_config - VSP atomic configuration parameters
> + * @pixelformat: plane pixel format (V4L2 4CC)
> + * @pitch: line pitch in bytes, for all planes
> + * @mem: DMA memory address for each plane of the frame buffer
> + * @src: source rectangle in the frame buffer (integer coordinates)
> + * @dst: destination rectangle on the display (integer coordinates)
> + * @alpha: alpha value (0: fully transparent, 255: fully opaque)
> + * @zpos: Z position of the plane (from 0 to number of planes minus 1)
> + */
> struct vsp1_du_atomic_config {
> u32 pixelformat;
> unsigned int pitch;
>
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 2/8] v4l: vsp1: Share the CLU, LIF and LUT set_fmt pad operation code
Hi again,
On Saturday, 28 April 2018 20:25:44 EEST Laurent Pinchart wrote:
> On Saturday, 28 April 2018 20:16:11 EEST Kieran Bingham wrote:
> > On 22/04/18 23:34, Laurent Pinchart wrote:
> > > The implementation of the set_fmt pad operation is identical in the
> > > three modules. Move it to a generic helper function.
> > >
> > > Signed-off-by: Laurent Pinchart
> > >
> >
> > Only a minor pair of comments below regarding source/sink pad
> > descriptions.
> >
> > If it's not convenient/accurate to define these with an enum then don't
> > worry about it.
>
> It's a good point. There are however other locations in vsp1_entity.c that
> hardcode pad numbers, so I'll submit a patch on top of this series to fix
> them all in one go.
Actually I can compare the pad number to entity->source_pad, I'll update this
patch accordingly in v3.
> > Otherwise,
> >
> > Reviewed-by: Kieran Bingham
> >
> > > ---
> > >
> > > drivers/media/platform/vsp1/vsp1_clu.c| 65 +---
> > > drivers/media/platform/vsp1/vsp1_entity.c | 75
> > > ++
> > > drivers/media/platform/vsp1/vsp1_entity.h | 6 +++
> > > drivers/media/platform/vsp1/vsp1_lif.c| 65
> > > +
> > > drivers/media/platform/vsp1/vsp1_lut.c| 65
> > > +
> > > 5 files changed, 116 insertions(+), 160 deletions(-)
> >
> > That's a nice diffstat :-)
> >
> > > diff --git a/drivers/media/platform/vsp1/vsp1_clu.c
> > > b/drivers/media/platform/vsp1/vsp1_clu.c index
> > > 9626b6308585..96a448e1504c
> > > 100644
> > > --- a/drivers/media/platform/vsp1/vsp1_clu.c
> > > +++ b/drivers/media/platform/vsp1/vsp1_clu.c
> > > @@ -114,18 +114,18 @@ static const struct v4l2_ctrl_config
> > > clu_mode_control = {>
> > >
> > > * V4L2 Subdevice Pad Operations
> > > */
> > >
> > > +static const unsigned int clu_codes[] = {
> > > + MEDIA_BUS_FMT_ARGB_1X32,
> > > + MEDIA_BUS_FMT_AHSV_1X32,
> > > + MEDIA_BUS_FMT_AYUV8_1X32,
> > > +};
> > > +
> > >
> > > static int clu_enum_mbus_code(struct v4l2_subdev *subdev,
> > >
> > > struct v4l2_subdev_pad_config *cfg,
> > > struct v4l2_subdev_mbus_code_enum *code)
> > >
> > > {
> > >
> > > - static const unsigned int codes[] = {
> > > - MEDIA_BUS_FMT_ARGB_1X32,
> > > - MEDIA_BUS_FMT_AHSV_1X32,
> > > - MEDIA_BUS_FMT_AYUV8_1X32,
> > > - };
> > > -
> > > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes,
> > > - ARRAY_SIZE(codes));
> > > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, clu_codes,
> > > + ARRAY_SIZE(clu_codes));
> > >
> > > }
> > >
> > > static int clu_enum_frame_size(struct v4l2_subdev *subdev,
> > >
> > > @@ -141,51 +141,10 @@ static int clu_set_format(struct v4l2_subdev
> > > *subdev,
> > >
> > > struct v4l2_subdev_pad_config *cfg,
> > > struct v4l2_subdev_format *fmt)
> > >
> > > {
> > >
> > > - struct vsp1_clu *clu = to_clu(subdev);
> > > - struct v4l2_subdev_pad_config *config;
> > > - struct v4l2_mbus_framefmt *format;
> > > - int ret = 0;
> > > -
> > > - mutex_lock(&clu->entity.lock);
> > > -
> > > - config = vsp1_entity_get_pad_config(&clu->entity, cfg, fmt->which);
> > > - if (!config) {
> > > - ret = -EINVAL;
> > > - goto done;
> > > - }
> > > -
> > > - /* Default to YUV if the requested format is not supported. */
> > > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB_1X32 &&
> > > - fmt->format.code != MEDIA_BUS_FMT_AHSV_1X32 &&
> > > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32)
> > > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32;
> > > -
> > > - format = vsp1_entity_get_pad_format(&clu->entity, config, fmt->pad);
> > > -
> > > - if (fmt->pad == CLU_PAD_SOURCE) {
> > > - /* The CLU output format can't be modified. */
> > > - fmt->format = *format;
> > > - goto done;
> > > - }
> > > -
> > > - format->code = fmt->format.code;
> > > - format->width = clamp_t(unsigned int, fmt->format.width,
> > > - CLU_MIN_SIZE, CLU_MAX_SIZE);
> > > - format->height = clamp_t(unsigned int, fmt->format.height,
> > > - CLU_MIN_SIZE, CLU_MAX_SIZE);
> > > - format->field = V4L2_FIELD_NONE;
> > > - format->colorspace = V4L2_COLORSPACE_SRGB;
> > > -
> > > - fmt->format = *format;
> > > -
> > > - /* Propagate the format to the source pad. */
> > > - format = vsp1_entity_get_pad_format(&clu->entity, config,
> > > - CLU_PAD_SOURCE);
> > > - *format = fmt->format;
> > > -
> > > -done:
> > > - mutex_unlock(&clu->entity.lock);
> > > - return ret;
> > > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, clu_codes,
> > > + ARRAY_SIZE(clu_codes),
> > > + CLU_MIN_SIZE, CLU_MIN_SIZE,
> > > +
Re: [PATCH v2 2/8] v4l: vsp1: Share the CLU, LIF and LUT set_fmt pad operation code
On 28/04/18 18:30, Laurent Pinchart wrote: > Hi again, > > On Saturday, 28 April 2018 20:25:44 EEST Laurent Pinchart wrote: >> On Saturday, 28 April 2018 20:16:11 EEST Kieran Bingham wrote: >>> On 22/04/18 23:34, Laurent Pinchart wrote: The implementation of the set_fmt pad operation is identical in the three modules. Move it to a generic helper function. Signed-off-by: Laurent Pinchart >>> >>> Only a minor pair of comments below regarding source/sink pad >>> descriptions. >>> >>> If it's not convenient/accurate to define these with an enum then don't >>> worry about it. >> >> It's a good point. There are however other locations in vsp1_entity.c that >> hardcode pad numbers, so I'll submit a patch on top of this series to fix >> them all in one go. > > Actually I can compare the pad number to entity->source_pad, I'll update this > patch accordingly in v3. Perfect, that sounds more explicit, easier to read, and future proof against entities with multiple sinks, such as the BRx. -- Kieran signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 1/2] pinctrl: sh-pfc: Add r8a77470 PFC support
Hello!
On 04/17/2018 04:23 PM, Geert Uytterhoeven wrote:
> + Sergei
Sorry for a delay replying, I seem to have ignored large private message and
then it got buried in other mail...
>>> Subject: Re: [PATCH v2 1/2] pinctrl: sh-pfc: Add r8a77470 PFC support
>>> On Wed, Apr 4, 2018 at 5:22 PM, Biju Das wrote:
Add PFC support for the R8A77470 SoC including pin groups for some
on-chip devices such as SCIF, AVB and MMC.
Signed-off-by: Biju Das
Reviewed-by: Fabrizio Castro
>
--- /dev/null
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a77470.c
>
+/* - AVB
+ */
static const
>>> unsigned int avb_link_pins[] = {
+ RCAR_GP_PIN(5, 14),
+};
+static const unsigned int avb_link_mux[] = {
+ AVB_LINK_MARK,
+};
+static const unsigned int avb_magic_pins[] = {
+ RCAR_GP_PIN(5, 15),
+};
+static const unsigned int avb_magic_mux[] = {
+ AVB_MAGIC_MARK,
+};
+static const unsigned int avb_phy_int_pins[] = {
+ RCAR_GP_PIN(5, 16),
+};
+static const unsigned int avb_phy_int_mux[] = {
+ AVB_PHY_INT_MARK,
+};
+static const unsigned int avb_mdio_pins[] = {
+ RCAR_GP_PIN(5, 12), RCAR_GP_PIN(5, 13), }; static const
+unsigned int avb_mdio_mux[] = {
+ AVB_MDC_MARK, AVB_MDIO_MARK,
+};
+static const unsigned int avb_mii_pins[] = {
+ RCAR_GP_PIN(3, 14), RCAR_GP_PIN(3, 15), RCAR_GP_PIN(3, 16),
+ RCAR_GP_PIN(3, 27),
+
+ RCAR_GP_PIN(3, 2), RCAR_GP_PIN(3, 3), RCAR_GP_PIN(3, 4),
+ RCAR_GP_PIN(3, 5),
+
+ RCAR_GP_PIN(3, 10), RCAR_GP_PIN(3, 0), RCAR_GP_PIN(3, 1),
+ RCAR_GP_PIN(5, 17), RCAR_GP_PIN(3, 13), RCAR_GP_PIN(5, 23),
+ RCAR_GP_PIN(3, 12),
+};
+static const unsigned int avb_mii_mux[] = {
+ AVB_TXD0_MARK, AVB_TXD1_MARK, AVB_TXD2_MARK,
+ AVB_TXD3_MARK,
+
+ AVB_RXD0_MARK, AVB_RXD1_MARK, AVB_RXD2_MARK,
+ AVB_RXD3_MARK,
+
+ AVB_RX_ER_MARK, AVB_RX_CLK_MARK, AVB_RX_DV_MARK,
+ AVB_CRS_MARK, AVB_TX_EN_MARK, AVB_TX_ER_MARK,
+ AVB_TX_CLK_MARK,
>>>
>>> You forgot AVB_COL, which is GP5_18?
>>
>> GP5_18 is shred between the signals AVB_COL and VI0_CLK. As per the RZ/G1C
>> board schematic /hardware user guide
>> Ethernet Phy collision pin(AVB_COL) pin is not populated on this board by
>> default. It is populated only for
>> Video Input Channel0 pixel clock.
>>
>> Since it is initial submission, I thought of adding only board specific pins
>> first and later
>> add this collision pin alone as a separate pin entry in the avb group. That
>> way we can support existing board
>> as well as any future RZ/G1C board which populates this pin. Are you ok for
>> this approach?
> Oops. That means our grouping is suboptimal. Perhaps we should revisit
> (for all SoCs, while keeping backwards compatibility)?
> After reading some wikipedia, I came up with:
>
> mii_tx
> mii_tx_er (optional)
> mii_rx
I don't see why we should have the separate TX/RX subgroups just because
they're described separately in the Wikipedia. Or did you have some special
reason to do it?
> mii_col_crs (optional, half duplex)
>
> However, given your board uses AVB_CRS without AVB_COL, that would still
> not be sufficient, so the last group should be split to cover your use case?
> BTW, how does it work with AVB_COL not wired to anything by default, and thus
> floating? Do you enable pull-up/down for that pin in the PFC, or is the pin
> just ignored in full-duplex mode?
Well, I'm seeing the strong possibility the half-duplex mode just doesn't
work as
the collision indication isn't available.
+};
+static const unsigned int avb_gmii_pins[] = {
+ RCAR_GP_PIN(3, 14), RCAR_GP_PIN(3, 15), RCAR_GP_PIN(3, 16),
+ RCAR_GP_PIN(3, 27), RCAR_GP_PIN(3, 28), RCAR_GP_PIN(3, 29),
+ RCAR_GP_PIN(4, 0), RCAR_GP_PIN(5, 22),
+
+ RCAR_GP_PIN(3, 2), RCAR_GP_PIN(3, 3), RCAR_GP_PIN(3, 4),
+ RCAR_GP_PIN(3, 5), RCAR_GP_PIN(3, 6), RCAR_GP_PIN(3, 7),
+ RCAR_GP_PIN(3, 8), RCAR_GP_PIN(3, 9),
+
+ RCAR_GP_PIN(3, 10), RCAR_GP_PIN(3, 0), RCAR_GP_PIN(3, 1),
+ RCAR_GP_PIN(5, 17), RCAR_GP_PIN(4, 1), RCAR_GP_PIN(3, 11),
+ RCAR_GP_PIN(3, 13), RCAR_GP_PIN(5, 23), RCAR_GP_PIN(3, 12), };
+static const unsigned int avb_gmii_mux[] = {
+ AVB_TXD0_MARK, AVB_TXD1_MARK, AVB_TXD2_MARK,
+ AVB_TXD3_MARK, AVB_TXD4_MARK, AVB_TXD5_MARK,
+ AVB_TXD6_MARK, AVB_TXD7_MARK,
+
+ AVB_RXD0_MARK, AVB_RXD1_MARK, AVB_RXD2_MARK,
+ AVB_RXD3_MARK, AVB_RXD4_MARK, AVB_RXD5_MARK,
+ AVB_RXD6_MARK, AVB_RXD7_MARK,
+
+ AVB_RX_ER_MARK, AVB_RX_CLK_MARK, AVB_RX_DV_MARK,
+
Re: [PATCH v2 5/8] v4l: vsp1: Extend the DU API to support CRC computation
Hi Laurent,
On 22/04/18 23:34, Laurent Pinchart wrote:
> Add a parameter (in the form of a structure to ease future API
> extensions) to the VSP atomic flush handler to pass CRC source
> configuration, and pass the CRC value to the completion callback.
>
> Signed-off-by: Laurent Pinchart
Only a minor 'thought' below.
(And Jacopo already caught the enum typos, so with those are fixed...)
Reviewed-by: Kieran Bingham
> ---
> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 6 --
> drivers/media/platform/vsp1/vsp1_drm.c | 6 --
> drivers/media/platform/vsp1/vsp1_drm.h | 2 +-
> include/media/vsp1.h | 29 +++--
> 4 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index 2c260c33840b..bdcec201591f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -31,7 +31,7 @@
> #include "rcar_du_kms.h"
> #include "rcar_du_vsp.h"
>
> -static void rcar_du_vsp_complete(void *private, bool completed)
> +static void rcar_du_vsp_complete(void *private, bool completed, u32 crc)
> {
> struct rcar_du_crtc *crtc = private;
>
> @@ -102,7 +102,9 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
>
> void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
> {
> - vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe);
> + struct vsp1_du_atomic_pipe_config cfg = { { 0, } }> +
> + vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
> }
>
> /* Keep the two tables in sync. */
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index 2b29a83dceb9..5fc31578f9b0 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -36,7 +36,7 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline
> *pipe,
> bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;
>
> if (drm_pipe->du_complete)
> - drm_pipe->du_complete(drm_pipe->du_private, complete);
> + drm_pipe->du_complete(drm_pipe->du_private, complete, 0);
>
> if (completion & VSP1_DL_FRAME_END_INTERNAL) {
> drm_pipe->force_brx_release = false;
> @@ -739,8 +739,10 @@ EXPORT_SYMBOL_GPL(vsp1_du_atomic_update);
> * vsp1_du_atomic_flush - Commit an atomic update
> * @dev: the VSP device
> * @pipe_index: the DRM pipeline index
> + * @cfg: atomic pipe configuration
> */
> -void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index)
> +void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
> + const struct vsp1_du_atomic_pipe_config *cfg)
> {
> struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.h
> b/drivers/media/platform/vsp1/vsp1_drm.h
> index f4af1b2b12d6..e5b88b28806c 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.h
> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> @@ -35,7 +35,7 @@ struct vsp1_drm_pipeline {
> wait_queue_head_t wait_queue;
>
> /* Frame synchronisation */
> - void (*du_complete)(void *, bool);
> + void (*du_complete)(void *data, bool completed, u32 crc);
> void *du_private;
> };
>
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index ff7ef894465d..ac63a9928a79 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -34,7 +34,7 @@ struct vsp1_du_lif_config {
> unsigned int width;
> unsigned int height;
>
> - void (*callback)(void *, bool);
> + void (*callback)(void *data, bool completed, u32 crc);
Did we have to add parameters to this callback for the dynamic BRx work ?
. If so we might have to consider turning
it into a struct perhaps! - but for now or otherwise I think this will be OK.
Ok - so I stopped being lazy and went to take a look. We turned completed into a
bitfield - so I think actually that's enough to store any more flags we might
come up with, and the CRC doesn't fit in there :D
So lets leave this as adding an extra parameter for now.
> void *callback_data;
> };
>
> @@ -61,11 +61,36 @@ struct vsp1_du_atomic_config {
> unsigned int zpos;
> };
>
> +/**
> + * enum vsp1_du_crc_source - Source used for CRC calculation
> + * @VSP1_DU_CRC_NONE: CRC calculation disabled
> + * @VSP_DU_CRC_PLANE: Perform CRC calculation on an input plane
> + * @VSP_DU_CRC_OUTPUT: Perform CRC calculation on the composed output
I see Jacopo has already caught these ...
> + */
> +enum vsp1_du_crc_source {
> + VSP1_DU_CRC_NONE,
> + VSP1_DU_CRC_PLANE,
> + VSP1_DU_CRC_OUTPUT,
> +};
> +
> +/**
> + * struct vsp1_du_atomic_pipe_config - VSP atomic pipe configuration
> parameters
> + * @crc.source: source for CRC calculation
> + * @crc.index: index of the CRC source plane (when crc.source is
Re: [PATCH v2 6/8] v4l: vsp1: Add support for the DISCOM entity
Hi Laurent,
On 22/04/18 23:34, Laurent Pinchart wrote:
> The DISCOM calculates a CRC on a configurable window of the frame. It
> interfaces to the VSP through the UIF glue, hence the name used in the
> code.
>
> The module supports configuration of the CRC window through the crop
> rectangle on the ink pad of the corresponding entity. However, unlike
> the traditional V4L2 subdevice model, the crop rectangle does not
> influence the format on the source pad.
>
> Modeling the DISCOM as a sink-only entity would allow adhering to the
> V4L2 subdevice model at the expense of more complex code in the driver,
> as at the hardware level the UIF is handled as a sink+source entity. As
> the DISCOM is only present in R-Car Gen3 VSP-D and VSP-DL instances it
> is not exposed to userspace through V4L2 but controlled through the DU
> driver. We can thus change this model later if needed without fear of
> affecting userspace.
>
> Signed-off-by: Laurent Pinchart
Registers check out against the data sheet, and I haven't spotted anything else
here on top of the comments Jacopo has highlighted.
I'll put a +1 on Jacopo's suggestion for handling the m3-w quirk though. It's an
adjustment to the width/height in the event the quirk is active - so adjusting
the values after they are set feels 'right' to me.
Reviewed-by: Kieran Bingham
> ---
> Changes since v1:
>
> - Don't return uninitialized value from uif_set_selection()
> ---
> drivers/media/platform/vsp1/Makefile | 2 +-
> drivers/media/platform/vsp1/vsp1.h| 4 +
> drivers/media/platform/vsp1/vsp1_drv.c| 20 +++
> drivers/media/platform/vsp1/vsp1_entity.c | 6 +
> drivers/media/platform/vsp1/vsp1_entity.h | 1 +
> drivers/media/platform/vsp1/vsp1_regs.h | 41 +
> drivers/media/platform/vsp1/vsp1_uif.c| 271
> ++
> drivers/media/platform/vsp1/vsp1_uif.h| 32
> 8 files changed, 376 insertions(+), 1 deletion(-)
> create mode 100644 drivers/media/platform/vsp1/vsp1_uif.c
> create mode 100644 drivers/media/platform/vsp1/vsp1_uif.h
>
> diff --git a/drivers/media/platform/vsp1/Makefile
> b/drivers/media/platform/vsp1/Makefile
> index 596775f932c0..4bb4dcbef7b5 100644
> --- a/drivers/media/platform/vsp1/Makefile
> +++ b/drivers/media/platform/vsp1/Makefile
> @@ -5,6 +5,6 @@ vsp1-y+= vsp1_rpf.o
> vsp1_rwpf.o vsp1_wpf.o
> vsp1-y += vsp1_clu.o vsp1_hsit.o
> vsp1_lut.o
> vsp1-y += vsp1_brx.o vsp1_sru.o
> vsp1_uds.o
> vsp1-y += vsp1_hgo.o vsp1_hgt.o
> vsp1_histo.o
> -vsp1-y += vsp1_lif.o
> +vsp1-y += vsp1_lif.o vsp1_uif.o
>
> obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1.o
> diff --git a/drivers/media/platform/vsp1/vsp1.h
> b/drivers/media/platform/vsp1/vsp1.h
> index 9cf4e1c4b036..33f632331474 100644
> --- a/drivers/media/platform/vsp1/vsp1.h
> +++ b/drivers/media/platform/vsp1/vsp1.h
> @@ -36,10 +36,12 @@ struct vsp1_lut;
> struct vsp1_rwpf;
> struct vsp1_sru;
> struct vsp1_uds;
> +struct vsp1_uif;
>
> #define VSP1_MAX_LIF 2
> #define VSP1_MAX_RPF 5
> #define VSP1_MAX_UDS 3
> +#define VSP1_MAX_UIF 2
> #define VSP1_MAX_WPF 4
>
> #define VSP1_HAS_LUT (1 << 1)
> @@ -60,6 +62,7 @@ struct vsp1_device_info {
> unsigned int lif_count;
> unsigned int rpf_count;
> unsigned int uds_count;
> + unsigned int uif_count;
> unsigned int wpf_count;
> unsigned int num_bru_inputs;
> bool uapi;
> @@ -86,6 +89,7 @@ struct vsp1_device {
> struct vsp1_rwpf *rpf[VSP1_MAX_RPF];
> struct vsp1_sru *sru;
> struct vsp1_uds *uds[VSP1_MAX_UDS];
> + struct vsp1_uif *uif[VSP1_MAX_UIF];
> struct vsp1_rwpf *wpf[VSP1_MAX_WPF];
>
> struct list_head entities;
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> b/drivers/media/platform/vsp1/vsp1_drv.c
> index 331a2e0af0d3..d29f9c4baebe 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -35,6 +35,7 @@
> #include "vsp1_rwpf.h"
> #include "vsp1_sru.h"
> #include "vsp1_uds.h"
> +#include "vsp1_uif.h"
> #include "vsp1_video.h"
>
> /*
> -
> @@ -409,6 +410,19 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
> list_add_tail(&uds->entity.list_dev, &vsp1->entities);
> }
>
> + for (i = 0; i < vsp1->info->uif_count; ++i) {
> + struct vsp1_uif *uif;
> +
> + uif = vsp1_uif_create(vsp1, i);
> + if (IS_ERR(uif)) {
> + ret = PTR_ERR(uif);
> + goto done;
> + }
> +
> + vsp1->uif[i] = uif;
> + list_add_tail(&uif->entity.list_dev, &vsp1->entitie
Re: [PATCH v2 1/2] pinctrl: sh-pfc: Add r8a77470 PFC support
On 04/28/2018 08:40 PM, Sergei Shtylyov wrote:
Subject: Re: [PATCH v2 1/2] pinctrl: sh-pfc: Add r8a77470 PFC support
On Wed, Apr 4, 2018 at 5:22 PM, Biju Das wrote:
> Add PFC support for the R8A77470 SoC including pin groups for some
> on-chip devices such as SCIF, AVB and MMC.
>
> Signed-off-by: Biju Das
> Reviewed-by: Fabrizio Castro
>>
> --- /dev/null
> +++ b/drivers/pinctrl/sh-pfc/pfc-r8a77470.c
>>
> +/* - AVB
> + */
> static const
unsigned int avb_link_pins[] = {
> + RCAR_GP_PIN(5, 14),
> +};
> +static const unsigned int avb_link_mux[] = {
> + AVB_LINK_MARK,
> +};
> +static const unsigned int avb_magic_pins[] = {
> + RCAR_GP_PIN(5, 15),
> +};
> +static const unsigned int avb_magic_mux[] = {
> + AVB_MAGIC_MARK,
> +};
> +static const unsigned int avb_phy_int_pins[] = {
> + RCAR_GP_PIN(5, 16),
> +};
> +static const unsigned int avb_phy_int_mux[] = {
> + AVB_PHY_INT_MARK,
> +};
> +static const unsigned int avb_mdio_pins[] = {
> + RCAR_GP_PIN(5, 12), RCAR_GP_PIN(5, 13), }; static const
> +unsigned int avb_mdio_mux[] = {
> + AVB_MDC_MARK, AVB_MDIO_MARK,
> +};
> +static const unsigned int avb_mii_pins[] = {
> + RCAR_GP_PIN(3, 14), RCAR_GP_PIN(3, 15), RCAR_GP_PIN(3, 16),
> + RCAR_GP_PIN(3, 27),
> +
> + RCAR_GP_PIN(3, 2), RCAR_GP_PIN(3, 3), RCAR_GP_PIN(3, 4),
> + RCAR_GP_PIN(3, 5),
> +
> + RCAR_GP_PIN(3, 10), RCAR_GP_PIN(3, 0), RCAR_GP_PIN(3, 1),
> + RCAR_GP_PIN(5, 17), RCAR_GP_PIN(3, 13), RCAR_GP_PIN(5, 23),
> + RCAR_GP_PIN(3, 12),
> +};
> +static const unsigned int avb_mii_mux[] = {
> + AVB_TXD0_MARK, AVB_TXD1_MARK, AVB_TXD2_MARK,
> + AVB_TXD3_MARK,
> +
> + AVB_RXD0_MARK, AVB_RXD1_MARK, AVB_RXD2_MARK,
> + AVB_RXD3_MARK,
> +
> + AVB_RX_ER_MARK, AVB_RX_CLK_MARK, AVB_RX_DV_MARK,
> + AVB_CRS_MARK, AVB_TX_EN_MARK, AVB_TX_ER_MARK,
> + AVB_TX_CLK_MARK,
You forgot AVB_COL, which is GP5_18?
>>>
>>> GP5_18 is shred between the signals AVB_COL and VI0_CLK. As per the RZ/G1C
>>> board schematic /hardware user guide
>>> Ethernet Phy collision pin(AVB_COL) pin is not populated on this board by
>>> default. It is populated only for
>>> Video Input Channel0 pixel clock.
>>>
>>> Since it is initial submission, I thought of adding only board specific
>>> pins first and later
>>> add this collision pin alone as a separate pin entry in the avb group.
>>> That way we can support existing board
>>> as well as any future RZ/G1C board which populates this pin. Are you ok for
>>> this approach?
>
>> Oops. That means our grouping is suboptimal. Perhaps we should revisit
>> (for all SoCs, while keeping backwards compatibility)?
>
>
>> After reading some wikipedia, I came up with:
>>
>> mii_tx
>> mii_tx_er (optional)
>> mii_rx
>
>I don't see why we should have the separate TX/RX subgroups just because
> they're
> described separately in the Wikipedia. Or did you have some special reason to
> do it?
>
>> mii_col_crs (optional, half duplex)
>>
>> However, given your board uses AVB_CRS without AVB_COL, that would still
>> not be sufficient, so the last group should be split to cover your use case?
>
>> BTW, how does it work with AVB_COL not wired to anything by default, and thus
>> floating? Do you enable pull-up/down for that pin in the PFC, or is the pin
>> just ignored in full-duplex mode?
>
>Well, I'm seeing the strong possibility the half-duplex mode just doesn't
> work as
> the collision indication isn't available.
And indeed, the manuals tell me that RZ/G (and R-Car gen2) EtherAVB only
supports
the full-duplex mode.
> [...]
>
>> Gr{oetje,eeting}s,
>>
>> Geert
MBR, Sergei
Re: [PATCH v2 7/8] v4l: vsp1: Integrate DISCOM in display pipeline
Hi Laurent,
On 22/04/18 23:34, Laurent Pinchart wrote:
> The DISCOM is used to compute CRCs on display frames. Integrate it in
> the display pipeline at the output of the blending unit to process
> output frames.
>
> Computing CRCs on input frames is possible by positioning the DISCOM at
> a different point in the pipeline. This use case isn't supported at the
> moment and could be implemented by extending the API between the VSP1
> and DU drivers if needed.
>
> Signed-off-by: Laurent Pinchart
Only a couple of small questions - but nothing to block an RB tag.
Reviewed-by: Kieran Bingham
> ---
> drivers/media/platform/vsp1/vsp1_drm.c | 115
> -
> drivers/media/platform/vsp1/vsp1_drm.h | 12
> 2 files changed, 124 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index 5fc31578f9b0..7864b43a90e1 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -22,6 +22,7 @@
> #include "vsp1_lif.h"
> #include "vsp1_pipe.h"
> #include "vsp1_rwpf.h"
> +#include "vsp1_uif.h"
>
> #define BRX_NAME(e) (e)->type == VSP1_ENTITY_BRU ? "BRU" : "BRS"
>
> @@ -35,8 +36,13 @@ static void vsp1_du_pipeline_frame_end(struct
> vsp1_pipeline *pipe,
> struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
> bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;
>
> - if (drm_pipe->du_complete)
> - drm_pipe->du_complete(drm_pipe->du_private, complete, 0);
> + if (drm_pipe->du_complete) {
> + struct vsp1_entity *uif = drm_pipe->uif;
> + u32 crc;
> +
> + crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
> + drm_pipe->du_complete(drm_pipe->du_private, complete, crc);
> + }
>
> if (completion & VSP1_DL_FRAME_END_INTERNAL) {
> drm_pipe->force_brx_release = false;
> @@ -48,10 +54,65 @@ static void vsp1_du_pipeline_frame_end(struct
> vsp1_pipeline *pipe,
> * Pipeline Configuration
> */
>
> +/*
> + * Insert the UIF in the pipeline between the prev and next entities. If no
> UIF
> + * is available connect the two entities directly.
> + */
> +static int vsp1_du_insert_uif(struct vsp1_device *vsp1,
> + struct vsp1_pipeline *pipe,
> + struct vsp1_entity *uif,
> + struct vsp1_entity *prev, unsigned int prev_pad,
> + struct vsp1_entity *next, unsigned int next_pad)
> +{
> + int ret;
> +
> + if (uif) {
> + struct v4l2_subdev_format format;
> +
> + prev->sink = uif;
> + prev->sink_pad = UIF_PAD_SINK;
> +
> + memset(&format, 0, sizeof(format));
> + format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + format.pad = prev_pad;
> +
> + ret = v4l2_subdev_call(&prev->subdev, pad, get_fmt, NULL,
> +&format);
> + if (ret < 0)
> + return ret;
> +
> + format.pad = UIF_PAD_SINK;
> +
> + ret = v4l2_subdev_call(&uif->subdev, pad, set_fmt, NULL,
> +&format);
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on UIF sink\n",
> + __func__, format.format.width, format.format.height,
> + format.format.code);
> +
> + /*
> + * The UIF doesn't mangle the format between its sink and
> + * source pads, so there is no need to retrieve the format on
> + * its source pad.
> + */
> +
> + uif->sink = next;
> + uif->sink_pad = next_pad;
> + } else {
> + prev->sink = next;
> + prev->sink_pad = next_pad;
> + }
> +
> + return 0;
> +}
> +
> /* Setup one RPF and the connected BRx sink pad. */
> static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1,
> struct vsp1_pipeline *pipe,
> struct vsp1_rwpf *rpf,
> + struct vsp1_entity *uif,
> unsigned int brx_input)
> {
> struct v4l2_subdev_selection sel;
> @@ -122,6 +183,12 @@ static int vsp1_du_pipeline_setup_rpf(struct vsp1_device
> *vsp1,
> if (ret < 0)
> return ret;
>
> + /* Insert and configure the UIF if available. */
> + ret = vsp1_du_insert_uif(vsp1, pipe, uif, &rpf->entity, RWPF_PAD_SOURCE,
> + pipe->brx, brx_input);
> + if (ret < 0)
> + return ret;
> +
> /* BRx sink, propagate the format from the RPF source. */
> format.pad = brx_input;
>
> @@ -297,7 +364,10 @@ static unsigned int rpf_zpos(struct vsp1_device *vsp1,
> stru
Re: [PATCH v2 7/8] v4l: vsp1: Integrate DISCOM in display pipeline
Hi Kieran,
On Saturday, 28 April 2018 21:58:53 EEST Kieran Bingham wrote:
> On 22/04/18 23:34, Laurent Pinchart wrote:
> > The DISCOM is used to compute CRCs on display frames. Integrate it in
> > the display pipeline at the output of the blending unit to process
> > output frames.
> >
> > Computing CRCs on input frames is possible by positioning the DISCOM at
> > a different point in the pipeline. This use case isn't supported at the
> > moment and could be implemented by extending the API between the VSP1
> > and DU drivers if needed.
> >
> > Signed-off-by: Laurent Pinchart
> >
>
> Only a couple of small questions - but nothing to block an RB tag.
>
> Reviewed-by: Kieran Bingham
>
> > ---
> >
> > drivers/media/platform/vsp1/vsp1_drm.c | 115 +++-
> > drivers/media/platform/vsp1/vsp1_drm.h | 12
> > 2 files changed, 124 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > b/drivers/media/platform/vsp1/vsp1_drm.c index 5fc31578f9b0..7864b43a90e1
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
[snip]
> > @@ -748,6 +847,9 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned
> > int pipe_index,>
> > struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
> > struct vsp1_pipeline *pipe = &drm_pipe->pipe;
> >
> > + drm_pipe->crc.source = cfg->crc.source;
> > + drm_pipe->crc.index = cfg->crc.index;
>
> I think this could be shortened to
>
> drm_pipe->crc = cfg->crc;
>
> Or is that a GCC extension. Either way, it's just a matter of taste, and you
> might prefer to be more explicit.
That's what I had written in the first place, only to have gcc throwing an
error because the crc fields are anonymous structures.
> > +
> > vsp1_du_pipeline_setup_inputs(vsp1, pipe);
> > vsp1_du_pipeline_configure(pipe);
> > mutex_unlock(&vsp1->drm->lock);
[snip]
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.h
> > b/drivers/media/platform/vsp1/vsp1_drm.h index e5b88b28806c..1e7670955ef0
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.h
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> > @@ -13,6 +13,8 @@
> > #include
> > #include
> >
> > +#include
> > +
> > #include "vsp1_pipe.h"
> >
> > /**
> > @@ -22,6 +24,9 @@
> > * @height: output display height
> > * @force_brx_release: when set, release the BRx during the next
> > reconfiguration
> > * @wait_queue: wait queue to wait for BRx release completion
> > + * @uif: UIF entity if available for the pipeline
> > + * @crc.source: source for CRC calculation
> > + * @crc.index: index of the CRC source plane (when crc.source is set to
> > plane)
> > * @du_complete: frame completion callback for the DU driver (optional)
> > * @du_private: data to be passed to the du_complete callback
> > */
> > @@ -34,6 +39,13 @@ struct vsp1_drm_pipeline {
> > bool force_brx_release;
> > wait_queue_head_t wait_queue;
> >
> > + struct vsp1_entity *uif;
> > +
> > + struct {
> > + enum vsp1_du_crc_source source;
> > + unsigned int index;
> > + } crc;
>
> Does this have to be duplicated ? Or can it be included from the API
> header...
It's a good point. I thought it might not be worth it just for two fields, but
I see no compelling reason against it. I'll introduce a new structure.
> > +
> > /* Frame synchronisation */
> > void (*du_complete)(void *data, bool completed, u32 crc);
> > void *du_private;
--
Regards,
Laurent Pinchart
Re: [PATCH v2 8/8] drm: rcar-du: Add support for CRC computation
Hi Laurent,
On 22/04/18 23:34, Laurent Pinchart wrote:
> Implement CRC computation configuration and reporting through the DRM
> debugfs-based CRC API. The CRC source can be configured to any input
> plane or the pipeline output.
>
> Signed-off-by: Laurent Pinchart
I don't think I have any actual blocking questions here, so feel free to add a
Reviewed-by: Kieran Bingham
I'll not be in distress if the CRC structures remain duplicated (although I see
from your other mail you've considered defining the structure non-anonymously
--
Kieran
> ---
> Changes since v1:
>
> - Format the source names using plane IDs instead of plane indices
> ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 156
> +++--
> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 19
> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 7 ++
> 3 files changed, 176 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index c4420538ec85..d71d709fe3d9 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -691,6 +691,52 @@ static const struct drm_crtc_helper_funcs
> crtc_helper_funcs = {
> .atomic_disable = rcar_du_crtc_atomic_disable,
> };
>
> +static struct drm_crtc_state *
> +rcar_du_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
> +{
> + struct rcar_du_crtc_state *state;
> + struct rcar_du_crtc_state *copy;
> +
> + if (WARN_ON(!crtc->state))
> + return NULL;
> +
> + state = to_rcar_crtc_state(crtc->state);
> + copy = kmemdup(state, sizeof(*state), GFP_KERNEL);
> + if (copy == NULL)
> + return NULL;
> +
> + __drm_atomic_helper_crtc_duplicate_state(crtc, ©->state);
> +
> + return ©->state;
> +}
> +
> +static void rcar_du_crtc_atomic_destroy_state(struct drm_crtc *crtc,
> + struct drm_crtc_state *state)
> +{
> + __drm_atomic_helper_crtc_destroy_state(state);
> + kfree(to_rcar_crtc_state(state));
> +}
> +
> +static void rcar_du_crtc_reset(struct drm_crtc *crtc)
> +{
> + struct rcar_du_crtc_state *state;
> +
> + if (crtc->state) {
> + rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
> + crtc->state = NULL;
> + }
> +
> + state = kzalloc(sizeof(*state), GFP_KERNEL);
> + if (state == NULL)
> + return;
> +
> + state->crc.source = VSP1_DU_CRC_NONE;
> + state->crc.index = 0;
> +
> + crtc->state = &state->state;
> + crtc->state->crtc = crtc;
> +}
> +
> static int rcar_du_crtc_enable_vblank(struct drm_crtc *crtc)
> {
> struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> @@ -710,15 +756,111 @@ static void rcar_du_crtc_disable_vblank(struct
> drm_crtc *crtc)
> rcrtc->vblank_enable = false;
> }
>
> -static const struct drm_crtc_funcs crtc_funcs = {
> - .reset = drm_atomic_helper_crtc_reset,
> +static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
> +const char *source_name,
> +size_t *values_cnt)
> +{
> + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> + struct drm_modeset_acquire_ctx ctx;
> + struct drm_crtc_state *crtc_state;
> + struct drm_atomic_state *state;
> + enum vsp1_du_crc_source source;
> + unsigned int index = 0;
> + unsigned int i;
> + int ret;
> +
> + /*
> + * Parse the source name. Supported values are "plane%u" to compute the
> + * CRC on an input plane (%u is the plane ID), and "auto" to compute the
> + * CRC on the composer (VSP) output.
> + */
> + if (!source_name) {
> + source = VSP1_DU_CRC_NONE;
> + } else if (!strcmp(source_name, "auto")) {
> + source = VSP1_DU_CRC_OUTPUT;
> + } else if (strstarts(source_name, "plane")) {
> + source = VSP1_DU_CRC_PLANE;
> +
> + ret = kstrtouint(source_name + strlen("plane"), 10, &index);
> + if (ret < 0)
> + return ret;
> +
> + for (i = 0; i < rcrtc->vsp->num_planes; ++i) {
> + if (index == rcrtc->vsp->planes[i].plane.base.id) {
> + index = i;
> + break;
> + }
> + }
> +
> + if (i >= rcrtc->vsp->num_planes)
> + return -EINVAL;
> + } else {
> + return -EINVAL;
> + }
> +
> + *values_cnt = 1;
> +
> + /* Perform an atomic commit to set the CRC source. */
> + drm_modeset_acquire_init(&ctx, 0);
> +
> + state = drm_atomic_state_alloc(crtc->dev);
> + if (!state) {
> + ret = -ENOMEM;
> + goto unlock;
> + }
> +
> + state->acquire_ctx = &ctx;
> +
> +retry:
> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> + if (!IS_ERR(crtc_state)) {
> + struct rcar_du_crtc_state *rcr
[PATCH] i2c: rcar: enhance comment to avoid regressions
From: Wolfram Sang Give a clear testcase for people wishing to change this code. It is also a reminder for me if people ask about it. Signed-off-by: Wolfram Sang --- drivers/i2c/busses/i2c-rcar.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index c6915b835396..9d8d5b91220f 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -542,6 +542,8 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) * If next received data is the _LAST_, go to STOP phase. Might be * overwritten by REP START when setting up a new msg. Not elegant * but the only stable sequence for REP START I have found so far. +* If you want to change this code, make sure sending one transfer with +* four messages (WR-RD-WR-RD) works! */ if (priv->pos + 1 >= msg->len) rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP); -- 2.11.0
Re: [PATCH v2 8/8] drm: rcar-du: Add support for CRC computation
Hi Kieran,
On Saturday, 28 April 2018 22:16:48 EEST Kieran Bingham wrote:
> On 22/04/18 23:34, Laurent Pinchart wrote:
> > Implement CRC computation configuration and reporting through the DRM
> > debugfs-based CRC API. The CRC source can be configured to any input
> > plane or the pipeline output.
> >
> > Signed-off-by: Laurent Pinchart
> >
>
> I don't think I have any actual blocking questions here, so feel free to add
> a
>
> Reviewed-by: Kieran Bingham
>
> I'll not be in distress if the CRC structures remain duplicated (although I
> see from your other mail you've considered defining the structure
> non-anonymously
>
> > ---
> > Changes since v1:
> >
> > - Format the source names using plane IDs instead of plane indices
> > ---
> >
> > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 156 ++--
> > drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 19
> > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 7 ++
> > 3 files changed, 176 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index c4420538ec85..d71d709fe3d9
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
[snip]
> > @@ -710,15 +756,111 @@ static void rcar_du_crtc_disable_vblank(struct
> > drm_crtc *crtc)
> > rcrtc->vblank_enable = false;
> > }
> >
> > -static const struct drm_crtc_funcs crtc_funcs = {
> > - .reset = drm_atomic_helper_crtc_reset,
> > +static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
> > + const char *source_name,
> > + size_t *values_cnt)
> > +{
> > + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> > + struct drm_modeset_acquire_ctx ctx;
> > + struct drm_crtc_state *crtc_state;
> > + struct drm_atomic_state *state;
> > + enum vsp1_du_crc_source source;
> > + unsigned int index = 0;
> > + unsigned int i;
> > + int ret;
> > +
> > + /*
> > +* Parse the source name. Supported values are "plane%u" to compute the
> > +* CRC on an input plane (%u is the plane ID), and "auto" to compute
> > the
> > +* CRC on the composer (VSP) output.
> > +*/
> > + if (!source_name) {
> > + source = VSP1_DU_CRC_NONE;
> > + } else if (!strcmp(source_name, "auto")) {
> > + source = VSP1_DU_CRC_OUTPUT;
> > + } else if (strstarts(source_name, "plane")) {
> > + source = VSP1_DU_CRC_PLANE;
> > +
> > + ret = kstrtouint(source_name + strlen("plane"), 10, &index);
> > + if (ret < 0)
> > + return ret;
> > +
> > + for (i = 0; i < rcrtc->vsp->num_planes; ++i) {
> > + if (index == rcrtc->vsp->planes[i].plane.base.id) {
> > + index = i;
> > + break;
> > + }
> > + }
> > +
> > + if (i >= rcrtc->vsp->num_planes)
> > + return -EINVAL;
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + *values_cnt = 1;
> > +
> > + /* Perform an atomic commit to set the CRC source. */
> > + drm_modeset_acquire_init(&ctx, 0);
> > +
> > + state = drm_atomic_state_alloc(crtc->dev);
> > + if (!state) {
> > + ret = -ENOMEM;
> > + goto unlock;
> > + }
> > +
> > + state->acquire_ctx = &ctx;
> > +
> > +retry:
> > + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > + if (!IS_ERR(crtc_state)) {
> > + struct rcar_du_crtc_state *rcrtc_state;
> > +
> > + rcrtc_state = to_rcar_crtc_state(crtc_state);
> > + rcrtc_state->crc.source = source;
> > + rcrtc_state->crc.index = index;
> > +
> > + ret = drm_atomic_commit(state);
>
> Does this 'cost' a vblank ? (as in - does this action being performed from
> userspace have the capability to cause flicker, or loss of frame?)
It shouldn't cause flicker, but it could delay atomic commits queued by
userspace by one frame. It's not ideal, but given that changing the CRC source
requires a pipeline update on the VSP side, creating an atomic commit
internally in the driver was the easiest solution.
Now that the BRU/BRS series has created infrastructure on the VSP side to
perform pipeline reconfiguration we could also take advantage of that, but it
would still incur a delay of one frame, so I don't think that would really
help.
> > + } else {
> > + ret = PTR_ERR(crtc_state);
> > + }
> > +
> > + if (ret == -EDEADLK) {
> > + drm_atomic_state_clear(state);
> > + drm_modeset_backoff(&ctx);
> > + goto retry;
>
> Not knowing what the -EDEADLK represents yet, this isn't an infinite loop
> opportunity is it ? (I assume the state_clear(),backoff() clean up and
> prevent that.)
-EDEADLK comes from the drm_atomic_get_crtc_state() or drm_atomic_commit()
calls trying to lock a ww-mutex in a way that would cause a deadlock.
