Hi Vishal,
On Tue, Jun 09, 2020 at 06:23:43PM +0000, Vishal Sagar wrote:
> On Wednesday, May 6, 2020 8:42 PM, Laurent Pinchart wrote:
> > On Wed, Apr 29, 2020 at 07:47:04PM +0530, 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.
> >
> > As commented on patch 1/2, I don't see a mention of 12 bpc in the
> > documentation. Let's discuss it as part of patch 1/2.
>
> Ok.
>
> > > The driver currently supports only the AXI4-Stream IP configuration.
> > >
> > > Signed-off-by: Vishal Sagar <vishal.sagar@xxxxxxxxxx>
> > > ---
> > > 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 | 2162 +++++++++++++++++
> > > include/uapi/linux/xilinx-sdirxss.h | 179 ++
> > > include/uapi/linux/xilinx-v4l2-controls.h | 67 +
> > > 5 files changed, 2420 insertions(+)
> > > create mode 100644 drivers/media/platform/xilinx/xilinx-sdirxss.c
> > > create mode 100644 include/uapi/linux/xilinx-sdirxss.h
[snip]
> > > diff --git a/drivers/media/platform/xilinx/xilinx-sdirxss.c
> > b/drivers/media/platform/xilinx/xilinx-sdirxss.c
> > > new file mode 100644
> > > index 000000000000..c536ea3aaa0d
> > > --- /dev/null
> > > +++ b/drivers/media/platform/xilinx/xilinx-sdirxss.c
> > > @@ -0,0 +1,2162 @@
[snip]
> > > +/* Maximum number of events per file handle. */
> > > +#define XSDIRX_MAX_EVENTS (128)
> >
> > Do we really need such a high number ? How often are the overflow and
> > underflow expected ?
>
> An overflow or underflow events will occur when a resolution / rate change occurs.
> But since we are stopping the bridges in irq handler, these events will stop coming.
> I can reduce this to 8 events but it is arbitrary. Is that ok?
Yes, I think that's a better value.
[snip]
> > > +/**
> > > + * struct xsdirxss_core - Core configuration SDI Rx Subsystem device structure
> > > + * @dev: Platform structure
> > > + * @iomem: Base address of subsystem
> > > + * @irq: requested irq number
> > > + * @include_edh: EDH processor presence
> > > + * @mode: 3G/6G/12G mode
> > > + * @clks: array of clocks
> > > + * @num_clks: number of clocks
> > > + * @bpc: Bits per component, can be 10 or 12
> > > + */
> > > +struct xsdirxss_core {
> > > + struct device *dev;
> > > + void __iomem *iomem;
> > > + int irq;
> > > + bool include_edh;
> > > + int mode;
> > > + struct clk_bulk_data *clks;
> > > + int num_clks;
> >
> > num_clks is never negative, you can make it an unsigned int.
>
> Ok. I will make num_clks as unsigned int.
> So why devm_clk_bulk_get() and clk_bulk_prepare_enable() take in num_clks argument as int ?
That's a good question. I assume that's because nobody has cared to make
it unsigned. It makes little difference in practice (there's a small
impact on code generation depending on what operation you perform on the
value, but it's usually negligible). Where it makes a difference, I
believe, is in expressing the intent. If you make field unsigned, you
express that it doesn't make sense to assign it a negative value. It's
API documentation in a way. It can also avoid some errors:
struct foo {
u8 elements[16];
};
int zero_foo_elements(struct foo *foo, int num_elements)
{
if (num_elements > ARRAY_SIZE(foo->elements))
return -EOVERFLOW;
memset(foo->elements, 0, num_elements);
return 0;
}
If called with a negative value, bad things will happen. If num_elements
was unsigned, there would be no issue. Of course the function could be
written
int zero_foo_elements(struct foo *foo, int num_elements)
{
if (num_elements < 0 || num_elements > ARRAY_SIZE(foo->elements))
return -EOVERFLOW;
memset(foo->elements, 0, num_elements);
return 0;
}
but it's easy to overlook the need for a < 0 check when there's no valid
use case for passing a negative value.
> > > + u32 bpc;
> > > +};
[snip]
> > > +/* TODO - Add YUV 444/420 and RBG 10/12 bpc mbus formats here */
> >
> > s/RBG/RGB/ ?
> >
>
> No RBG is correct because as per Xilinx User Guide 934 Figure 1-4 and Table 1-4,
> the Blue is middle component in the AXI4-Stream protocol followed by all Xilinx Video IPs.
> https://www.xilinx.com/support/documentation/ip_documentation/axi_videoip/v1_0/ug934_axi_videoIP.pdf
Indeed, my bad. I wonder why the AXI4 stream protocol uses RBG, it's a
quite unusual order.
> We will add the RBG 10 and 12 bpc formats later.
[snip]
> > > +
> > > +/**
> > > + * 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 xsdirxss_core *core = &xsdirxss->core;
> > > + struct device *dev = core->dev;
> > > +
> > > + switch (ctrl->id) {
> > > + case V4L2_CID_XILINX_SDIRX_MODE_DETECT:
> > > + if (!xsdirxss->vidlocked) {
> > > + dev_err(dev, "Can't get values when video not locked!\n");
> > > + return -EINVAL;
> > > + }
> > > + val = xsdirxss_read(core, 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:
> > > + ctrl->val = XSDIRX_MODE_3G_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;
> > > + }
> > > + break;
> >
> > Hans commented that the dv timings structure will report whether the
> > mode is interlaced, I wonder if reporting the SDI mode shouldn't go
> > through that structure too.
> >
>
> I don’t know about this.
> We may add a feature in V4L2 / DRM framework to set / get the SDI modes.
I'd like to get Hans' opinion on this, he has more experience with DV
timings than I do.
> > > + case V4L2_CID_XILINX_SDIRX_CRC:
> > > + ctrl->val = xsdirxss_read(core, XSDIRX_CRC_ERRCNT_REG);
> > > + xsdirxss_write(core, XSDIRX_CRC_ERRCNT_REG, 0xFFFF);
> > > + break;
> >
> > Are CRC errors common (and recoverable), or are they rare and fatal
> > errors ? In other words, does this report information that would be
> > classified as link quality, or fatal errors ? In the later case I wonder
> > if an event wouldn't make more sense. It could be reported as another
> > bit in the overflow/underflow event.
>
> CRC errors will reflect link quality.
> There is no interrupt for CRC errors.
> So driver software won't know when to read this register.
> Also this is specific to Xilinx implementation.
Thanks for the clarification, this makes sense.
> > > + case V4L2_CID_XILINX_SDIRX_EDH_ERRCNT:
> > > + val = xsdirxss_read(core, XSDIRX_MODE_DET_STAT_REG);
> > > + val &= XSDIRX_MODE_DET_STAT_RX_MODE_MASK;
> > > + if (val == XSDIRX_MODE_SD_MASK) {
> > > + ctrl->val = xsdirxss_read(core, XSDIRX_EDH_ERRCNT_REG);
> > > + } else {
> > > + dev_dbg(dev, "%d - not in SD mode\n", ctrl->id);
> > > + return -EINVAL;
> > > + }
> > > + break;
> >
> > If my understanding of the datasheet is correct, this will be reset for
> > every frame, right ? Reporting the information this way is thus quite
> > racy. Isn't it better to send it through an event ?
>
> This is not reset per frame.
> This is an error reporting mechanism that application can use to get errors in stream while
> running SD modes (720x487 and 720x576).
> So I think it is ok to send it like this.
I'm not sure why I thought it was reset per frame, sorry about the
noise.
> > > + case V4L2_CID_XILINX_SDIRX_EDH_STATUS:
> > > + val = xsdirxss_read(core, XSDIRX_MODE_DET_STAT_REG);
> > > + val &= XSDIRX_MODE_DET_STAT_RX_MODE_MASK;
> > > + if (val == XSDIRX_MODE_SD_MASK) {
> > > + ctrl->val = xsdirxss_read(core, XSDIRX_EDH_STAT_REG);
> > > + } else {
> > > + dev_dbg(dev, "%d - not in SD mode\n", ctrl->id);
> > > + return -EINVAL;
> > > + }
> > > + break;
> >
> > Same here, seems quite racy.
> >
>
> Same here as earlier about EDH Counter. There is no interrupt here too.
>
> > > + case V4L2_CID_XILINX_SDIRX_TS_IS_INTERLACED:
> > > + if (!xsdirxss->vidlocked) {
> > > + dev_err(dev, "Can't get values when video not
> > locked!\n");
> > > + return -EINVAL;
> > > + }
> > > + ctrl->val = xsdirxss->ts_is_interlaced;
> > > + break;
> > > + case V4L2_CID_XILINX_SDIRX_ACTIVE_STREAMS:
> > > + if (!xsdirxss->vidlocked) {
> > > + dev_err(dev, "Can't get values when video not
> > locked!\n");
> > > + return -EINVAL;
> > > + }
> > > + val = xsdirxss_read(core, XSDIRX_MODE_DET_STAT_REG);
> > > + val &= XSDIRX_MODE_DET_STAT_ACT_STREAM_MASK;
> > > + val >>= XSDIRX_MODE_DET_STAT_ACT_STREAM_OFFSET;
> > > + ctrl->val = 1 << val;
> > > + break;
> > > + case V4L2_CID_XILINX_SDIRX_IS_3GB:
> > > + if (!xsdirxss->vidlocked) {
> > > + dev_err(dev, "Can't get values when video not
> > locked!\n");
> > > + return -EINVAL;
> > > + }
> > > + val = xsdirxss_read(core, XSDIRX_MODE_DET_STAT_REG);
> > > + val &= XSDIRX_MODE_DET_STAT_LVLB_3G_MASK;
> > > + ctrl->val = val ? true : false;
> > > + break;
> >
> > Shouldn't this also go through DV timings ? If not, I think it should at
> > least be combined with V4L2_CID_XILINX_SDIRX_MODE_DETECT.
>
> I am trying to use the same values to set mode and detect it.
> As there is no different bit to specifically detect 3GB Mode, hence this differentiation is there.
> I will try to combine this in later patch.
>
> > > + default:
> > > + dev_err(dev, "Get Invalid control id 0x%0x\n", ctrl->id);
> > > + return -EINVAL;
> > > + }
> > > + dev_dbg(dev, "Get ctrl id = 0x%08x val = 0x%08x\n", ctrl->id,
> > > + ctrl->val);
> >
> > I would drop this line.
> >
>
> Ok I will remove this in next patch.
>
> > > + return 0;
> > > +}
[snip]
> > > +static int xsdirxss_parse_of(struct xsdirxss_state *xsdirxss)
> > > +{
> > > + struct device_node *node = xsdirxss->core.dev->of_node;
> > > + struct xsdirxss_core *core = &xsdirxss->core;
> > > + struct device *dev = core->dev;
> > > + struct fwnode_handle *ep, *rep;
> > > + int ret;
> > > + const char *sdi_std;
> > > +
> > > + core->include_edh = of_property_read_bool(node, "xlnx,include-edh");
> > > + dev_dbg(dev, "EDH property = %s\n",
> > > + core->include_edh ? "Present" : "Absent");
> > > +
> > > + ret = of_property_read_string(node, "xlnx,line-rate", &sdi_std);
> > > + if (ret < 0) {
> > > + dev_err(dev, "xlnx,line-rate property not found\n");
> > > + return ret;
> > > + }
> > > +
> > > + if (!strncmp(sdi_std, "12G_SDI_8DS", XSDIRX_MAX_STR_LENGTH)) {
> >
> > Strings in DT are null-terminated, you can use strcmp() and drop
> > XSDIRX_MAX_STR_LENGTH.
>
> I will remove the strings from DT and have values 0, 1 and 2 to
> represent 3G, 6G and 12G 8DS mode configurations.
Then please add a header file in include/dt-bindings/media with macros
for these values. An alternative would be to use 3, 6 and 12
respectively, in which case we could do without macros I think. I'm
fine with either option.
> > > + core->mode = XSDIRXSS_SDI_STD_12G_8DS;
> > > + } else if (!strncmp(sdi_std, "6G_SDI", XSDIRX_MAX_STR_LENGTH)) {
> > > + core->mode = XSDIRXSS_SDI_STD_6G;
> > > + } else if (!strncmp(sdi_std, "3G_SDI", XSDIRX_MAX_STR_LENGTH)) {
> > > + core->mode = XSDIRXSS_SDI_STD_3G;
> > > + } else {
> > > + dev_err(dev, "Invalid Line Rate\n");
> > > + return -EINVAL;
> > > + }
> > > + dev_dbg(dev, "SDI Rx Line Rate = %s, mode = %d\n", sdi_std,
> > > + core->mode);
> > > +
> > > + ret = of_property_read_u32(node, "xlnx,bpp", &core->bpc);
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to get xlnx,bpp\n");
> > > + return ret;
> > > + }
> > > +
> > > + if (core->bpc != 10 && core->bpc != 12) {
> > > + dev_err(dev, "bits per component=%u. Can be 10 or 12 only\n",
> > > + core->bpc);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> > > + FWNODE_GRAPH_ENDPOINT_NEXT);
> > > + if (!ep) {
> > > + dev_err(dev, "no source port found");
> > > + ret = -EINVAL;
> > > + goto dt_parse_done;
> > > + }
> > > +
> > > + rep = fwnode_graph_get_remote_endpoint(ep);
> > > + if (!rep) {
> > > + dev_err(dev, "no remote sink endpoint found");
> > > + ret = -EINVAL;
> > > + }
> >
> > I don't think you need to check this, the subdev won't be linked
> > properly in the pipeline if there's no port anyway.
>
> Ok. I thought it was subdev driver's responsibility to validate this too. Hence added.
> I will remove this parsing in the next version.
>
> > > +
> > > + fwnode_handle_put(rep);
> > > +dt_parse_done:
> > > + fwnode_handle_put(ep);
> > > + return ret;
> > > +}
[snip]
--
Regards,
Laurent Pinchart
