Hi Hans, Thanks for the review. Please see my comments below. > -----Original Message----- > From: Hans Verkuil <hverkuil@xxxxxxxxx> > Sent: Thursday, June 25, 2020 3:13 PM > To: Vishal Sagar <vsagar@xxxxxxxxxx>; Hyun Kwon <hyunk@xxxxxxxxxx>; > laurent.pinchart@xxxxxxxxxxxxxxxx; mchehab@xxxxxxxxxx; > robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; Michal Simek > <michals@xxxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; joe@xxxxxxxxxxx > Cc: Sandip Kothari <sandipk@xxxxxxxxxx>; Dinesh Kumar <dineshk@xxxxxxxxxx> > Subject: Re: [PATCH v3 3/3] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem > driver > > On 18/06/2020 07:33, Vishal Sagar wrote: > > The Xilinx UHD-SDI Rx subsystem soft IP is used to capture native SDI > > streams from SDI sources like SDI broadcast equipment like cameras and > > mixers. This block outputs either native SDI, native video or > > AXI4-Stream compliant data stream for further processing. Please refer > > to PG290 for details. > > > > The driver is used to configure the IP to add framer, search for > > specific modes, get the detected mode, stream parameters, errors, etc. > > It also generates events for video lock/unlock, bridge over/under flow. > > > > The driver supports 10/12 bpc YUV 422 media bus format currently. It > > also decodes the stream parameters based on the ST352 packet embedded in > the > > stream. In case the ST352 packet isn't present in the stream, the core's > > detected properties are used to set stream properties. > > > > The driver currently supports only the AXI4-Stream IP configuration. > > > > Signed-off-by: Vishal Sagar <vishal.sagar@xxxxxxxxxx> > > --- > > v3 > > - fixed KConfig with better description > > - removed unnecessary header files > > - converted uppercase to lowercase for all hex values > > - merged core struct to state struct > > - removed most one line functions and replaced with direct reg > > read/write or macros > > - dt property bpp to bpc. default 10. not mandatory. > > - fixed subscribe events, log_status, s_stream > > - merged overflow/underflow to one event > > - moved all controls to xilinx-sdirxss.h > > - max events from 128 to 8 > > - used FIELD_GET() instead of custom macro > > - updated the controls documentation > > - added spinlock > > - removed 3GB control and added mode to detect bitmask > > - fixed format for (width, height, colorspace, xfer func, etc) > > - added dv_timings_cap, s/g_dv_timings > > - fixed set/get_format > > - fix v4l control registrations > > - fix order of registration / deregistration in probe() remove() > > - fixed other comments from Hyun, Laurent and Hans > > - things yet to close > > - adding source port for connector (Laurent's suggestion) > > - adding new FIELD type for Transport Stream > V4L2_FIELD_ALTERNATE_PROG (Han's suggestion) > > - Update / remove EDH or CRC related controls > > > > v2 > > - Added DV timing support based on Hans Verkuilś feedback > > - More documentation to custom v4l controls and events > > - Fixed Hyunś comments > > - Added macro for masking and shifting as per Joe Perches comments > > - Updated to latest as per Xilinx github repo driver like > > adding new DV timings not in mainline yet uptill 03/21/20 > > > > drivers/media/platform/xilinx/Kconfig | 11 + > > drivers/media/platform/xilinx/Makefile | 1 + > > .../media/platform/xilinx/xilinx-sdirxss.c | 2121 +++++++++++++++++ > > include/uapi/linux/v4l2-controls.h | 6 + > > include/uapi/linux/xilinx-sdirxss.h | 283 +++ > > 5 files changed, 2422 insertions(+) > > create mode 100644 drivers/media/platform/xilinx/xilinx-sdirxss.c > > create mode 100644 include/uapi/linux/xilinx-sdirxss.h > > > > diff --git a/drivers/media/platform/xilinx/Kconfig > b/drivers/media/platform/xilinx/Kconfig > > index 01c96fb66414..578cdcc1036e 100644 > > --- a/drivers/media/platform/xilinx/Kconfig > > +++ b/drivers/media/platform/xilinx/Kconfig > > @@ -12,6 +12,17 @@ config VIDEO_XILINX > > > > if VIDEO_XILINX > > > > +config VIDEO_XILINX_SDIRXSS > > + tristate "Xilinx UHD SDI Rx Subsystem" > > + help > > + Driver for Xilinx UHD-SDI Rx Subsystem. This is a V4L sub-device > > + based driver that takes input from a SDI source like SDI camera and > > + converts it into an AXI4-Stream. The subsystem comprises a SMPTE > > + UHD-SDI Rx core, a SDI Rx to Native Video bridge and a Video In to > > + AXI4-Stream bridge. The driver is used to set different stream > > + detection modes and identify stream properties to properly configure > > + downstream. > > + > > config VIDEO_XILINX_TPG > > tristate "Xilinx Video Test Pattern Generator" > > depends on VIDEO_XILINX > > diff --git a/drivers/media/platform/xilinx/Makefile > b/drivers/media/platform/xilinx/Makefile > > index 4cdc0b1ec7a5..3beaf24d832c 100644 > > --- a/drivers/media/platform/xilinx/Makefile > > +++ b/drivers/media/platform/xilinx/Makefile > > @@ -3,5 +3,6 @@ > > xilinx-video-objs += xilinx-dma.o xilinx-vip.o xilinx-vipp.o > > > > obj-$(CONFIG_VIDEO_XILINX) += xilinx-video.o > > +obj-$(CONFIG_VIDEO_XILINX_SDIRXSS) += xilinx-sdirxss.o > > obj-$(CONFIG_VIDEO_XILINX_TPG) += xilinx-tpg.o > > obj-$(CONFIG_VIDEO_XILINX_VTC) += xilinx-vtc.o > > diff --git a/drivers/media/platform/xilinx/xilinx-sdirxss.c > b/drivers/media/platform/xilinx/xilinx-sdirxss.c > > new file mode 100644 > > index 000000000000..e39aab7c656a > > --- /dev/null > > +++ b/drivers/media/platform/xilinx/xilinx-sdirxss.c > > @@ -0,0 +1,2121 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Driver for Xilinx SDI Rx Subsystem > > + * > > + * Copyright (C) 2017 - 2020 Xilinx, Inc. > > + * > > + * Contacts: Vishal Sagar <vishal.sagar@xxxxxxxxxx> > > + */ > > + > > +#include <dt-bindings/media/xilinx-sdi.h> > > +#include <linux/bits.h> > > +#include <linux/bitfield.h> > > +#include <linux/clk.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/xilinx-sdirxss.h> > > +#include <media/media-entity.h> > > +#include <media/v4l2-ctrls.h> > > +#include <media/v4l2-dv-timings.h> > > +#include <media/v4l2-event.h> > > +#include <media/v4l2-subdev.h> > > + <snip> > > + V4L2_DV_BT_CEA_3840X2160P60, > > + V4L2_DV_BT_CEA_4096X2160P24, > > + V4L2_DV_BT_CEA_4096X2160P25, > > + V4L2_DV_BT_CEA_4096X2160P30, > > + V4L2_DV_BT_CEA_4096X2160P48, > > + V4L2_DV_BT_CEA_4096X2160P50, > > + V4L2_DV_BT_CEA_4096X2160P60, > > + > > + XLNX_V4L2_DV_BT_2048X1080P24, > > + XLNX_V4L2_DV_BT_2048X1080P25, > > + XLNX_V4L2_DV_BT_2048X1080P30, > > + XLNX_V4L2_DV_BT_2048X1080I48, > > + XLNX_V4L2_DV_BT_2048X1080I50, > > + XLNX_V4L2_DV_BT_2048X1080I60, > > + XLNX_V4L2_DV_BT_2048X1080P48, > > + XLNX_V4L2_DV_BT_2048X1080P50, > > + XLNX_V4L2_DV_BT_2048X1080P60, > > + XLNX_V4L2_DV_BT_1920X1080I48, > > Looking at this: > > https://www.smpte.org/sites/default/files/images/SMPTE%20wallchart%232.6_ > 20_17-JULY%202017.pdf > > I would think that these formats are defined in one of the smpte standards, > probably ST 2048-2. > > Can you take a look if you have this standard? > I checked relevant SMPTE standards and mailed the DCI at dci[dot]info[at]dcimovies[dot]com. But I didn't get the timings for these DCI 2K resolutions. I wish to keep this as is for now. > > +}; > > + > > +struct xsdirxss_dv_map { > > + u32 width; > > + u32 height; > > + u32 fps; > > + struct v4l2_dv_timings timing; > > +}; > > + > > +static const struct xsdirxss_dv_map xsdirxss_dv_timings[] = { > > + /* SD - 720x487i60 */ > > + { 720, 243, 30, V4L2_DV_BT_SDI_720X487I60 }, > > + /* SD - 720x576i50 */ > > + { 720, 288, 25, V4L2_DV_BT_CEA_720X576I50 }, <snip> > > + > > + /* Refer Table 3 SMPTE ST 2081-10:2018 */ > > + switch (colorimetry) { > > + case XST352_BYTE3_COLORIMETRY_BT709: > > + format->colorspace = V4L2_COLORSPACE_REC709; > > + break; > > + /* When HDR support is added UHDTV will have BT2020 colorspace */ > > + case XST352_BYTE3_COLORIMETRY_UHDTV: > > + case XST352_BYTE3_COLORIMETRY_COLOR_VANC: > > + case XST352_BYTE3_COLORIMETRY_UNKNOWN: > > + default: > > + dev_err(dev, "Unknown colorimetry : %d\n", colorimetry); > > I'm a bit surprised. I would expect that SDTV over SDI would use > V4L2_COLORSPACE_SMPTE170M, but I don't see that here. > Yes you are correct about the colorspace for SD mode. Thanks for catching this. I will update the colorspace to V4L2_COLORSPACE_SMPTE170M in case of SD mode. > > + return -EINVAL; > > + } > > + > > + xsdirxss_get_framerate(&state->frame_interval, framerate); > > + <snip> > > + * video lock interrupt. > > + */ > > + xsdirxss->vidlockwin = ctrl->val; > > + xsdirxss_write(xsdirxss, XSDIRX_VID_LOCK_WINDOW_REG, > > + xsdirxss->vidlockwin); > > + break; > > + case V4L2_CID_XILINX_SDIRX_EDH_ERROR_SOURCES: > > + xsdirxss->edhmask = ctrl->val & XSDIRX_EDH_ALLERR_MASK; > > The '& XSDIRX_EDH_ALLERR_MASK' is not needed since the control's 'max' > value > is set to XSDIRX_EDH_ALLERR_MASK, so ctrl->val can't contain any other bits. > The control framework ensures that. > Ok. I will remove the '& XSDIRX_EDH_ALLERR_MASK' in next version. > > + xsdirxss_write(xsdirxss, XSDIRX_EDH_ERRCNT_EN_REG, > > + xsdirxss->edhmask); > > + break; > > + case V4L2_CID_XILINX_SDIRX_SEARCH_MODES: > > + if (!ctrl->val) { > > This check should be done in a try_ctrl function instead of s_ctrl. > > Interesting, this is the first bitmask control where you don't want it to > be 0. If we get more of these in the future, then it would make sense if this > is supported in the control framework itself (e.g. if min is set to 1, then > that means that the value can't be 0). But for now just check this in the > try_ctrl() function. > Agreed. I will implement a try_ctrl() for this. > > + spin_unlock_irqrestore(&xsdirxss->slock, flags); > > + dev_err(dev, "Select at least one mode!\n"); > > + return -EINVAL; > > + } > > + > > + if (xsdirxss->mode == XSDI_STD_3G) { > > + dev_dbg(dev, "Upto 3G supported\n"); > > + ctrl->val &= ~(BIT(XSDIRX_MODE_6G_OFFSET) | > > + BIT(XSDIRX_MODE_12GI_OFFSET) | > > + BIT(XSDIRX_MODE_12GF_OFFSET)); > > + } > > + > > + if (xsdirxss->mode == XSDI_STD_6G) { > > + dev_dbg(dev, "Upto 6G supported\n"); > > + ctrl->val &= ~(BIT(XSDIRX_MODE_12GI_OFFSET) | > > + BIT(XSDIRX_MODE_12GF_OFFSET)); > > + } > > This shouldn't be done here. Instead the 'max' field of the control must be > set correctly based on the mode. You can call v4l2_ctrl_modify_range() in > xsdirxss_probe() to update the max value. > Thanks for the suggestion I will update the max value accordingly and remove these checks. > > + > > + ret = xsdirx_set_modedetect(xsdirxss, ctrl->val); > > + if (!ret) > > + xsdirxss->searchmask = ctrl->val; > > + break; > > + default: > > + ret = -EINVAL; > > + break; > > + } > > + XSDIRX_CORE_ENABLE(xsdirxss); > > + > > + spin_unlock_irqrestore(&xsdirxss->slock, flags); > > + return ret; > > +} > > + > > +/** > > + * xsdirxss_g_volatile_ctrl - get the Xilinx SDI Rx controls > > + * @ctrl: Pointer to V4L2 control > > + * > > + * Return: 0 on success, errors otherwise > > + */ > > +static int xsdirxss_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + u32 val; > > + struct xsdirxss_state *xsdirxss = > > + container_of(ctrl->handler, > > + struct xsdirxss_state, ctrl_handler); > > + struct device *dev = xsdirxss->dev; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&xsdirxss->slock, flags); > > + if (!xsdirxss->vidlocked) { > > + spin_unlock_irqrestore(&xsdirxss->slock, flags); > > + dev_err(dev, "Can't get values when video not locked!\n"); > > + return -EINVAL; > > + } > > + switch (ctrl->id) { > > + case V4L2_CID_XILINX_SDIRX_MODE_DETECT: > > + val = xsdirxss_read(xsdirxss, XSDIRX_MODE_DET_STAT_REG); > > + val &= XSDIRX_MODE_DET_STAT_RX_MODE_MASK; > > + > > + switch (val) { > > + case XSDIRX_MODE_SD_MASK: > > + ctrl->val = XSDIRX_MODE_SD_OFFSET; > > + break; > > + case XSDIRX_MODE_HD_MASK: > > + ctrl->val = XSDIRX_MODE_HD_OFFSET; > > + break; > > + case XSDIRX_MODE_3G_MASK: > > + val = xsdirxss_read(xsdirxss, > XSDIRX_MODE_DET_STAT_REG); > > + val &= XSDIRX_MODE_DET_STAT_LVLB_3G_MASK; > > + ctrl->val = val ? XSDIRX_MODE_3GB_OFFSET : > > + XSDIRX_MODE_3GA_OFFSET; > > + break; > > + case XSDIRX_MODE_6G_MASK: > > + ctrl->val = XSDIRX_MODE_6G_OFFSET; > > + break; > > + case XSDIRX_MODE_12GI_MASK: > > + ctrl->val = XSDIRX_MODE_12GI_OFFSET; > > + break; > > + case XSDIRX_MODE_12GF_MASK: > > + ctrl->val = XSDIRX_MODE_12GF_OFFSET; > > + break; > > + } > > There is no interrupt that will tell you when the mode changes? It's much > nicer if updating this control is interrupt driven rather than requiring > userspace to poll. > The application need not poll this. When the mode changes, the V4L2_EVENT_SOURCE_CHANGE event will also be generated. The application can then get this control and get to know which mode is switched to. On the other hand, this info is also dumped as a part of log_status(). So this control will be dropped in the next version. > > + break; > > + case V4L2_CID_XILINX_SDIRX_CRC: > > + ctrl->val = xsdirxss_read(xsdirxss, XSDIRX_CRC_ERRCNT_REG); > > + xsdirxss_write(xsdirxss, XSDIRX_CRC_ERRCNT_REG, 0xffff); > > + break; > > + case V4L2_CID_XILINX_SDIRX_EDH_ERRCNT: > > + val = xsdirxss_read(xsdirxss, XSDIRX_MODE_DET_STAT_REG); > > + val &= XSDIRX_MODE_DET_STAT_RX_MODE_MASK; > > + if (val == XSDIRX_MODE_SD_MASK) { > > + ctrl->val = xsdirxss_read(xsdirxss, > > + XSDIRX_EDH_ERRCNT_REG); > > + } else { > > + spin_unlock_irqrestore(&xsdirxss->slock, flags); > > + dev_dbg(dev, "%d - not in SD mode\n", ctrl->id); > > + return -EINVAL; > > Getting a control value shouldn't fail. Just set ctrl->val to 0 and > return 0. You can leave the dev_dbg though, that can be useful. > Ok. I will set the ctrl->val to 0 in invalid case. > > + } > > + break; > > + case V4L2_CID_XILINX_SDIRX_EDH_STATUS: > > + val = xsdirxss_read(xsdirxss, XSDIRX_MODE_DET_STAT_REG); > > + val &= XSDIRX_MODE_DET_STAT_RX_MODE_MASK; > > + if (val == XSDIRX_MODE_SD_MASK) { > > + ctrl->val = xsdirxss_read(xsdirxss, > > + XSDIRX_EDH_STAT_REG); > > + } else { > > + spin_unlock_irqrestore(&xsdirxss->slock, flags); > > + dev_dbg(dev, "%d - not in SD mode\n", ctrl->id); > > + return -EINVAL; > > Ditto. > Noted. > > + } > > + break; > > + case V4L2_CID_XILINX_SDIRX_TS_IS_INTERLACED: > > + ctrl->val = xsdirxss->ts_is_interlaced; > > + break; > > I assume this control will disappear once you added support for > FIELD_ALTERNATE_PROG? > Yes this will be removed when V4L2_FIELD_ALTERNATE_PROG is added which means the video frame captured is progressive in nature but is being transported as 2 interlaced fields. > > + default: > > + spin_unlock_irqrestore(&xsdirxss->slock, flags); > > + dev_err(dev, "Get Invalid control id 0x%0x\n", ctrl->id); > > + return -EINVAL; > > You can drop the default case altogether: this function will only be called > for volatile controls. > Ok. I will remove the default case. > > + } > > + > > + spin_unlock_irqrestore(&xsdirxss->slock, flags); > > + return 0; > > +} > > + <snip> > > +static int xsdirxss_query_dv_timings(struct v4l2_subdev *sd, > > + struct v4l2_dv_timings *timings) > > +{ > > + struct xsdirxss_state *state = to_xsdirxssstate(sd); > > + unsigned int i; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&state->slock, flags); > > + if (!state->vidlocked) { > > + spin_unlock_irqrestore(&state->slock, flags); > > + return -ENOLCK; > > + } > > + > > + for (i = 0; i < ARRAY_SIZE(xsdirxss_dv_timings); i++) { > > + if (state->format.width == xsdirxss_dv_timings[i].width && > > + state->format.height == xsdirxss_dv_timings[i].height && > > + state->frame_interval.denominator == > > + (xsdirxss_dv_timings[i].fps * 1000)) { > > + *timings = xsdirxss_dv_timings[i].timing; > > + state->detected_timings_index = i; > > + spin_unlock_irqrestore(&state->slock, flags); > > + return 0; > > + } > > + } > > This limits the available timings to those explicitly supported by this > driver. Is that intended? What it you receive something that's not in this > list? Do you still want to be able to receive it? > The available timings are restricted to what the IP supports. If something that is not in this list is sent, then either the IP will not lock onto the video signal or it is missed from the list of resolutions + frame rates supported based on the SMPTE standards. > > + spin_unlock_irqrestore(&state->slock, flags); > > + > > + return -ERANGE; > > +} > > + > > +static int xsdirxss_s_dv_timings(struct v4l2_subdev *sd, > > + struct v4l2_dv_timings *timings) > > +{ > > + struct xsdirxss_state *state = to_xsdirxssstate(sd); > > + u32 i = state->detected_timings_index; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&state->slock, flags); > > + if (!state->vidlocked) { > > + spin_unlock_irqrestore(&state->slock, flags); > > + return -EINVAL; > > + } > > + > > + /* input timing should match query dv_timing */ > > + if (!v4l2_match_dv_timings(timings, > > + &xsdirxss_dv_timings[i].timing, > > + 0, false)) { > > + spin_unlock_irqrestore(&state->slock, flags); > > + return -EINVAL; > > + } > > + > > + state->current_timings = *timings; > > + > > + /* Update the media bus format */ > > + state->src_format = state->format; > > + spin_unlock_irqrestore(&state->slock, flags); > > + > > + return 0; > > +} > > + > > +static int xsdirxss_g_dv_timings(struct v4l2_subdev *sd, > > + struct v4l2_dv_timings *timings) > > +{ > > + struct xsdirxss_state *state = to_xsdirxssstate(sd); > > + > > + *timings = state->current_timings; > > + return 0; > > +} > > + > > +static int xsdirxss_dv_timings_cap(struct v4l2_subdev *sd, > > + struct v4l2_dv_timings_cap *cap) > > +{ > > + if (cap->pad != 0) > > + return -EINVAL; > > + > > + *cap = xsdirxss_timings_cap; > > + return 0; > > +} > > + > > +/* ----------------------------------------------------------------------------- > > + * Media Operations > > + */ > > + > > +static const struct media_entity_operations xsdirxss_media_ops = { > > + .link_validate = v4l2_subdev_link_validate > > +}; > > + > > +static const struct v4l2_ctrl_ops xsdirxss_ctrl_ops = { > > + .g_volatile_ctrl = xsdirxss_g_volatile_ctrl, > > + .s_ctrl = xsdirxss_s_ctrl > > +}; > > + > > +static const struct v4l2_ctrl_config xsdirxss_edh_ctrls[] = { > > + { > > + .ops = &xsdirxss_ctrl_ops, > > + .id = V4L2_CID_XILINX_SDIRX_EDH_ERROR_SOURCES, > > + .name = "SDI Rx : EDH Error Count Enable", > > No space before ':'. > Noted. I will fix this in next version. > > + .type = V4L2_CTRL_TYPE_BITMASK, > > + .min = 0, > > + .max = XSDIRX_EDH_ALLERR_MASK, > > + .def = 0, > > + }, { > > + .ops = &xsdirxss_ctrl_ops, > > + .id = V4L2_CID_XILINX_SDIRX_EDH_ERRCNT, > > + .name = "SDI Rx : EDH Error Count", > > + .type = V4L2_CTRL_TYPE_INTEGER, > > + .min = 0, > > + .max = 0xffff, > > + .step = 1, > > + .def = 0, > > + .flags = V4L2_CTRL_FLAG_VOLATILE | > V4L2_CTRL_FLAG_READ_ONLY, > > + }, { > > + .ops = &xsdirxss_ctrl_ops, > > + .id = V4L2_CID_XILINX_SDIRX_EDH_STATUS, > > + .name = "SDI Rx : EDH Status", > > + .type = V4L2_CTRL_TYPE_INTEGER, > > This should be a bitmask type. Should this be a control at all? Isn't this more > something to log with log_status? Is this something that an application needs to > use, or is it just a debugging aid? It feels like the latter. > Yes this is just a debugging aid like the EDH ERRCNT control. I agree it can be removed since it is logged in log_status. I will remove these controls in the next version. > > + .min = 0, > > + .max = 0xffffffff, > > + .step = 1, > > + .def = 0, > > + .flags = V4L2_CTRL_FLAG_VOLATILE | > V4L2_CTRL_FLAG_READ_ONLY, > > + } > > +}; > > + > > +static const struct v4l2_ctrl_config xsdirxss_ctrls[] = { > > + { > > + .ops = &xsdirxss_ctrl_ops, > > + .id = V4L2_CID_XILINX_SDIRX_FRAMER, > > + .name = "SDI Rx : Enable Framer", > > + .type = V4L2_CTRL_TYPE_BOOLEAN, > > + .min = false, > > + .max = true, > > + .step = 1, > > + .def = true, > > + }, { > > + .ops = &xsdirxss_ctrl_ops, > > + .id = V4L2_CID_XILINX_SDIRX_VIDLOCK_WINDOW, > > + .name = "SDI Rx : Video Lock Window", > > + .type = V4L2_CTRL_TYPE_INTEGER, > > + .min = 0, > > + .max = 0xffffffff, > > max should be 0x7fffffff since this is a s32. The video lock window register is 32 bit register and can accept 0 to 0xffffffff. So I will set max as -1 as -1 = 0xffffffff in s32. But this won't make sense. What would be the right way in this case to ensure the control values range is correct? > > > + .step = 1, > > + .def = XSDIRX_DEFAULT_VIDEO_LOCK_WINDOW, > > + }, { > > + .ops = &xsdirxss_ctrl_ops, > > + .id = V4L2_CID_XILINX_SDIRX_SEARCH_MODES, > > + .name = "SDI Rx : Modes search Mask", > > search -> Search > Noted. Will be fixed in next version. > > + .type = V4L2_CTRL_TYPE_BITMASK, > > + .min = 0, > > + .max = XSDIRX_DETECT_ALL_MODES, > > + .def = XSDIRX_DETECT_ALL_MODES, > > + }, { > > + .ops = &xsdirxss_ctrl_ops, > > + .id = V4L2_CID_XILINX_SDIRX_MODE_DETECT, > > + .name = "SDI Rx : Mode Detect Status", > > Mode Detect Status -> Detected Mode > > > + .type = V4L2_CTRL_TYPE_INTEGER, > > This is really a menu control. Noted. Will be fixed in next version. > > > + .min = XSDIRX_MODE_SD_OFFSET, > > + .max = XSDIRX_MODE_12GF_OFFSET, > > + .step = 1, > > + .flags = V4L2_CTRL_FLAG_VOLATILE | > V4L2_CTRL_FLAG_READ_ONLY, > > + }, { > > + .ops = &xsdirxss_ctrl_ops, > > + .id = V4L2_CID_XILINX_SDIRX_CRC, > > + .name = "SDI Rx : CRC Error status", > > + .type = V4L2_CTRL_TYPE_INTEGER, > > This is really two controls based on the description in the header: > > One bitmask for the 16 data streams and one accumulated error count. > This is very device specific operation which we can do in one control too. Let me know if you still think this should be split into 2 parts. > > + .min = 0, > > + .max = 0xffffffff, > > + .step = 1, > > + .def = 0, > > + .flags = V4L2_CTRL_FLAG_VOLATILE | > V4L2_CTRL_FLAG_READ_ONLY, > > + }, { <snip> > > + * This enum is used to prepare the bitmask of modes to be detected > > + */ > > +enum { > > + XSDIRX_MODE_SD_OFFSET = 0, > > + XSDIRX_MODE_HD_OFFSET, > > + XSDIRX_MODE_3GA_OFFSET, > > + XSDIRX_MODE_3GB_OFFSET, > > + XSDIRX_MODE_6G_OFFSET, > > + XSDIRX_MODE_12GI_OFFSET, > > + XSDIRX_MODE_12GF_OFFSET, > > + XSDIRX_MODE_NUM_SUPPORTED, > > +}; > > These are all standard SDI modes, right? > Most are standard modes like SD and HD mode. But 12G mode here is split as integer or fractional mode. So not exactly standard here. > > + > > +#define XSDIRX_DETECT_ALL_MODES > (BIT(XSDIRX_MODE_SD_OFFSET) | \ > > + BIT(XSDIRX_MODE_HD_OFFSET) | \ > > + BIT(XSDIRX_MODE_3GA_OFFSET) | \ > > + BIT(XSDIRX_MODE_3GB_OFFSET) | \ > > + BIT(XSDIRX_MODE_6G_OFFSET) | \ > > + BIT(XSDIRX_MODE_12GI_OFFSET) | \ > > + BIT(XSDIRX_MODE_12GF_OFFSET)) > > + > > +/* > > + * EDH - Error Detection and Handling. > > + * In the SD-SDI mode, the UHD-SDI core fully supports RP 165. > > + * The bitmask is named as XSDIRX_EDH_ERRCNT_XX_YY_ERR except > > + * for packet checksum error. > > + * > > + * XX - EDH Error Types > > + * ANC - Ancillary Data Packet Errors > > + * FF - Full Field Errors > > + * AP - Active Portion Errors > > + * > > + * YY - Error Flags > > + * EDH - error detected here > > + * EDA - error Detected already > > + * IDH - internal error detected here > > + * IDA - internal error detected already > > + * UES - unknown error status > > + * > > + * Refer to Sec 4.3 Error Flags in RP 165-1994 for details > > + */ > > + > > +#define XSDIRX_EDH_ERRCNT_ANC_EDH_ERR BIT(0) > > +#define XSDIRX_EDH_ERRCNT_ANC_EDA_ERR BIT(1) > > +#define XSDIRX_EDH_ERRCNT_ANC_IDH_ERR BIT(2) > > +#define XSDIRX_EDH_ERRCNT_ANC_IDA_ERR BIT(3) > > +#define XSDIRX_EDH_ERRCNT_ANC_UES_ERR BIT(4) > > +#define XSDIRX_EDH_ERRCNT_FF_EDH_ERR BIT(5) > > +#define XSDIRX_EDH_ERRCNT_FF_EDA_ERR BIT(6) > > +#define XSDIRX_EDH_ERRCNT_FF_IDH_ERR BIT(7) > > +#define XSDIRX_EDH_ERRCNT_FF_IDA_ERR BIT(8) > > +#define XSDIRX_EDH_ERRCNT_FF_UES_ERR BIT(9) > > +#define XSDIRX_EDH_ERRCNT_AP_EDH_ERR BIT(10) > > +#define XSDIRX_EDH_ERRCNT_AP_EDA_ERR BIT(11) > > +#define XSDIRX_EDH_ERRCNT_AP_IDH_ERR BIT(12) > > +#define XSDIRX_EDH_ERRCNT_AP_IDA_ERR BIT(13) > > +#define XSDIRX_EDH_ERRCNT_AP_UES_ERR BIT(14) > > +#define XSDIRX_EDH_ERRCNT_PKT_CHKSUM_ERR BIT(15) > > + > > +#define XSDIRX_EDH_ALLERR_MASK 0xFFFF > > Lowercase 0xffff. > > And these error conditions are also standardized? > > If so, then I think these defines/enums can be part of V4L2 itself rather > than Xilinx specific. > Yes the error conditions are standardized as mentioned in Sec 4.3 Error Flags in RP 165-1994. But I plan on removing these from the SDI Rx driver as they will be updated every frame and there is no way for the driver to even keep a count of them every frame as a local error counter mechanism as there is no Frame received interrupt from the IP. I can still add these errors as #define SDI_SD_EDH_ANC_EDH_ERR BIT(0) #define SDI_SD_EDH_ANC_EDA_ERR BIT(1) .. and so on in a new header file (include/linux/sdi.h ? ) but this driver may not be using it. Is it ok to add macros which are not used currently? > > + > > +/* > > + * V4L2 Controls - We reserved 16 controls for this driver. > > I'd increase that to 32. > Ok. I will make this to 32. > > + * > > + * The V4L2_CID_XILINX_SDIRX_EDH_* controls are present only if > > + * EDH is enabled. > > + * The controls which can be set should only be set before enabling > > + * streaming. The controls which can be got should be called while > > + * streaming to get correct values. > > + * The V4L2_CID_XILINX_SDIRX_MODE_DETECT can be called when query > dv timing > > query dv timing -> query_dv_timings > Noted. Will be updated in the next version. > > + * returns a valid timing. > > + */ > > + > > +/* > > + * Framer Control to enable or disable the framer. When this is set, the > framer > > + * automatically readjusts the output word alignment to match the > alignment of > > + * each timing reference signal(TRS). Normally this should be set. But user > may > > + * control this input to implement TRS filtering to prevent a signal > misaligned > > + * TRS from causing erroneous alignment changes. > > + * Refer to PG205 rx_frame_en for more details. > > + */ > > +#define V4L2_CID_XILINX_SDIRX_FRAMER > (V4L2_CID_USER_XILINX_SDIRX_BASE + 1) > > + > > +/* > > + * Video Lock Window Control to set the video lock window value > > + * This is the amount of time the mode and transport stream need > > + * to be locked before a video lock interrupt occurs. > > + */ > > +#define V4L2_CID_XILINX_SDIRX_VIDLOCK_WINDOW > (V4L2_CID_USER_XILINX_SDIRX_BASE + 2) > > + > > +/* > > + * EDH Error Mask Control to enable EDH error count > > + * This control takes in the bitmask of XSDIRX_EDH_ERRCNT_*_ERR to > enable counting > > + * such errors. > > + */ > > +#define V4L2_CID_XILINX_SDIRX_EDH_ERROR_SOURCES > (V4L2_CID_USER_XILINX_SDIRX_BASE + 3) > > If these EDH error sources are from the SDI standard, then this can become a > standard > control as well. > True but now I plan to remove this for the current driver. But I will add this to a new header file like include/linux/sdi.h > > + > > +/* > > + * Mode search Control to pass the bit mask of modes to detect. > > + * If only 1 bit is set, the driver programs IP to be in fixed mode else > > + * in multi detection mode. > > + * > > + * Set this when not streaming. > > + * > > + * bit 0 set to detect SD mode, > > + * bit 1 set to detect HD mode, > > + * bit 2 set to detect 3GA mode, > > + * bit 3 set to detect 3GB mode, > > + * bit 4 set to detect 6G mode, > > + * bit 5 set to detect 12G integer frame rate mode, > > + * bit 6 set to detect 12G fractional frame rate mode, > > + */ > > +#define V4L2_CID_XILINX_SDIRX_SEARCH_MODES > (V4L2_CID_USER_XILINX_SDIRX_BASE + 4) > > Same here if these modes are standardized. > Some of these modes are specific to Xilinx like the 12G integer and fractional modes. So I will keep them here like this. In future we can probably have a look at standardizing these modes. Then this control will become redundant. > > + > > +/* > > + * Get Detected SDI Mode control (read only) > > + * > > + * Control Value - Mode detected > > + * 0 - SD > > + * 1 - HD > > + * 2 - 3GA > > + * 3 - 3GB > > + * 4 - 6G > > + * 5 - 12G integer frame rate > > + * 6 - 12G fractional frame rate > > + */ > > +#define V4L2_CID_XILINX_SDIRX_MODE_DETECT > (V4L2_CID_USER_XILINX_SDIRX_BASE + 5) > > Ditto. > Noted. Same response as above. > > + > > +/* Get number of CRC errors status control > > + * > > + * When a CRC is detected on a line, the CRC error signal of that data > stream > > + * becomes asserted starting a few clock cycles after the last CRC word is > > + * output on the data stream ports following the EAV that ends the line > > + * containing the error. The CRC signal remains asserted for one line time. > > + * > > + * The LSB 16 bits of value returned by thsi control represent the error > > + * signal on each of 16 data streams. The MSB 16 bits contains the > accumulated > > + * error count. > > + * > > + * Refer to PG205 rx_crc_err_dsX (X = 1 to 16) description for details. > > + */ > > +#define V4L2_CID_XILINX_SDIRX_CRC > (V4L2_CID_USER_XILINX_SDIRX_BASE + 6) > > As suggested earlier, I think this should be split into two controls. It would be good to keep it a single control to check the CRC error count register as this is very device specific control. > > > + > > +/* > > + * Get EDH error count control > > + * > > + * Reading this control will give the number of EDH errors occurred based > > + * on the bitmask passed in > V4L2_CID_XILINX_SDIRX_EDH_ERROR_SOURCES. > > + * > > + * It increments once per field when any of the error conditions enabled by > > + * the RX_EDH_ERRCNT_EN register bit(s) occur during that field. > > + * > > + * Refer to PG205 rx_edh_errcnt > > + */ > > +#define V4L2_CID_XILINX_SDIRX_EDH_ERRCNT > (V4L2_CID_USER_XILINX_SDIRX_BASE + 7) > > Even though the EDH errors appear to be standard, I'm not sure if this specific > control > can be standardized. The precise behavior of a counter like this might differ > between > HW implementations. > Right so I am removing this control in next version as this has no practical use from software point of view with the current IP implementation. > > + > > +/* > > + * Get EDH status control > > + * > > + * This control returns the RX_EDH_STS register contents. > > + * Refer to PG290 register space section for more details. > > + */ > > +#define V4L2_CID_XILINX_SDIRX_EDH_STATUS > (V4L2_CID_USER_XILINX_SDIRX_BASE + 8) > > As mentioned above: this is a dubious control, reporting it in log_status seems > a more logical approach. > Correct. So I will be removing this control too. > > + > > +/* Get Transport Interlaced status whether it is interlaced or not */ > > +#define V4L2_CID_XILINX_SDIRX_TS_IS_INTERLACED > (V4L2_CID_USER_XILINX_SDIRX_BASE + 9) > > And as also mentioned above, this will be replaced by a new > FIELD_ALTERNATE_PROG? > Yes I will add the new V4L2 FIELD ALTERNATE PROG to represent streams whose content is progressive but are sent over an interlaced transport. So 2 received buffers, whose content is captured at same time, will represent one frame. > > + > > +/* > > + * Xilinx DV timings > > + * TODO - Remove these once they are in v4l2-dv-timings.h > > + */ > > +#define XLNX_V4L2_DV_BT_2048X1080P24 { \ > > + .type = V4L2_DV_BT_656_1120, \ > > + V4L2_INIT_BT_TIMINGS(2048, 1080, 0, \ > > + V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \ > > + 74250000, 510, 44, 148, 4, 5, 36, 0, 0, 0, \ > > + V4L2_DV_BT_STD_SDI) \ > > +} > > + <snip> > > + > > +#define XLNX_V4L2_DV_BT_1920X1080I48 { \ > > + .type = V4L2_DV_BT_656_1120, \ > > + V4L2_INIT_BT_TIMINGS(1920, 1080, 1, \ > > + V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \ > > + 148500000, 371, 88, 371, 2, 5, 15, 3, 5, 15, \ > > + V4L2_DV_BT_STD_SDI) \ > > +} > > + > > +#endif /* __UAPI_XILINX_SDIRXSS_H__ */ > > > > Regards, > > Hans Regards Vishal Sagar
