Re: [PATCH 05/16] media: cadence: csi2rx: Add external DPHY support
On 08/04/2021 11:13, Chunfeng Yun wrote:
On Wed, 2021-04-07 at 00:24 +0530, Pratyush Yadav wrote:
On 31/03/21 05:24PM, Chunfeng Yun wrote:
On Tue, 2021-03-30 at 23:03 +0530, Pratyush Yadav wrote:
Some platforms like TI's J721E can have the CSI2RX paired with an
external DPHY. Add support to enable and configure the DPHY using the
generic PHY framework.
Get the pixel rate and bpp from the subdev and pass them on to the DPHY
along with the number of lanes. All other settings are left to their
default values.
Signed-off-by: Pratyush Yadav
---
drivers/media/platform/cadence/cdns-csi2rx.c | 147 +--
1 file changed, 137 insertions(+), 10 deletions(-)
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
b/drivers/media/platform/cadence/cdns-csi2rx.c
index c68a3eac62cd..31bd80e3f780 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -30,6 +30,12 @@
#define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 +
(llane) * 4))
#define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8)
+#define CSI2RX_DPHY_LANE_CTRL_REG 0x40
+#define CSI2RX_DPHY_CL_RST BIT(16)
+#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12)
+#define CSI2RX_DPHY_CL_EN BIT(4)
+#define CSI2RX_DPHY_DL_EN(i) BIT(i)
+
#define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100)
#define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000)
@@ -54,6 +60,11 @@ enum csi2rx_pads {
CSI2RX_PAD_MAX,
};
+struct csi2rx_fmt {
+ u32 code;
+ u8 bpp;
+};
+
struct csi2rx_priv {
struct device *dev;
unsigned intcount;
@@ -85,6 +96,52 @@ struct csi2rx_priv {
int source_pad;
};
+static const struct csi2rx_fmt formats[] = {
+ {
+ .code = MEDIA_BUS_FMT_YUYV8_2X8,
+ .bpp= 16,
+ },
+ {
+ .code = MEDIA_BUS_FMT_UYVY8_2X8,
+ .bpp= 16,
+ },
+ {
+ .code = MEDIA_BUS_FMT_YVYU8_2X8,
+ .bpp= 16,
+ },
+ {
+ .code = MEDIA_BUS_FMT_VYUY8_2X8,
+ .bpp= 16,
+ },
+};
+
+static u8 csi2rx_get_bpp(u32 code)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(formats); i++) {
+ if (formats[i].code == code)
+ return formats[i].bpp;
+ }
+
+ return 0;
+}
+
+static s64 csi2rx_get_pixel_rate(struct csi2rx_priv *csi2rx)
+{
+ struct v4l2_ctrl *ctrl;
+
+ ctrl = v4l2_ctrl_find(csi2rx->source_subdev->ctrl_handler,
+ V4L2_CID_PIXEL_RATE);
+ if (!ctrl) {
+ dev_err(csi2rx->dev, "no pixel rate control in subdev: %s\n",
+ csi2rx->source_subdev->name);
+ return -EINVAL;
+ }
+
+ return v4l2_ctrl_g_ctrl_int64(ctrl);
+}
+
static inline
struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
{
@@ -101,6 +158,55 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx)
writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
}
+static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx)
+{
+ union phy_configure_opts opts = { };
+ struct phy_configure_opts_mipi_dphy *cfg = _dphy;
+ struct v4l2_subdev_format sd_fmt;
+ s64 pixel_rate;
+ int ret;
+ u8 bpp;
+
+ sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ sd_fmt.pad = 0;
+
+ ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL,
+ _fmt);
+ if (ret)
+ return ret;
+
+ bpp = csi2rx_get_bpp(sd_fmt.format.code);
+ if (!bpp)
+ return -EINVAL;
+
+ pixel_rate = csi2rx_get_pixel_rate(csi2rx);
+ if (pixel_rate < 0)
+ return pixel_rate;
+
+ ret = phy_mipi_dphy_get_default_config(pixel_rate, bpp,
+ csi2rx->num_lanes, cfg);
+ if (ret)
+ return ret;
+
+ ret = phy_set_mode_ext(csi2rx->dphy, PHY_MODE_MIPI_DPHY,
+ PHY_MIPI_DPHY_SUBMODE_RX);
+ if (ret)
+ return ret;
+
+ ret = phy_power_on(csi2rx->dphy);
+ if (ret)
+ return ret;
Seems phy_power_on, then phy_set_mode_ext?
Shouldn't the mode be set before the PHY is powered on so the correct
power on procedure can be performed based on the mode of operation?
Of course, it is fine for cnds-dphy.
But it depends on HW design and also phy driver;
if the mode is controlled in PHY IP register, we can't access it before
phy_power_on if no phy_init called (e.g. clock/power is not enabled).
Just let you pay attention on the phy sequence.
I don't think the phy configuration should depend
Re: [PATCH 13/16] media: ti-vpe: csi2rx: Add CSI2RX support
Hi, On 30/03/2021 20:33, Pratyush Yadav wrote: TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate capture over a CSI-2 bus. The Cadence CSI2RX IP acts as a bridge between the TI specific parts and the CSI-2 protocol parts. TI then has a wrapper on top of this bridge called the SHIM layer. It takes in data from stream 0, repacks it, and sends it to memory over PSI-L DMA. This driver acts as the "front end" to V4L2 client applications. It implements the required ioctls and buffer operations, passes the necessary calls on to the bridge, programs the SHIM layer, and performs DMA via the dmaengine API to finally return the data to a buffer supplied by the application. Signed-off-by: Pratyush Yadav --- MAINTAINERS | 7 + drivers/media/platform/Kconfig| 11 + drivers/media/platform/ti-vpe/Makefile| 1 + drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++ 4 files changed, 983 insertions(+) create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c Some quick comments: "ti-vpe" directory is not correct, this has nothing to do with VPE. That said, the directory has already been abused by having CAL driver there, perhaps we should rename the directory just to "ti". But if we do that, I think we should have subdirs for cal, vpe and this new one. "ti-csi2rx" is rather generic name. TI has had CSI-2 RX IPs before (CAL) and probably will also have new ones in the future. If there's no clear model name for the IP, as I think is the case here, it's probably best to just use the SoC model in the name. E.g. the DSS on J7 is "ti,j721e-dss". This driver implements the legacy video API. I think it would be better (and easier to maintain) to only implement the media-controller API, unless you specifically need to support the legacy API for existing userspace. Tomi
Re: [PATCH] [v2] drivers: gpu: drm: Remove duplicate declaration
On 25/03/2021 15:05, Laurent Pinchart wrote: Hi Wan, Thank you for the patch. On Thu, Mar 25, 2021 at 07:10:24PM +0800, Wan Jiabing wrote: struct dss_device has been declared. Remove the duplicate. And sort these forward declarations alphabetically. Signed-off-by: Wan Jiabing Reviewed-by: Laurent Pinchart Tomi, I assume you'll handle this patch, please let me know if you don't plan to do so. Yep, picked this up. Thanks! Tomi
Re: [PATCH v2] drm/omap: dsi: Add missing IRQF_ONESHOT
On 23/03/2021 13:15, Sebastian Reichel wrote:
Hi,
On Tue, Mar 23, 2021 at 05:34:53PM +0800, Yang Li wrote:
fixed the following coccicheck:
./drivers/gpu/drm/omapdrm/dss/dsi.c:4329:7-27: ERROR: Threaded IRQ with
no primary handler requested without IRQF_ONESHOT
Reported-by: Abaci Robot
Signed-off-by: Yang Li
---
Reviewed-by: Sebastian Reichel
Also maybe add this, so that it is properly backported. OTOH old code did
not have IRQF_ONESHOT either.
Fixes: 4c1b935fea54 ("drm/omap: dsi: move TE GPIO handling into core")
Thanks, I have applied this.
Tomi
Re: [PATCH] drm/omap: fix misleading indentation in pixinc()
On 22/03/2021 18:41, Arnd Bergmann wrote:
From: Arnd Bergmann
An old patch added a 'return' statement after each BUG() in this driver,
which was necessary at the time, but has become redundant after the BUG()
definition was updated to handle this properly.
gcc-11 now warns about one such instance, where the 'return' statement
was incorrectly indented:
drivers/gpu/drm/omapdrm/dss/dispc.c: In function ‘pixinc’:
drivers/gpu/drm/omapdrm/dss/dispc.c:2093:9: error: this ‘else’ clause does not
guard... [-Werror=misleading-indentation]
2093 | else
| ^~~~
drivers/gpu/drm/omapdrm/dss/dispc.c:2095:17: note: ...this statement, but the
latter is misleadingly indented as if it were guarded by the ‘else’
2095 | return 0;
| ^~
Address this by removing the return again and changing the BUG()
to be unconditional to make this more intuitive.
Fixes: c6eee968d40d ("OMAPDSS: remove compiler warnings when CONFIG_BUG=n")
Signed-off-by: Arnd Bergmann
---
drivers/gpu/drm/omapdrm/dss/dispc.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
b/drivers/gpu/drm/omapdrm/dss/dispc.c
index f4cbef8ccace..5619420cc2cc 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -2090,9 +2090,8 @@ static s32 pixinc(int pixels, u8 ps)
return 1 + (pixels - 1) * ps;
else if (pixels < 0)
return 1 - (-pixels + 1) * ps;
- else
- BUG();
- return 0;
+
+ BUG();
}
static void calc_offset(u16 screen_width, u16 width,
Thanks, I'll pick this up.
Tomi
Re: [PATCH v2 0/3] drm/tilcdc: fix LCD pixel clock setting
On 21/03/2021 21:08, Jyri Sarha wrote: On 2021-03-21 10:31, Dario Binacchi wrote: The series was born from a patch to fix the LCD pixel clock setting. Two additional patches have been added to this. One renames a misleading variable name that was probably the cause of the bug and the other fixes a warning message. Thanks you, I think this looks good now. Reviewed-by: Jyri Sarha For the series. I'll wait a day or two if Tomi has something more to say and merge this to drm-misc-next. I had one comment about the print, but otherwise: Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH v2 3/3] drm/tilcdc: fix pixel clock setting warning message
On 21/03/2021 10:31, Dario Binacchi wrote:
The warning message did not printed the LCD pixel clock rate but the LCD
clock divisor input rate. As a consequence, the required and real pixel
clock rates are now passed to the tilcdc_pclk_diff().
Signed-off-by: Dario Binacchi
---
Changes in v2:
- The patch has been added in version 2.
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index ac6228cb04d9..c0792c52dc02 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -203,7 +203,7 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc)
struct drm_device *dev = crtc->dev;
struct tilcdc_drm_private *priv = dev->dev_private;
struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
- unsigned long clk_rate, real_rate, real_pclk_rate, pclk_rate;
+ unsigned long clk_rate, real_pclk_rate, pclk_rate;
unsigned int clkdiv;
int ret;
@@ -239,12 +239,12 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc)
* 5% is an arbitrary value - LCDs are usually quite tolerant
* about pixel clock rates.
*/
- real_rate = clkdiv * pclk_rate;
+ real_pclk_rate = clk_rate / clkdiv;
- if (tilcdc_pclk_diff(clk_rate, real_rate) > 5) {
+ if (tilcdc_pclk_diff(pclk_rate, real_pclk_rate) > 5) {
dev_warn(dev->dev,
"effective pixel clock rate (%luHz) differs from
the calculated rate (%luHz)\n",
-clk_rate, real_rate);
+pclk_rate, real_pclk_rate);
Aren't these backwards? "Effective" is the real one in the HW. I'm not
sure what "calculated" means here, I guess it should be "requested".
Tomi
Re: linux-next: manual merge of the drm tree with the drm-misc-fixes tree
On 18/03/2021 03:02, Stephen Rothwell wrote:
Hi all,
Today's linux-next merge of the drm tree got a conflict in:
drivers/gpu/drm/omapdrm/dss/dsi.c
between commit:
690911544275 ("drm/omap: dsi: fix unsigned expression compared with zero")
from the drm-misc-fixes tree and commit:
bbd13d6a7b2e ("drm/omap: dsi: fix unreachable code in dsi_vc_send_short()")
from the drm tree.
I fixed it up (these do basically the same thing, so I used the former
version) and can carry the fix as necessary. This is now fixed as far as
linux-next is concerned, but any non trivial conflicts should be mentioned
to your upstream maintainer when your tree is submitted for merging.
You may also want to consider cooperating with the maintainer of the
conflicting tree to minimise any particularly complex conflicts.
Yes, I messed that up. I accidentally merged a fix to drm-misc-fixes,
but almost similar fix was already in drm-misc-next. Sorry about that.
Tomi
Re: [PATCH] omapdrm/dss/dsi.c:modify 'u32'->'int'
Hi,
On 17/03/2021 11:48, ChunyouTang wrote:
From: tangchunyou
1.the type of mipi_dsi_create_packet id int
2.u32 can not < 0
Signed-off-by: tangchunyou
---
drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c
b/drivers/gpu/drm/omapdrm/dss/dsi.c
index 8e11612..11c502d 100644
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -2149,7 +2149,7 @@ static int dsi_vc_send_short(struct dsi_data *dsi, int vc,
const struct mipi_dsi_msg *msg)
{
struct mipi_dsi_packet pkt;
- u32 r;
+ int r;
r = mipi_dsi_create_packet(, msg);
if (r < 0)
I have already applies a similar patch "drm/omap: dsi: fix unsigned
expression compared with zero".
Tomi
Re: [PATCH] drm/tilcdc: fix LCD pixel clock setting
On 14/03/2021 17:13, Dario Binacchi wrote:
As reported by TI spruh73x RM, the LCD pixel clock (LCD_PCLK) frequency
is obtained by dividing LCD_CLK, the LCD controller reference clock,
for CLKDIV:
LCD_PCLK = LCD_CLK / CLKDIV
where CLKDIV must be greater than 1.
Therefore LCD_CLK must be set to 'req_rate * CLKDIV' instead of req_rate
The above doesn't make sense, the code already sets LCD_CLK to 'req_rate
* clkdiv', not req_rate.
and the real LCD_CLK rate must be compared with 'req_rate * CLKDIV' and
not with req_rate.
This is true, the code looks at the wrong value.
Passing req_rate instead of 'req_rate * CLKDIV' to the tilcdc_pclk_diff
routine caused it to fail even if LCD_CLK was properly set.
Signed-off-by: Dario Binacchi
---
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 30213708fc99..02f56c9a5da5 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -203,7 +203,7 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc)
struct drm_device *dev = crtc->dev;
struct tilcdc_drm_private *priv = dev->dev_private;
struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
- unsigned long clk_rate, real_rate, req_rate;
+ unsigned long clk_rate, real_rate, req_rate, clk_div_rate;
unsigned int clkdiv;
int ret;
@@ -211,10 +211,11 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc)
/* mode.clock is in KHz, set_rate wants parameter in Hz */
req_rate = crtc->mode.clock * 1000;
-
- ret = clk_set_rate(priv->clk, req_rate * clkdiv);
+ /* LCD clock divisor input rate */
+ clk_div_rate = req_rate * clkdiv;
"clk_div_rate" sounds a bit odd to me. Why not lcd_fck_rate, as that's
the name used later? Or lcd_clk_rate. Or maybe lcd_clk_req_rate...
+ ret = clk_set_rate(priv->clk, clk_div_rate);
clk_rate = clk_get_rate(priv->clk);
- if (ret < 0 || tilcdc_pclk_diff(req_rate, clk_rate) > 5) {
+ if (ret < 0 || tilcdc_pclk_diff(clk_div_rate, clk_rate) > 5) {
/*
* If we fail to set the clock rate (some architectures don't
* use the common clock framework yet and may not implement
I think this fix is fine, but looking at the current code, it's calling
tilcdc_pclk_diff(), but doesn't actually provide pixel clocks to the
function, but fclk.
Tomi
Re: [PATCH] drm/omap: dsi: fix unsigned expression compared with zero
On 14/03/2021 04:15, Laurent Pinchart wrote: Hi Junlin, Thank you for the patch. On Fri, Mar 12, 2021 at 03:14:45PM +0800, angkery wrote: From: Junlin Yang r is "u32" always >= 0,mipi_dsi_create_packet may return little than zero. so r < 0 condition is never accessible. Fixes coccicheck warnings: ./drivers/gpu/drm/omapdrm/dss/dsi.c:2155:5-6: WARNING: Unsigned expression compared with zero: r < 0 Signed-off-by: Junlin Yang Reviewed-by: Laurent Pinchart Tomi, will you take this in your tree ? Thanks. Yes, I'll pick this up. Tomi
Re: [RESEND PATCH] drm/tilcdc: fix raster control register setting
On 16/02/2021 22:22, Dario Binacchi wrote: > The fdd property of the tilcdc_panel_info structure must set the reqdly > bit field (bit 12 to 19) of the raster control register. The previous > statement set the least significant bit instead. > > Signed-off-by: Dario Binacchi > > --- > > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > index 30213708fc99..238068e28729 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > @@ -393,7 +393,7 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc) > return; > } > } > - reg |= info->fdd < 12; > + reg |= info->fdd << 12; > tilcdc_write(dev, LCDC_RASTER_CTRL_REG, reg); > > if (info->invert_pxl_clk) > This is interesting, looks like this has always been broken, and in many cases sets bits 0, which is the enable bit. So we enable LCDC before even setting the fb address. How does this not blow up LCDC totally? The fix looks correct to me, but it will change the register value for boards that have apparently been working for years. Dario, did you test this on actual HW, or did you just spot the error? Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH] mfd: syscon: Don't free allocated name for regmap_config
Hi Marc,
On 03/09/2020 19:02, Marc Zyngier wrote:
> The name allocated for the regmap_config structure is freed
> pretty early, right after the registration of the MMIO region.
>
> Unfortunately, that doesn't follow the life cycle that debugfs
> expects, as it can access the name field long after the free
> has occured.
>
> Move the free on the error path, and keep it forever otherwise.
>
> Fixes: e15d7f2b81d2 ("mfd: syscon: Use a unique name with regmap_config")
> Signed-off-by: Marc Zyngier
> ---
> drivers/mfd/syscon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 75859e492984..7a660411c562 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -108,7 +108,6 @@ static struct syscon *of_syscon_register(struct
> device_node *np, bool check_clk)
> syscon_config.max_register = resource_size() - reg_io_width;
>
> regmap = regmap_init_mmio(NULL, base, _config);
> - kfree(syscon_config.name);
> if (IS_ERR(regmap)) {
> pr_err("regmap init failed\n");
> ret = PTR_ERR(regmap);
> @@ -145,6 +144,7 @@ static struct syscon *of_syscon_register(struct
> device_node *np, bool check_clk)
> regmap_exit(regmap);
> err_regmap:
> iounmap(base);
> + kfree(syscon_config.name);
> err_map:
> kfree(syscon);
> return ERR_PTR(ret);
>
This patch causes lots of kmemleak reports, for example:
unreferenced object 0xc8e6f000 (size 64):
comm "kworker/1:1", pid 22, jiffies 4294938454 (age 95.540s)
hex dump (first 32 bytes):
64 73 70 5f 73 79 73 74 65 6d 40 34 30 64 30 30 dsp_system@40d00
30 30 30 00 e0 09 4d c1 ac 1b 4d c1 64 74 4c c1 000...M...M.dtL.
backtrace:
[<(ptrval)>] __kmalloc_track_caller+0x2bc/0x418
[<(ptrval)>] kvasprintf+0x9c/0x124
[<(ptrval)>] kasprintf+0x70/0xac
[<(ptrval)>] of_syscon_register+0x1f0/0x4f0
[<(ptrval)>] device_node_get_regmap+0x12c/0x158
[<(ptrval)>] syscon_regmap_lookup_by_phandle+0x5c/0x6c
[<(ptrval)>] omap_iommu_probe+0x6ac/0xc28
[<(ptrval)>] platform_probe+0x120/0x1e0
[<(ptrval)>] really_probe+0x2b4/0x121c
[<(ptrval)>] driver_probe_device+0x10c/0x4c0
[<(ptrval)>] __device_attach_driver+0x1d8/0x26c
[<(ptrval)>] bus_for_each_drv+0x174/0x200
[<(ptrval)>] __device_attach+0x2f0/0x45c
[<(ptrval)>] device_initial_probe+0x1c/0x20
[<(ptrval)>] bus_probe_device+0x224/0x2b8
[<(ptrval)>] device_add+0xad0/0x1e18
Tomi
Re: [PATCH] drm/tilcdc: fix raster control register setting
Adding Jyri with the non-TI address. Tomi On 14/02/2021 15:16, Dario Binacchi wrote: > The fdd property of the tilcdc_panel_info structure must set the reqdly > bit field (bit 12 to 19) of the raster control register. The previous > statement set the least significant bit instead. > > Signed-off-by: Dario Binacchi > > --- > > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > index 30213708fc99..238068e28729 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > @@ -393,7 +393,7 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc) > return; > } > } > - reg |= info->fdd < 12; > + reg |= info->fdd << 12; > tilcdc_write(dev, LCDC_RASTER_CTRL_REG, reg); > > if (info->invert_pxl_clk) >
Re: [PATCH] drm/tilcdc: send vblank event when disabling crtc
Dropped the @ti.com addresses and added the new ones.
Tomi
On 29/01/2021 07:58, quanyang.w...@windriver.com wrote:
> From: Quanyang Wang
>
> When run xrandr to change resolution on Beaglebone Black board, it will
> print the error information:
>
> root@beaglebone:~# xrandr -display :0 --output HDMI-1 --mode 720x400
> [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out
> [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:32:tilcdc crtc]
> commit wait timed out
> [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out
> [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CONNECTOR:34:HDMI-A-1]
> commit wait timed out
> [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out
> [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:31:plane-0]
> commit wait timed out
> tilcdc 4830e000.lcdc: already pending page flip!
>
> This is because there is operation sequence as below:
>
> drm_atomic_connector_commit_dpms(mode is DRM_MODE_DPMS_OFF):
> ...
> drm_atomic_helper_setup_commit <- init_completion(commit_A->flip_done)
> drm_atomic_helper_commit_tail
> tilcdc_crtc_atomic_disable
> tilcdc_plane_atomic_update <- drm_crtc_send_vblank_event in
> tilcdc_crtc_irq
> is skipped since tilcdc_crtc->enabled
> is 0
> tilcdc_crtc_atomic_flush <- drm_crtc_send_vblank_event is skipped
> since
> crtc->state->event is set to be NULL in
> tilcdc_plane_atomic_update
> drm_mode_setcrtc:
> ...
> drm_atomic_helper_setup_commit <- init_completion(commit_B->flip_done)
> drm_atomic_helper_wait_for_dependencies
> drm_crtc_commit_wait <- wait for commit_A->flip_done completing
>
> Just as shown above, the steps which could complete commit_A->flip_done
> are all skipped and commit_A->flip_done will never be completed. This will
> result a time-out ERROR when drm_crtc_commit_wait check the
> commit_A->flip_done.
> So add drm_crtc_send_vblank_event in tilcdc_crtc_atomic_disable to
> complete commit_A->flip_done.
>
> Fixes: cb345decb4d2 ("drm/tilcdc: Use standard drm_atomic_helper_commit")
> Signed-off-by: Quanyang Wang
> ---
> drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 30213708fc99..d99afd19ca08 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -515,6 +515,15 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool
> shutdown)
>
> drm_crtc_vblank_off(crtc);
>
> + spin_lock_irq(>dev->event_lock);
> +
> + if (crtc->state->event) {
> + drm_crtc_send_vblank_event(crtc, crtc->state->event);
> + crtc->state->event = NULL;
> + }
> +
> + spin_unlock_irq(>dev->event_lock);
> +
> tilcdc_crtc_disable_irqs(dev);
>
> pm_runtime_put_sync(dev->dev);
>
Re: [PATCH v2] drm/omap: dsi: fix unreachable code in dsi_vc_send_short()
On 27/01/2021 03:51, menglong8.d...@gmail.com wrote:
> From: Menglong Dong
>
> The 'r' in dsi_vc_send_short() is of type 'unsigned int', so the
> 'r < 0' can't be true.
>
> Fix this by introducing a 'err' of type 'int' insteaded.
>
> Fixes: 1ed6253856cb ("drm/omap: dsi: switch dsi_vc_send_long/short to
> mipi_dsi_msg")
>
> Signed-off-by: Menglong Dong
> Reviewed-by: Sebastian Reichel
> ---
> v2:
> - remove word wrap in 'Fixes' tag
> ---
> drivers/gpu/drm/omapdrm/dss/dsi.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c
> b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index 8e11612f5fe1..febcc87ddfe1 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -2149,11 +2149,12 @@ static int dsi_vc_send_short(struct dsi_data *dsi,
> int vc,
>const struct mipi_dsi_msg *msg)
> {
> struct mipi_dsi_packet pkt;
> + int err;
> u32 r;
>
> - r = mipi_dsi_create_packet(, msg);
> - if (r < 0)
> - return r;
> + err = mipi_dsi_create_packet(, msg);
> + if (err)
> + return err;
>
> WARN_ON(!dsi_bus_is_locked(dsi));
>
Thanks! I'll apply to drm-misc-next.
Tomi
Re: [PATCHv1] video: omapfb2: Make standard and custom DSI command mode panel driver mutually exclusive
Hi,
On 12/01/2021 14:02, Sebastian Reichel wrote:
> [replace Tomi's TI mail address with something working]
>
> Hi,
>
> On Fri, Jan 08, 2021 at 08:58:39PM +0100, Sam Ravnborg wrote:
>> Hi Sebastian,
>>
>> On Fri, Jan 08, 2021 at 12:24:41PM +0100, Sebastian Reichel wrote:
>>> Standard DRM panel driver for DSI command mode panel used by omapfb2 is also
>>> available now. Just like the other panels its module name clashes with the
>>> module from drivers/video/fbdev/omap2/omapfb/displays, part of the
>>> deprecated
>>> omapfb2 fbdev driver. As omapfb2 can only be compiled when the omapdrm
>>> driver
>>> is disabled, and the DRM panel drivers are useless in that case, make the
>>> omapfb2 panel depend on the standard DRM panels being disabled to fix
>>> the name clash.
>>>
>>> Fixes: cf64148abcfd ("drm/panel: Move OMAP's DSI command mode panel driver")
>>> Reported-by: Stephen Rothwell
>>> Signed-off-by: Sebastian Reichel
>>
>> For a backport this looks good:
>> Acked-by: Sam Ravnborg
>
> Thanks.
Thanks. I'll push to drm-misc-next, as that's where the commit that
breaks this is.
>> But why is it it we need omapfb at all when we have omapdrm?
>
> I think there are two reasons omapfb has not been killed yet. One
> reason was missing support for manually updated DSI panels, which
> have been working since 1 or 2 kernel releases now. The other reason
> is some people using it in combination with an out-of-tree PowerVR
> kernel driver. There is currently work going on to use a more recent
> PowerVR driver based on omapdrm driven by Maemo Leste people.
omapfb also has a custom sysfw API, so applications that depend on it
would not work anymore. I don't know if there are such applications, though.
>> Can we sunset all or some parts of omap support in video/?
>> If not, what is missing to do so.
>
> IDK the exact status of the PowerVR work and have not been using
> omapfb myself for years. I don't think there is a reason to rush
> this, so my suggestion is removing it in 3 steps giving people
> the chance to complain:
>
> 1. Add 'depends on EXPERT' to 'FB_OMAP2' and add deprecation notice
>referencing omapdrm in help text in 5.12
> 2. Add 'depends on BROKEN' in 5.13
> 3. Drop drivers/video/fbdev/omap2 afterwards
I'd love to remove omapfb, but I also fear that there are still people
using it. We can try the above sequence, but it's probably better to go
slower, as people may not be using the latest kernels.
Tomi
[PATCH] MAINTAINERS: Update addresses for TI display drivers
Update the maintainer email addresses for TI display drivers. Signed-off-by: Tomi Valkeinen --- MAINTAINERS | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 281de213ef47..c21471497a18 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5932,8 +5932,8 @@ F: Documentation/devicetree/bindings/display/st,stm32-ltdc.yaml F: drivers/gpu/drm/stm DRM DRIVERS FOR TI KEYSTONE -M: Jyri Sarha -M: Tomi Valkeinen +M: Jyri Sarha +M: Tomi Valkeinen L: dri-de...@lists.freedesktop.org S: Maintained T: git git://anongit.freedesktop.org/drm/drm-misc @@ -5943,15 +5943,15 @@ F: Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml F: drivers/gpu/drm/tidss/ DRM DRIVERS FOR TI LCDC -M: Jyri Sarha -R: Tomi Valkeinen +M: Jyri Sarha +R: Tomi Valkeinen L: dri-de...@lists.freedesktop.org S: Maintained F: Documentation/devicetree/bindings/display/tilcdc/ F: drivers/gpu/drm/tilcdc/ DRM DRIVERS FOR TI OMAP -M: Tomi Valkeinen +M: Tomi Valkeinen L: dri-de...@lists.freedesktop.org S: Maintained F: Documentation/devicetree/bindings/display/ti/ -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH -next] drm: omapdrm: Delete useless kfree code
On 14/12/2020 15:46, Zheng Yongjun wrote: The parameter of kfree function is NULL, so kfree code is useless, delete it. Signed-off-by: Zheng Yongjun --- drivers/gpu/drm/omapdrm/tcm-sita.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/omapdrm/tcm-sita.c b/drivers/gpu/drm/omapdrm/tcm-sita.c index 817be3c41863..af59e9c03917 100644 --- a/drivers/gpu/drm/omapdrm/tcm-sita.c +++ b/drivers/gpu/drm/omapdrm/tcm-sita.c @@ -254,6 +254,5 @@ struct tcm *sita_init(u16 width, u16 height) return tcm; error: - kfree(tcm); return NULL; } Thanks, I'll merge this. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm/tidss: Use the new api devm_drm_irq_install
On 09/12/2020 14:08, Daniel Vetter wrote: > On Wed, Dec 9, 2020 at 1:06 PM Tomi Valkeinen wrote: >> >> On 09/12/2020 13:56, Daniel Vetter wrote: >>> On Wed, Dec 9, 2020 at 12:29 PM Tomi Valkeinen >>> wrote: >>>> >>>> On 09/12/2020 02:48, Daniel Vetter wrote: >>>>> On Tue, Dec 08, 2020 at 03:50:59PM +0800, Tian Tao wrote: >>>>>> Use devm_drm_irq_install to register interrupts so that >>>>>> drm_irq_uninstall is not needed to be called. >>>>>> >>>>>> Signed-off-by: Tian Tao >>>>> >>>>> There's another drm_irq_install in the error path. But I'm not sure this >>>>> is safe since you're chaning the order in which things get cleaned up now. >>>>> So leaving this up to Tomi. >>>> >>>> Right, I don't think this works. tidss irq_uninstall uses runtime_get/put, >>>> which needs to happen >>>> before pm_runtime_disable. With devm_drm_irq_install that's not the case. >>> >>> Hm I don't spot devm_ versions of these, surely we're not the only >>> ones with this problem? >> >> drm-misc-next has these. hisilicon uses it, but doesn't have an >> irq_uninstall hook, so possibly late >> uninstall is fine there. > > I meant a devm_ version of pm_runtime_enable. Or some other way to > make this just work. I see. No, I don't think we have. Also, I feel a bit uncomfortable with devm'ified irq request/free. devm is fine for allocs and reserving stuff, but this one affects the HW state, and your irq handler could get called until devm frees the irq at some late point of time. Well, it can be made to work, but just need to be careful. I've had my irq handlers getting called too early or too late so many times that I'm a bit paranoid about it =). Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm/tidss: Use the new api devm_drm_irq_install
On 09/12/2020 13:56, Daniel Vetter wrote: > On Wed, Dec 9, 2020 at 12:29 PM Tomi Valkeinen wrote: >> >> On 09/12/2020 02:48, Daniel Vetter wrote: >>> On Tue, Dec 08, 2020 at 03:50:59PM +0800, Tian Tao wrote: >>>> Use devm_drm_irq_install to register interrupts so that >>>> drm_irq_uninstall is not needed to be called. >>>> >>>> Signed-off-by: Tian Tao >>> >>> There's another drm_irq_install in the error path. But I'm not sure this >>> is safe since you're chaning the order in which things get cleaned up now. >>> So leaving this up to Tomi. >> >> Right, I don't think this works. tidss irq_uninstall uses runtime_get/put, >> which needs to happen >> before pm_runtime_disable. With devm_drm_irq_install that's not the case. > > Hm I don't spot devm_ versions of these, surely we're not the only > ones with this problem? drm-misc-next has these. hisilicon uses it, but doesn't have an irq_uninstall hook, so possibly late uninstall is fine there. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm/tidss: Use the new api devm_drm_irq_install
On 09/12/2020 02:48, Daniel Vetter wrote: > On Tue, Dec 08, 2020 at 03:50:59PM +0800, Tian Tao wrote: >> Use devm_drm_irq_install to register interrupts so that >> drm_irq_uninstall is not needed to be called. >> >> Signed-off-by: Tian Tao > > There's another drm_irq_install in the error path. But I'm not sure this > is safe since you're chaning the order in which things get cleaned up now. > So leaving this up to Tomi. Right, I don't think this works. tidss irq_uninstall uses runtime_get/put, which needs to happen before pm_runtime_disable. With devm_drm_irq_install that's not the case. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [REGRESSION] omapdrm/N900 display broken
On 27/11/2020 17:37, Ivaylo Dimitrov wrote: > With 5.9.11 and the patch on top, n900 boots fine, albeit display remains > blank, could be related to > brightness, we're still investigating. Ok. A DSS regdump for a working version and the latest one would be good too. There's a omapdss debugfs dir, with dss, dispc and clk files which are of interest here. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [REGRESSION] omapdrm/N900 display broken
On 27/11/2020 01:17, Ivaylo Dimitrov wrote: > Hi Tomi, > > On 26.11.20 г. 16:11 ч., Tomi Valkeinen wrote: >> Hi Aaro, Ivaylo, >> >> On 24/11/2020 23:03, Ivaylo Dimitrov wrote: >> >>> Is there any progress on the issue? I tried 5.9.1 and still nothing >>> displayed. >> >> Can you test the attached patch? >> > > With this patch I don't see oops that Aaro reported, so: > > Tested-by: Ivaylo Dimitrov > > Seems to fix the particular issue, however, now we get another oops. As this > is not upstream kernel > but one with PVR related patches, I will try again with vanilla 5.9. > > Just in case oops rings any bells (the line in question is > https://github.com/maemo-leste/droid4-linux/blob/maemo-5.9/drivers/gpu/drm/omapdrm/omap_gem.c#L801) Do the PVR patches touch omapdrm? The call stack looks like normal boot time probing stuff, not something happening later (possibly from PVR). Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [REGRESSION] omapdrm/N900 display broken
On 25/11/2020 11:07, Daniel Vetter wrote: >> Laurent, does this ring any bells? The WARN comes in >> drm_atomic_bridge_chain_enable() when >> drm_atomic_get_old_bridge_state() returns null for (presumably) sdi bridge. >> >> I'm not sure why the bridge state would not be there. > > Lack of state on first modeset usually means your > drm_mode_config_reset didn't create one. Or whatever it is you're > using. I didn't look whether you're wiring this up correctly or not. > We might even want to add a ->reset function to > drm_private_state_funcs to make this work for everyone. The bridge driver set atomic_enable and atomic_disable, but no other atomic funcs. It was supposed to set the legacy enable & disable. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [REGRESSION] omapdrm/N900 display broken
Hi Aaro, Ivaylo,
On 24/11/2020 23:03, Ivaylo Dimitrov wrote:
> Is there any progress on the issue? I tried 5.9.1 and still nothing displayed.
Can you test the attached patch?
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>From 97c55032ac5c44885b0ec219467699af0b6153c1 Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen
Date: Thu, 26 Nov 2020 16:04:24 +0200
Subject: [PATCH] drm/omap: sdi: fix bridge enable/disable
When the SDI output was converted to DRM bridge, the atomic versions of
enable and disable funcs were used. This was not intended, as that would
require implementing other atomic funcs too. This leads to:
WARNING: CPU: 0 PID: 18 at drivers/gpu/drm/drm_bridge.c:708 drm_atomic_helper_commit_modeset_enables+0x134/0x268
and display not working.
Fix this by using the legacy enable/disable funcs.
Signed-off-by: Tomi Valkeinen
Reported-by: Aaro Koskinen
Fixes: 8bef8a6d5da81b909a190822b96805a47348146f ("drm/omap: sdi: Register a drm_bridge")
Cc: sta...@vger.kernel.org #v5.7+
---
drivers/gpu/drm/omapdrm/dss/sdi.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/dss/sdi.c b/drivers/gpu/drm/omapdrm/dss/sdi.c
index 033fd30074b0..282e4c837cd9 100644
--- a/drivers/gpu/drm/omapdrm/dss/sdi.c
+++ b/drivers/gpu/drm/omapdrm/dss/sdi.c
@@ -195,8 +195,7 @@ static void sdi_bridge_mode_set(struct drm_bridge *bridge,
sdi->pixelclock = adjusted_mode->clock * 1000;
}
-static void sdi_bridge_enable(struct drm_bridge *bridge,
- struct drm_bridge_state *bridge_state)
+static void sdi_bridge_enable(struct drm_bridge *bridge)
{
struct sdi_device *sdi = drm_bridge_to_sdi(bridge);
struct dispc_clock_info dispc_cinfo;
@@ -259,8 +258,7 @@ static void sdi_bridge_enable(struct drm_bridge *bridge,
regulator_disable(sdi->vdds_sdi_reg);
}
-static void sdi_bridge_disable(struct drm_bridge *bridge,
- struct drm_bridge_state *bridge_state)
+static void sdi_bridge_disable(struct drm_bridge *bridge)
{
struct sdi_device *sdi = drm_bridge_to_sdi(bridge);
@@ -278,8 +276,8 @@ static const struct drm_bridge_funcs sdi_bridge_funcs = {
.mode_valid = sdi_bridge_mode_valid,
.mode_fixup = sdi_bridge_mode_fixup,
.mode_set = sdi_bridge_mode_set,
- .atomic_enable = sdi_bridge_enable,
- .atomic_disable = sdi_bridge_disable,
+ .enable = sdi_bridge_enable,
+ .disable = sdi_bridge_disable,
};
static void sdi_bridge_init(struct sdi_device *sdi)
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm/omap: dmm_tiler: fix return error code in omap_dmm_probe()
On 17/11/2020 15:41, Thomas Zimmermann wrote:
> Hi
>
> Am 17.11.20 um 07:10 schrieb Yang Yingliang:
>> Return -ENOMEM when allocating refill memory failed.
>>
>> Fixes: 71e8831f6407 ("drm/omap: DMM/TILER support for OMAP4+ platform")
>> Reported-by: Hulk Robot
>> Signed-off-by: Yang Yingliang
>> ---
>> drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
>> b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
>> index 42ec51bb7b1b..7f4317248812 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
>> @@ -889,6 +889,7 @@ static int omap_dmm_probe(struct platform_device *dev)
>> _dmm->refill_pa, GFP_KERNEL);
>> if (!omap_dmm->refill_va) {
>> dev_err(>dev, "could not allocate refill memory\n");
>> +ret = -ENOMEM;
>
> Reviewed-by: Thomas Zimmermann
>
> Thanks for the patch. I'll add it to drm-misc-next. There are more such
> errors here. If the very first allocation fails, the function returns
> -EFAULT, which makes no sense.
Indeed. -EFAULT is quite an odd default value for ret... I'll drop the default
and assign a real
error value where needed.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v2 00/17] Refactor fw_devlink to significantly improve boot time
Hi, On 21/11/2020 04:02, Saravana Kannan wrote: > The current implementation of fw_devlink is very inefficient because it > tries to get away without creating fwnode links in the name of saving > memory usage. Past attempts to optimize runtime at the cost of memory > usage were blocked with request for data showing that the optimization > made significant improvement for real world scenarios. > > We have those scenarios now. There have been several reports of boot > time increase in the order of seconds in this thread [1]. Several OEMs > and SoC manufacturers have also privately reported significant > (350-400ms) increase in boot time due to all the parsing done by > fw_devlink. > > So this patch series refactors fw_devlink to be more efficient. The key > difference now is the addition of support for fwnode links -- just a few > simple APIs. This also allows most of the code to be moved out of > firmware specific (DT mostly) code into driver core. > > This brings the following benefits: > - Instead of parsing the device tree multiple times (complexity was > close to O(N^3) where N in the number of properties) during bootup, > fw_devlink parses each fwnode node/property only once and creates > fwnode links. The rest of the fw_devlink code then just looks at these > fwnode links to do rest of the work. > > - Makes it much easier to debug probe issue due to fw_devlink in the > future. fw_devlink=on blocks the probing of devices if they depend on > a device that hasn't been added yet. With this refactor, it'll be very > easy to tell what that device is because we now have a reference to > the fwnode of the device. > > - Much easier to add fw_devlink support to ACPI and other firmware > types. A refactor to move the common bits from DT specific code to > driver core was in my TODO list as a prerequisite to adding ACPI > support to fw_devlink. This series gets that done. > > Laurent and Grygorii tested the v1 series and they saw boot time > improvment of about 12 seconds and 3 seconds, respectively. Tested v2 on OMAP4 SDP. With my particular config, boot time to starting init went from 18.5 seconds to 12.5 seconds. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v1 0/4] Add bus format negotiation support for Cadence MHDP8546 driver
Hi, On 13/11/2020 11:46, Yuti Amonkar wrote: > This patch series add bus format negotiation support for Cadence MHDP8546 > bridge > driver. > > The patch series has four patches in the below sequence: > 1. drm: bridge: cdns-mhdp8546: Add output bus format negotiation > Add minimal output bus format negotiation support. > 2. drm: bridge: cdns-mhdp8546: Modify atomic_get_input_bus_format bridge > function. > Get the input format based on output format supported. > 3. drm: bridge: cdns-mhdp8546: Remove setting of bus format using connector > info > Remove the bus format configuration using connector info structure. > 4. drm: bridge: cdns-mhdp8546: Retrieve the pixel format and bpc based on bus > format > Get the pixel format and bpc based on negotiated output bus format. > > This patch series is dependent on tidss series [1] for the new connector > model support. > > [1] > https://patchwork.kernel.org/project/dri-devel/cover/20201109170601.21557-1-nikhil...@ti.com/ > Can you explain how this works? Afaics, what we should be doing is: - We don't have proper bus formats for DP output, so we need to use MEDIA_BUS_FMT_FIXED as the output format. This is what you do in the first patch. (But is that patch even needed, if MEDIA_BUS_FMT_FIXED is the default anyway) - In cdns_mhdp_get_input_bus_fmts, the function should exit if the given output format is not MEDIA_BUS_FMT_FIXED. - In cdns_mhdp_get_input_bus_fmts, the driver should return all the RGB bus formats, if MHDP is able to upscale/downscale RGB (e.g. RGB 8-bpc to RGB 12-bpc). - If the monitor supports YUV modes according to the display_info, cdns_mhdp_get_input_bus_fmts can also return those. - Then later, in atomic_check and commit, mhdp driver has the negotiated bus format, which it should use to program the registers. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] [v2] drm/omap: Fix runtime PM imbalance on error
On 22/08/2020 09:57, Dinghao Liu wrote: > pm_runtime_get_sync() increments the runtime PM usage counter > even when it returns an error code. However, users of its > direct wrappers in omapdrm assume that PM usage counter will > not change on error. Thus a pairing decrement is needed on > the error handling path for these wrappers to keep the counter > balanced. > > Signed-off-by: Dinghao Liu > --- > > Changelog: > > v2: - Fix 5 additional similar cases in omapdrm. Thanks, I'll apply to drm-misc-next. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm: omapdrm: Replace HTTP links with HTTPS ones
On 13/07/2020 15:28, Alexander A. Klimov wrote: > Rationale: > Reduces attack surface on kernel devs opening the links for MITM > as HTTPS traffic is much harder to manipulate. > > Deterministic algorithm: > For each file: > If not .svg: > For each line: > If doesn't contain `\bxmlns\b`: > For each link, `\bhttp://[^# \t\r\n]*(?:\w|/)`: > If neither `\bgnu\.org/license`, nor `\bmozilla\.org/MPL\b`: > If both the HTTP and HTTPS versions > return 200 OK and serve the same content: > Replace HTTP with HTTPS. > > Signed-off-by: Alexander A. Klimov > --- > Continuing my work started at 93431e0607e5. > See also: git log --oneline '--author=Alexander A. Klimov > ' v5.7..master > (Actually letting a shell for loop submit all this stuff for me.) > > If there are any URLs to be removed completely or at least not just > HTTPSified: > Just clearly say so and I'll *undo my change*. > See also: https://lkml.org/lkml/2020/6/27/64 > > If there are any valid, but yet not changed URLs: > See: https://lkml.org/lkml/2020/6/26/837 > > If you apply the patch, please let me know. Thanks, I'll apply to drm-misc-next. Sorry it took so long =) Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 08/19] drm/omapdrm/omap_gem: Fix misnamed and missing parameter descriptions
On 06/11/2020 23:49, Lee Jones wrote: > Fixes the following W=1 kernel build warning(s): > > drivers/gpu/drm/omapdrm/omap_gem.c:593: warning: Function parameter or > member 'file' not described in 'omap_gem_dumb_create' > drivers/gpu/drm/omapdrm/omap_gem.c:593: warning: Excess function parameter > 'drm_file' description in 'omap_gem_dumb_create' > drivers/gpu/drm/omapdrm/omap_gem.c:619: warning: Function parameter or > member 'offset' not described in 'omap_gem_dumb_map_offset' > > Cc: Tomi Valkeinen > Cc: David Airlie > Cc: Daniel Vetter > Cc: Sumit Semwal > Cc: "Christian König" > Cc: Rob Clark > Cc: dri-de...@lists.freedesktop.org > Cc: linux-me...@vger.kernel.org > Cc: linaro-mm-...@lists.linaro.org > Signed-off-by: Lee Jones > --- > drivers/gpu/drm/omapdrm/omap_gem.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Thanks! I'll pick this one and the next to drm-misc-next. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 08/19] gpu: drm: omapdrm: dss: dsi: Rework and remove a few unused variables
On 05/11/2020 20:07, Lee Jones wrote: > On Thu, 05 Nov 2020, Tomi Valkeinen wrote: > >> On 05/11/2020 16:45, Lee Jones wrote: >>> Fixes the following W=1 kernel build warning(s): >>> >>> drivers/gpu/drm/omapdrm/dss/dsi.c: In function ‘_dsi_print_reset_status’: >>> drivers/gpu/drm/omapdrm/dss/dsi.c:1131:6: warning: variable ‘l’ set but >>> not used [-Wunused-but-set-variable] >>> drivers/gpu/drm/omapdrm/dss/dsi.c: In function ‘dsi_update’: >>> drivers/gpu/drm/omapdrm/dss/dsi.c:3943:10: warning: variable ‘dh’ set but >>> not used [-Wunused-but-set-variable] >>> drivers/gpu/drm/omapdrm/dss/dsi.c:3943:6: warning: variable ‘dw’ set but >>> not used [-Wunused-but-set-variable] >>> >>> Cc: Tomi Valkeinen >>> Cc: David Airlie >>> Cc: Daniel Vetter >>> Cc: Laurent Pinchart >>> Cc: dri-de...@lists.freedesktop.org >>> Signed-off-by: Lee Jones >>> --- >>> drivers/gpu/drm/omapdrm/dss/dsi.c | 9 ++--- >>> 1 file changed, 2 insertions(+), 7 deletions(-) >> >> I'd use "drm/omap: dsi: " subject prefix, the current one is fine too: >> >> Reviewed-by: Tomi Valkeinen >> >> Should I pick this up or do you want to keep the series intact? > > If you are in a position to take it, please do so. > > I rebase every day, so it will just vanish from my working set. I have a 56 patch series on dsi.c that I sent yesterday, and it conflicts with this one. I'll pick this one on top of my series. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 04/19] gpu: drm: omapdrm: omap_irq: Fix a couple of doc-rot issues
On 05/11/2020 16:45, Lee Jones wrote: > The API has been updated, but the header was not. > > Fixes the following W=1 kernel build warning(s): > > drivers/gpu/drm/omapdrm/omap_irq.c:115: warning: Function parameter or > member 'crtc' not described in 'omap_irq_enable_vblank' > drivers/gpu/drm/omapdrm/omap_irq.c:115: warning: Excess function parameter > 'dev' description in 'omap_irq_enable_vblank' > drivers/gpu/drm/omapdrm/omap_irq.c:115: warning: Excess function parameter > 'pipe' description in 'omap_irq_enable_vblank' > drivers/gpu/drm/omapdrm/omap_irq.c:142: warning: Function parameter or > member 'crtc' not described in 'omap_irq_disable_vblank' > drivers/gpu/drm/omapdrm/omap_irq.c:142: warning: Excess function parameter > 'dev' description in 'omap_irq_disable_vblank' > drivers/gpu/drm/omapdrm/omap_irq.c:142: warning: Excess function parameter > 'pipe' description in 'omap_irq_disable_vblank' > > Cc: Tomi Valkeinen > Cc: David Airlie > Cc: Daniel Vetter > Cc: Rob Clark > Cc: dri-de...@lists.freedesktop.org > Signed-off-by: Lee Jones > --- > drivers/gpu/drm/omapdrm/omap_irq.c | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c > b/drivers/gpu/drm/omapdrm/omap_irq.c > index 382bcdc72ac06..8643871e23a83 100644 > --- a/drivers/gpu/drm/omapdrm/omap_irq.c > +++ b/drivers/gpu/drm/omapdrm/omap_irq.c > @@ -100,8 +100,7 @@ int omap_irq_enable_framedone(struct drm_crtc *crtc, bool > enable) > > /** > * enable_vblank - enable vblank interrupt events > - * @dev: DRM device > - * @pipe: which irq to enable > + * @crtc: DRM CRTC > * > * Enable vblank interrupts for @crtc. If the device doesn't have > * a hardware vblank counter, this routine should be a no-op, since > @@ -131,8 +130,7 @@ int omap_irq_enable_vblank(struct drm_crtc *crtc) > > /** > * disable_vblank - disable vblank interrupt events > - * @dev: DRM device > - * @pipe: which irq to enable > + * @crtc: DRM CRTC > * > * Disable vblank interrupts for @crtc. If the device doesn't have > * a hardware vblank counter, this routine should be a no-op, since > Hmm, I don't know why we have the doc texts there. These are omapdrm internal functions, and the text sounds like it was copied from a framework function. I think we can drop the texts here. But this patch is fine too, and I can drop the text later: Reviewed-by: Tomi Valkeinen Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 08/19] gpu: drm: omapdrm: dss: dsi: Rework and remove a few unused variables
On 05/11/2020 16:45, Lee Jones wrote: > Fixes the following W=1 kernel build warning(s): > > drivers/gpu/drm/omapdrm/dss/dsi.c: In function ‘_dsi_print_reset_status’: > drivers/gpu/drm/omapdrm/dss/dsi.c:1131:6: warning: variable ‘l’ set but not > used [-Wunused-but-set-variable] > drivers/gpu/drm/omapdrm/dss/dsi.c: In function ‘dsi_update’: > drivers/gpu/drm/omapdrm/dss/dsi.c:3943:10: warning: variable ‘dh’ set but > not used [-Wunused-but-set-variable] > drivers/gpu/drm/omapdrm/dss/dsi.c:3943:6: warning: variable ‘dw’ set but not > used [-Wunused-but-set-variable] > > Cc: Tomi Valkeinen > Cc: David Airlie > Cc: Daniel Vetter > Cc: Laurent Pinchart > Cc: dri-de...@lists.freedesktop.org > Signed-off-by: Lee Jones > --- > drivers/gpu/drm/omapdrm/dss/dsi.c | 9 ++--- > 1 file changed, 2 insertions(+), 7 deletions(-) I'd use "drm/omap: dsi: " subject prefix, the current one is fine too: Reviewed-by: Tomi Valkeinen Should I pick this up or do you want to keep the series intact? Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 2/4] arm64: dts: ti: k3-j721e*: Cleanup disabled nodes at SoC dtsi level
On 05/11/2020 00:43, Nishanth Menon wrote:
> The device tree standard sets the default node behavior when status
> property as enabled. There are many reasons for doing the same, number
> of strings in device tree, default power management functionality etc
> are few of the reasons.
>
> In general, after a few rounds of discussions [1] there are few
> options one could take when dealing with SoC dtsi and board dts
>
> a. SoC dtsi provide nodes as a super-set default (aka enabled) state and
>to prevent messy board files, when more boards are added per SoC, we
>optimize and disable commonly un-used nodes in board-common.dtsi
> b. SoC dtsi disables all hardware dependent nodes by default and board
>dts files enable nodes based on a need basis.
> c. Subjectively pick and choose which nodes we will disable by default
>in SoC dtsi and over the years we can optimize things and change
>default state depending on the need.
>
> While there are pros and cons on each of these approaches, the right
> thing to do will be to stick with device tree default standards and
> work within those established rules. So, we choose to go with option
> (a).
>
> Lets cleanup defaults of j721e SoC dtsi before this gets more harder
> to cleanup later on and new SoCs are added.
>
> The only functional difference between the dtb generated is
> status='okay' is no longer necessary for mcasp10 and depends on the
> default state.
>
> [1]
> https://lore.kernel.org/linux-arm-kernel/20201027130701.ge5...@atomide.com/
>
> Fixes: 1c4d35265fb2 ("arm64: dts: ti: k3-j721e-main: Add McASP nodes")
> Fixes: 76921f15acc0 ("arm64: dts: ti: k3-j721e-main: Add DSS node")
> Cc: Jyri Sarha
> Cc: Tomi Valkeinen
> Cc: Peter Ujfalusi
> Cc: Tony Lindgren
> Signed-off-by: Nishanth Menon
> ---
> .../dts/ti/k3-j721e-common-proc-board.dts | 48 ++-
> arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 26 --
> 2 files changed, 47 insertions(+), 27 deletions(-)
Reviewed-by: Tomi Valkeinen
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 1/4] arm64: dts: ti: k3-am65*: Cleanup disabled nodes at SoC dtsi level
On 05/11/2020 00:43, Nishanth Menon wrote:
> The device tree standard sets the default node behavior when status
> property as enabled. There are many reasons for doing the same, number
> of strings in device tree, default power management functionality etc
> are few of the reasons.
>
> In general, after a few rounds of discussions [1] there are few
> options one could take when dealing with SoC dtsi and board dts
>
> a. SoC dtsi provide nodes as a super-set default (aka enabled) state and
>to prevent messy board files, when more boards are added per SoC, we
>optimize and disable commonly un-used nodes in board-common.dtsi
> b. SoC dtsi disables all hardware dependent nodes by default and board
>dts files enable nodes based on a need basis.
> c. Subjectively pick and choose which nodes we will disable by default
>in SoC dtsi and over the years we can optimize things and change
>default state depending on the need.
>
> While there are pros and cons on each of these approaches, the right
> thing to do will be to stick with device tree default standards and
> work within those established rules. So, we choose to go with option
> (a).
>
> Lets cleanup defaults of am654 SoC dtsi before this gets more harder
> to cleanup later on and new SoCs are added.
>
> The dtb generated is identical with the patch and it is just cleanup to
> ensure we have a clean usage model
>
> [1]
> https://lore.kernel.org/linux-arm-kernel/20201027130701.ge5...@atomide.com/
>
> Fixes: 9bcb631e9953 ("arm64: dts: ti: k3-am654-main: Add McASP nodes")
> Fixes: fc539b90eda2 ("arm64: dts: ti: am654: Add DSS node")
> Cc: Jyri Sarha
> Cc: Tomi Valkeinen
> Cc: Peter Ujfalusi
> Cc: Tony Lindgren
> Signed-off-by: Nishanth Menon
> ---
> arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 8
> arch/arm64/boot/dts/ti/k3-am654-base-board.dts | 16
> 2 files changed, 16 insertions(+), 8 deletions(-)
Reviewed-by: Tomi Valkeinen
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] arm64: dts: ti: k3-am65*/j721e*: Fix unit address format error for dss node
On 05/11/2020 00:25, Nishanth Menon wrote:
> Fix the node address to follow the device tree convention.
>
> This fixes the dtc warning:
> : Warning (simple_bus_reg): /bus@10/dss@04a0: simple-bus
> unit address format error, expected "4a0"
>
> Fixes: 76921f15acc0 ("arm64: dts: ti: k3-j721e-main: Add DSS node")
> Fixes: fc539b90eda2 ("arm64: dts: ti: am654: Add DSS node")
> Cc: Jyri Sarha
> Cc: Tomi Valkeinen
> Signed-off-by: Nishanth Menon
> ---
> arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 2 +-
> arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> index 533525229a8d..27f6fd9eaa0a 100644
> --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> @@ -834,7 +834,7 @@ csi2_0: port@0 {
> };
> };
>
> - dss: dss@04a0 {
> + dss: dss@4a0 {
> compatible = "ti,am65x-dss";
> reg = <0x0 0x04a0 0x0 0x1000>, /* common */
> <0x0 0x04a02000 0x0 0x1000>, /* vidl1 */
> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> index e2a96b2c423c..c66ded9079be 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> @@ -1278,7 +1278,7 @@ ufs@4e84000 {
> };
> };
>
> - dss: dss@04a0 {
> + dss: dss@4a00000 {
> compatible = "ti,j721e-dss";
> reg =
> <0x00 0x04a0 0x00 0x1>, /* common_m */
>
Reviewed-by: Tomi Valkeinen
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v2] drm/bridge: tpd12s015: Fix irq registering in tpd12s015_probe
On 02/11/2020 16:30, YueHaibing wrote:
> gpiod_to_irq() return negative value in case of error,
> the existing code doesn't handle negative error codes.
> If the HPD gpio supports IRQs (gpiod_to_irq returns a
> valid number), we use the IRQ. If it doesn't (gpiod_to_irq
> returns an error), it gets polled via detect().
>
> Fixes: cff5e6f7e83f ("drm/bridge: Add driver for the TI TPD12S015 HDMI level
> shifter")
> Signed-off-by: YueHaibing
> ---
> v2: Add checking for >= 0 and update commit message
> ---
> drivers/gpu/drm/bridge/ti-tpd12s015.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-tpd12s015.c
> b/drivers/gpu/drm/bridge/ti-tpd12s015.c
> index 514cbf0eac75..e0e015243a60 100644
> --- a/drivers/gpu/drm/bridge/ti-tpd12s015.c
> +++ b/drivers/gpu/drm/bridge/ti-tpd12s015.c
> @@ -160,7 +160,7 @@ static int tpd12s015_probe(struct platform_device *pdev)
>
> /* Register the IRQ if the HPD GPIO is IRQ-capable. */
> tpd->hpd_irq = gpiod_to_irq(tpd->hpd_gpio);
> - if (tpd->hpd_irq) {
> + if (tpd->hpd_irq >= 0) {
> ret = devm_request_threaded_irq(>dev, tpd->hpd_irq, NULL,
> tpd12s015_hpd_isr,
> IRQF_TRIGGER_RISING |
>
Reviewed-by: Tomi Valkeinen
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm/bridge: tpd12s015: Fix irq registering in tpd12s015_probe
On 02/11/2020 11:36, Yuehaibing wrote:
> On 2020/11/2 14:57, Tomi Valkeinen wrote:
>> On 31/10/2020 09:19, Sam Ravnborg wrote:
>>> Hi YueHaibing
>>>
>>> Thanks for the fix. Appreciated but please update as per comments below.
>>>
>>> On Sat, Oct 31, 2020 at 11:16:48AM +0800, YueHaibing wrote:
>>>> gpiod_to_irq() return negative value in case of error,
>>>> the existing code handle negative error codes wrongly.
>>>>
>>>> Fixes: cff5e6f7e83f ("drm/bridge: Add driver for the TI TPD12S015 HDMI
>>>> level shifter")
>>>> Signed-off-by: YueHaibing
>>>> ---
>>>> drivers/gpu/drm/bridge/ti-tpd12s015.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/ti-tpd12s015.c
>>>> b/drivers/gpu/drm/bridge/ti-tpd12s015.c
>>>> index 514cbf0eac75..a18d5197c16c 100644
>>>> --- a/drivers/gpu/drm/bridge/ti-tpd12s015.c
>>>> +++ b/drivers/gpu/drm/bridge/ti-tpd12s015.c
>>>> @@ -160,7 +160,7 @@ static int tpd12s015_probe(struct platform_device
>>>> *pdev)
>>>>
>>>>/* Register the IRQ if the HPD GPIO is IRQ-capable. */
>>>>tpd->hpd_irq = gpiod_to_irq(tpd->hpd_gpio);
>>>> - if (tpd->hpd_irq) {
>>>> + if (tpd->hpd_irq > 0) {
>>>>ret = devm_request_threaded_irq(>dev, tpd->hpd_irq, NULL,
>>>>tpd12s015_hpd_isr,
>>>>IRQF_TRIGGER_RISING |
>>>
>>> The current implmentation will skip devm_request_threaded_irq() in case
>>> or error - but continue with the rest of the function. So the
>>> driver fails to return an error code.
>>
>> That is intended. If the HPD gpio supports IRQs (gpiod_to_irq returns a
>> valid number), we use the
>> IRQ. If it doesn't (gpiod_to_irq returns an error), it gets polled via
>> detect(). Both are ok.
>>
>> I don't know if the gpiod_to_irq never returning 0 is something we should
>> rely on. The docs say
>> gpiod_to_irq returns the irq number or an error, so I think checking for >=
>> 0 matches the docs better.
>>
>
> gpiod_to_irq() now never returns 0, see:
> https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/gpio/gpiolib.c#L3183
>
> Also commit 4c37ce8608a8 ("gpio: make gpiod_to_irq() return negative for
> NO_IRQ") says:
>
> commit 4c37ce8608a8c6521726d4cd1d4f54424e8d095f
> Author: Linus Walleij
> Date: Mon May 2 13:13:10 2016 +0200
>
> gpio: make gpiod_to_irq() return negative for NO_IRQ
>
> If a translation returns zero, that means NO_IRQ, so we
> should return an error since the function is documented to
> return a negative code on error.
>
> So checking for >0 is enough, my patch is correct.
Yes, but that is a gpiod_to_irq internal implementation detail. The
documentation says it returns an
error code (negative) or a valid irq number. The documentation does not say
gpiod_to_irq never
returns 0.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm/bridge: tpd12s015: Fix irq registering in tpd12s015_probe
On 31/10/2020 09:19, Sam Ravnborg wrote:
> Hi YueHaibing
>
> Thanks for the fix. Appreciated but please update as per comments below.
>
> On Sat, Oct 31, 2020 at 11:16:48AM +0800, YueHaibing wrote:
>> gpiod_to_irq() return negative value in case of error,
>> the existing code handle negative error codes wrongly.
>>
>> Fixes: cff5e6f7e83f ("drm/bridge: Add driver for the TI TPD12S015 HDMI level
>> shifter")
>> Signed-off-by: YueHaibing
>> ---
>> drivers/gpu/drm/bridge/ti-tpd12s015.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-tpd12s015.c
>> b/drivers/gpu/drm/bridge/ti-tpd12s015.c
>> index 514cbf0eac75..a18d5197c16c 100644
>> --- a/drivers/gpu/drm/bridge/ti-tpd12s015.c
>> +++ b/drivers/gpu/drm/bridge/ti-tpd12s015.c
>> @@ -160,7 +160,7 @@ static int tpd12s015_probe(struct platform_device *pdev)
>>
>> /* Register the IRQ if the HPD GPIO is IRQ-capable. */
>> tpd->hpd_irq = gpiod_to_irq(tpd->hpd_gpio);
>> -if (tpd->hpd_irq) {
>> +if (tpd->hpd_irq > 0) {
>> ret = devm_request_threaded_irq(>dev, tpd->hpd_irq, NULL,
>> tpd12s015_hpd_isr,
>> IRQF_TRIGGER_RISING |
>
> The current implmentation will skip devm_request_threaded_irq() in case
> or error - but continue with the rest of the function. So the
> driver fails to return an error code.
That is intended. If the HPD gpio supports IRQs (gpiod_to_irq returns a valid
number), we use the
IRQ. If it doesn't (gpiod_to_irq returns an error), it gets polled via
detect(). Both are ok.
I don't know if the gpiod_to_irq never returning 0 is something we should rely
on. The docs say
gpiod_to_irq returns the irq number or an error, so I think checking for >= 0
matches the docs better.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v1] of: platform: Batch fwnode parsing in the init_machine() path
On 27/10/2020 05:29, Saravana Kannan wrote: > Can you try throwing around fw_devlink_pause/resume() around the > of_platform_populate() call in arch/arm/mach-omap2/pdata-quirks.c? > Just trying to verify the cause/fix. AM5 EVM on v5.10-rc1: [1.139945] cpuidle: using governor menu [ 13.126461] No ATAGs? After adding fw_devlink_pause/resume around of_platform_populate: [1.139587] cpuidle: using governor menu [1.899913] No ATAGs? Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm: bridge: cdns: Kconfig: Switch over dependency to ARCH_K3
On 26/10/2020 18:54, Nishanth Menon wrote:
> With the integration of chip-id detection scheme in kernel[1], there
> is no specific need to maintain multitudes of SoC specific config
> options, discussed as per [2], we have deprecated the usage in other
> places for v5.10-rc1. Fix the missing user so that we can clean up the
> configs in v5.11.
>
> [1] drivers/soc/ti/k3-socinfo.c commit 907a2b7e2fc7 ("soc: ti: add k3
> platforms chipid module driver")
> [2]
> https://lore.kernel.org/linux-arm-kernel/20200908112534.t5bgrjf7y3a6l2ss@akan/
>
> Fixes: afba7e6c5fc1 ("rm: bridge: cdns-mhdp8546: Add TI J721E wrapper")
> Cc: Swapnil Jakhade
> Cc: Tomi Valkeinen
> Cc: Laurent Pinchart
> Cc: Yuti Amonkar
> Cc: Jyri Sarha
> Signed-off-by: Nishanth Menon
> ---
> drivers/gpu/drm/bridge/cadence/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/cadence/Kconfig
> b/drivers/gpu/drm/bridge/cadence/Kconfig
> index 511d67b16d14..ef8c230e0f62 100644
> --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
> @@ -13,7 +13,7 @@ config DRM_CDNS_MHDP8546
> if DRM_CDNS_MHDP8546
>
> config DRM_CDNS_MHDP8546_J721E
> - depends on ARCH_K3_J721E_SOC || COMPILE_TEST
> + depends on ARCH_K3 || COMPILE_TEST
> bool "J721E Cadence DPI/DP wrapper support"
> default y
> help
>
Reviewed-by: Tomi Valkeinen
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v3] leds: tlc591xx: fix leak of device node iterator
On 26/09/2020 19:27, Tobias Jordan wrote:
> In one of the error paths of the for_each_child_of_node loop in
> tlc591xx_probe, add missing call to of_node_put.
>
> Fixes: 1ab4531ad132 ("leds: tlc591xx: simplify driver by using the managed
> led API")
> Signed-off-by: Tobias Jordan
> Reviewed-by: Marek Behún
>
> ---
> v3: removed linebreak from Fixes: tag in commit message
> v2: rebased to Pavel's for-next branch
>
> drivers/leds/leds-tlc591xx.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/leds/leds-tlc591xx.c b/drivers/leds/leds-tlc591xx.c
> index 0929f1275814..a8cc49752cd5 100644
> --- a/drivers/leds/leds-tlc591xx.c
> +++ b/drivers/leds/leds-tlc591xx.c
> @@ -214,6 +214,7 @@ tlc591xx_probe(struct i2c_client *client,
> err = devm_led_classdev_register_ext(dev, >ldev,
>_data);
> if (err < 0) {
> + of_node_put(child);
> if (err != -EPROBE_DEFER)
> dev_err(dev, "couldn't register LED %s\n",
> led->ldev.name);
>
Reviewed-by: Tomi Valkeinen
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: linux-next: build warning after merge of the drm tree
Hi Stephen,
On 23/09/2020 06:36, Stephen Rothwell wrote:
> Hi all,
>
> After merging the drm tree, today's linux-next build (x86_64 allmodconfig)
> produced this warning:
>
> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c: In function
> 'cdns_mhdp_fw_activate':
> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:751:10: warning:
> conversion from 'long unsigned int' to 'unsigned int' changes value from
> '18446744073709551613' to '4294967293' [-Woverflow]
> 751 | writel(~CDNS_APB_INT_MASK_SW_EVENT_INT,
> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c: In function
> 'cdns_mhdp_attach':
> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:1692:10: warning:
> conversion from 'long unsigned int' to 'unsigned int' changes value from
> '18446744073709551613' to '4294967293' [-Woverflow]
> 1692 | writel(~CDNS_APB_INT_MASK_SW_EVENT_INT,
> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c: In function
> 'cdns_mhdp_bridge_hpd_enable':
> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:2125:10: warning:
> conversion from 'long unsigned int' to 'unsigned int' changes value from
> '18446744073709551613' to '4294967293' [-Woverflow]
> 2125 | writel(~CDNS_APB_INT_MASK_SW_EVENT_INT,
>
> Introduced by commit
>
> fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge")
>
Thanks. I think we can just do:
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 621ebdbff8a3..d0c65610ebb5 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -748,7 +748,7 @@ static int cdns_mhdp_fw_activate(const struct firmware *fw,
* bridge should already be detached.
*/
if (mhdp->bridge_attached)
- writel(~CDNS_APB_INT_MASK_SW_EVENT_INT,
+ writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
mhdp->regs + CDNS_APB_INT_MASK);
spin_unlock(>start_lock);
@@ -1689,7 +1689,7 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge,
/* Enable SW event interrupts */
if (hw_ready)
- writel(~CDNS_APB_INT_MASK_SW_EVENT_INT,
+ writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
mhdp->regs + CDNS_APB_INT_MASK);
return 0;
@@ -2122,7 +2122,7 @@ static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge
*bridge)
/* Enable SW event interrupts */
if (mhdp->bridge_attached)
- writel(~CDNS_APB_INT_MASK_SW_EVENT_INT,
+ writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
mhdp->regs + CDNS_APB_INT_MASK);
}
I'll send a patch.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH -next] drm: omapdrm: dss: simplify the return expression of hdmi_init_pll_data
Hi,
On 21/09/2020 16:10, Qinglang Miao wrote:
> Simplify the return expression.
>
> Signed-off-by: Qinglang Miao
> ---
> drivers/gpu/drm/omapdrm/dss/hdmi_pll.c | 7 +--
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi_pll.c
> b/drivers/gpu/drm/omapdrm/dss/hdmi_pll.c
> index cf2b000f3..c3e85b636 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi_pll.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi_pll.c
> @@ -131,7 +131,6 @@ static int hdmi_init_pll_data(struct dss_device *dss,
> {
> struct dss_pll *pll = >pll;
> struct clk *clk;
> - int r;
>
> clk = devm_clk_get(>dev, "sys_clk");
> if (IS_ERR(clk)) {
> @@ -151,11 +150,7 @@ static int hdmi_init_pll_data(struct dss_device *dss,
>
> pll->ops = _pll_ops;
>
> - r = dss_pll_register(dss, pll);
> - if (r)
> - return r;
> -
> - return 0;
> + return dss_pll_register(dss, pll);
> }
>
> int hdmi_pll_init(struct dss_device *dss, struct platform_device *pdev,
>
I like it more when there's a return 0 at the end of the function, especially
in functions where
there are multiple cases of if(...) return r. It's more easily readable, at
least to my eyes.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
[PATCH v2 1/2] dt-bindings: phy: cdns,torrent-phy: add reset-names
Add reset-names as a required property. There are no dts files using torrent phy yet, so it is safe to add a new required property. Signed-off-by: Tomi Valkeinen --- Changes in v2: * Base on phy-next * Update example * Add torrent_apb .../devicetree/bindings/phy/phy-cadence-torrent.yaml | 10 ++ 1 file changed, 10 insertions(+) diff --git a/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml b/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml index 26480f89627d..e266ade53d87 100644 --- a/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml +++ b/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml @@ -57,6 +57,13 @@ properties: - description: Torrent PHY reset. - description: Torrent APB reset. This is optional. + reset-names: +minItems: 1 +maxItems: 2 +items: + - const: torrent_reset + - const: torrent_apb + patternProperties: '^phy@[0-3]$': type: object @@ -127,6 +134,7 @@ required: - reg - reg-names - resets + - reset-names additionalProperties: false @@ -144,6 +152,7 @@ examples: <0xf0 0xfb030a00 0x0 0x0040>; reg-names = "torrent_phy", "dptx_phy"; resets = < 0>; +reset-names = "torrent_reset"; clocks = <_clk>; clock-names = "refclk"; #address-cells = <1>; @@ -172,6 +181,7 @@ examples: reg = <0xf0 0xfb50 0x0 0x0010>; reg-names = "torrent_phy"; resets = < 0>, < 1>; +reset-names = "torrent_reset", "torrent_apb"; clocks = <_clk>; clock-names = "refclk"; #address-cells = <1>; -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
[PATCH v2 2/2] dt-bindings: phy: ti,phy-j721e-wiz: fix bindings for torrent phy
When WIZ wraps a Cadence Torrent PHY (instead of Cadence Sierra PHY)
there is a difference in the refclk-dig node: Torrent only has two
clocks instead of Sierra's four clocks. Add minItems: 2 to solve this.
Additionally, in our use case we only need to use assigned-clock for a
single clock, but the current binding requires either no assigned-clocks
or two. Fix this by adding minItems: 1 to all the assigned-clock
properties.
There was also an extra trailing whitespace, which this patch removes.
Signed-off-by: Tomi Valkeinen
---
Changes in v2:
* Base on phy-next
.../devicetree/bindings/phy/ti,phy-j721e-wiz.yaml | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/phy/ti,phy-j721e-wiz.yaml
b/Documentation/devicetree/bindings/phy/ti,phy-j721e-wiz.yaml
index 5ffc95c62909..c33e9bc79521 100644
--- a/Documentation/devicetree/bindings/phy/ti,phy-j721e-wiz.yaml
+++ b/Documentation/devicetree/bindings/phy/ti,phy-j721e-wiz.yaml
@@ -45,9 +45,15 @@ properties:
ranges: true
assigned-clocks:
+minItems: 1
maxItems: 2
assigned-clock-parents:
+minItems: 1
+maxItems: 2
+
+ assigned-clock-rates:
+minItems: 1
maxItems: 2
typec-dir-gpios:
@@ -119,9 +125,10 @@ patternProperties:
logic.
properties:
clocks:
+minItems: 2
maxItems: 4
-description: Phandle to four clock nodes representing the inputs to
- refclk_dig
+description: Phandle to two (Torrent) or four (Sierra) clock nodes
representing
+ the inputs to refclk_dig
"#clock-cells":
const: 0
@@ -203,7 +210,7 @@ examples:
};
refclk-dig {
- clocks = <_clks 292 11>, <_clks 292 0>,
+ clocks = <_clks 292 11>, <_clks 292 0>,
<_cmn_refclk>, <_cmn_refclk1>;
#clock-cells = <0>;
assigned-clocks = <_refclk_dig>;
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
[PATCH 2/2] dt-bindings: phy: cdns,torrent-phy: add reset-names
[PATCH 1/2] dt-bindings: phy: ti,phy-j721e-wiz: fix bindings for torrent phy
When WIZ wraps a Cadence Torrent PHY (instead of Cadence Sierra PHY)
there is a difference in the refclk-dig node: Torrent only has two
clocks instead of Sierra's four clocks. Add minItems: 2 to solve this.
Additionally, in our use case we only need to use assigned-clock for a
single clock, but the current binding requires either no assigned-clocks
or two. Fix this by adding minItems: 1 to all the assigned-clock
properties.
There was also an extra trailing whitespace, which this patch removes.
Signed-off-by: Tomi Valkeinen
---
.../devicetree/bindings/phy/ti,phy-j721e-wiz.yaml | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/phy/ti,phy-j721e-wiz.yaml
b/Documentation/devicetree/bindings/phy/ti,phy-j721e-wiz.yaml
index 5ffc95c62909..c33e9bc79521 100644
--- a/Documentation/devicetree/bindings/phy/ti,phy-j721e-wiz.yaml
+++ b/Documentation/devicetree/bindings/phy/ti,phy-j721e-wiz.yaml
@@ -45,9 +45,15 @@ properties:
ranges: true
assigned-clocks:
+minItems: 1
maxItems: 2
assigned-clock-parents:
+minItems: 1
+maxItems: 2
+
+ assigned-clock-rates:
+minItems: 1
maxItems: 2
typec-dir-gpios:
@@ -119,9 +125,10 @@ patternProperties:
logic.
properties:
clocks:
+minItems: 2
maxItems: 4
-description: Phandle to four clock nodes representing the inputs to
- refclk_dig
+description: Phandle to two (Torrent) or four (Sierra) clock nodes
representing
+ the inputs to refclk_dig
"#clock-cells":
const: 0
@@ -203,7 +210,7 @@ examples:
};
refclk-dig {
- clocks = <_clks 292 11>, <_clks 292 0>,
+ clocks = <_clks 292 11>, <_clks 292 0>,
<_cmn_refclk>, <_cmn_refclk1>;
#clock-cells = <0>;
assigned-clocks = <_refclk_dig>;
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v10 1/3] dt-bindings: drm/bridge: Document Cadence MHDP8546 bridge bindings
Re: [PATCH] drm: mxsfb: check framebuffer pitch
Hi,
On 08/09/2020 10:55, Stefan Agner wrote:
> On 2020-09-07 20:18, Daniel Vetter wrote:
>> On Mon, Sep 07, 2020 at 07:17:12PM +0300, Laurent Pinchart wrote:
>>> Hi Stefan,
>>>
>>> Thank you for the patch.
>>>
>>> On Mon, Sep 07, 2020 at 06:03:43PM +0200, Stefan Agner wrote:
The lcdif IP does not support a framebuffer pitch (stride) other than
the CRTC width. Check for equality and reject the state otherwise.
This prevents a distorted picture when using 640x800 and running the
Mesa graphics stack. Mesa tires to use a cache aligned stride, which
>>>
>>> s/tires/tries/
>>>
leads at that particular resolution to width != stride. Currently
Mesa has no fallback behavior, but rejecting this configuration allows
userspace to handle the issue correctly.
>>>
>>> I'm increasingly impressed by how featureful this IP core is :-)
>>>
Signed-off-by: Stefan Agner
---
drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index b721b8b262ce..79aa14027f91 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struct drm_plane
*plane,
{
struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev);
struct drm_crtc_state *crtc_state;
+ unsigned int pitch;
+ int ret;
crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
>crtc);
- return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
- DRM_PLANE_HELPER_NO_SCALING,
- DRM_PLANE_HELPER_NO_SCALING,
- false, true);
+ ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+DRM_PLANE_HELPER_NO_SCALING,
+DRM_PLANE_HELPER_NO_SCALING,
+false, true);
+ if (ret || !plane_state->visible)
>>>
>>> Would it be more explict to check for !plane_state->fb ? Otherwise I'll
>>> have to verify that !fb always implies !visible :-)
>>>
+ return ret;
+
+ pitch = crtc_state->mode.hdisplay *
+ plane_state->fb->format->cpp[0];
>>>
>>> This holds on a single line.
>>>
+ if (plane_state->fb->pitches[0] != pitch) {
+ dev_err(plane->dev->dev,
+ "Invalid pitch: fb and crtc widths must be the same");
>>>
>>> I'd turn this into a dev_dbg(), printing error messages to the kernel
>>> log in response to user-triggered conditions is a bit too verbose and
>>> could flood the log.
>>>
>>> Wouldn't it be best to catch this issue when creating the framebuffer ?
>>
>> Yeah this should be verified at addfb time. We try to validate as early as
>> possible.
>> -Daniel
>>
>
> Sounds sensible. From what I can tell fb_create is the proper callback
> to implement this at addfb time. Will give this a try.
>
> FWIW, I got the idea from drivers/gpu/drm/tilcdc/tilcdc_plane.c. Maybe
> should be moved to addfb there too?
But you don't know the crtc width when creating the framebuffer.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
Hi, On 04/09/2020 05:29, Laurent Pinchart wrote: >> Laurent mentioned that atomic_check should not change state. Note that >> cdns_mhdp_validate_mode_params also changes state, as it calculates tu_size, >> vs and line_thresh. > > .atomic_check() isn't allowed to change any global state, which means > both hardware state and data in cdns_mhdp_device. The drm_bridge_state > (and thus the cdns_mhdp_bridge_state) can be modified as it stores the > state for the atomic commit being checked. > >> There seems to be issues with mode changes, but I think the first step would >> be to clarify the >> related code a bit. cdns_mhdp_validate_mode_params() is misnamed, I think it >> should be renamed to >> calculate_tu or something like that. >> >> cdns_mhdp_bandwidth_ok() should take display_fmt or bpp as a parameter, as >> currently it digs that up >> from the current state. >> >> Probably cdns_mhdp_validate_mode_params() would be better if it doesn't >> write the result to the >> state, but returns the values. That way it could also be used to verify if >> suitable settings can be >> found, without changing the state. > > This use case is actually a very good example of proper usage of the > atomic state :-) .atomic_check() has to perform computations to verify > the atomic commit, and storing the results in the commit's state > prevents duplicating the same calculation at .atomic_commit() time. Yes, you're right. But it's still not good, as cdns_mhdp_validate_mode_params uses link details to do the calculations, but we do link training only later and thus the calculations are invalid. cdns_mhdp_validate_mode_params is also called from the HPD interrupt, and there it changes the current bridge state. link_mutex is being held in every place where cdns_mhdp_validate_mode_params is called, so I guess it's fine. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
Hi Milind, On 03/09/2020 09:22, Milind Parab wrote: > Also, note that CDNS MHDP implements DP_FRAMER_TU_p where bits 5:0 is > tu_valid_symbols. So max programmable value is 63. > Register document gives following explanation > "Number of valid symbols per Transfer Unit (TU). Rounded down to lower > integer value (Allowed values are 1 to (TU_size-1)" > > So, it says in case vs calculates to 64 (where Avail BW and Req BW are same) > we program tu_valid_symbols = 63 Hmm, so "Rounded down to lower integer value" means floor(x) - 1 ? If that's the case, we need to subtract 1 in all cases, not only when req bw == avail bw. > Third, is about the line_threshold calculation > Unlike TU_SIZE and Valid_Symbols, line_threshold is implementation dependent > > CDNS MHDP register specs gives the definition as " Video FIFO latency > threshold" > Bits 5:0, Name "cfg_active_line_tresh", Description "Video Fifo Latency > threshold. Defines the number of FIFO rows before reading starts. This > setting depends on the transmitted video format and link rate." > > This parameter is the Threshold of the FIFO. For optimal performance > (considering equal write and read clock) we normally put the threshold in the > mid of the FIFO. > Hence the reset value is fixed as 32. > Since symbol FIFO is accessed by Pxl clock and Symbol Link Clock the > Threshold is set to a value which is dependent on the ratio of these clocks > > line_threshold = full_fifo - fifo_ratio_due_to_clock_diff + 2 > where, > full_fifo = (vs+1) * (8/bpp) > fifo_ratio_due_to_clock_diff = ((vs+1) * pxlclock/mhdp->link.rate - 1) / > mhdp->link.num_lanes > > Note that line_threshold can take a max value of 63 That doesn't result in anything sensible. 8/bpp is always 0. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v9 14/32] drm: omapdrm: fix common struct sg_table related issues
On 01/09/2020 22:33, Robin Murphy wrote:
> On 2020-08-26 07:32, Marek Szyprowski wrote:
>> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
>> returns the number of the created entries in the DMA address space.
>> However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
>> dma_unmap_sg must be called with the original number of the entries
>> passed to the dma_map_sg().
>>
>> struct sg_table is a common structure used for describing a non-contiguous
>> memory buffer, used commonly in the DRM and graphics subsystems. It
>> consists of a scatterlist with memory pages and DMA addresses (sgl entry),
>> as well as the number of scatterlist entries: CPU pages (orig_nents entry)
>> and DMA mapped pages (nents entry).
>>
>> It turned out that it was a common mistake to misuse nents and orig_nents
>> entries, calling DMA-mapping functions with a wrong number of entries or
>> ignoring the number of mapped entries returned by the dma_map_sg()
>> function.
>>
>> Fix the code to refer to proper nents or orig_nents entries. This driver
>> checks for a buffer contiguity in DMA address space, so it should test
>> sg_table->nents entry.
>>
>> Signed-off-by: Marek Szyprowski
>> ---
>> drivers/gpu/drm/omapdrm/omap_gem.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
>> b/drivers/gpu/drm/omapdrm/omap_gem.c
>> index ff0c4b0c3fd0..a7a9a0afe2b6 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
>> @@ -48,7 +48,7 @@ struct omap_gem_object {
>> * OMAP_BO_MEM_DMA_API flag set)
>> *
>> * - buffers imported from dmabuf (with the OMAP_BO_MEM_DMABUF flag
>> set)
>> - * if they are physically contiguous (when sgt->orig_nents == 1)
>> + * if they are physically contiguous (when sgt->nents == 1)
>
> Hmm, if this really does mean *physically* contiguous - i.e. if buffers might
> be shared between
> DMA-translatable and non-DMA-translatable devices - then these changes might
> not be appropriate. If
> not and it only actually means DMA-contiguous, then it would be good to
> clarify the comments to that
> effect.
>
> Can anyone familiar with omapdrm clarify what exactly the case is here? I
> know that IOMMUs might be
> involved to some degree, and I've skimmed the interconnect chapters of enough
> OMAP TRMs to be scared
> by the reference to the tiler aperture in the context below :)
DSS (like many other IPs in OMAP) does not have any MMU/PAT, and can only use
contiguous buffers
(contiguous in the RAM).
There's a special case with TILER (which is not part of DSS but of the memory
subsystem, but it's
still handled internally by the omapdrm driver), which has a PAT. PAT can
create a contiguous view
of scattered pages, and DSS can then use this contiguous view ("tiler
aperture", which to DSS looks
just like normal contiguous memory).
Note that omapdrm does not use dma_map_sg() & co. mentioned in the patch
description.
If there's no MMU/PAT, is orig_nents always the same as nents? Or can we have
multiple physically
contiguous pages listed separately in the sgt (so orig_nents > 1) but as the
pages form one big
contiguous area, nents == 1?
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
Hi Swapnil, On 31/08/2020 11:23, Swapnil Jakhade wrote: > + line_thresh1 = ((vs + 1) << 5) * 8 / bpp; > + line_thresh2 = (pxlclock << 5) / 1000 / rate * (vs + 1) - (1 << 5); > + line_thresh = line_thresh1 - line_thresh2 / mhdp->link.num_lanes; > + line_thresh = (line_thresh >> 5) + 2; These calculations do not seem to go correctly. There's no comment what's the logic here, but e.g. for 640x480 (pxlclock 31500) with 1.62Gbps link, I get vs=4, and then the second line above comes to: (31500 << 5) / 1000 / 162 * (4+1) - (1<<5) = -0.8893 The result is line_thresh of 100663299. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
On 01/09/2020 10:46, Tomi Valkeinen wrote: > I think the above suggests that the driver is not properly updating all the > registers based on the > new mode and link. I tried adding cdns_mhdp_validate_mode_params() call to > cdns_mhdp_atomic_enable(), so that tu-size etc will be calculated, but that > did not fix the problem. Oh, it actually did fix the problem. It was just that my first hack updated the old state, but after changing the code to call cdns_mhdp_atomic_enable() with new_state I see it helps with the issue. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
Hi Swapnil,
On 31/08/2020 11:23, Swapnil Jakhade wrote:
> +static int cdns_mhdp_validate_mode_params(struct cdns_mhdp_device *mhdp,
> + const struct drm_display_mode *mode,
> + struct drm_bridge_state *bridge_state)
> +{
> + u32 tu_size = 30, line_thresh1, line_thresh2, line_thresh = 0;
> + u32 rate, vs, vs_f, required_bandwidth, available_bandwidth;
> + struct cdns_mhdp_bridge_state *state;
> + int pxlclock;
> + u32 bpp;
> +
> + state = to_cdns_mhdp_bridge_state(bridge_state);
> +
> + pxlclock = mode->crtc_clock;
> +
> + /* Get rate in MSymbols per second per lane */
> + rate = mhdp->link.rate / 1000;
> +
> + bpp = cdns_mhdp_get_bpp(>display_fmt);
None of the above are used when calling cdns_mhdp_bandwidth_ok(). For clarity,
I'd move the above
lines a bit closer to where they are needed, as currently it makes me think the
above values are
used when checking the bandwidth.
> + if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
> + mhdp->link.rate)) {
> + dev_err(mhdp->dev, "%s: Not enough BW for %s (%u lanes at %u
> Mbps)\n",
> + __func__, mode->name, mhdp->link.num_lanes,
> + mhdp->link.rate / 100);
> + return -EINVAL;
> + }
> +
> + /* find optimal tu_size */
> + required_bandwidth = pxlclock * bpp / 8;
> + available_bandwidth = mhdp->link.num_lanes * rate;
> + do {
> + tu_size += 2;
> +
> + vs_f = tu_size * required_bandwidth / available_bandwidth;
> + vs = vs_f / 1000;
> + vs_f = vs_f % 1000;
> + /* Downspreading is unused currently */
> + } while ((vs == 1 || ((vs_f > 850 || vs_f < 100) && vs_f != 0) ||
> + tu_size - vs < 2) && tu_size < 64);
> +
> + if (vs > 64) {
> + dev_err(mhdp->dev,
> + "%s: No space for framing %s (%u lanes at %u Mbps)\n",
> + __func__, mode->name, mhdp->link.num_lanes,
> + mhdp->link.rate / 100);
> + return -EINVAL;
> + }
> +
> + if (vs == tu_size)
> + vs = tu_size - 1;
> +
> + line_thresh1 = ((vs + 1) << 5) * 8 / bpp;
> + line_thresh2 = (pxlclock << 5) / 1000 / rate * (vs + 1) - (1 << 5);
> + line_thresh = line_thresh1 - line_thresh2 / mhdp->link.num_lanes;
> + line_thresh = (line_thresh >> 5) + 2;
> +
> + state->vs = vs;
> + state->tu_size = tu_size;
> + state->line_thresh = line_thresh;
> +
> + return 0;
> +}
> +
> +static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
> + const struct drm_display_mode *mode = _state->adjusted_mode;
> + int ret;
> +
> + mutex_lock(>link_mutex);
> +
> + if (!mhdp->plugged) {
> + mhdp->link.rate = mhdp->host.link_rate;
> + mhdp->link.num_lanes = mhdp->host.lanes_cnt;
> + }
> +
> + ret = cdns_mhdp_validate_mode_params(mhdp, mode, bridge_state);
> +
> + mutex_unlock(>link_mutex);
> +
> + return ret;
> +}
Laurent mentioned that atomic_check should not change state. Note that
cdns_mhdp_validate_mode_params also changes state, as it calculates tu_size, vs
and line_thresh.
There seems to be issues with mode changes, but I think the first step would be
to clarify the
related code a bit. cdns_mhdp_validate_mode_params() is misnamed, I think it
should be renamed to
calculate_tu or something like that.
cdns_mhdp_bandwidth_ok() should take display_fmt or bpp as a parameter, as
currently it digs that up
from the current state.
Probably cdns_mhdp_validate_mode_params() would be better if it doesn't write
the result to the
state, but returns the values. That way it could also be used to verify if
suitable settings can be
found, without changing the state.
The are two issues I see with some testing, which are probably related.
The first one is that if I run kmstest with a new mode, I see tu-size & co
being calculated. But the
calculation happens before link training, which doesn't make sense. So I think
what's done here is
that atomic_check causes tu-size calculations, then atomic_enable does link
training and enables the
video.
The second happens when my monitor fails with the first CR after power-on, and
the driver drops
number-of-lanes to 2. It goes like this:
The driver is loaded. Based on EDID, fbdev is created with 1920x1200. Link
training is done, which
has the CR issue, and because of that the actual mode that we get is 1280x960.
I get a proper
picture here, so far so good.
Then if I run kmstest, it only allows 1280x960 as the
Re: [PATCH v8 2/3] drm: bridge: Add support for Cadence MHDP DPI/DP bridge
Hi,
On 11/08/2020 05:36, Laurent Pinchart wrote:
>> +{
>> +u32 max_bw, req_bw, bpp;
>> +
>> +bpp = cdns_mhdp_get_bpp(>display_fmt);
>> +req_bw = mode->clock * bpp / 8;
>> +max_bw = lanes * rate;
> mode->clock is in kHz, while rate is expressed in 10kHz unit if I'm not
> mistaken.
Rate is in 10 kbps units. And 10 bits in DP gives you one byte (8b/10b
encoding). So max_bw is in
kBps, the same as req_bw.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [REGRESSION] omapdrm/N900 display broken
Hi Laurent, On 23/08/2020 19:26, Aaro Koskinen wrote: > Hi, > > On Tue, Aug 04, 2020 at 03:39:37PM +0300, Tomi Valkeinen wrote: >> On 04/08/2020 15:13, Tomi Valkeinen wrote: > >>> Can you try to pinpoint a bit where the hang happens? Maybe add >>> DRM/omapdrm debug prints, or perhaps sysrq works and it shows a lock >>> that's in deadlock. >> >> Also, one data point would be to disable venc, e.g. set venc status to >> "disabled" in dts. > > Disabling venc makes no difference. > > The hang happens in drm_fb_helper_initial_config(). I followed the > "HANG DEBUGGING" tips in the function comment text and enabled > fb.lockless_register_fb=1 to get more (serial) console output. > > Now I get this: > > [6.514739] omapdss_dss 4805.dss: supply vdda_video not found, using > dummy regulator > [6.566375] DSS: OMAP DSS rev 2.0 > [6.571807] omapdss_dss 4805.dss: bound 48050400.dispc (ops > dispc_component_ops) > [6.580749] omapdrm omapdrm.0: DMM not available, disable DMM support > [6.587982] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > [6.626617] [ cut here ] > [6.631774] WARNING: CPU: 0 PID: 18 at drivers/gpu/drm/drm_bridge.c:708 > drm_atomic_helper_commit_modeset_enables+0x134/0x268 > [6.643768] Modules linked in: > [6.647033] CPU: 0 PID: 18 Comm: kworker/0:1 Tainted: G U > 5.8.0-omap3-los_16068+-4-g2e7d4a7efefd-dirty #2 > [6.658966] Hardware name: Nokia RX-51 board > [6.663635] Workqueue: events deferred_probe_work_func > [6.669097] [] (unwind_backtrace) from [] > (show_stack+0x10/0x14) > [6.677429] [] (show_stack) from [] (__warn+0xbc/0xd4) > [6.684844] [] (__warn) from [] > (warn_slowpath_fmt+0x60/0xb8) > [6.692901] [] (warn_slowpath_fmt) from [] > (drm_atomic_helper_commit_modeset_enables+0x134/0x268) > [6.704254] [] (drm_atomic_helper_commit_modeset_enables) from > [] (omap_atomic_commit_tail+0xb4/0xc0) > [6.715972] [] (omap_atomic_commit_tail) from [] > (commit_tail+0x9c/0x1a8) > [6.725128] [] (commit_tail) from [] > (drm_atomic_helper_commit+0x134/0x158) > [6.734466] [] (drm_atomic_helper_commit) from [] > (drm_client_modeset_commit_atomic+0x16c/0x208) > [6.745727] [] (drm_client_modeset_commit_atomic) from > [] (drm_client_modeset_commit_locked+0x58/0x184) > [6.757629] [] (drm_client_modeset_commit_locked) from > [] (drm_client_modeset_commit+0x24/0x40) > [6.768798] [] (drm_client_modeset_commit) from [] > (__drm_fb_helper_restore_fbdev_mode_unlocked+0xa0/0xc8) > [6.780975] [] (__drm_fb_helper_restore_fbdev_mode_unlocked) > from [] (drm_fb_helper_set_par+0x38/0x64) > [6.792785] [] (drm_fb_helper_set_par) from [] > (fbcon_init+0x3d4/0x568) > [6.801757] [] (fbcon_init) from [] > (visual_init+0xb8/0xfc) > [6.809631] [] (visual_init) from [] > (do_bind_con_driver+0x1e0/0x3bc) > [6.818267] [] (do_bind_con_driver) from [] > (do_take_over_console+0x138/0x1d8) > [6.827880] [] (do_take_over_console) from [] > (do_fbcon_takeover+0x74/0xd4) > [6.837219] [] (do_fbcon_takeover) from [] > (register_framebuffer+0x204/0x2d8) > [6.846740] [] (register_framebuffer) from [] > (__drm_fb_helper_initial_config_and_unlock+0x3a4/0x554) > [6.858459] [] (__drm_fb_helper_initial_config_and_unlock) from > [] (omap_fbdev_init+0x84/0xbc) > [6.869537] [] (omap_fbdev_init) from [] > (pdev_probe+0x580/0x7d8) > [6.877807] [] (pdev_probe) from [] > (platform_drv_probe+0x48/0x98) Laurent, does this ring any bells? The WARN comes in drm_atomic_bridge_chain_enable() when drm_atomic_get_old_bridge_state() returns null for (presumably) sdi bridge. I'm not sure why the bridge state would not be there. Aaro, you can probably debug easier if you disable CONFIG_FRAMEBUFFER_CONSOLE, or even CONFIG_DRM_FBDEV_EMULATION. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm/omap: Fix runtime PM imbalance in dsi_runtime_get
Hi,
On 21/08/2020 10:45, Dinghao Liu wrote:
> pm_runtime_get_sync() increments the runtime PM usage counter
> even when it returns an error code. However, users of
> dsi_runtime_get(), a direct wrapper of pm_runtime_get_sync(),
> assume that PM usage counter will not change on error. Thus a
> pairing decrement is needed on the error handling path to keep
> the counter balanced.
>
> Fixes: 4fbafaf371be7 ("OMAP: DSS2: Use PM runtime & HWMOD support")
> Signed-off-by: Dinghao Liu
> ---
> drivers/gpu/drm/omapdrm/dss/dsi.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c
> b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index eeccf40bae41..973bfa14a104 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -1112,8 +1112,11 @@ static int dsi_runtime_get(struct dsi_data *dsi)
> DSSDBG("dsi_runtime_get\n");
>
> r = pm_runtime_get_sync(dsi->dev);
> - WARN_ON(r < 0);
> - return r < 0 ? r : 0;
> + if (WARN_ON(r < 0)) {
> + pm_runtime_put_noidle(dsi->dev);
> + return r;
> + }
> + return 0;
> }
Thanks! Good catch. I think this is broken in all the other modules in omapdrm
too (e.g. dispc.c,
venc.c, etc).
Would you like to update the patch to cover the whole omapdrm?
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v8 2/3] drm: bridge: Add support for Cadence MHDP DPI/DP bridge
On 11/08/2020 05:36, Laurent Pinchart wrote:
>> +static int cdns_mhdp_mailbox_write(struct cdns_mhdp_device *mhdp, u8 val)
>> +{
>> +int ret, full;
>> +
>> +WARN_ON(!mutex_is_locked(>mbox_mutex));
>> +
>> +ret = readx_poll_timeout(readl, mhdp->regs + CDNS_MAILBOX_FULL,
>> + full, !full, MAILBOX_RETRY_US,
>> + MAILBOX_TIMEOUT_US);
>> +if (ret < 0)
>> +return ret;
>> +
>> +writel(val, mhdp->regs + CDNS_MAILBOX_TX_DATA);
>> +
>> +return 0;
>> +}
>
> As commented previously, I think there's room for optimization here. Two
> options that I think should be investigated are using the mailbox
> interrupts, and only polling for the first byte of the message
> (depending on whether the firmware implementation can guarantee that
> when the first byte is available, the rest of the message will be
> immediately available too). This can be done on top of this patch
> though.
I made some tests on this.
I cannot see mailbox_write ever looping, mailbox is never full. So in this case
the
readx_poll_timeout() call is there just for safety to catch the cases where
something has gone
totally wrong or perhaps once in a while the mbox can be full for a tiny
moment. But we always do
need to check CDNS_MAILBOX_FULL before each write to CDNS_MAILBOX_TX_DATA, so
we can as well use
readx_poll_timeout for that to catch the odd cases (afaics, there's no real
overhead if the exit
condition is true immediately).
mailbox_read polls sometimes. Most often it does not poll, as the data is ready
in the mbox, and in
these cases the situation is the same as for mailbox_write.
The cases where it does poll are related to things where the fw has to wait for
something. The
longest poll waits seemed to be EDID read (16 ms wait) and adjusting LT (1.7 ms
wait). And afaics,
when the first byte of the received message is there, the rest of the bytes
will be available
without wait.
For mailbox_write and for most mailbox_reads I think using interrupts makes no
sense, as the
overhead would be big.
For those few long read operations, interrupts would make sense. I guess a
simple way to handle this
would be to add a new function, wait_for_mbox_data() or such, which would use
the interrupts to wait
for mbox not empty. This function could be used in selected functions (edid,
LT) after
cdns_mhdp_mailbox_send().
Although I think it's not that bad currently, MAILBOX_RETRY_US is 1ms, so it's
quite lazy polling,
so perhaps this can be considered TODO optimization.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v8 2/3] drm: bridge: Add support for Cadence MHDP DPI/DP bridge
On 11/08/2020 05:36, Laurent Pinchart wrote:
>> +static int cdns_mhdp_connector_init(struct cdns_mhdp_device *mhdp)
>> +{
>> +u32 bus_format = MEDIA_BUS_FMT_RGB121212_1X36;
>> +struct drm_connector *conn = >connector;
>> +struct drm_bridge *bridge = >bridge;
>> +int ret;
>> +
>> +if (!bridge->encoder) {
>> +DRM_ERROR("Parent encoder object not found");
>> +return -ENODEV;
>> +}
>> +
>> +conn->polled = DRM_CONNECTOR_POLL_HPD;
>> +
>> +ret = drm_connector_init(bridge->dev, conn, _mhdp_conn_funcs,
>> + DRM_MODE_CONNECTOR_DisplayPort);
>> +if (ret) {
>> +DRM_ERROR("Failed to initialize connector with drm\n");
>> +return ret;
>> +}
>> +
>> +drm_connector_helper_add(conn, _mhdp_conn_helper_funcs);
>> +
>> +ret = drm_display_info_set_bus_formats(>display_info,
>> + _format, 1);
>> +if (ret)
>> +return ret;
>> +
>> +conn->display_info.bus_flags = DRM_BUS_FLAG_DE_HIGH;
>
> Aren't these supposed to be retrieved from the display ? Why do we need
> to override them here ?
DE_HIGH is meant for the display controller. I think this should be in
bridge->timings->input_bus_flags
>> +static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
>> + struct drm_bridge_state *bridge_state,
>> + struct drm_crtc_state *crtc_state,
>> + struct drm_connector_state *conn_state)
>> +{
>> +struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
>> +const struct drm_display_mode *mode = _state->adjusted_mode;
>> +int ret;
>> +
>> +if (!mhdp->plugged)
>> +return 0;
>> +
>> +mutex_lock(>link_mutex);
>> +
>> +if (!mhdp->link_up) {
>> +ret = cdns_mhdp_link_up(mhdp);
>> +if (ret < 0)
>> +goto err_check;
>> +}
>
> atomic_check isn't supposed to access the hardware. Is there a reason
> this is needed ?
We have been going back and forth with this. The basic problem is that to
understand which
videomodes can be used, you need to do link training to see the bandwidth
available.
I'm not sure if we strictly need to do LT in atomic check, though... It would
then pass modes that
can't be used, but perhaps that's not a big issue.
I think the main point with doing LT in certain places is to filter the list of
video modes passed
to userspace, as we can't pass the modes from EDID directly without filtering
them based on the link
bandwidth.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v8 1/3] dt-bindings: drm/bridge: Document Cadence MHDP bridge bindings
On 11/08/2020 03:36, Laurent Pinchart wrote: > I've got a chance to study the J721E datasheet, and it shows the DP > bridge has 4 inputs, to support MST. Shouldn't this already be reflected > in the DT bindings ? I think it should be as simple as having 4 input > ports (port@0 to port@3) and one output port (port@4). I think this is a good point, mhdp has 4 input streams. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v8 0/3] drm: Add support for Cadence MHDP DPI/DP bridge and J721E wrapper.
Hi Guido,
On 12/08/2020 11:39, Guido Günther wrote:
> Hi,
> On Thu, Aug 06, 2020 at 01:34:29PM +0200, Swapnil Jakhade wrote:
>> This patch series adds new DRM bridge driver for Cadence MHDP DPI/DP
>> bridge. The Cadence Display Port IP is also referred as MHDP (Mobile High
>> Definition Link, High-Definition Multimedia Interface, Display Port).
>> Cadence Display Port complies with VESA DisplayPort (DP) and embedded
>> Display Port (eDP) standards.
>
> Is there any relation to the cadence mhdp ip core used inthe imx8mq:
>
> https://lore.kernel.org/dri-devel/cover.1590982881.git.sandor...@nxp.com/
>
> It looks very similar in several places so should that use the same driver?
> Cheers,
> -- Guido
Interesting.
So the original Cadence DP patches for TI SoCs did create a common driver with
Rockchip's older mhdp
driver. And looks like the IMX series points to an early version of that patch
("drm/rockchip:
prepare common code for cdns and rk dpi/dp driver").
We gave up on that as the IPs did have differences and the firmwares used were
apparently quite
different. The end result was very difficult to maintain, especially as (afaik)
none of the people
involved had relevant Rockchip HW.
The idea was to get a stable DP driver for TI SoCs ready and upstream, and then
carefully try to
create common parts with Rockchip's driver in small pieces.
If the Rockchip and IMX mhdp have the same IP and same firmware, then they
obviously should share
code as done in the series you point to.
Perhaps Cadence can clarify the differences between IMX, TI and Rockchip IPs
and FWs?
I'm worried that if there are IP differences, even if not great ones, and if
the FWs are different
and developed separately, it'll be a constant "fix X for SoC A, and
accidentally break Y for SoC B
and C", especially if too much code is shared.
In the long run I'm all for a single driver (or large shared parts), but I'm
not sure if we should
start with that approach.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v8 2/3] drm: bridge: Add support for Cadence MHDP DPI/DP bridge
Hi Swapnil, On 06/08/2020 14:34, Swapnil Jakhade wrote: > Add a new DRM bridge driver for Cadence MHDP DPTX IP used in TI J721e SoC. > MHDP DPTX IP is the component that complies with VESA DisplayPort (DP) and > embedded Display Port (eDP) standards. It integrates uCPU running the > embedded Firmware (FW) interfaced over APB interface. > > Basically, it takes a DPI stream as input and outputs it encoded in DP > format. Currently, it supports only SST mode. > > Co-developed-by: Tomi Valkeinen > Signed-off-by: Tomi Valkeinen > Co-developed-by: Jyri Sarha > Signed-off-by: Jyri Sarha > Signed-off-by: Quentin Schulz > Signed-off-by: Yuti Amonkar > Signed-off-by: Swapnil Jakhade > --- > + mhdp_state = to_cdns_mhdp_bridge_state(new_state); > + > + mhdp_state->current_mode = drm_mode_duplicate(bridge->dev, mode); > + drm_mode_set_name(mhdp_state->current_mode); > + current_mode is never freed, so this leaks memory. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm/omap: fix spelling mistake "propert" -> "property"
Re: [REGRESSION] omapdrm/N900 display broken
On 04/08/2020 15:13, Tomi Valkeinen wrote:
> On 28/07/2020 21:14, Aaro Koskinen wrote:
>> Hi,
>>
>> Looks like N900 display support has broken after v5.6.
>>
>> When using v5.7, or the current mainline (v5.8-rc7), the boot hangs at:
>>
>> [6.269500] omapdss_dss 4805.dss: 4805.dss supply vdda_video not
>> found, using dummy regulator
>> [6.321685] DSS: OMAP DSS rev 2.0
>> [6.328002] omapdss_dss 4805.dss: bound 48050400.dispc (ops
>> dispc_component_ops)
>> [6.336364] omapdss_dss 4805.dss: bound 48050c00.encoder (ops
>> venc_component_ops)
>> [6.345153] omapdrm omapdrm.0: DMM not available, disable DMM support
>> [6.352447] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>>
>> with a blank display.
>>
>> I tried to bisect this, and the first commit that breaks the display is:
>>
>> 2f004792adadcf017fde50339b432a26039fff0c is the first bad commit
>> commit 2f004792adadcf017fde50339b432a26039fff0c
>> Author: Laurent Pinchart
>> Date: Wed Feb 26 13:24:57 2020 +0200
>>
>> drm/omap: venc: Register a drm_bridge
>>
>> This commit does not yet hang the kernel, but the display is blank and
>> the probe fails:
>>
>> [6.290100] omapdss_dss 4805.dss: 4805.dss supply vdda_video not
>> foun
>> d, using dummy regulator
>> [6.342346] DSS: OMAP DSS rev 2.0
>> [6.348663] omapdss_dss 4805.dss: bound 48050400.dispc (ops
>> dispc_component_ops)
>> [6.357025] omapdss_dss 4805.dss: bound 48050c00.encoder (ops
>> venc_component_ops)
>> [6.365814] omapdrm omapdrm.0: DMM not available, disable DMM support
>> [6.372863] omapdrm omapdrm.0: omap_modeset_init failed: ret=-22
>> [6.414611] omapdrm: probe of omapdrm.0 failed with error -22
>
> Afaics, this is caused by the patch doing
>
> if (!venc->output.next_bridge)
>
> instead of
>
> if (venc->output.next_bridge)
>
> Fixing that removes the error on beagle xm, but for some reason I don't see
> TV-out as a DRM output
> (but HDMI output works).
>
>> The hang seems to start with this commit:
>>
>> commit 8bef8a6d5da81b909a190822b96805a47348146f (HEAD -> master)
>> Author: Laurent Pinchart
>> Date: Wed Feb 26 13:25:10 2020 +0200
>>
>> drm/omap: sdi: Register a drm_bridge
>>
>> Confusingly, some of the commits between these two commits do provide a
>> working display, e.g. 13d2d52f59c0c79398d9c9e2ea3d661a0e5b6bbc ("drm/omap:
>> sdi: Sort includes alphabetically") works...
>
> The venc issue is fixed in "Switch the HDMI and VENC outputs to drm_bridge",
> which is a few commits
> after the "venc: Register a drm_bridge".
>
> So I'm guessing SDI works until "sdi: Register a drm_bridge", but as omapdrm
> doesn't probe, you get
> no displays.
>
> Can you try to pinpoint a bit where the hang happens? Maybe add DRM/omapdrm
> debug prints, or perhaps
> sysrq works and it shows a lock that's in deadlock.
Also, one data point would be to disable venc, e.g. set venc status to
"disabled" in dts.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [REGRESSION] omapdrm/N900 display broken
On 28/07/2020 21:14, Aaro Koskinen wrote:
> Hi,
>
> Looks like N900 display support has broken after v5.6.
>
> When using v5.7, or the current mainline (v5.8-rc7), the boot hangs at:
>
> [6.269500] omapdss_dss 4805.dss: 4805.dss supply vdda_video not
> found, using dummy regulator
> [6.321685] DSS: OMAP DSS rev 2.0
> [6.328002] omapdss_dss 4805.dss: bound 48050400.dispc (ops
> dispc_component_ops)
> [6.336364] omapdss_dss 4805.dss: bound 48050c00.encoder (ops
> venc_component_ops)
> [6.345153] omapdrm omapdrm.0: DMM not available, disable DMM support
> [6.352447] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>
> with a blank display.
>
> I tried to bisect this, and the first commit that breaks the display is:
>
> 2f004792adadcf017fde50339b432a26039fff0c is the first bad commit
> commit 2f004792adadcf017fde50339b432a26039fff0c
> Author: Laurent Pinchart
> Date: Wed Feb 26 13:24:57 2020 +0200
>
> drm/omap: venc: Register a drm_bridge
>
> This commit does not yet hang the kernel, but the display is blank and
> the probe fails:
>
> [6.290100] omapdss_dss 4805.dss: 4805.dss supply vdda_video not
> foun
> d, using dummy regulator
> [6.342346] DSS: OMAP DSS rev 2.0
> [6.348663] omapdss_dss 4805.dss: bound 48050400.dispc (ops
> dispc_component_ops)
> [6.357025] omapdss_dss 4805.dss: bound 48050c00.encoder (ops
> venc_component_ops)
> [6.365814] omapdrm omapdrm.0: DMM not available, disable DMM support
> [6.372863] omapdrm omapdrm.0: omap_modeset_init failed: ret=-22
> [6.414611] omapdrm: probe of omapdrm.0 failed with error -22
Afaics, this is caused by the patch doing
if (!venc->output.next_bridge)
instead of
if (venc->output.next_bridge)
Fixing that removes the error on beagle xm, but for some reason I don't see
TV-out as a DRM output
(but HDMI output works).
> The hang seems to start with this commit:
>
> commit 8bef8a6d5da81b909a190822b96805a47348146f (HEAD -> master)
> Author: Laurent Pinchart
> Date: Wed Feb 26 13:25:10 2020 +0200
>
> drm/omap: sdi: Register a drm_bridge
>
> Confusingly, some of the commits between these two commits do provide a
> working display, e.g. 13d2d52f59c0c79398d9c9e2ea3d661a0e5b6bbc ("drm/omap:
> sdi: Sort includes alphabetically") works...
The venc issue is fixed in "Switch the HDMI and VENC outputs to drm_bridge",
which is a few commits
after the "venc: Register a drm_bridge".
So I'm guessing SDI works until "sdi: Register a drm_bridge", but as omapdrm
doesn't probe, you get
no displays.
Can you try to pinpoint a bit where the hang happens? Maybe add DRM/omapdrm
debug prints, or perhaps
sysrq works and it shows a lock that's in deadlock.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] omapfb: dss: Fix max fclk divider for omap36xx
Hi,
On 03/07/2020 22:36, Sam Ravnborg wrote:
Hi Tomi.
On Fri, Jul 03, 2020 at 10:17:29AM +0300, Tomi Valkeinen wrote:
On 30/06/2020 21:26, Adam Ford wrote:
The drm/omap driver was fixed to correct an issue where using a
divider of 32 breaks the DSS despite the TRM stating 32 is a valid
number. Through experimentation, it appears that 31 works, and
it is consistent with the value used by the drm/omap driver.
This patch fixes the divider for fbdev driver instead of the drm.
Fixes: f76ee892a99e ("omapfb: copy omapdss & displays for omapfb")
Cc: #4.9+
Signed-off-by: Adam Ford
---
Linux 4.4 will need a similar patch, but it doesn't apply cleanly.
The DRM version of this same fix is:
e2c4ed148cf3 ("drm/omap: fix max fclk divider for omap36xx")
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
index 7252d22dd117..bfc5c4c5a26a 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
@@ -833,7 +833,7 @@ static const struct dss_features omap34xx_dss_feats = {
};
static const struct dss_features omap3630_dss_feats = {
- .fck_div_max= 32,
+ .fck_div_max= 31,
.dss_fck_multiplier = 1,
.parent_clk_name= "dpll4_ck",
.dpi_select_source = _dpi_select_source_omap2_omap3,
Reviewed-by: Tomi Valkeinen
Will you apply to drm-misc?
This is for fbdev, so I presume Bartlomiej will pick this one.
Note following output from "dim fixes":
$ dim fixes f76ee892a99e
Fixes: f76ee892a99e ("omapfb: copy omapdss & displays for omapfb")
Cc: Tomi Valkeinen
Cc: Dave Airlie
Cc: Rob Clark
Cc: Laurent Pinchart
Cc: Sam Ravnborg
Cc: Bartlomiej Zolnierkiewicz
Cc: Jason Yan
Cc: "Andrew F. Davis"
Cc: YueHaibing
Cc: # v4.5+
Here it says the fix is valid from v4.5 onwards.
Hmm... Adam, you marked the fix to apply to v4.9+, and then you said
v4.4 needs a new patch (that's before the big copy/rename). Did you
check the versions between 4.4 and 4.9? I would guess this one applies
to v4.5+.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] omapfb: dss: Fix max fclk divider for omap36xx
On 30/06/2020 21:26, Adam Ford wrote:
The drm/omap driver was fixed to correct an issue where using a
divider of 32 breaks the DSS despite the TRM stating 32 is a valid
number. Through experimentation, it appears that 31 works, and
it is consistent with the value used by the drm/omap driver.
This patch fixes the divider for fbdev driver instead of the drm.
Fixes: f76ee892a99e ("omapfb: copy omapdss & displays for omapfb")
Cc: #4.9+
Signed-off-by: Adam Ford
---
Linux 4.4 will need a similar patch, but it doesn't apply cleanly.
The DRM version of this same fix is:
e2c4ed148cf3 ("drm/omap: fix max fclk divider for omap36xx")
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
index 7252d22dd117..bfc5c4c5a26a 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
@@ -833,7 +833,7 @@ static const struct dss_features omap34xx_dss_feats = {
};
static const struct dss_features omap3630_dss_feats = {
- .fck_div_max= 32,
+ .fck_div_max= 31,
.dss_fck_multiplier = 1,
.parent_clk_name= "dpll4_ck",
.dpi_select_source = _dpi_select_source_omap2_omap3,
Reviewed-by: Tomi Valkeinen
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
On 16/06/2020 19:56, Grygorii Strashko wrote: On 16/06/2020 18:30, Tony Lindgren wrote: * Tomi Valkeinen [200616 13:02]: On 11/06/2020 17:00, Grygorii Strashko wrote: I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do pm_runtime_force_xxx() calls at noirq suspend stage by adding: SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) Am I missing smth? Isn't this almost exactly the same my patch does? I just used suspend_late and resume_early. Is noirq phase better than late & early? Well up to you as far as I'm concerned. The noirq phase comes with serious limitations, for let's say i2c bus usage if needed. Probably also harder to debug for suspend and resume. Unfortunately, you can't use PM runtime force API at .suspend() stage as pm_runtime_get will still work and there is no sync between suspend and pm_runtime. The PM runtime force API can be used only during late/noirq as at this time pm_runtime is disabled. Yes, but which one... Do you know what the diff is with late/noirq from driver's perspective? I guess noirq is atomic context, late is nto? Dispc's suspend uses synchronize_irq(), so that rules out noirq. Although the call is not needed if using noirq version, so that could also be managed with small change. But I wonder if there's any benefit in using noirq versus late. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
On 11/06/2020 17:00, Grygorii Strashko wrote:
On 09/06/2020 18:26, Tomi Valkeinen wrote:
On 09/06/2020 18:19, Tony Lindgren wrote:
But there's an extra runtime PM reference (dev.power.usage_count) that seems
to come out of nowhere. So when omap_drm_suspend is finished, there's still
usage_count of 1, and dispc never suspends fully.
Hmm no idea about that. My guess is that there might be an issue that was
masked earlier with omap_device calling the child runtime_suspend.
Yes. It's how PM works. It calls pm_runtime_get_noresume() before starting the suspend of a
device. So I guess omapdrm's suspend has been broken all the time, but it was "fixed" by omap_device.
I think I might have an idea what is going wrong.
Before:
+--+
|omap_device_pm_domain |
+---+--+--+
| device |
+-+
| omap_device |
+-+
omap_device is embedded in DD device and PM handled by omap_device_pm_domain.
static int _od_suspend_noirq(struct device *dev)
{
...
ret = pm_generic_suspend_noirq(dev);
[1] ^^ device suspend_noirq call
if (!ret && !pm_runtime_status_suspended(dev)) {
if (pm_generic_runtime_suspend(dev) == 0) {
[2] ^^ device pm_runtime_suspend force call
omap_device_idle(pdev);
[3] ^^ omap_device disable
od->flags |= OMAP_DEVICE_SUSPENDED;
}
}
return ret;
}
Now:
++
|ti sysc dev |
+-+--+
|
|
| +-+
| | device |
+-->+ |
+-+
With new approach the omap_device is not embedded in DD Device anymore,
instead ti-sysc (hwmod replacement) became parent of DD Device.
As result suspend sequence became the following
(Note. All PM runtime PUT calls became NOP during suspend by design):
device
|-> suspend() - in case of dss omap_drm_suspend() and Co if defined
|-> suspend_noirq() - in case of dss *not defined", equal to step [1] above
..
ti sysc dev (ti-sysc is parent, so called after device)
|-> sysc_noirq_suspend
|-> pm_runtime_force_suspend()
|-> sysc_runtime_suspend() - equal to step [3] above
And step [2] is missing as of now!
I think, suspend might be fixed if all devices, which are now child of ti-sysc,
will do
pm_runtime_force_xxx() calls at noirq suspend stage by adding:
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
pm_runtime_force_resume)
Am I missing smth?
Isn't this almost exactly the same my patch does? I just used suspend_late and resume_early. Is
noirq phase better than late & early?
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm/panel-simple: fix connector type for LogicPD Type28 Display
On 15/06/2020 17:53, Adam Ford wrote: On Mon, Jun 15, 2020 at 9:46 AM Fabio Estevam wrote: On Mon, Jun 15, 2020 at 10:19 AM Adam Ford wrote: The LogicPD Type28 display used by several Logic PD products has not worked since v5.5. Maybe you could tell which commit exactly and then put a Fixes tag? I honestly don't know. I reached out to the omap mailing list, because I noted this issue. Tomi V from TI responded with a link that he posted which fixes this for another display. https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg312208.html I tested that patch and it worked for a different LCD, so I did the same thing to the Logic PD Type 28 display as well. My patch and commit message were modeled after his, and his commit CC's stable with a note about being required for v5.5+ I added him to the CC list, so maybe he knows which hash needs to be referenced from a fixes tag. I was hoping to not have to go back and bisect if it's not required, but I will if necessary. No, I didn't check when exactly it broke. connector_type was added in v5.5, and my patch applies to v5.5, so I set that as stable version. But the WARN comes from panel bridge. Possibly 89958b7cd9555a5d82556cc9a1f4c62fffda6f96 is the one that adds requirement to have connector_type. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
On 09/06/2020 20:10, Tony Lindgren wrote: On beagle-x15 I see these errors after modprobe: DSS: OMAP DSS rev 6.1 omapdss_dss 5800.dss: bound 58001000.dispc (ops dispc_component_ops [omapdss]) omapdss_dss 5800.dss: bound 5804.encoder (ops hdmi5_component_ops [omapdss]) [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes [drm] Initialized omapdrm 1.0.0 20110917 for omapdrm.0 on minor 0 omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes aic_dvdd_fixed: disabling ldousb: disabling Maybe I'm missing some related module on x15? Still did not figure what I might be missing on x15 :) The log above shows that nothing is missing, omapdrm has probed fine. But it cannot see anything connected to the hdmi port. Are you booting with correct dtb for your x15 revision? And you have a monitor connected? =) Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
On 09/06/2020 18:19, Tony Lindgren wrote: But there's an extra runtime PM reference (dev.power.usage_count) that seems to come out of nowhere. So when omap_drm_suspend is finished, there's still usage_count of 1, and dispc never suspends fully. Hmm no idea about that. My guess is that there might be an issue that was masked earlier with omap_device calling the child runtime_suspend. Yes. It's how PM works. It calls pm_runtime_get_noresume() before starting the suspend of a device. So I guess omapdrm's suspend has been broken all the time, but it was "fixed" by omap_device. Currently I'm only able to rmmod -f omapdrm, not sure if these issues might be related. Hmm, I always use modules, and can unload omapdrm and drm fine. But there's a sequence that must be followed. However, the sequence starts with unloading omapdrm... What behavior you see with rmmod? Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
On 03/06/2020 17:06, Tony Lindgren wrote: * Tomi Valkeinen [200603 12:34]: Hi Tony, On 31/05/2020 22:39, Tony Lindgren wrote: When booting without legacy platform data, we no longer have omap_device calling PM runtime suspend for us on suspend. This causes the driver context not be saved as we have no suspend and resume functions defined. Let's fix the issue by switching over to use UNIVERSAL_DEV_PM_OPS as it will call the existing PM runtime suspend functions on suspend. I don't think we can use UNIVERSAL_DEV_PM_OPS, as we can't disable DSS modules in any order, but things have to be shut down in orderly manner. OK. I presume you talk about the order of dss child devices here. Yes, but not only that. E.g. the dispc driver hasn't been designed to be suspended while active. The only way to properly suspend the dispc HW is to first disable the outputs, wait until they've finished with their current frame, and only then can things be shut down. The suspend machinery doesn't handle all that (and it couldn't anyway, due to the dependencies to other DSS devices in the pipeline). omapdrm hasn't relied on omap_device calling runtime suspend for us (I didn't know it does that). We have system suspend hooks in omap_drv.c: We had omap_device sort of brute forcing things to idle on suspend which only really works for interconnect target modules with one device in them. SIMPLE_DEV_PM_OPS(omapdrm_pm_ops, omap_drm_suspend, omap_drm_resume) omap_drm_suspend() is supposed to turn off the displays, which then cause dispc_runtime_put (and other runtime_puts) to be called, which result in dispc_runtime_suspend (and other runtime PM suspends). OK thanks for explaining, I missed that part. So... For some reason that's no longer happening? I need to try to find a board with which suspend/resume works (without DSS)... Yes it seems something has changed. When diffing the dmesg debug output on suspend and resume, context save and restore functions are no longer called as the PM runtime suspend and resume functions are no longer called on suspend and resume. I now tested with AM4 SK, and I still can't get system suspend/resume work (without DSS). I have no clue about how to fix that. But if I use pm_test to prevent total suspend, I can reproduce this (or at least looks the same). And now that I look at this, I have a recollection that I've seen this before. What happens is that the system suspend hook (omap_drm_suspend) gets called fine, and it turns off the displays, which leads to dispc_runtime_puts etc. All goes fine. But there's an extra runtime PM reference (dev.power.usage_count) that seems to come out of nowhere. So when omap_drm_suspend is finished, there's still usage_count of 1, and dispc never suspends fully. I think the PM framework does this when starting system suspend process. Maybe this was also happening earlier, but omap_device used to do the final suspend (so omapdrm depended on that functionality, after all...). Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
Hi Tony, On 31/05/2020 22:39, Tony Lindgren wrote: When booting without legacy platform data, we no longer have omap_device calling PM runtime suspend for us on suspend. This causes the driver context not be saved as we have no suspend and resume functions defined. Let's fix the issue by switching over to use UNIVERSAL_DEV_PM_OPS as it will call the existing PM runtime suspend functions on suspend. I don't think we can use UNIVERSAL_DEV_PM_OPS, as we can't disable DSS modules in any order, but things have to be shut down in orderly manner. omapdrm hasn't relied on omap_device calling runtime suspend for us (I didn't know it does that). We have system suspend hooks in omap_drv.c: SIMPLE_DEV_PM_OPS(omapdrm_pm_ops, omap_drm_suspend, omap_drm_resume) omap_drm_suspend() is supposed to turn off the displays, which then cause dispc_runtime_put (and other runtime_puts) to be called, which result in dispc_runtime_suspend (and other runtime PM suspends). So... For some reason that's no longer happening? I need to try to find a board with which suspend/resume works (without DSS)... Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCHv2] media: videobuf2-dma-contig: fix bad kfree in vb2_dma_contig_clear_max_seg_size
On 28/05/2020 12:14, Ulf Hansson wrote:
On Wed, 27 May 2020 at 10:23, Tomi Valkeinen wrote:
Commit 9495b7e92f716ab2bd6814fab5e97ab4a39adfdd ("driver core: platform:
Initialize dma_parms for platform devices") in v5.7-rc5 causes
vb2_dma_contig_clear_max_seg_size() to kfree memory that was not
allocated by vb2_dma_contig_set_max_seg_size().
The assumption in vb2_dma_contig_set_max_seg_size() seems to be that
dev->dma_parms is always NULL when the driver is probed, and the case
where dev->dma_parms has bee initialized by someone else than the driver
(by calling vb2_dma_contig_set_max_seg_size) will cause a failure.
All the current users of these functions are platform devices, which now
always have dma_parms set by the driver core. To fix the issue for v5.7,
make vb2_dma_contig_set_max_seg_size() return an error if dma_parms is
NULL to be on the safe side, and remove the kfree code from
vb2_dma_contig_clear_max_seg_size().
For v5.8 we should remove the two functions and move the
dma_set_max_seg_size() calls into the drivers.
Signed-off-by: Tomi Valkeinen
Fixes: 9495b7e92f71 ("driver core: platform: Initialize dma_parms for platform
devices")
Cc: sta...@vger.kernel.org
Thanks for fixing this!
However, as I tried to point out in v1, don't you need to care about
drivers/media/platform/s5p-mfc/s5p_mfc.c, which allocates its own type
of struct device (non-platform). No?
Oh my bad. I thought Marek posted a patch for it, but now that I look, Marek's patch was for
ExynosDRM. Somehow I managed to mix up that with the s5p in my head.
I'll try to find time to look at s5p too, but if anyone gets there first, feel
free to fix it.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
[PATCHv2] media: videobuf2-dma-contig: fix bad kfree in vb2_dma_contig_clear_max_seg_size
Commit 9495b7e92f716ab2bd6814fab5e97ab4a39adfdd ("driver core: platform:
Initialize dma_parms for platform devices") in v5.7-rc5 causes
vb2_dma_contig_clear_max_seg_size() to kfree memory that was not
allocated by vb2_dma_contig_set_max_seg_size().
The assumption in vb2_dma_contig_set_max_seg_size() seems to be that
dev->dma_parms is always NULL when the driver is probed, and the case
where dev->dma_parms has bee initialized by someone else than the driver
(by calling vb2_dma_contig_set_max_seg_size) will cause a failure.
All the current users of these functions are platform devices, which now
always have dma_parms set by the driver core. To fix the issue for v5.7,
make vb2_dma_contig_set_max_seg_size() return an error if dma_parms is
NULL to be on the safe side, and remove the kfree code from
vb2_dma_contig_clear_max_seg_size().
For v5.8 we should remove the two functions and move the
dma_set_max_seg_size() calls into the drivers.
Signed-off-by: Tomi Valkeinen
Fixes: 9495b7e92f71 ("driver core: platform: Initialize dma_parms for platform
devices")
Cc: sta...@vger.kernel.org
---
Changes in v2:
* vb2_dma_contig_clear_max_seg_size to empty static inline
* Added cc: stable and fixes tag
.../common/videobuf2/videobuf2-dma-contig.c | 20 ++-
include/media/videobuf2-dma-contig.h | 2 +-
2 files changed, 3 insertions(+), 19 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index d3a3ee5b597b..f4b4a7c135eb 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -726,9 +726,8 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size)
{
if (!dev->dma_parms) {
- dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL);
- if (!dev->dma_parms)
- return -ENOMEM;
+ dev_err(dev, "Failed to set max_seg_size: dma_parms is NULL\n");
+ return -ENODEV;
}
if (dma_get_max_seg_size(dev) < size)
return dma_set_max_seg_size(dev, size);
@@ -737,21 +736,6 @@ int vb2_dma_contig_set_max_seg_size(struct device *dev,
unsigned int size)
}
EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size);
-/*
- * vb2_dma_contig_clear_max_seg_size() - release resources for DMA parameters
- * @dev: device for configuring DMA parameters
- *
- * This function releases resources allocated to configure DMA parameters
- * (see vb2_dma_contig_set_max_seg_size() function). It should be called from
- * device drivers on driver remove.
- */
-void vb2_dma_contig_clear_max_seg_size(struct device *dev)
-{
- kfree(dev->dma_parms);
- dev->dma_parms = NULL;
-}
-EXPORT_SYMBOL_GPL(vb2_dma_contig_clear_max_seg_size);
-
MODULE_DESCRIPTION("DMA-contig memory handling routines for videobuf2");
MODULE_AUTHOR("Pawel Osciak ");
MODULE_LICENSE("GPL");
diff --git a/include/media/videobuf2-dma-contig.h
b/include/media/videobuf2-dma-contig.h
index 5604818d137e..5be313cbf7d7 100644
--- a/include/media/videobuf2-dma-contig.h
+++ b/include/media/videobuf2-dma-contig.h
@@ -25,7 +25,7 @@ vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned
int plane_no)
}
int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size);
-void vb2_dma_contig_clear_max_seg_size(struct device *dev);
+static inline void vb2_dma_contig_clear_max_seg_size(struct device *dev) { }
extern const struct vb2_mem_ops vb2_dma_contig_memops;
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
[PATCH] media: videobuf2-dma-contig: fix bad kfree in vb2_dma_contig_clear_max_seg_size
Commit 9495b7e92f716ab2bd6814fab5e97ab4a39adfdd ("driver core: platform:
Initialize dma_parms for platform devices") in v5.7-rc5 causes
vb2_dma_contig_clear_max_seg_size() to kfree memory that was not
allocated by vb2_dma_contig_set_max_seg_size().
The assumption in vb2_dma_contig_set_max_seg_size() seems to be that
dev->dma_parms is always NULL when the driver is probed, and the case
where dev->dma_parms has bee initialized by someone else than the driver
(by calling vb2_dma_contig_set_max_seg_size) will cause a failure.
All the current users of these functions are platform devices, which now
always have dma_parms set by the driver core. To fix the issue for v5.7,
make vb2_dma_contig_set_max_seg_size() return an error if dma_parms is
NULL to be on the safe side, and remove the kfree code from
vb2_dma_contig_clear_max_seg_size().
For v5.8 we should remove the two functions and move the
dma_set_max_seg_size() calls into the drivers.
Signed-off-by: Tomi Valkeinen
---
Note: I have only fully tested this on linux-next, as the capture driver
I use doesn't support unloading modules in v5.7.
drivers/media/common/videobuf2/videobuf2-dma-contig.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index d3a3ee5b597b..24f80b62ef94 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -726,9 +726,8 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size)
{
if (!dev->dma_parms) {
- dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL);
- if (!dev->dma_parms)
- return -ENOMEM;
+ dev_err(dev, "Failed to set max_seg_size: dma_parms is NULL\n");
+ return -ENODEV;
}
if (dma_get_max_seg_size(dev) < size)
return dma_set_max_seg_size(dev, size);
@@ -747,8 +746,6 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size);
*/
void vb2_dma_contig_clear_max_seg_size(struct device *dev)
{
- kfree(dev->dma_parms);
- dev->dma_parms = NULL;
}
EXPORT_SYMBOL_GPL(vb2_dma_contig_clear_max_seg_size);
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: Bad kfree of dma_parms in v5.7-rc5
On 20/05/2020 12:22, Marek Szyprowski wrote:
Hi Tomi,
On 20.05.2020 11:18, Tomi Valkeinen wrote:
On 20/05/2020 12:13, Marek Szyprowski wrote:
On 20.05.2020 11:00, Tomi Valkeinen wrote:
Commit 9495b7e92f716ab2bd6814fab5e97ab4a39adfdd ("driver core:
platform: Initialize dma_parms for platform devices") v5.7-rc5 causes
at least some v4l2 platform drivers to break when freeing resources.
E.g. drivers/media/platform/ti-vpe/cal.c uses
vb2_dma_contig_set_max_seg_size() and
vb2_dma_contig_clear_max_seg_size() to manage the dma_params, and
similar pattern is seen in other drivers too.
After 9495b7e92f716ab2, vb2_dma_contig_set_max_seg_size() will not
allocate anything, but vb2_dma_contig_clear_max_seg_size() will still
kfree the dma_params.
I'm not sure what's the proper fix here. A flag somewhere to indicate
that vb2_dma_contig_set_max_seg_size() did allocate, and thus
vb2_dma_contig_clear_max_seg_size() must free?
Or drop the kzalloc and kfree totally, if dma_params is now supposed
to always be there?
Thanks for reporting this issue!
Once the mentioned commit has been merged, the code should assume that
the platform devices does have a struct dma_params allocated, so the
proper fix is to alloc dma_params only if the bus is not a platform bus:
if (!dev_is_platform(dev) && !dev->dma_parms) {
dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL);
same check for the free path.
There is also "amba: Initialize dma_parms for amba devices". And the
commit message says PCI devices do this too.
Guessing this based on the device type doesn't sound like a good idea
to me.
Indeed. Then replace the allocation with a simple check for NULL
dma_parms and return an error in such case. This should be enough for
v5.8. Later we can simply get rid of those helpers and inline setting
max segment size directly to the drivers.
Is that valid either? Then we assume that dma_parms is always set up by someone else. That's true
for platform devices and apparently some other devices, but is it true for all devices now?
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: Bad kfree of dma_parms in v5.7-rc5
Hi Marek,
On 20/05/2020 12:13, Marek Szyprowski wrote:
Hi Tomi,
On 20.05.2020 11:00, Tomi Valkeinen wrote:
Commit 9495b7e92f716ab2bd6814fab5e97ab4a39adfdd ("driver core:
platform: Initialize dma_parms for platform devices") v5.7-rc5 causes
at least some v4l2 platform drivers to break when freeing resources.
E.g. drivers/media/platform/ti-vpe/cal.c uses
vb2_dma_contig_set_max_seg_size() and
vb2_dma_contig_clear_max_seg_size() to manage the dma_params, and
similar pattern is seen in other drivers too.
After 9495b7e92f716ab2, vb2_dma_contig_set_max_seg_size() will not
allocate anything, but vb2_dma_contig_clear_max_seg_size() will still
kfree the dma_params.
I'm not sure what's the proper fix here. A flag somewhere to indicate
that vb2_dma_contig_set_max_seg_size() did allocate, and thus
vb2_dma_contig_clear_max_seg_size() must free?
Or drop the kzalloc and kfree totally, if dma_params is now supposed
to always be there?
Thanks for reporting this issue!
Once the mentioned commit has been merged, the code should assume that
the platform devices does have a struct dma_params allocated, so the
proper fix is to alloc dma_params only if the bus is not a platform bus:
if (!dev_is_platform(dev) && !dev->dma_parms) {
dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL);
same check for the free path.
There is also "amba: Initialize dma_parms for amba devices". And the commit message says PCI devices
do this too.
Guessing this based on the device type doesn't sound like a good idea to me.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Bad kfree of dma_parms in v5.7-rc5
Hi,
Commit 9495b7e92f716ab2bd6814fab5e97ab4a39adfdd ("driver core: platform: Initialize dma_parms for
platform devices") v5.7-rc5 causes at least some v4l2 platform drivers to break when freeing resources.
E.g. drivers/media/platform/ti-vpe/cal.c uses vb2_dma_contig_set_max_seg_size() and
vb2_dma_contig_clear_max_seg_size() to manage the dma_params, and similar pattern is seen in other
drivers too.
After 9495b7e92f716ab2, vb2_dma_contig_set_max_seg_size() will not allocate anything, but
vb2_dma_contig_clear_max_seg_size() will still kfree the dma_params.
I'm not sure what's the proper fix here. A flag somewhere to indicate that
vb2_dma_contig_set_max_seg_size() did allocate, and thus vb2_dma_contig_clear_max_seg_size() must free?
Or drop the kzalloc and kfree totally, if dma_params is now supposed to always
be there?
Or revert 9495b7e92f716ab2, as it was added so late?
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v2] arm: dts: Move am33xx and am43xx mmc nodes to sdhci-omap driver
(Dropping DT from cc) On 19/05/2020 18:48, Tony Lindgren wrote: Suspend/resume on am43xx-gpevm is broken right now in mainline and the regression looks like it is caused by the display subsystem. I have reported this to Tomi and its being investigated. Meanwhile I have tested this patch with display configs disabled and Keerthy's suspend/resume tests pass on both am3 and am4. OK great thanks for checking it. Do you have the display subsystem related commit that broke PM? I'm wondering if my recent DSS platform data removal changes might have caused the regression. I spent a bit time looking at this, but unfortunately I wasn't even able to resume my AM4 evm from suspend. I tried with rtcwake and with plain console (with no_console_suspend). I did not have DSS loaded. Anyone have quick hints on how to debug why resume doesn't seem to happen? Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: linux-next: manual merge of the drm-misc tree with the drm-misc-fixes tree
Hi Stephen,
On 23/04/2020 06:17, Stephen Rothwell wrote:
> Hi all,
>
> On Tue, 21 Apr 2020 09:10:25 +0300 Tomi Valkeinen
> wrote:
>>
>> On 21/04/2020 04:52, Stephen Rothwell wrote:
>>>
>>> Today's linux-next merge of the drm-misc tree got a conflict in:he drm-misc
>>> tree with the drm-misc-fixes tree
>>>
>>> drivers/gpu/drm/tidss/tidss_encoder.c
>>>
>>> between commit:
>>>
>>> 9da67433f64e ("drm/tidss: fix crash related to accessing freed memory")
>>>
>>> from the drm-misc-fixes tree and commit:
>>>
>>> b28ad7deb2f2 ("drm/tidss: Use simple encoder")
>>>
>>> from the drm-misc tree.
>>>
>>> I fixed it up (I just used the latter version of this file) and can
>>
>> We need to use "drm/tidss: fix crash related to accessing freed memory"
>> version.
>>
>>> carry the fix as necessary. This is now fixed as far as linux-next is
>>> concerned, but any non trivial conflicts should be mentioned to your
>>> upstream maintainer when your tree is submitted for merging. You may
>>> also want to consider cooperating with the maintainer of the conflicting
>>> tree to minimise any particularly complex conflicts.
>>
>> I have fixed this with drm-misc's dim tool, so I presume the conflict goes
>> away when drm-misc-fixes
>> is merged to drm-fixes, and drm-fixes is then at some point merged to
>> drm-misc-next.
>>
>> It was a bit bad timing with the "drm/tidss: Use simple encoder", which
>> removes the plumbing I
>> needed to implement the fix. So I effectively revert the "drm/tidss: Use
>> simple encoder".
>>
>>Tomi
>>
>
> This is now a conflict between the drm and drm-misc-fixes trees.
The fix ("drm/tidss: fix crash related to accessing freed memory") is in
v5.7-rc3, and the conflicting change ("drm/tidss: Use simple encoder") in
drm-next.
The conflict resolution in linux-next drops the fix and take the change from
drm-next, which causes crash on module removal.
Here's the diff I made on top of linux-next to fix it. Essentially dropping the
simple-encoder change, and reapplying the fix. This should be fixed in drm-next
when Dave next time pulls in Linus' branch.
diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c
b/drivers/gpu/drm/tidss/tidss_encoder.c
index 4c0558286f5e..e624cdcbb567 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.c
+++ b/drivers/gpu/drm/tidss/tidss_encoder.c
@@ -56,25 +56,38 @@ static int tidss_encoder_atomic_check(struct drm_encoder
*encoder,
return 0;
}
+static void tidss_encoder_destroy(struct drm_encoder *encoder)
+{
+ drm_encoder_cleanup(encoder);
+ kfree(encoder);
+}
+
static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
.atomic_check = tidss_encoder_atomic_check,
};
+static const struct drm_encoder_funcs encoder_funcs = {
+ .destroy = tidss_encoder_destroy,
+};
+
struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
u32 encoder_type, u32 possible_crtcs)
{
struct drm_encoder *enc;
int ret;
- enc = devm_kzalloc(tidss->dev, sizeof(*enc), GFP_KERNEL);
+ enc = kzalloc(sizeof(*enc), GFP_KERNEL);
if (!enc)
return ERR_PTR(-ENOMEM);
enc->possible_crtcs = possible_crtcs;
- ret = drm_simple_encoder_init(>ddev, enc, encoder_type);
- if (ret < 0)
+ ret = drm_encoder_init(>ddev, enc, _funcs,
+ encoder_type, NULL);
+ if (ret < 0) {
+ kfree(enc);
return ERR_PTR(ret);
+ }
drm_encoder_helper_add(enc, _helper_funcs);
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes
On 07/05/2020 20:17, Maxime Ripard wrote: Actually, for this particular case, consumer driver will be the Cadence MHDP bridge driver for DisplayPort which is also under review process for upstreaming [1]. So this DRM bridge driver will make use of the PHY APIs phy_get_bus_width() and phy_get_max_link_rate() during execution of probe function to get the number of lanes and maximum link rate supported by Cadence Torrent PHY. This information is required to set the host capabilities in the DRM bridge driver, based on which initial values for DisplayPort link training will be determined. The changes in this PHY patch series are based on suggestions in the review comments in [1] which asks to use kernel PHY APIs to read these properties instead of directly accessing PHY device node. The complete driver and actual use of these APIs can be found in [2]. This is how we are planning to use these APIs. I haven't really looked into the displayport spec, but I'd assume that there's a lot more parameters that would need to be negociated between the phy and the DP block? If so, then it would make more sense to follow the path we did for MIPI-DSI where the parameters can be negociated through the phy_configure / phy_validate interface. I don't think this is negotiation, but just exposing the (max) capabilities of PHY, inside which the configure can work. Maybe all the capabilities could handled with a struct (struct phy_attrs), instead of adding separate functions for each, though. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm/bridge: fix stack usage warning on old gcc
On 29/04/2020 11:02, Arnd Bergmann wrote:
On Wed, Apr 29, 2020 at 9:56 AM Tomi Valkeinen wrote:
diff --git a/drivers/gpu/drm/bridge/tc358768.c
b/drivers/gpu/drm/bridge/tc358768.c
index 1b39e8d37834..6650fe4cfc20 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -178,6 +178,8 @@ static int tc358768_clear_error(struct tc358768_priv *priv)
static void tc358768_write(struct tc358768_priv *priv, u32 reg, u32 val)
{
+ /* work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715 */
+ int tmpval = val;
tc358768_write is not inline. What is the inline function here? Or is
tc358768_write optimized to
inline by the compiler?
I missed the lack of an explicit inline tag when looking at the bug. gcc
usually decides which functions to inline on its own, so there is little
difference in practice. Let me know if I should clarify the changelog and
resend it.
Ok. I think this is fine.
Reviewed-by: Tomi Valkeinen
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm/bridge: fix stack usage warning on old gcc
On 29/04/2020 00:53, Arnd Bergmann wrote:
Some older versions of gcc badly optimize code that passes
an inline function argument into another function by reference,
causing huge stack usage:
drivers/gpu/drm/bridge/tc358768.c: In function 'tc358768_bridge_pre_enable':
drivers/gpu/drm/bridge/tc358768.c:840:1: error: the frame size of 2256 bytes is
larger than 2048 bytes [-Werror=frame-larger-than=]
Use a temporary variable as a workaround and add a comment pointing
to the gcc bug.
Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
Signed-off-by: Arnd Bergmann
---
drivers/gpu/drm/bridge/tc358768.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/tc358768.c
b/drivers/gpu/drm/bridge/tc358768.c
index 1b39e8d37834..6650fe4cfc20 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -178,6 +178,8 @@ static int tc358768_clear_error(struct tc358768_priv *priv)
static void tc358768_write(struct tc358768_priv *priv, u32 reg, u32 val)
{
+ /* work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715 */
+ int tmpval = val;
size_t count = 2;
if (priv->error)
@@ -187,7 +189,7 @@ static void tc358768_write(struct tc358768_priv *priv, u32
reg, u32 val)
if (reg < 0x100 || reg >= 0x600)
count = 1;
- priv->error = regmap_bulk_write(priv->regmap, reg, , count);
+ priv->error = regmap_bulk_write(priv->regmap, reg, , count);
}
static void tc358768_read(struct tc358768_priv *priv, u32 reg, u32 *val)
tc358768_write is not inline. What is the inline function here? Or is tc358768_write optimized to
inline by the compiler?
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] fbdev/omap: fix max fclk divider for omap36xx
On 18/10/2019 16:05, Adam Ford wrote:
The OMAP36xx and AM/DM37x TRMs say that the maximum divider for DSS fclk
(in CM_CLKSEL_DSS) is 32. Experimentation shows that this is not
correct, and using divider of 32 breaks DSS with a flood or underflows
and sync losts. Dividers up to 31 seem to work fine.
There is another patch to the DT files to limit the divider correctly,
but as the DSS driver also needs to know the maximum divider to be able
to iteratively find good rates, we also need to do the fix in the DSS
driver.
Signed-off-by: Adam Ford
Cc: Tomi Valkeinen
Cc: sta...@vger.kernel.org # linux-4.4.y only
diff --git a/drivers/video/fbdev/omap2/dss/dss.c
b/drivers/video/fbdev/omap2/dss/dss.c
index 9200a8668b49..a57c3a5f4bf8 100644
--- a/drivers/video/fbdev/omap2/dss/dss.c
+++ b/drivers/video/fbdev/omap2/dss/dss.c
@@ -843,7 +843,7 @@ static const struct dss_features omap34xx_dss_feats = {
};
static const struct dss_features omap3630_dss_feats = {
- .fck_div_max= 32,
+ .fck_div_max= 31,
.dss_fck_multiplier = 1,
.parent_clk_name= "dpll4_ck",
.dpi_select_source = _dpi_select_source_omap2_omap3,
To clarify, this patch is only for the v4.4 stable, whereas the previous
patch was for next merge window and v4.9+ stable. Right?
Reviewed-by: Tomi Valkeinen
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] fbdev/omap: fix max fclk divider for omap36xx
On 18/10/2019 15:49, Adam Ford wrote:
The OMAP36xx and AM/DM37x TRMs say that the maximum divider for DSS fclk
(in CM_CLKSEL_DSS) is 32. Experimentation shows that this is not
correct, and using divider of 32 breaks DSS with a flood or underflows
and sync losts. Dividers up to 31 seem to work fine.
There is another patch to the DT files to limit the divider correctly,
but as the DSS driver also needs to know the maximum divider to be able
to iteratively find good rates, we also need to do the fix in the DSS
driver.
Signed-off-by: Adam Ford
Cc: Tomi Valkeinen
Cc: sta...@vger.kernel.org #linux-4.9.y+
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
index 48c6500c24e1..4429ad37b64c 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
@@ -843,7 +843,7 @@ static const struct dss_features omap34xx_dss_feats = {
};
static const struct dss_features omap3630_dss_feats = {
- .fck_div_max= 32,
+ .fck_div_max= 31,
.dss_fck_multiplier = 1,
.parent_clk_name= "dpll4_ck",
.dpi_select_source = _dpi_select_source_omap2_omap3,
Reviewed-by: Tomi Valkeinen
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm/omap: Migrate minimum FCK/PCK ratio from Kconfig to dts
On 01/10/2019 11:12, Tero Kristo wrote: On 01/10/2019 08:07, Tomi Valkeinen wrote: On 30/09/2019 20:48, Tero Kristo wrote: Hmmh, after some testing, it seems there is bad stuff happening with the divider clock implementation, I am re-working it as of now. Basically what is wrong is that with a divider max value of say 16, the driver attempts to craft the max value into a mask, but this ends up being 0x1f. If the max value is 15, it ends up into 0xf which is correct. Ok, that explains the max not working. It doesn't explain the other issue, where the TRM says the max div is 32, but it does not work. But taking the max div from the old SoCs, 16, is not correct either, as it seems that dividers up to 31 work ok. Tomi Ok, attached a series that hopefully fixes it, any testing feedback welcome before I post this properly. This also supports omap36xx dpll4_m4_ck divider up-to 31, other omap3 family is limited to 16. Works for me. This also needs the change to dss.c to change the max from 32 to 31. I'll send a patch for that separately. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] drm/omap: Migrate minimum FCK/PCK ratio from Kconfig to dts
On 26/09/2019 17:12, Adam Ford wrote: And what is the hdmi5_configure there? I don't see anything in the driver that would print hdmi5_configure. And, of course, there's no hdmi5 on that platform. Hmm, ok... it's from component.c, using "%ps". Somehow that goes wrong. Which is a bit alarming, but perhaps a totally different issue. I'll try to take a look later. For Logic PD distributions, we create a custom defconfig with all those drivers removed, so I'm not worked up about it, but it would be nice to not call drivers that don't exist. So you have CONFIG_OMAP5_DSS_HDMI=n? Then it's even more disturbing, as there's no way the string "hdmi5_configure" can be in the kernel image... Maybe it's nothing, but... It's just so odd. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v4 2/4] leds: Add managed API to get a LED from a device driver
On 17/07/2019 17:15, Jean-Jacques Hiblot wrote:
+struct led_classdev *__must_check devm_led_get(struct device *dev,
+ int index)
+{
+ struct led_classdev *led;
+ struct led_classdev **dr;
+
Should you check here if dev->of_node == NULL? Or should of_led_get()
check it.
+ led = of_led_get(dev->of_node, index);
+ if (IS_ERR(led))
+ return led;
+
+ dr = devres_alloc(devm_led_release, sizeof(struct led_classdev *),
+ GFP_KERNEL);
+ if (!dr) {
+ led_put(led);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ *dr = led;
+ devres_add(dev, dr);
+
+ return led;
+}
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v2 0/2] leds: tlc591xx: switch to OF and managed API
On 08/07/2019 13:06, Jean-Jacques Hiblot wrote: This mini-series updates the tlc591xx driver to use the managed API. The driver is also modified to pass the DT node to the LED core layer. The goal is to be able to the generic led-backlight [0] driver on top of it. changes in v2: - fixed LED indexing. Previous version did not allow for holes: if n LEDs were used, they had to be led(0) to led(n-1) Jean-Jacques Hiblot (2): leds: tlc591xx: simplify driver by using the managed led API leds: tlc591xx: Use the OF version of the LED registration function drivers/leds/leds-tlc591xx.c | 79 +--- 1 file changed, 20 insertions(+), 59 deletions(-) For the series: Reviewed-by: Tomi Valkeinen Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCHv6 0/4] omapdrm: DSI command mode panel support
On 28/05/2019 13:18, Tony Lindgren wrote:
This change lets me boot. I don't know that's the correct place, though:
diff --git a/arch/arm/boot/dts/am5728.dtsi b/arch/arm/boot/dts/am5728.dtsi
index 82e5427ef6a9..c778f9a86b3a 100644
--- a/arch/arm/boot/dts/am5728.dtsi
+++ b/arch/arm/boot/dts/am5728.dtsi
@@ -31,3 +31,7 @@
_tm {
status = "disabled";
};
+
+ {
+ status = "disabled";
+};
OK we should disable it at the target-module level though. Interesting
that reading the revision register works with the above, or maybe you
still get some warning?
I don't see anything odd in the boot log with the above change. At least
no kernel WARNs, nor anything with grepping "timer", "err", or "warn".
I just verified with clean 5.2-rc2, that it doesn't boot, and with the
above change, boots.
My board is x15 rev A3, attached to AM5 EVM. I've also attached my kernel
config.
Strange that this is not affecting other x15? I think timer12 would
be blocked on HS devices though?
I don't know... I can't believe my x15 would be unique =). Can it be
something in the kernel config? u-boot?
Peter should have the same A3. Peter, can you try with my config?
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v2 08/15] drm/bridge: tc358767: Increase AUX transfer length limit
On 22/03/2019 05:28, Andrey Smirnov wrote: > According to the datasheet tc358767 can transfer up to 16 bytes via > its AUX channel, so the artificial limit of 8 apperas to be too > low. However only up to 15-bytes seem to be actually supported and > trying to use 16-byte transfers results in transfers failing > sporadically (with bogus status in case of I2C transfers), so limit it > to 15. 16 is the limit from the DP spec. I agree, 8 looks odd. 15 looks odd too, so I think it warrants a comment there in the code. Does 15 byte transfers ever work? Or mostly works but sometimes fails? Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v2 05/15] drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors
On 22/03/2019 05:28, Andrey Smirnov wrote: > A very unfortunate aspect of tc_write()/tc_read() macro helpers is > that they capture quite a bit of context around them and thus require > the caller to have magic variables 'ret' and 'tc' as well as label > 'err'. That makes a number of code paths rather counterintuitive and > somewhat clunky, for example tc_stream_clock_calc() ends up being like > this: > > int ret; > > tc_write(DP0_VIDMNGEN1, 32768); > > return 0; > err: > return ret; > > which is rather surprising when you read the code for the first > time. Since those helpers arguably aren't really saving that much code > and there's no way of fixing them without making them too verbose to > be worth it change the driver code to not use them at all. I fully agree with this patch and thought about the same thing during my work. However, the timing of this patch is not too good, as this one will totally conflict with any other patch for tc358767, and my series is still evolving. We need to figure out how to combine this series and mine, but I think either this patch should be dropped for now, and reapplied after the other patches have stabilized, or I think preferably, this one could be rebased on top of 5.1-rc1, and used as a base for all other tc358767 work. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v2 03/15] drm/bridge: tc358767: Simplify polling in tc_link_training()
On 22/03/2019 05:28, Andrey Smirnov wrote: > Replace explicit polling in tc_link_training() with equivalent call to > tc_poll_timeout() for simplicity. No functional change intended (not > including slightly altered debug output). > > Signed-off-by: Andrey Smirnov > Cc: Archit Taneja > Cc: Andrzej Hajda > Cc: Laurent Pinchart > Cc: Tomi Valkeinen > Cc: Andrey Gusakov > Cc: Philipp Zabel > Cc: Chris Healy > Cc: Lucas Stach > Cc: dri-de...@lists.freedesktop.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/gpu/drm/bridge/tc358767.c | 13 + > 1 file changed, 5 insertions(+), 8 deletions(-) Reviewed-by: Tomi Valkeinen Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v2 0/2] drm/omap: panel-tpo-td028ttec1: add backlight support
On 15/02/2019 09:12, Andreas Kemnade wrote: > Hi, > > On Fri, 8 Feb 2019 11:13:33 +0200 > Tomi Valkeinen wrote: > >> On 05/02/2019 08:38, Andreas Kemnade wrote: >>> This panel has a backlight, so add a property describing that and >>> add the code to use that. >>> This makes things like xset dpms force off >>> also turn off the backlight, so we do not need to rely on additional >>> userspace programs to do that. >>> >>> Andreas Kemnade (2): >>> drm/omap: panel-tpo-td028ttec1: add backlight support >>> dt-bindings: panel: td028ttec1: add backlight property >>> >>> .../devicetree/bindings/display/panel/tpo,td028ttec1.txt | 2 ++ >>> drivers/gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c| 10 >>> ++ >>> 2 files changed, 12 insertions(+) >>> >> >> Thanks, I'll pick these up. >> > > hmm, that sounds like: patch accepted, will appear in linux-next soon. > I have not seen it there, or is it just in some branch not merged into > linux-next. Any new obstacles? It's too late to get it to the next merge window, so I'll have to wait until the merge window is closed before I can send these to drm-next. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
