Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754406AbdLGNAf (ORCPT ); Thu, 7 Dec 2017 08:00:35 -0500 Received: from us01smtprelay-2.synopsys.com ([198.182.60.111]:57794 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754370AbdLGNA2 (ORCPT ); Thu, 7 Dec 2017 08:00:28 -0500 Subject: Re: [PATCH v9 4/4] [media] platform: Add Synopsys DesignWare HDMI RX Controller Driver To: Hans Verkuil , , References: CC: Joao Pinto , Mauro Carvalho Chehab , Hans Verkuil , "Sylwester Nawrocki" , Sakari Ailus From: Jose Abreu Message-ID: <63dac51b-a4da-57ae-6963-7322cef23435@synopsys.com> Date: Thu, 7 Dec 2017 13:00:20 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.107.19.63] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13566 Lines: 415 Hi Hans, On 07-12-2017 12:33, Hans Verkuil wrote: > Hi Jose, > > Some (small) comments below: Thanks for the review! > > On 12/07/17 10:47, Jose Abreu wrote: >> This is an initial submission for the Synopsys DesignWare HDMI RX >> Controller Driver. This driver interacts with a phy driver so that >> a communication between them is created and a video pipeline is >> configured. >> >> The controller + phy pipeline can then be integrated into a fully >> featured system that can be able to receive video up to 4k@60Hz >> with deep color 48bit RGB, depending on the platform. Although, >> this initial version does not yet handle deep color modes. >> >> This driver was implemented as a standard V4L2 subdevice and its >> main features are: >> - Internal state machine that reconfigures phy until the >> video is not stable >> - JTAG communication with phy >> - Inter-module communication with phy driver >> - Debug write/read ioctls >> >> Some notes: >> - RX sense controller (cable connection/disconnection) must >> be handled by the platform wrapper as this is not integrated >> into the controller RTL >> - The same goes for EDID ROM's >> - ZCAL calibration is needed only in FPGA platforms, in ASIC >> this is not needed >> - The state machine is not an ideal solution as it creates a >> kthread but it is needed because some sources might not be >> very stable at sending the video (i.e. we must react >> accordingly). >> >> Signed-off-by: Jose Abreu >> Cc: Joao Pinto >> Cc: Mauro Carvalho Chehab >> Cc: Hans Verkuil >> Cc: Sylwester Nawrocki >> Cc: Sakari Ailus >> --- >> Changes from v8: >> - Incorporate Sakari's work on ASYNC subdevs >> Changes from v6: >> - edid-phandle now also looks for parent node (Sylwester) >> - Fix kbuild build warnings >> Changes from v5: >> - Removed HDCP 1.4 support (Hans) >> - Removed some CEC debug messages (Hans) >> - Add s_dv_timings callback (Hans) >> - Add V4L2_CID_DV_RX_POWER_PRESENT ctrl (Hans) >> Changes from v4: >> - Add flag V4L2_SUBDEV_FL_HAS_DEVNODE (Sylwester) >> - Remove some comments and change some messages to dev_dbg (Sylwester) >> - Use v4l2_async_subnotifier_register() (Sylwester) >> Changes from v3: >> - Use v4l2 async API (Sylwester) >> - Do not block waiting for phy >> - Do not use busy waiting delays (Sylwester) >> - Simplify dw_hdmi_power_on (Sylwester) >> - Use clock API (Sylwester) >> - Use compatible string (Sylwester) >> - Minor fixes (Sylwester) >> Changes from v2: >> - Address review comments from Hans regarding CEC >> - Use CEC notifier >> - Enable SCDC >> Changes from v1: >> - Add support for CEC >> - Correct typo errors >> - Correctly detect interlaced video modes >> - Correct VIC parsing >> Changes from RFC: >> - Add support for HDCP 1.4 >> - Fixup HDMI_VIC not being parsed (Hans) >> - Send source change signal when powering off (Hans) >> - Add a "wait stable delay" >> - Detect interlaced video modes (Hans) >> - Restrain g/s_register from reading/writing to HDCP regs (Hans) >> --- >> drivers/media/platform/dwc/Kconfig | 15 + >> drivers/media/platform/dwc/Makefile | 1 + >> drivers/media/platform/dwc/dw-hdmi-rx.c | 1834 +++++++++++++++++++++++++++++++ >> drivers/media/platform/dwc/dw-hdmi-rx.h | 441 ++++++++ >> include/media/dwc/dw-hdmi-rx-pdata.h | 70 ++ >> 5 files changed, 2361 insertions(+) >> create mode 100644 drivers/media/platform/dwc/dw-hdmi-rx.c >> create mode 100644 drivers/media/platform/dwc/dw-hdmi-rx.h >> create mode 100644 include/media/dwc/dw-hdmi-rx-pdata.h >> > > >> +static void dw_hdmi_cec_tx_raw_status(struct dw_hdmi_dev *dw_dev, u32 stat) >> +{ >> + if (hdmi_readl(dw_dev, HDMI_CEC_CTRL) & HDMI_CEC_CTRL_SEND_MASK) { >> + dev_dbg(dw_dev->dev, "%s: tx is busy\n", __func__); >> + return; >> + } >> + >> + if (stat & HDMI_AUD_CEC_ISTS_ARBLST) { >> + cec_transmit_attempt_done(dw_dev->cec_adap, >> + CEC_TX_STATUS_ARB_LOST); >> + return; >> + } >> + >> + if (stat & HDMI_AUD_CEC_ISTS_NACK) { >> + cec_transmit_attempt_done(dw_dev->cec_adap, CEC_TX_STATUS_NACK); >> + return; >> + } >> + >> + if (stat & HDMI_AUD_CEC_ISTS_ERROR_INIT) { >> + dev_dbg(dw_dev->dev, "%s: got initiator error\n", __func__); >> + cec_transmit_attempt_done(dw_dev->cec_adap, CEC_TX_STATUS_ERROR); > There is no separate 'low drive' interrupt? Do you know what happens if a low drive > is received during a transmit? I think it launches this interrupt, i.e. Initiator Error. > > FYI: I've been working on error injection support for my cec-gpio driver, allowing > me to test all these nasty little corner cases. And that includes Arbitration Lost. > It's available here: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__git.linuxtv.org_hverkuil_media-5Ftree.git_log_-3Fh-3Dcec-2Derror-2Dinj&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=lM9aIhXqx9F8VZg9CADcx_qJbK_B2NTyHn8ZNIEpAEo&s=x0PASA3DhAHKXPrpj2G31D5xOFu0ERI3ewWb-yyTLMg&e= > > It works like a charm with my Rpi3. Nice! But for this I would need to have access to the physical pin for cec, right? I don't have that ... > >> + >> +static int dw_hdmi_query_dv_timings(struct v4l2_subdev *sd, >> + struct v4l2_dv_timings *timings) >> +{ >> + struct dw_hdmi_dev *dw_dev = to_dw_dev(sd); >> + struct v4l2_bt_timings *bt = &timings->bt; >> + bool is_hdmi_vic; >> + u32 htot, hofs; >> + u32 vtot; >> + u8 vic; >> + >> + dev_dbg(dw_dev->dev, "%s\n", __func__); >> + >> + memset(timings, 0, sizeof(*timings)); >> + >> + timings->type = V4L2_DV_BT_656_1120; >> + bt->width = hdmi_readl(dw_dev, HDMI_MD_HACT_PX); >> + bt->height = hdmi_readl(dw_dev, HDMI_MD_VAL); >> + bt->interlaced = hdmi_readl(dw_dev, HDMI_MD_STS) & HDMI_MD_STS_ILACE; >> + >> + if (hdmi_readl(dw_dev, HDMI_ISTS) & HDMI_ISTS_VS_POL_ADJ) >> + bt->polarities |= V4L2_DV_VSYNC_POS_POL; >> + if (hdmi_readl(dw_dev, HDMI_ISTS) & HDMI_ISTS_HS_POL_ADJ) >> + bt->polarities |= V4L2_DV_HSYNC_POS_POL; >> + >> + bt->pixelclock = dw_hdmi_get_pixelclk(dw_dev); >> + >> + /* HTOT = HACT + HFRONT + HSYNC + HBACK */ >> + htot = hdmi_mask_readl(dw_dev, HDMI_MD_HT1, >> + HDMI_MD_HT1_HTOT_PIX_OFFSET, >> + HDMI_MD_HT1_HTOT_PIX_MASK); >> + /* HOFS = HSYNC + HBACK */ >> + hofs = hdmi_mask_readl(dw_dev, HDMI_MD_HT1, >> + HDMI_MD_HT1_HOFS_PIX_OFFSET, >> + HDMI_MD_HT1_HOFS_PIX_MASK); >> + >> + bt->hfrontporch = htot - hofs - bt->width; >> + bt->hsync = hdmi_mask_readl(dw_dev, HDMI_MD_HT0, >> + HDMI_MD_HT0_HS_CLK_OFFSET, >> + HDMI_MD_HT0_HS_CLK_MASK); >> + bt->hbackporch = hofs - bt->hsync; >> + >> + /* VTOT = VACT + VFRONT + VSYNC + VBACK */ >> + vtot = hdmi_readl(dw_dev, HDMI_MD_VTL); >> + >> + hdmi_mask_writel(dw_dev, 0x1, HDMI_MD_VCTRL, >> + HDMI_MD_VCTRL_V_OFFS_LIN_MODE_OFFSET, >> + HDMI_MD_VCTRL_V_OFFS_LIN_MODE_MASK); >> + msleep(50); >> + bt->vsync = hdmi_readl(dw_dev, HDMI_MD_VOL); >> + >> + hdmi_mask_writel(dw_dev, 0x0, HDMI_MD_VCTRL, >> + HDMI_MD_VCTRL_V_OFFS_LIN_MODE_OFFSET, >> + HDMI_MD_VCTRL_V_OFFS_LIN_MODE_MASK); >> + msleep(50); >> + bt->vbackporch = hdmi_readl(dw_dev, HDMI_MD_VOL); >> + bt->vfrontporch = vtot - bt->height - bt->vsync - bt->vbackporch; > For interlaced formats the bt->il_* fields should also be filled in. See [1]. > >> + bt->standards = V4L2_DV_BT_STD_CEA861; >> + >> + vic = dw_hdmi_get_curr_vic(dw_dev, &is_hdmi_vic); >> + if (vic) { >> + if (is_hdmi_vic) { >> + bt->flags |= V4L2_DV_FL_HAS_HDMI_VIC; >> + bt->hdmi_vic = vic; >> + bt->cea861_vic = 0; >> + } else { >> + bt->flags |= V4L2_DV_FL_HAS_CEA861_VIC; >> + bt->hdmi_vic = 0; >> + bt->cea861_vic = vic; >> + } >> + } > You can simplify this. We have this function in v4l2-dv-timings.c: > v4l2_find_dv_timings_cea861_vic(). If you read a CEA861 vic code, > then you can call it to find the corresponding timings and just return > that. > > I thought I made a v4l2_find_dv_timings_hdmi_vic as well, but apparently > I didn't. It's trivial to add it, though. Ok, but how will then we handle 59.94 vs 60Hz video modes? Because the vic remains the same in these modes. > >> + >> + return 0; >> +} >> + >> +static int dw_hdmi_enum_mbus_code(struct v4l2_subdev *sd, >> + struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_mbus_code_enum *code) >> +{ >> + struct dw_hdmi_dev *dw_dev = to_dw_dev(sd); >> + >> + dev_dbg(dw_dev->dev, "%s\n", __func__); >> + if (code->index != 0) >> + return -EINVAL; >> + >> + code->code = dw_dev->mbus_code; >> + return 0; >> +} >> + >> +static int dw_hdmi_fill_format(struct dw_hdmi_dev *dw_dev, >> + struct v4l2_mbus_framefmt *format) >> +{ >> + memset(format, 0, sizeof(*format)); >> + >> + format->width = dw_dev->timings.bt.width; >> + format->height = dw_dev->timings.bt.height; >> + format->colorspace = V4L2_COLORSPACE_SRGB; >> + format->code = dw_dev->mbus_code; >> + if (dw_dev->timings.bt.interlaced) >> + format->field = V4L2_FIELD_ALTERNATE; > Were interlaced formats tested? (Apologies if I have asked this before) > > Interlaced is tricky and my recommendation is to only add support for it > to a driver if you have been able to test it. > > I see that dw_hdmi_timings_cap only supports progressive, so I conclude > that this hasn't been tested. It's better to just fix field to NONE in > that case. [1] Ok, but then I will still need to fill bt->il_* fields ? >> + >> +static int dw_hdmi_registered(struct v4l2_subdev *sd) >> +{ >> + struct dw_hdmi_dev *dw_dev = to_dw_dev(sd); >> + int ret; >> + >> + ret = cec_register_adapter(dw_dev->cec_adap, dw_dev->dev); >> + if (ret) { >> + dev_err(dw_dev->dev, "failed to register CEC adapter\n"); >> + cec_delete_adapter(dw_dev->cec_adap); >> + return ret; >> + } >> + >> + cec_register_cec_notifier(dw_dev->cec_adap, dw_dev->cec_notifier); >> + dw_dev->registered = true; >> + >> + return 0; >> + /*return v4l2_async_subdev_notifier_register(&dw_dev->sd, >> + &dw_dev->v4l2_notifier);*/ > Comment can be dropped, I guess. Yeah, leftovers from the previous version... > >> + >> +static int dw_hdmi_rx_probe(struct platform_device *pdev) >> +{ >> + const struct v4l2_dv_timings timings_def = HDMI_DEFAULT_TIMING; >> + struct dw_hdmi_rx_pdata *pdata = pdev->dev.platform_data; >> + struct device *dev = &pdev->dev; >> + struct v4l2_ctrl_handler *hdl; >> + struct dw_hdmi_dev *dw_dev; >> + struct v4l2_subdev *sd; >> + struct resource *res; >> + int ret, irq; >> + >> + dev_dbg(dev, "%s\n", __func__); >> + >> + dw_dev = devm_kzalloc(dev, sizeof(*dw_dev), GFP_KERNEL); >> + if (!dw_dev) >> + return -ENOMEM; >> + >> + if (!pdata) { >> + dev_err(dev, "missing platform data\n"); >> + return -EINVAL; >> + } >> + >> + dw_dev->dev = dev; >> + dw_dev->config = pdata; >> + dw_dev->state = HDMI_STATE_NO_INIT; >> + dw_dev->of_node = dev->of_node; >> + spin_lock_init(&dw_dev->lock); >> + >> + ret = dw_hdmi_parse_dt(dw_dev); >> + if (ret) >> + return ret; >> + >> + /* Deferred work */ >> + dw_dev->wq = create_singlethread_workqueue(DW_HDMI_RX_DRVNAME); >> + if (!dw_dev->wq) { >> + dev_err(dev, "failed to create workqueue\n"); >> + return -ENOMEM; >> + } >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + dw_dev->regs = devm_ioremap_resource(dev, res); >> + if (IS_ERR(dw_dev->regs)) { >> + dev_err(dev, "failed to remap resource\n"); >> + ret = PTR_ERR(dw_dev->regs); >> + goto err_wq; >> + } >> + >> + /* Disable HPD as soon as posssible */ >> + dw_hdmi_disable_hpd(dw_dev); >> + /* Prevent HDCP from tampering video */ >> + dw_hdmi_config_hdcp(dw_dev); >> + >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) { >> + ret = irq; >> + goto err_wq; >> + } >> + >> + ret = devm_request_threaded_irq(dev, irq, NULL, dw_hdmi_irq_handler, >> + IRQF_ONESHOT, DW_HDMI_RX_DRVNAME, dw_dev); >> + if (ret) >> + goto err_wq; >> + >> + /* V4L2 initialization */ >> + sd = &dw_dev->sd; >> + v4l2_subdev_init(sd, &dw_hdmi_sd_ops); >> + strlcpy(sd->name, dev_name(dev), sizeof(sd->name)); >> + sd->dev = dev; >> + sd->internal_ops = &dw_hdmi_internal_ops; >> + sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_HAS_DEVNODE; >> + >> + /* Control handlers */ >> + hdl = &dw_dev->hdl; >> + v4l2_ctrl_handler_init(hdl, 1); >> + dw_dev->detect_tx_5v_ctrl = v4l2_ctrl_new_std(hdl, NULL, >> + V4L2_CID_DV_RX_POWER_PRESENT, 0, BIT(4) - 1, 0, 0); >> + >> + sd->ctrl_handler = hdl; >> + if (hdl->error) { >> + ret = hdl->error; >> + goto err_hdl; >> + } >> + >> + /* Wait for ctrl handler register before requesting 5v interrupt */ >> + irq = platform_get_irq(pdev, 1); >> + if (irq < 0) { >> + ret = irq; >> + goto err_hdl; >> + } >> + >> + ret = devm_request_threaded_irq(dev, irq, dw_hdmi_5v_hard_irq_handler, >> + dw_hdmi_5v_irq_handler, IRQF_ONESHOT, >> + DW_HDMI_RX_DRVNAME "-5v-handler", dw_dev); >> + if (ret) >> + goto err_hdl; >> + >> + /* Notifier for subdev binding */ >> + ret = dw_hdmi_v4l2_init_notifier(dw_dev); >> + if (ret) { >> + dev_err(dev, "failed to init v4l2 notifier\n"); >> + goto err_hdl; >> + } >> + >> + /* PHY loading */ >> + ret = dw_hdmi_phy_init(dw_dev); >> + if (ret) >> + goto err_hdl; >> + >> + /* CEC */ >> +#if IS_ENABLED(CONFIG_VIDEO_DWC_HDMI_RX_CEC) >> + dw_dev->cec_adap = cec_allocate_adapter(&dw_hdmi_cec_adap_ops, >> + dw_dev, dev_name(dev), CEC_CAP_TRANSMIT | >> + CEC_CAP_LOG_ADDRS | CEC_CAP_RC | CEC_CAP_PASSTHROUGH, > Use CEC_CAP_DEFAULTS instead of specifying these caps separately. Ok. Thanks and Best Regards, Jose Miguel Abreu > > Regards, > > Hans