From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.6 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 359D9C433DF for ; Thu, 20 Aug 2020 06:34:29 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id F380620786 for ; Thu, 20 Aug 2020 06:34:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1597905269; bh=BfHozWh6kPOwVZnZ5alNRgHDl/8qIGOcbL0zFhOlxr4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=YR6JuP2VD1hgIo5hAYwcgm5xSQObme2mjNjYVn5j3fVFjdQq9ySabqhCpAc0L+F2m 9h1tv3zxvgb9DeZFIzyj3qGSXpuQC0jncCr5kZH8uUDaZa0fbPyyjcHtzK1KmzXg/V Ql8dtFF/frtPVNpknuPIinwsvCyptlYb3cz64Kg8= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726759AbgHTGeZ (ORCPT ); Thu, 20 Aug 2020 02:34:25 -0400 Received: from mail.kernel.org ([198.145.29.99]:38658 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725778AbgHTGeZ (ORCPT ); Thu, 20 Aug 2020 02:34:25 -0400 Received: from coco.lan (ip5f5ad5a3.dynamic.kabel-deutschland.de [95.90.213.163]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 4F6AF2076E; Thu, 20 Aug 2020 06:34:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1597905262; bh=BfHozWh6kPOwVZnZ5alNRgHDl/8qIGOcbL0zFhOlxr4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=f+Tx8ZeGGaMHlAiE9gFg/CKrAKL3lsTdxURn32tRpWkKB/KYx3sDACAmO+S92wdL0 e2msrlg6vEkzmEAhy55+sRXIFwphAUjD2/CbcfK+qSGveJr6YZuniPlAWlVkJG8pLX xPZ+GETDlIXsbMoHo3LJOmrvLBFIWvxcEVUc7ztE= Date: Thu, 20 Aug 2020 08:34:11 +0200 From: Mauro Carvalho Chehab To: John Stultz Cc: Greg Kroah-Hartman , linuxarm@huawei.com, mauro.chehab@huawei.com, Manivannan Sadhasivam , Daniel Vetter , dri-devel , Bogdan Togorean , Liwei Cai , linux-arm-kernel , Daniel Borkmann , Rob Herring , "David S. Miller" , Xinliang Liu , Neil Armstrong , Wanchun Zheng , driverdevel , BPF Mailing List , Xiubin Zhang , linux-media , Tomi Valkeinen , Jesper Dangaard Brouer , Laurent Pinchart , Xinwei Kong , Alexei Starovoitov , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Rob Clark , Laurentiu Palcu , Andrzej Hajda , John Fastabend , Liuyao An , "moderated list:DMA BUFFER SHARING FRAMEWORK" , Wei Xu , Rongrong Zou , Philipp Zabel , Sam Ravnborg , Network Development , Sumit Semwal , lkml , Jakub Kicinski , David Airlie , Chen Feng Subject: Re: [PATCH 00/49] DRM driver for Hikey 970 Message-ID: <20200820083411.666a4d13@coco.lan> In-Reply-To: References: X-Mailer: Claws Mail 3.17.5 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Em Wed, 19 Aug 2020 14:13:05 -0700 John Stultz escreveu: > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab > wrote: > > Yet, I'm submitting it via staging due to the following reasons: > > > > - It depends on the LDO3 power supply, which is provided by > > a regulator driver that it is currently on staging; > > - Due to legal reasons, I need to preserve the authorship of > > each one responsbile for each patch. So, I need to start from > > the original patch from Kernel 4.4; > > - There are still some problems I need to figure out how to solve: > > - The adv7535 can't get EDID data. Maybe it is a timing issue, > > but it requires more research to be sure about how to solve it; > > I've seen this on the HiKey960 as well. There is a patch to the > adv7533 driver I have to add a mdelay that seems to consistently > resolve the timing problem. At some point I mentioned it to one of > the maintainers who seems open to having it added, but it seemed silly > to submit it until there was a upstream driver that needed such a > change. So I think that patch can be submitted as a follow on to this > (hopefully cleaned up) series. Yeah, I saw that mdelay() patch on your tree. While it should be cheap to add a mdelay() or udelay_range() there, I'm not sure if this is just a timing issue or if something else is missing. The 4.9 driver does some extra setups on adv7535 (see the enclosed patch). I'm wondering if something from that is missing. Btw, IMHO, one interesting setting on the downstream code is support for colorbar test. This was helpful when I was making this driver work upstream, as it could be useful for someone trying to make some other DRM driver using it. > > > - The driver only accept resolutions on a defined list, as there's > > a known bug that this driver may have troubles with random > > resolutions. Probably due to a bug at the pixel clock settings; > > So, yes, the SoC clks can't generate proper signals for HDMI > frequencies (apparently it's not an issue for panels). There is a > fixed set that we can get "close enough" that most monitors will work, > but its always a bit iffy (some monitors are strict in what they > take). There is an extra logic for Kirin 620 that seems to be validating the frequencies that would be set to the clocks. If they're out of the range, it would return MODE_BAD. I suspect that something similar would be needed for Kirin 970 and 960. With that regards, 970 is different from 960. I actually tried to write a patch for it, but I guess there are still some things missing on my code, for 970. This patch: https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commit/4614415770b33e27a9f15c7dde20895fb750592f Splits the part of the code which calculates the clk_Hz from the part which sets it. So, now, dss_calculate_clock() checks the pixel clock frequency and adjusts it, if needed. I have another patch that were checking if the code modified the clock_Hz at dss_crtc_mode_fixup(). With that, it would be easy to return MODE_INVALID if something bad happens. However, such patch didn't work as I would expect, so I ended dropping it. > On the kirin driver, we were able to do a calculation to figure out if > the generated frequency would be close enough: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c#n615 > > I suspect we could do something similar for the hikey960/70, but I've > not really had time to dig in deeply there. Yeah, I tried something similar to that. Maybe it will work properly for Hikey 960, but Hikey 970 has a much more complex code to setup the pixel clock. On 960, just one clock is set: clk_set_rate(ctx->dss_pxl0_clk, clk_Hz); While, on 970, pxl0 is always set to the same frequency (144 MHz), at least for pixel clocks up to 255 MHz[1], but the code do some complex calculus to setup PLL7 registers using the clock frequency. [1] pixel clocks above 255 MHz will be rejected anyway, as they'll return MODE_BAD due to the checks if a mode is valid. > > Personally, I don't see the allow-list as a problematic short term > solution, and again, not sure its worth pushing to staging for. Yeah, I guess we can live with that for a while. > > > - Sometimes (at least with 1080p), it generates LDI underflow > > errors, which in turn causes the DRM to stop working. That > > happens for example when using gdm on Wayland and > > gnome on X11; > > Interestingly, I've not seen this on HiKey960 (at least with > Android/Surfaceflinger). Here, it happens all the time when the monitor returns back from DPMS suspend. It also happens on some specific cases (X11 x Wayland), console mode + startx, etc. It sounds to me that it occurs when the driver tries to setup a new resolution. Maybe something needs to be disabled before changing res (and re-enabled afterwards). > The original HiKey board does have the > trouble where at 1080p the screen sometimes comes up horizontally > offset due to the LDI underflow, but the patches to address it have > been worse then the problem, so we reverted those. The "visible" effect on my monitor is that the image disappears, and the monitor complains that there's an "Invalid Mode" at the HDMI signal. > > > - Probably related to the previous issue, when the monitor > > suspends due to DPMS, it doesn't return back to life. > > > > I don't believe I see this on HiKey960. But if it's the LDI issue on > the 970 that may explain it. Yes, when this happens, I get LDI underflow messages. > > So, IMO, the best is to keep it on staging for a while, until those > > remaining bugs gets solved. > > I'm not sure I see all of these as compelling for pushing it in via > staging. And I suspect in the process of submitting the patches for > review folks may find the cause of some of the problems you list here. Yeah, I hope we can get rid of at least part of those during the review process ;-) Yet, I'd like to preserve the history. As the enclosed patch shows, maybe there are some hidden gems at the previously existing code. I may also have done something wrong when converting the driver. So, preserving the history seems to be a good idea to me. One of the advantages of using staging is that the full history will be preserved at the incremental code at staging, while the final patch moving the code out of staging will have all the changes folded together into a single patch. That makes easier for people to do a final review on it. Also, some subsystem maintainers don't like the idea of merging the entire history on their subsystems. > Nit: This is a display driver and has little to do with the GPU (other > then it will eventually live in drivers/gpu/drm/...), so I might > suggest using more conventional subject prefix, "drm: hisilicon:" Ah, OK! I'll change the prefix at the next versions of this series. Thanks, Mauro HACK: Some changes from downstream to adv7511 code diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index a9bb734366ae..1440c08caf42 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -333,6 +333,7 @@ struct adv7511 { struct regmap *regmap; struct regmap *regmap_cec; + struct regmap *regmap_packet; enum drm_connector_status status; bool powered; diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index d59b37a2ae23..c9fbcb40f962 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -325,6 +325,153 @@ static void adv7511_set_link_config(struct adv7511 *adv7511, adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB; } +static void adv7511_dsi_config_tgen(struct adv7511 *adv7511) +{ + struct drm_display_mode *mode = &adv7511->curr_mode; + unsigned int hsw, hfp, hbp, vsw, vfp, vbp; +#ifndef TEST_COLORBAR_DISPLAY + u8 clock_div_by_lanes[] = { 6, 4, 3 }; /* 2, 3, 4 lanes */ +#endif + + hsw = mode->hsync_end - mode->hsync_start; + hfp = mode->hsync_start - mode->hdisplay; + hbp = mode->htotal - mode->hsync_end; + vsw = mode->vsync_end - mode->vsync_start; + vfp = mode->vsync_start - mode->vdisplay; + vbp = mode->vtotal - mode->vsync_end; + +#ifdef TEST_COLORBAR_DISPLAY + /* set pixel clock auto mode */ + regmap_write(adv7511->regmap_cec, 0x16, + 0x00); +#else + /* set pixel clock divider mode */ + regmap_write(adv7511->regmap_cec, 0x16, + clock_div_by_lanes[adv7511->num_dsi_lanes - 2] << 3); +#endif + + /* horizontal porch params */ + regmap_write(adv7511->regmap_cec, 0x28, mode->htotal >> 4); + regmap_write(adv7511->regmap_cec, 0x29, (mode->htotal << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x2a, hsw >> 4); + regmap_write(adv7511->regmap_cec, 0x2b, (hsw << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x2c, hfp >> 4); + regmap_write(adv7511->regmap_cec, 0x2d, (hfp << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x2e, hbp >> 4); + regmap_write(adv7511->regmap_cec, 0x2f, (hbp << 4) & 0xff); + + /* vertical porch params */ + regmap_write(adv7511->regmap_cec, 0x30, mode->vtotal >> 4); + regmap_write(adv7511->regmap_cec, 0x31, (mode->vtotal << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x32, vsw >> 4); + regmap_write(adv7511->regmap_cec, 0x33, (vsw << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x34, vfp >> 4); + regmap_write(adv7511->regmap_cec, 0x35, (vfp << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x36, vbp >> 4); + regmap_write(adv7511->regmap_cec, 0x37, (vbp << 4) & 0xff); +} + +static void hikey970_hack_dpms(struct adv7511 *adv7511) +{ + +DRM_ERROR("Calling %s\n", __func__); + + struct mipi_dsi_device *dsi = adv7511->dsi; + + adv7511_dsi_config_tgen(adv7511); + + /* set number of dsi lanes */ + regmap_write(adv7511->regmap_cec, 0x1c, dsi->lanes << 4); + +#ifdef TEST_COLORBAR_DISPLAY + /* reset internal timing generator */ + regmap_write(adv7511->regmap_cec, 0x27, 0xcb); + regmap_write(adv7511->regmap_cec, 0x27, 0x8b); + regmap_write(adv7511->regmap_cec, 0x27, 0xcb); +#else + /* disable internal timing generator */ + regmap_write(adv7511->regmap_cec, 0x27, 0x0b); +#endif + + + /* enable hdmi */ + regmap_write(adv7511->regmap_cec, 0x03, 0x89); +#ifdef TEST_COLORBAR_DISPLAY + /*enable test mode */ + regmap_write(adv7511->regmap_cec, 0x55, 0x80);//display colorbar +#else + /* disable test mode */ + regmap_write(adv7511->regmap_cec, 0x55, 0x00); +#endif + /* disable test mode */ + //regmap_write(adv7511->regmap_cec, 0x55, 0x00); + /* SPD */ + { + static const unsigned char spd_if[] = { + 0x83, 0x01, 25, 0x00, + 'L', 'i', 'n', 'a', 'r', 'o', 0, 0, + '9', '6', 'b', 'o', 'a', 'r', 'd', 's', + ':', 'H', 'i', 'k', 'e', 'y', 0, 0, + }; + int n; + + for (n = 0; n < sizeof(spd_if); n++) + regmap_write(adv7511->regmap_packet, n, spd_if[n]); + + /* enable send SPD */ + regmap_update_bits(adv7511->regmap, 0x40, BIT(6), BIT(6)); + } + + /* force audio */ + /* hide Audio infoframe updates */ + regmap_update_bits(adv7511->regmap, 0x4a, BIT(5), BIT(5)); + + /* i2s, internal mclk, mclk-256 */ + regmap_update_bits(adv7511->regmap, 0x0a, 0x1f, 1); + regmap_update_bits(adv7511->regmap, 0x0b, 0xe0, 0); + /* enable i2s, use i2s format, sample rate from i2s */ + regmap_update_bits(adv7511->regmap, 0x0c, 0xc7, BIT(2)); + /* 16 bit audio */ + regmap_update_bits(adv7511->regmap, 0x0d, 0xff, 16); + /* 16-bit audio */ + regmap_update_bits(adv7511->regmap, 0x14, 0x0f, 2 << 4); + /* 48kHz */ + regmap_update_bits(adv7511->regmap, 0x15, 0xf0, 2 << 4); + /* enable N/CTS, enable Audio sample packets */ + regmap_update_bits(adv7511->regmap, 0x44, BIT(5), BIT(5)); + /* N = 6144 */ + regmap_write(adv7511->regmap, 1, (6144 >> 16) & 0xf); + regmap_write(adv7511->regmap, 2, (6144 >> 8) & 0xff); + regmap_write(adv7511->regmap, 3, (6144) & 0xff); + /* automatic cts */ + regmap_update_bits(adv7511->regmap, 0x0a, BIT(7), 0); + /* enable N/CTS */ + regmap_update_bits(adv7511->regmap, 0x44, BIT(6), BIT(6)); + /* not copyrighted */ + regmap_update_bits(adv7511->regmap, 0x12, BIT(5), BIT(5)); + + /* left source */ + regmap_update_bits(adv7511->regmap, 0x0e, 7 << 3, 0); + /* right source */ + regmap_update_bits(adv7511->regmap, 0x0e, 7 << 0, 1); + /* number of channels: sect 4.5.4: set to 0 */ + regmap_update_bits(adv7511->regmap, 0x73, 7, 1); + /* number of channels: sect 4.5.4: set to 0 */ + regmap_update_bits(adv7511->regmap, 0x73, 0xf0, 1 << 4); + /* sample rate: 48kHz */ + regmap_update_bits(adv7511->regmap, 0x74, 7 << 2, 3 << 2); + /* channel allocation reg: sect 4.5.4: set to 0 */ + regmap_update_bits(adv7511->regmap, 0x76, 0xff, 0); + /* enable audio infoframes */ + regmap_update_bits(adv7511->regmap, 0x44, BIT(3), BIT(3)); + + /* AV mute disable */ + regmap_update_bits(adv7511->regmap, 0x4b, BIT(7) | BIT(6), BIT(7)); + + /* use Audio infoframe updated info */ + regmap_update_bits(adv7511->regmap, 0x4a, BIT(5), 0); +} + static void __adv7511_power_on(struct adv7511 *adv7511) { adv7511->current_edid_segment = -1; @@ -367,8 +514,10 @@ static void adv7511_power_on(struct adv7511 *adv7511) */ regcache_sync(adv7511->regmap); - if (adv7511->type == ADV7533 || adv7511->type == ADV7535) + if (adv7511->type == ADV7533 || adv7511->type == ADV7535) { adv7533_dsi_power_on(adv7511); + hikey970_hack_dpms(adv7511); + } adv7511->powered = true; } @@ -981,8 +1130,10 @@ static int adv7511_init_regulators(struct adv7511 *adv) adv->supplies[i].supply = supply_names[i]; ret = devm_regulator_bulk_get(dev, adv->num_supplies, adv->supplies); - if (ret) + if (ret) { + dev_err(dev, "Can't get all regulators\n"); return ret; + } return regulator_bulk_enable(adv->num_supplies, adv->supplies); } @@ -1022,6 +1173,14 @@ static const struct regmap_config adv7511_cec_regmap_config = { .volatile_reg = adv7511_cec_register_volatile, }; +static const struct regmap_config adv7533_packet_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + + .max_register = 0xff, + .cache_type = REGCACHE_RBTREE, +}; + static int adv7511_init_cec_regmap(struct adv7511 *adv) { int ret; @@ -1045,6 +1204,14 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv) goto err; } + adv->regmap_packet = devm_regmap_init_i2c(adv->i2c_packet, + &adv7533_packet_regmap_config); + if (IS_ERR(adv->regmap_packet)) { + ret = PTR_ERR(adv->regmap_packet); + goto err; + } + + return 0; err: i2c_unregister_device(adv->i2c_cec); @@ -1168,8 +1335,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) return ret; ret = adv7511_init_regulators(adv7511); - if (ret != -EPROBE_DEFER) { - dev_err(dev, "failed to init regulators\n"); + if (ret && ret != -EPROBE_DEFER) { + dev_err(dev, "failed to init regulators: %d\n", ret); return ret; } From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.3 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0752EC433E5 for ; Thu, 20 Aug 2020 06:34:36 +0000 (UTC) Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D106A2076E for ; Thu, 20 Aug 2020 06:34:35 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="f+Tx8ZeG" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D106A2076E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=driverdev-devel-bounces@linuxdriverproject.org Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 81D712050C; Thu, 20 Aug 2020 06:34:35 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id j+RXJK1k+-mF; Thu, 20 Aug 2020 06:34:25 +0000 (UTC) Received: from ash.osuosl.org (ash.osuosl.org [140.211.166.34]) by silver.osuosl.org (Postfix) with ESMTP id 786FE20510; Thu, 20 Aug 2020 06:34:25 +0000 (UTC) Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by ash.osuosl.org (Postfix) with ESMTP id E69751BF388 for ; Thu, 20 Aug 2020 06:34:24 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id E26BB8824B for ; Thu, 20 Aug 2020 06:34:24 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zotVAOEC5t4a for ; Thu, 20 Aug 2020 06:34:23 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by hemlock.osuosl.org (Postfix) with ESMTPS id EF95D88231 for ; Thu, 20 Aug 2020 06:34:22 +0000 (UTC) Received: from coco.lan (ip5f5ad5a3.dynamic.kabel-deutschland.de [95.90.213.163]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 4F6AF2076E; Thu, 20 Aug 2020 06:34:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1597905262; bh=BfHozWh6kPOwVZnZ5alNRgHDl/8qIGOcbL0zFhOlxr4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=f+Tx8ZeGGaMHlAiE9gFg/CKrAKL3lsTdxURn32tRpWkKB/KYx3sDACAmO+S92wdL0 e2msrlg6vEkzmEAhy55+sRXIFwphAUjD2/CbcfK+qSGveJr6YZuniPlAWlVkJG8pLX xPZ+GETDlIXsbMoHo3LJOmrvLBFIWvxcEVUc7ztE= Date: Thu, 20 Aug 2020 08:34:11 +0200 From: Mauro Carvalho Chehab To: John Stultz Subject: Re: [PATCH 00/49] DRM driver for Hikey 970 Message-ID: <20200820083411.666a4d13@coco.lan> In-Reply-To: References: X-Mailer: Claws Mail 3.17.5 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 X-BeenThere: driverdev-devel@linuxdriverproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux Driver Project Developer List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Neil Armstrong , Xinliang Liu , Wanchun Zheng , linuxarm@huawei.com, dri-devel , Andrzej Hajda , Laurent Pinchart , Sam Ravnborg , Sumit Semwal , driverdevel , Daniel Borkmann , John Fastabend , Xiubin Zhang , Wei Xu , David Airlie , Xinwei Kong , Tomi Valkeinen , Bogdan Togorean , Philipp Zabel , Laurentiu Palcu , linux-media , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Liwei Cai , Jesper Dangaard Brouer , Manivannan Sadhasivam , Chen Feng , Alexei Starovoitov , "moderated list:DMA BUFFER SHARING FRAMEWORK" , Rob Herring , Jakub Kicinski , mauro.chehab@huawei.com, Rob Clark , linux-arm-kernel , Greg Kroah-Hartman , lkml , Liuyao An , Daniel Vetter , Network Development , Rongrong Zou , BPF Mailing List , "David S. Miller" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" Em Wed, 19 Aug 2020 14:13:05 -0700 John Stultz escreveu: > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab > wrote: > > Yet, I'm submitting it via staging due to the following reasons: > > > > - It depends on the LDO3 power supply, which is provided by > > a regulator driver that it is currently on staging; > > - Due to legal reasons, I need to preserve the authorship of > > each one responsbile for each patch. So, I need to start from > > the original patch from Kernel 4.4; > > - There are still some problems I need to figure out how to solve: > > - The adv7535 can't get EDID data. Maybe it is a timing issue, > > but it requires more research to be sure about how to solve it; > > I've seen this on the HiKey960 as well. There is a patch to the > adv7533 driver I have to add a mdelay that seems to consistently > resolve the timing problem. At some point I mentioned it to one of > the maintainers who seems open to having it added, but it seemed silly > to submit it until there was a upstream driver that needed such a > change. So I think that patch can be submitted as a follow on to this > (hopefully cleaned up) series. Yeah, I saw that mdelay() patch on your tree. While it should be cheap to add a mdelay() or udelay_range() there, I'm not sure if this is just a timing issue or if something else is missing. The 4.9 driver does some extra setups on adv7535 (see the enclosed patch). I'm wondering if something from that is missing. Btw, IMHO, one interesting setting on the downstream code is support for colorbar test. This was helpful when I was making this driver work upstream, as it could be useful for someone trying to make some other DRM driver using it. > > > - The driver only accept resolutions on a defined list, as there's > > a known bug that this driver may have troubles with random > > resolutions. Probably due to a bug at the pixel clock settings; > > So, yes, the SoC clks can't generate proper signals for HDMI > frequencies (apparently it's not an issue for panels). There is a > fixed set that we can get "close enough" that most monitors will work, > but its always a bit iffy (some monitors are strict in what they > take). There is an extra logic for Kirin 620 that seems to be validating the frequencies that would be set to the clocks. If they're out of the range, it would return MODE_BAD. I suspect that something similar would be needed for Kirin 970 and 960. With that regards, 970 is different from 960. I actually tried to write a patch for it, but I guess there are still some things missing on my code, for 970. This patch: https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commit/4614415770b33e27a9f15c7dde20895fb750592f Splits the part of the code which calculates the clk_Hz from the part which sets it. So, now, dss_calculate_clock() checks the pixel clock frequency and adjusts it, if needed. I have another patch that were checking if the code modified the clock_Hz at dss_crtc_mode_fixup(). With that, it would be easy to return MODE_INVALID if something bad happens. However, such patch didn't work as I would expect, so I ended dropping it. > On the kirin driver, we were able to do a calculation to figure out if > the generated frequency would be close enough: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c#n615 > > I suspect we could do something similar for the hikey960/70, but I've > not really had time to dig in deeply there. Yeah, I tried something similar to that. Maybe it will work properly for Hikey 960, but Hikey 970 has a much more complex code to setup the pixel clock. On 960, just one clock is set: clk_set_rate(ctx->dss_pxl0_clk, clk_Hz); While, on 970, pxl0 is always set to the same frequency (144 MHz), at least for pixel clocks up to 255 MHz[1], but the code do some complex calculus to setup PLL7 registers using the clock frequency. [1] pixel clocks above 255 MHz will be rejected anyway, as they'll return MODE_BAD due to the checks if a mode is valid. > > Personally, I don't see the allow-list as a problematic short term > solution, and again, not sure its worth pushing to staging for. Yeah, I guess we can live with that for a while. > > > - Sometimes (at least with 1080p), it generates LDI underflow > > errors, which in turn causes the DRM to stop working. That > > happens for example when using gdm on Wayland and > > gnome on X11; > > Interestingly, I've not seen this on HiKey960 (at least with > Android/Surfaceflinger). Here, it happens all the time when the monitor returns back from DPMS suspend. It also happens on some specific cases (X11 x Wayland), console mode + startx, etc. It sounds to me that it occurs when the driver tries to setup a new resolution. Maybe something needs to be disabled before changing res (and re-enabled afterwards). > The original HiKey board does have the > trouble where at 1080p the screen sometimes comes up horizontally > offset due to the LDI underflow, but the patches to address it have > been worse then the problem, so we reverted those. The "visible" effect on my monitor is that the image disappears, and the monitor complains that there's an "Invalid Mode" at the HDMI signal. > > > - Probably related to the previous issue, when the monitor > > suspends due to DPMS, it doesn't return back to life. > > > > I don't believe I see this on HiKey960. But if it's the LDI issue on > the 970 that may explain it. Yes, when this happens, I get LDI underflow messages. > > So, IMO, the best is to keep it on staging for a while, until those > > remaining bugs gets solved. > > I'm not sure I see all of these as compelling for pushing it in via > staging. And I suspect in the process of submitting the patches for > review folks may find the cause of some of the problems you list here. Yeah, I hope we can get rid of at least part of those during the review process ;-) Yet, I'd like to preserve the history. As the enclosed patch shows, maybe there are some hidden gems at the previously existing code. I may also have done something wrong when converting the driver. So, preserving the history seems to be a good idea to me. One of the advantages of using staging is that the full history will be preserved at the incremental code at staging, while the final patch moving the code out of staging will have all the changes folded together into a single patch. That makes easier for people to do a final review on it. Also, some subsystem maintainers don't like the idea of merging the entire history on their subsystems. > Nit: This is a display driver and has little to do with the GPU (other > then it will eventually live in drivers/gpu/drm/...), so I might > suggest using more conventional subject prefix, "drm: hisilicon:" Ah, OK! I'll change the prefix at the next versions of this series. Thanks, Mauro HACK: Some changes from downstream to adv7511 code diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index a9bb734366ae..1440c08caf42 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -333,6 +333,7 @@ struct adv7511 { struct regmap *regmap; struct regmap *regmap_cec; + struct regmap *regmap_packet; enum drm_connector_status status; bool powered; diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index d59b37a2ae23..c9fbcb40f962 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -325,6 +325,153 @@ static void adv7511_set_link_config(struct adv7511 *adv7511, adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB; } +static void adv7511_dsi_config_tgen(struct adv7511 *adv7511) +{ + struct drm_display_mode *mode = &adv7511->curr_mode; + unsigned int hsw, hfp, hbp, vsw, vfp, vbp; +#ifndef TEST_COLORBAR_DISPLAY + u8 clock_div_by_lanes[] = { 6, 4, 3 }; /* 2, 3, 4 lanes */ +#endif + + hsw = mode->hsync_end - mode->hsync_start; + hfp = mode->hsync_start - mode->hdisplay; + hbp = mode->htotal - mode->hsync_end; + vsw = mode->vsync_end - mode->vsync_start; + vfp = mode->vsync_start - mode->vdisplay; + vbp = mode->vtotal - mode->vsync_end; + +#ifdef TEST_COLORBAR_DISPLAY + /* set pixel clock auto mode */ + regmap_write(adv7511->regmap_cec, 0x16, + 0x00); +#else + /* set pixel clock divider mode */ + regmap_write(adv7511->regmap_cec, 0x16, + clock_div_by_lanes[adv7511->num_dsi_lanes - 2] << 3); +#endif + + /* horizontal porch params */ + regmap_write(adv7511->regmap_cec, 0x28, mode->htotal >> 4); + regmap_write(adv7511->regmap_cec, 0x29, (mode->htotal << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x2a, hsw >> 4); + regmap_write(adv7511->regmap_cec, 0x2b, (hsw << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x2c, hfp >> 4); + regmap_write(adv7511->regmap_cec, 0x2d, (hfp << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x2e, hbp >> 4); + regmap_write(adv7511->regmap_cec, 0x2f, (hbp << 4) & 0xff); + + /* vertical porch params */ + regmap_write(adv7511->regmap_cec, 0x30, mode->vtotal >> 4); + regmap_write(adv7511->regmap_cec, 0x31, (mode->vtotal << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x32, vsw >> 4); + regmap_write(adv7511->regmap_cec, 0x33, (vsw << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x34, vfp >> 4); + regmap_write(adv7511->regmap_cec, 0x35, (vfp << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x36, vbp >> 4); + regmap_write(adv7511->regmap_cec, 0x37, (vbp << 4) & 0xff); +} + +static void hikey970_hack_dpms(struct adv7511 *adv7511) +{ + +DRM_ERROR("Calling %s\n", __func__); + + struct mipi_dsi_device *dsi = adv7511->dsi; + + adv7511_dsi_config_tgen(adv7511); + + /* set number of dsi lanes */ + regmap_write(adv7511->regmap_cec, 0x1c, dsi->lanes << 4); + +#ifdef TEST_COLORBAR_DISPLAY + /* reset internal timing generator */ + regmap_write(adv7511->regmap_cec, 0x27, 0xcb); + regmap_write(adv7511->regmap_cec, 0x27, 0x8b); + regmap_write(adv7511->regmap_cec, 0x27, 0xcb); +#else + /* disable internal timing generator */ + regmap_write(adv7511->regmap_cec, 0x27, 0x0b); +#endif + + + /* enable hdmi */ + regmap_write(adv7511->regmap_cec, 0x03, 0x89); +#ifdef TEST_COLORBAR_DISPLAY + /*enable test mode */ + regmap_write(adv7511->regmap_cec, 0x55, 0x80);//display colorbar +#else + /* disable test mode */ + regmap_write(adv7511->regmap_cec, 0x55, 0x00); +#endif + /* disable test mode */ + //regmap_write(adv7511->regmap_cec, 0x55, 0x00); + /* SPD */ + { + static const unsigned char spd_if[] = { + 0x83, 0x01, 25, 0x00, + 'L', 'i', 'n', 'a', 'r', 'o', 0, 0, + '9', '6', 'b', 'o', 'a', 'r', 'd', 's', + ':', 'H', 'i', 'k', 'e', 'y', 0, 0, + }; + int n; + + for (n = 0; n < sizeof(spd_if); n++) + regmap_write(adv7511->regmap_packet, n, spd_if[n]); + + /* enable send SPD */ + regmap_update_bits(adv7511->regmap, 0x40, BIT(6), BIT(6)); + } + + /* force audio */ + /* hide Audio infoframe updates */ + regmap_update_bits(adv7511->regmap, 0x4a, BIT(5), BIT(5)); + + /* i2s, internal mclk, mclk-256 */ + regmap_update_bits(adv7511->regmap, 0x0a, 0x1f, 1); + regmap_update_bits(adv7511->regmap, 0x0b, 0xe0, 0); + /* enable i2s, use i2s format, sample rate from i2s */ + regmap_update_bits(adv7511->regmap, 0x0c, 0xc7, BIT(2)); + /* 16 bit audio */ + regmap_update_bits(adv7511->regmap, 0x0d, 0xff, 16); + /* 16-bit audio */ + regmap_update_bits(adv7511->regmap, 0x14, 0x0f, 2 << 4); + /* 48kHz */ + regmap_update_bits(adv7511->regmap, 0x15, 0xf0, 2 << 4); + /* enable N/CTS, enable Audio sample packets */ + regmap_update_bits(adv7511->regmap, 0x44, BIT(5), BIT(5)); + /* N = 6144 */ + regmap_write(adv7511->regmap, 1, (6144 >> 16) & 0xf); + regmap_write(adv7511->regmap, 2, (6144 >> 8) & 0xff); + regmap_write(adv7511->regmap, 3, (6144) & 0xff); + /* automatic cts */ + regmap_update_bits(adv7511->regmap, 0x0a, BIT(7), 0); + /* enable N/CTS */ + regmap_update_bits(adv7511->regmap, 0x44, BIT(6), BIT(6)); + /* not copyrighted */ + regmap_update_bits(adv7511->regmap, 0x12, BIT(5), BIT(5)); + + /* left source */ + regmap_update_bits(adv7511->regmap, 0x0e, 7 << 3, 0); + /* right source */ + regmap_update_bits(adv7511->regmap, 0x0e, 7 << 0, 1); + /* number of channels: sect 4.5.4: set to 0 */ + regmap_update_bits(adv7511->regmap, 0x73, 7, 1); + /* number of channels: sect 4.5.4: set to 0 */ + regmap_update_bits(adv7511->regmap, 0x73, 0xf0, 1 << 4); + /* sample rate: 48kHz */ + regmap_update_bits(adv7511->regmap, 0x74, 7 << 2, 3 << 2); + /* channel allocation reg: sect 4.5.4: set to 0 */ + regmap_update_bits(adv7511->regmap, 0x76, 0xff, 0); + /* enable audio infoframes */ + regmap_update_bits(adv7511->regmap, 0x44, BIT(3), BIT(3)); + + /* AV mute disable */ + regmap_update_bits(adv7511->regmap, 0x4b, BIT(7) | BIT(6), BIT(7)); + + /* use Audio infoframe updated info */ + regmap_update_bits(adv7511->regmap, 0x4a, BIT(5), 0); +} + static void __adv7511_power_on(struct adv7511 *adv7511) { adv7511->current_edid_segment = -1; @@ -367,8 +514,10 @@ static void adv7511_power_on(struct adv7511 *adv7511) */ regcache_sync(adv7511->regmap); - if (adv7511->type == ADV7533 || adv7511->type == ADV7535) + if (adv7511->type == ADV7533 || adv7511->type == ADV7535) { adv7533_dsi_power_on(adv7511); + hikey970_hack_dpms(adv7511); + } adv7511->powered = true; } @@ -981,8 +1130,10 @@ static int adv7511_init_regulators(struct adv7511 *adv) adv->supplies[i].supply = supply_names[i]; ret = devm_regulator_bulk_get(dev, adv->num_supplies, adv->supplies); - if (ret) + if (ret) { + dev_err(dev, "Can't get all regulators\n"); return ret; + } return regulator_bulk_enable(adv->num_supplies, adv->supplies); } @@ -1022,6 +1173,14 @@ static const struct regmap_config adv7511_cec_regmap_config = { .volatile_reg = adv7511_cec_register_volatile, }; +static const struct regmap_config adv7533_packet_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + + .max_register = 0xff, + .cache_type = REGCACHE_RBTREE, +}; + static int adv7511_init_cec_regmap(struct adv7511 *adv) { int ret; @@ -1045,6 +1204,14 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv) goto err; } + adv->regmap_packet = devm_regmap_init_i2c(adv->i2c_packet, + &adv7533_packet_regmap_config); + if (IS_ERR(adv->regmap_packet)) { + ret = PTR_ERR(adv->regmap_packet); + goto err; + } + + return 0; err: i2c_unregister_device(adv->i2c_cec); @@ -1168,8 +1335,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) return ret; ret = adv7511_init_regulators(adv7511); - if (ret != -EPROBE_DEFER) { - dev_err(dev, "failed to init regulators\n"); + if (ret && ret != -EPROBE_DEFER) { + dev_err(dev, "failed to init regulators: %d\n", ret); return ret; } _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A06B4C433DF for ; Thu, 20 Aug 2020 06:35:56 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 61EA22078B for ; Thu, 20 Aug 2020 06:35:56 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="NCjf8L2s"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="f+Tx8ZeG" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 61EA22078B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=lcOmJDdEu8UJi6Ubcrvm0JouM66fUwfkon8qYIf0yUo=; b=NCjf8L2s2akQGrv6e8CrIMtlT wTz5Cvr22VF55ZSxUvuEHGPbwyRNfLgoY+zePU4fZeqAmDcD7kz2OiMFl90mBtZUyl2ZlA/8YYqju XCxLno5nhaRe2Ztg0gl4uCvcQdii43lo/R5sGpCbGV897Cl9YBC1xN/ckLqp+XuYC7DTGSlOMBsdw jpXwJfFq2nBZot0gwhkVUyIPn1SEVjQAqbaupdDY6h7nUdaCDHxOBTGpd56yO56uWpfXj6BoA11Pz BtqRm+wWnTO2bcDk1iWhAslBFV0dqPNhjG2Tr6+BpGeDbyGEkUcp1MF1K6EIueJS5OAhj9acUomOG qtpN1ZoMA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k8e9b-0004ec-2x; Thu, 20 Aug 2020 06:34:27 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1k8e9X-0004dn-IO for linux-arm-kernel@lists.infradead.org; Thu, 20 Aug 2020 06:34:24 +0000 Received: from coco.lan (ip5f5ad5a3.dynamic.kabel-deutschland.de [95.90.213.163]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 4F6AF2076E; Thu, 20 Aug 2020 06:34:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1597905262; bh=BfHozWh6kPOwVZnZ5alNRgHDl/8qIGOcbL0zFhOlxr4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=f+Tx8ZeGGaMHlAiE9gFg/CKrAKL3lsTdxURn32tRpWkKB/KYx3sDACAmO+S92wdL0 e2msrlg6vEkzmEAhy55+sRXIFwphAUjD2/CbcfK+qSGveJr6YZuniPlAWlVkJG8pLX xPZ+GETDlIXsbMoHo3LJOmrvLBFIWvxcEVUc7ztE= Date: Thu, 20 Aug 2020 08:34:11 +0200 From: Mauro Carvalho Chehab To: John Stultz Subject: Re: [PATCH 00/49] DRM driver for Hikey 970 Message-ID: <20200820083411.666a4d13@coco.lan> In-Reply-To: References: X-Mailer: Claws Mail 3.17.5 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200820_023423_758619_5D86D9A2 X-CRM114-Status: GOOD ( 63.50 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Neil Armstrong , Xinliang Liu , Wanchun Zheng , linuxarm@huawei.com, dri-devel , Andrzej Hajda , Laurent Pinchart , Sam Ravnborg , Sumit Semwal , driverdevel , Daniel Borkmann , John Fastabend , Xiubin Zhang , Wei Xu , David Airlie , Xinwei Kong , Tomi Valkeinen , Bogdan Togorean , Philipp Zabel , Laurentiu Palcu , linux-media , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Liwei Cai , Jesper Dangaard Brouer , Manivannan Sadhasivam , Chen Feng , Alexei Starovoitov , "moderated list:DMA BUFFER SHARING FRAMEWORK" , Rob Herring , Jakub Kicinski , mauro.chehab@huawei.com, Rob Clark , linux-arm-kernel , Greg Kroah-Hartman , lkml , Liuyao An , Daniel Vetter , Network Development , Rongrong Zou , BPF Mailing List , "David S. Miller" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Em Wed, 19 Aug 2020 14:13:05 -0700 John Stultz escreveu: > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab > wrote: > > Yet, I'm submitting it via staging due to the following reasons: > > > > - It depends on the LDO3 power supply, which is provided by > > a regulator driver that it is currently on staging; > > - Due to legal reasons, I need to preserve the authorship of > > each one responsbile for each patch. So, I need to start from > > the original patch from Kernel 4.4; > > - There are still some problems I need to figure out how to solve: > > - The adv7535 can't get EDID data. Maybe it is a timing issue, > > but it requires more research to be sure about how to solve it; > > I've seen this on the HiKey960 as well. There is a patch to the > adv7533 driver I have to add a mdelay that seems to consistently > resolve the timing problem. At some point I mentioned it to one of > the maintainers who seems open to having it added, but it seemed silly > to submit it until there was a upstream driver that needed such a > change. So I think that patch can be submitted as a follow on to this > (hopefully cleaned up) series. Yeah, I saw that mdelay() patch on your tree. While it should be cheap to add a mdelay() or udelay_range() there, I'm not sure if this is just a timing issue or if something else is missing. The 4.9 driver does some extra setups on adv7535 (see the enclosed patch). I'm wondering if something from that is missing. Btw, IMHO, one interesting setting on the downstream code is support for colorbar test. This was helpful when I was making this driver work upstream, as it could be useful for someone trying to make some other DRM driver using it. > > > - The driver only accept resolutions on a defined list, as there's > > a known bug that this driver may have troubles with random > > resolutions. Probably due to a bug at the pixel clock settings; > > So, yes, the SoC clks can't generate proper signals for HDMI > frequencies (apparently it's not an issue for panels). There is a > fixed set that we can get "close enough" that most monitors will work, > but its always a bit iffy (some monitors are strict in what they > take). There is an extra logic for Kirin 620 that seems to be validating the frequencies that would be set to the clocks. If they're out of the range, it would return MODE_BAD. I suspect that something similar would be needed for Kirin 970 and 960. With that regards, 970 is different from 960. I actually tried to write a patch for it, but I guess there are still some things missing on my code, for 970. This patch: https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commit/4614415770b33e27a9f15c7dde20895fb750592f Splits the part of the code which calculates the clk_Hz from the part which sets it. So, now, dss_calculate_clock() checks the pixel clock frequency and adjusts it, if needed. I have another patch that were checking if the code modified the clock_Hz at dss_crtc_mode_fixup(). With that, it would be easy to return MODE_INVALID if something bad happens. However, such patch didn't work as I would expect, so I ended dropping it. > On the kirin driver, we were able to do a calculation to figure out if > the generated frequency would be close enough: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c#n615 > > I suspect we could do something similar for the hikey960/70, but I've > not really had time to dig in deeply there. Yeah, I tried something similar to that. Maybe it will work properly for Hikey 960, but Hikey 970 has a much more complex code to setup the pixel clock. On 960, just one clock is set: clk_set_rate(ctx->dss_pxl0_clk, clk_Hz); While, on 970, pxl0 is always set to the same frequency (144 MHz), at least for pixel clocks up to 255 MHz[1], but the code do some complex calculus to setup PLL7 registers using the clock frequency. [1] pixel clocks above 255 MHz will be rejected anyway, as they'll return MODE_BAD due to the checks if a mode is valid. > > Personally, I don't see the allow-list as a problematic short term > solution, and again, not sure its worth pushing to staging for. Yeah, I guess we can live with that for a while. > > > - Sometimes (at least with 1080p), it generates LDI underflow > > errors, which in turn causes the DRM to stop working. That > > happens for example when using gdm on Wayland and > > gnome on X11; > > Interestingly, I've not seen this on HiKey960 (at least with > Android/Surfaceflinger). Here, it happens all the time when the monitor returns back from DPMS suspend. It also happens on some specific cases (X11 x Wayland), console mode + startx, etc. It sounds to me that it occurs when the driver tries to setup a new resolution. Maybe something needs to be disabled before changing res (and re-enabled afterwards). > The original HiKey board does have the > trouble where at 1080p the screen sometimes comes up horizontally > offset due to the LDI underflow, but the patches to address it have > been worse then the problem, so we reverted those. The "visible" effect on my monitor is that the image disappears, and the monitor complains that there's an "Invalid Mode" at the HDMI signal. > > > - Probably related to the previous issue, when the monitor > > suspends due to DPMS, it doesn't return back to life. > > > > I don't believe I see this on HiKey960. But if it's the LDI issue on > the 970 that may explain it. Yes, when this happens, I get LDI underflow messages. > > So, IMO, the best is to keep it on staging for a while, until those > > remaining bugs gets solved. > > I'm not sure I see all of these as compelling for pushing it in via > staging. And I suspect in the process of submitting the patches for > review folks may find the cause of some of the problems you list here. Yeah, I hope we can get rid of at least part of those during the review process ;-) Yet, I'd like to preserve the history. As the enclosed patch shows, maybe there are some hidden gems at the previously existing code. I may also have done something wrong when converting the driver. So, preserving the history seems to be a good idea to me. One of the advantages of using staging is that the full history will be preserved at the incremental code at staging, while the final patch moving the code out of staging will have all the changes folded together into a single patch. That makes easier for people to do a final review on it. Also, some subsystem maintainers don't like the idea of merging the entire history on their subsystems. > Nit: This is a display driver and has little to do with the GPU (other > then it will eventually live in drivers/gpu/drm/...), so I might > suggest using more conventional subject prefix, "drm: hisilicon:" Ah, OK! I'll change the prefix at the next versions of this series. Thanks, Mauro HACK: Some changes from downstream to adv7511 code diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index a9bb734366ae..1440c08caf42 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -333,6 +333,7 @@ struct adv7511 { struct regmap *regmap; struct regmap *regmap_cec; + struct regmap *regmap_packet; enum drm_connector_status status; bool powered; diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index d59b37a2ae23..c9fbcb40f962 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -325,6 +325,153 @@ static void adv7511_set_link_config(struct adv7511 *adv7511, adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB; } +static void adv7511_dsi_config_tgen(struct adv7511 *adv7511) +{ + struct drm_display_mode *mode = &adv7511->curr_mode; + unsigned int hsw, hfp, hbp, vsw, vfp, vbp; +#ifndef TEST_COLORBAR_DISPLAY + u8 clock_div_by_lanes[] = { 6, 4, 3 }; /* 2, 3, 4 lanes */ +#endif + + hsw = mode->hsync_end - mode->hsync_start; + hfp = mode->hsync_start - mode->hdisplay; + hbp = mode->htotal - mode->hsync_end; + vsw = mode->vsync_end - mode->vsync_start; + vfp = mode->vsync_start - mode->vdisplay; + vbp = mode->vtotal - mode->vsync_end; + +#ifdef TEST_COLORBAR_DISPLAY + /* set pixel clock auto mode */ + regmap_write(adv7511->regmap_cec, 0x16, + 0x00); +#else + /* set pixel clock divider mode */ + regmap_write(adv7511->regmap_cec, 0x16, + clock_div_by_lanes[adv7511->num_dsi_lanes - 2] << 3); +#endif + + /* horizontal porch params */ + regmap_write(adv7511->regmap_cec, 0x28, mode->htotal >> 4); + regmap_write(adv7511->regmap_cec, 0x29, (mode->htotal << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x2a, hsw >> 4); + regmap_write(adv7511->regmap_cec, 0x2b, (hsw << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x2c, hfp >> 4); + regmap_write(adv7511->regmap_cec, 0x2d, (hfp << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x2e, hbp >> 4); + regmap_write(adv7511->regmap_cec, 0x2f, (hbp << 4) & 0xff); + + /* vertical porch params */ + regmap_write(adv7511->regmap_cec, 0x30, mode->vtotal >> 4); + regmap_write(adv7511->regmap_cec, 0x31, (mode->vtotal << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x32, vsw >> 4); + regmap_write(adv7511->regmap_cec, 0x33, (vsw << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x34, vfp >> 4); + regmap_write(adv7511->regmap_cec, 0x35, (vfp << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x36, vbp >> 4); + regmap_write(adv7511->regmap_cec, 0x37, (vbp << 4) & 0xff); +} + +static void hikey970_hack_dpms(struct adv7511 *adv7511) +{ + +DRM_ERROR("Calling %s\n", __func__); + + struct mipi_dsi_device *dsi = adv7511->dsi; + + adv7511_dsi_config_tgen(adv7511); + + /* set number of dsi lanes */ + regmap_write(adv7511->regmap_cec, 0x1c, dsi->lanes << 4); + +#ifdef TEST_COLORBAR_DISPLAY + /* reset internal timing generator */ + regmap_write(adv7511->regmap_cec, 0x27, 0xcb); + regmap_write(adv7511->regmap_cec, 0x27, 0x8b); + regmap_write(adv7511->regmap_cec, 0x27, 0xcb); +#else + /* disable internal timing generator */ + regmap_write(adv7511->regmap_cec, 0x27, 0x0b); +#endif + + + /* enable hdmi */ + regmap_write(adv7511->regmap_cec, 0x03, 0x89); +#ifdef TEST_COLORBAR_DISPLAY + /*enable test mode */ + regmap_write(adv7511->regmap_cec, 0x55, 0x80);//display colorbar +#else + /* disable test mode */ + regmap_write(adv7511->regmap_cec, 0x55, 0x00); +#endif + /* disable test mode */ + //regmap_write(adv7511->regmap_cec, 0x55, 0x00); + /* SPD */ + { + static const unsigned char spd_if[] = { + 0x83, 0x01, 25, 0x00, + 'L', 'i', 'n', 'a', 'r', 'o', 0, 0, + '9', '6', 'b', 'o', 'a', 'r', 'd', 's', + ':', 'H', 'i', 'k', 'e', 'y', 0, 0, + }; + int n; + + for (n = 0; n < sizeof(spd_if); n++) + regmap_write(adv7511->regmap_packet, n, spd_if[n]); + + /* enable send SPD */ + regmap_update_bits(adv7511->regmap, 0x40, BIT(6), BIT(6)); + } + + /* force audio */ + /* hide Audio infoframe updates */ + regmap_update_bits(adv7511->regmap, 0x4a, BIT(5), BIT(5)); + + /* i2s, internal mclk, mclk-256 */ + regmap_update_bits(adv7511->regmap, 0x0a, 0x1f, 1); + regmap_update_bits(adv7511->regmap, 0x0b, 0xe0, 0); + /* enable i2s, use i2s format, sample rate from i2s */ + regmap_update_bits(adv7511->regmap, 0x0c, 0xc7, BIT(2)); + /* 16 bit audio */ + regmap_update_bits(adv7511->regmap, 0x0d, 0xff, 16); + /* 16-bit audio */ + regmap_update_bits(adv7511->regmap, 0x14, 0x0f, 2 << 4); + /* 48kHz */ + regmap_update_bits(adv7511->regmap, 0x15, 0xf0, 2 << 4); + /* enable N/CTS, enable Audio sample packets */ + regmap_update_bits(adv7511->regmap, 0x44, BIT(5), BIT(5)); + /* N = 6144 */ + regmap_write(adv7511->regmap, 1, (6144 >> 16) & 0xf); + regmap_write(adv7511->regmap, 2, (6144 >> 8) & 0xff); + regmap_write(adv7511->regmap, 3, (6144) & 0xff); + /* automatic cts */ + regmap_update_bits(adv7511->regmap, 0x0a, BIT(7), 0); + /* enable N/CTS */ + regmap_update_bits(adv7511->regmap, 0x44, BIT(6), BIT(6)); + /* not copyrighted */ + regmap_update_bits(adv7511->regmap, 0x12, BIT(5), BIT(5)); + + /* left source */ + regmap_update_bits(adv7511->regmap, 0x0e, 7 << 3, 0); + /* right source */ + regmap_update_bits(adv7511->regmap, 0x0e, 7 << 0, 1); + /* number of channels: sect 4.5.4: set to 0 */ + regmap_update_bits(adv7511->regmap, 0x73, 7, 1); + /* number of channels: sect 4.5.4: set to 0 */ + regmap_update_bits(adv7511->regmap, 0x73, 0xf0, 1 << 4); + /* sample rate: 48kHz */ + regmap_update_bits(adv7511->regmap, 0x74, 7 << 2, 3 << 2); + /* channel allocation reg: sect 4.5.4: set to 0 */ + regmap_update_bits(adv7511->regmap, 0x76, 0xff, 0); + /* enable audio infoframes */ + regmap_update_bits(adv7511->regmap, 0x44, BIT(3), BIT(3)); + + /* AV mute disable */ + regmap_update_bits(adv7511->regmap, 0x4b, BIT(7) | BIT(6), BIT(7)); + + /* use Audio infoframe updated info */ + regmap_update_bits(adv7511->regmap, 0x4a, BIT(5), 0); +} + static void __adv7511_power_on(struct adv7511 *adv7511) { adv7511->current_edid_segment = -1; @@ -367,8 +514,10 @@ static void adv7511_power_on(struct adv7511 *adv7511) */ regcache_sync(adv7511->regmap); - if (adv7511->type == ADV7533 || adv7511->type == ADV7535) + if (adv7511->type == ADV7533 || adv7511->type == ADV7535) { adv7533_dsi_power_on(adv7511); + hikey970_hack_dpms(adv7511); + } adv7511->powered = true; } @@ -981,8 +1130,10 @@ static int adv7511_init_regulators(struct adv7511 *adv) adv->supplies[i].supply = supply_names[i]; ret = devm_regulator_bulk_get(dev, adv->num_supplies, adv->supplies); - if (ret) + if (ret) { + dev_err(dev, "Can't get all regulators\n"); return ret; + } return regulator_bulk_enable(adv->num_supplies, adv->supplies); } @@ -1022,6 +1173,14 @@ static const struct regmap_config adv7511_cec_regmap_config = { .volatile_reg = adv7511_cec_register_volatile, }; +static const struct regmap_config adv7533_packet_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + + .max_register = 0xff, + .cache_type = REGCACHE_RBTREE, +}; + static int adv7511_init_cec_regmap(struct adv7511 *adv) { int ret; @@ -1045,6 +1204,14 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv) goto err; } + adv->regmap_packet = devm_regmap_init_i2c(adv->i2c_packet, + &adv7533_packet_regmap_config); + if (IS_ERR(adv->regmap_packet)) { + ret = PTR_ERR(adv->regmap_packet); + goto err; + } + + return 0; err: i2c_unregister_device(adv->i2c_cec); @@ -1168,8 +1335,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) return ret; ret = adv7511_init_regulators(adv7511); - if (ret != -EPROBE_DEFER) { - dev_err(dev, "failed to init regulators\n"); + if (ret && ret != -EPROBE_DEFER) { + dev_err(dev, "failed to init regulators: %d\n", ret); return ret; } _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.3 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A39D0C433E1 for ; Thu, 20 Aug 2020 06:34:25 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 73D5420786 for ; Thu, 20 Aug 2020 06:34:25 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="f+Tx8ZeG" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 73D5420786 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id ADEA86E8B0; Thu, 20 Aug 2020 06:34:24 +0000 (UTC) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by gabe.freedesktop.org (Postfix) with ESMTPS id 17BA26E8AF for ; Thu, 20 Aug 2020 06:34:23 +0000 (UTC) Received: from coco.lan (ip5f5ad5a3.dynamic.kabel-deutschland.de [95.90.213.163]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 4F6AF2076E; Thu, 20 Aug 2020 06:34:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1597905262; bh=BfHozWh6kPOwVZnZ5alNRgHDl/8qIGOcbL0zFhOlxr4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=f+Tx8ZeGGaMHlAiE9gFg/CKrAKL3lsTdxURn32tRpWkKB/KYx3sDACAmO+S92wdL0 e2msrlg6vEkzmEAhy55+sRXIFwphAUjD2/CbcfK+qSGveJr6YZuniPlAWlVkJG8pLX xPZ+GETDlIXsbMoHo3LJOmrvLBFIWvxcEVUc7ztE= Date: Thu, 20 Aug 2020 08:34:11 +0200 From: Mauro Carvalho Chehab To: John Stultz Subject: Re: [PATCH 00/49] DRM driver for Hikey 970 Message-ID: <20200820083411.666a4d13@coco.lan> In-Reply-To: References: X-Mailer: Claws Mail 3.17.5 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Neil Armstrong , Xinliang Liu , Wanchun Zheng , linuxarm@huawei.com, dri-devel , Andrzej Hajda , Laurent Pinchart , Sam Ravnborg , driverdevel , Daniel Borkmann , John Fastabend , Xiubin Zhang , Wei Xu , David Airlie , Xinwei Kong , Tomi Valkeinen , Bogdan Togorean , Laurentiu Palcu , linux-media , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Liwei Cai , Jesper Dangaard Brouer , Manivannan Sadhasivam , Chen Feng , Alexei Starovoitov , "moderated list:DMA BUFFER SHARING FRAMEWORK" , Rob Herring , Jakub Kicinski , mauro.chehab@huawei.com, Rob Clark , linux-arm-kernel , Greg Kroah-Hartman , lkml , Liuyao An , Network Development , Rongrong Zou , BPF Mailing List , "David S. Miller" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Em Wed, 19 Aug 2020 14:13:05 -0700 John Stultz escreveu: > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab > wrote: > > Yet, I'm submitting it via staging due to the following reasons: > > > > - It depends on the LDO3 power supply, which is provided by > > a regulator driver that it is currently on staging; > > - Due to legal reasons, I need to preserve the authorship of > > each one responsbile for each patch. So, I need to start from > > the original patch from Kernel 4.4; > > - There are still some problems I need to figure out how to solve: > > - The adv7535 can't get EDID data. Maybe it is a timing issue, > > but it requires more research to be sure about how to solve it; > > I've seen this on the HiKey960 as well. There is a patch to the > adv7533 driver I have to add a mdelay that seems to consistently > resolve the timing problem. At some point I mentioned it to one of > the maintainers who seems open to having it added, but it seemed silly > to submit it until there was a upstream driver that needed such a > change. So I think that patch can be submitted as a follow on to this > (hopefully cleaned up) series. Yeah, I saw that mdelay() patch on your tree. While it should be cheap to add a mdelay() or udelay_range() there, I'm not sure if this is just a timing issue or if something else is missing. The 4.9 driver does some extra setups on adv7535 (see the enclosed patch). I'm wondering if something from that is missing. Btw, IMHO, one interesting setting on the downstream code is support for colorbar test. This was helpful when I was making this driver work upstream, as it could be useful for someone trying to make some other DRM driver using it. > > > - The driver only accept resolutions on a defined list, as there's > > a known bug that this driver may have troubles with random > > resolutions. Probably due to a bug at the pixel clock settings; > > So, yes, the SoC clks can't generate proper signals for HDMI > frequencies (apparently it's not an issue for panels). There is a > fixed set that we can get "close enough" that most monitors will work, > but its always a bit iffy (some monitors are strict in what they > take). There is an extra logic for Kirin 620 that seems to be validating the frequencies that would be set to the clocks. If they're out of the range, it would return MODE_BAD. I suspect that something similar would be needed for Kirin 970 and 960. With that regards, 970 is different from 960. I actually tried to write a patch for it, but I guess there are still some things missing on my code, for 970. This patch: https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commit/4614415770b33e27a9f15c7dde20895fb750592f Splits the part of the code which calculates the clk_Hz from the part which sets it. So, now, dss_calculate_clock() checks the pixel clock frequency and adjusts it, if needed. I have another patch that were checking if the code modified the clock_Hz at dss_crtc_mode_fixup(). With that, it would be easy to return MODE_INVALID if something bad happens. However, such patch didn't work as I would expect, so I ended dropping it. > On the kirin driver, we were able to do a calculation to figure out if > the generated frequency would be close enough: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c#n615 > > I suspect we could do something similar for the hikey960/70, but I've > not really had time to dig in deeply there. Yeah, I tried something similar to that. Maybe it will work properly for Hikey 960, but Hikey 970 has a much more complex code to setup the pixel clock. On 960, just one clock is set: clk_set_rate(ctx->dss_pxl0_clk, clk_Hz); While, on 970, pxl0 is always set to the same frequency (144 MHz), at least for pixel clocks up to 255 MHz[1], but the code do some complex calculus to setup PLL7 registers using the clock frequency. [1] pixel clocks above 255 MHz will be rejected anyway, as they'll return MODE_BAD due to the checks if a mode is valid. > > Personally, I don't see the allow-list as a problematic short term > solution, and again, not sure its worth pushing to staging for. Yeah, I guess we can live with that for a while. > > > - Sometimes (at least with 1080p), it generates LDI underflow > > errors, which in turn causes the DRM to stop working. That > > happens for example when using gdm on Wayland and > > gnome on X11; > > Interestingly, I've not seen this on HiKey960 (at least with > Android/Surfaceflinger). Here, it happens all the time when the monitor returns back from DPMS suspend. It also happens on some specific cases (X11 x Wayland), console mode + startx, etc. It sounds to me that it occurs when the driver tries to setup a new resolution. Maybe something needs to be disabled before changing res (and re-enabled afterwards). > The original HiKey board does have the > trouble where at 1080p the screen sometimes comes up horizontally > offset due to the LDI underflow, but the patches to address it have > been worse then the problem, so we reverted those. The "visible" effect on my monitor is that the image disappears, and the monitor complains that there's an "Invalid Mode" at the HDMI signal. > > > - Probably related to the previous issue, when the monitor > > suspends due to DPMS, it doesn't return back to life. > > > > I don't believe I see this on HiKey960. But if it's the LDI issue on > the 970 that may explain it. Yes, when this happens, I get LDI underflow messages. > > So, IMO, the best is to keep it on staging for a while, until those > > remaining bugs gets solved. > > I'm not sure I see all of these as compelling for pushing it in via > staging. And I suspect in the process of submitting the patches for > review folks may find the cause of some of the problems you list here. Yeah, I hope we can get rid of at least part of those during the review process ;-) Yet, I'd like to preserve the history. As the enclosed patch shows, maybe there are some hidden gems at the previously existing code. I may also have done something wrong when converting the driver. So, preserving the history seems to be a good idea to me. One of the advantages of using staging is that the full history will be preserved at the incremental code at staging, while the final patch moving the code out of staging will have all the changes folded together into a single patch. That makes easier for people to do a final review on it. Also, some subsystem maintainers don't like the idea of merging the entire history on their subsystems. > Nit: This is a display driver and has little to do with the GPU (other > then it will eventually live in drivers/gpu/drm/...), so I might > suggest using more conventional subject prefix, "drm: hisilicon:" Ah, OK! I'll change the prefix at the next versions of this series. Thanks, Mauro HACK: Some changes from downstream to adv7511 code diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index a9bb734366ae..1440c08caf42 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -333,6 +333,7 @@ struct adv7511 { struct regmap *regmap; struct regmap *regmap_cec; + struct regmap *regmap_packet; enum drm_connector_status status; bool powered; diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index d59b37a2ae23..c9fbcb40f962 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -325,6 +325,153 @@ static void adv7511_set_link_config(struct adv7511 *adv7511, adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB; } +static void adv7511_dsi_config_tgen(struct adv7511 *adv7511) +{ + struct drm_display_mode *mode = &adv7511->curr_mode; + unsigned int hsw, hfp, hbp, vsw, vfp, vbp; +#ifndef TEST_COLORBAR_DISPLAY + u8 clock_div_by_lanes[] = { 6, 4, 3 }; /* 2, 3, 4 lanes */ +#endif + + hsw = mode->hsync_end - mode->hsync_start; + hfp = mode->hsync_start - mode->hdisplay; + hbp = mode->htotal - mode->hsync_end; + vsw = mode->vsync_end - mode->vsync_start; + vfp = mode->vsync_start - mode->vdisplay; + vbp = mode->vtotal - mode->vsync_end; + +#ifdef TEST_COLORBAR_DISPLAY + /* set pixel clock auto mode */ + regmap_write(adv7511->regmap_cec, 0x16, + 0x00); +#else + /* set pixel clock divider mode */ + regmap_write(adv7511->regmap_cec, 0x16, + clock_div_by_lanes[adv7511->num_dsi_lanes - 2] << 3); +#endif + + /* horizontal porch params */ + regmap_write(adv7511->regmap_cec, 0x28, mode->htotal >> 4); + regmap_write(adv7511->regmap_cec, 0x29, (mode->htotal << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x2a, hsw >> 4); + regmap_write(adv7511->regmap_cec, 0x2b, (hsw << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x2c, hfp >> 4); + regmap_write(adv7511->regmap_cec, 0x2d, (hfp << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x2e, hbp >> 4); + regmap_write(adv7511->regmap_cec, 0x2f, (hbp << 4) & 0xff); + + /* vertical porch params */ + regmap_write(adv7511->regmap_cec, 0x30, mode->vtotal >> 4); + regmap_write(adv7511->regmap_cec, 0x31, (mode->vtotal << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x32, vsw >> 4); + regmap_write(adv7511->regmap_cec, 0x33, (vsw << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x34, vfp >> 4); + regmap_write(adv7511->regmap_cec, 0x35, (vfp << 4) & 0xff); + regmap_write(adv7511->regmap_cec, 0x36, vbp >> 4); + regmap_write(adv7511->regmap_cec, 0x37, (vbp << 4) & 0xff); +} + +static void hikey970_hack_dpms(struct adv7511 *adv7511) +{ + +DRM_ERROR("Calling %s\n", __func__); + + struct mipi_dsi_device *dsi = adv7511->dsi; + + adv7511_dsi_config_tgen(adv7511); + + /* set number of dsi lanes */ + regmap_write(adv7511->regmap_cec, 0x1c, dsi->lanes << 4); + +#ifdef TEST_COLORBAR_DISPLAY + /* reset internal timing generator */ + regmap_write(adv7511->regmap_cec, 0x27, 0xcb); + regmap_write(adv7511->regmap_cec, 0x27, 0x8b); + regmap_write(adv7511->regmap_cec, 0x27, 0xcb); +#else + /* disable internal timing generator */ + regmap_write(adv7511->regmap_cec, 0x27, 0x0b); +#endif + + + /* enable hdmi */ + regmap_write(adv7511->regmap_cec, 0x03, 0x89); +#ifdef TEST_COLORBAR_DISPLAY + /*enable test mode */ + regmap_write(adv7511->regmap_cec, 0x55, 0x80);//display colorbar +#else + /* disable test mode */ + regmap_write(adv7511->regmap_cec, 0x55, 0x00); +#endif + /* disable test mode */ + //regmap_write(adv7511->regmap_cec, 0x55, 0x00); + /* SPD */ + { + static const unsigned char spd_if[] = { + 0x83, 0x01, 25, 0x00, + 'L', 'i', 'n', 'a', 'r', 'o', 0, 0, + '9', '6', 'b', 'o', 'a', 'r', 'd', 's', + ':', 'H', 'i', 'k', 'e', 'y', 0, 0, + }; + int n; + + for (n = 0; n < sizeof(spd_if); n++) + regmap_write(adv7511->regmap_packet, n, spd_if[n]); + + /* enable send SPD */ + regmap_update_bits(adv7511->regmap, 0x40, BIT(6), BIT(6)); + } + + /* force audio */ + /* hide Audio infoframe updates */ + regmap_update_bits(adv7511->regmap, 0x4a, BIT(5), BIT(5)); + + /* i2s, internal mclk, mclk-256 */ + regmap_update_bits(adv7511->regmap, 0x0a, 0x1f, 1); + regmap_update_bits(adv7511->regmap, 0x0b, 0xe0, 0); + /* enable i2s, use i2s format, sample rate from i2s */ + regmap_update_bits(adv7511->regmap, 0x0c, 0xc7, BIT(2)); + /* 16 bit audio */ + regmap_update_bits(adv7511->regmap, 0x0d, 0xff, 16); + /* 16-bit audio */ + regmap_update_bits(adv7511->regmap, 0x14, 0x0f, 2 << 4); + /* 48kHz */ + regmap_update_bits(adv7511->regmap, 0x15, 0xf0, 2 << 4); + /* enable N/CTS, enable Audio sample packets */ + regmap_update_bits(adv7511->regmap, 0x44, BIT(5), BIT(5)); + /* N = 6144 */ + regmap_write(adv7511->regmap, 1, (6144 >> 16) & 0xf); + regmap_write(adv7511->regmap, 2, (6144 >> 8) & 0xff); + regmap_write(adv7511->regmap, 3, (6144) & 0xff); + /* automatic cts */ + regmap_update_bits(adv7511->regmap, 0x0a, BIT(7), 0); + /* enable N/CTS */ + regmap_update_bits(adv7511->regmap, 0x44, BIT(6), BIT(6)); + /* not copyrighted */ + regmap_update_bits(adv7511->regmap, 0x12, BIT(5), BIT(5)); + + /* left source */ + regmap_update_bits(adv7511->regmap, 0x0e, 7 << 3, 0); + /* right source */ + regmap_update_bits(adv7511->regmap, 0x0e, 7 << 0, 1); + /* number of channels: sect 4.5.4: set to 0 */ + regmap_update_bits(adv7511->regmap, 0x73, 7, 1); + /* number of channels: sect 4.5.4: set to 0 */ + regmap_update_bits(adv7511->regmap, 0x73, 0xf0, 1 << 4); + /* sample rate: 48kHz */ + regmap_update_bits(adv7511->regmap, 0x74, 7 << 2, 3 << 2); + /* channel allocation reg: sect 4.5.4: set to 0 */ + regmap_update_bits(adv7511->regmap, 0x76, 0xff, 0); + /* enable audio infoframes */ + regmap_update_bits(adv7511->regmap, 0x44, BIT(3), BIT(3)); + + /* AV mute disable */ + regmap_update_bits(adv7511->regmap, 0x4b, BIT(7) | BIT(6), BIT(7)); + + /* use Audio infoframe updated info */ + regmap_update_bits(adv7511->regmap, 0x4a, BIT(5), 0); +} + static void __adv7511_power_on(struct adv7511 *adv7511) { adv7511->current_edid_segment = -1; @@ -367,8 +514,10 @@ static void adv7511_power_on(struct adv7511 *adv7511) */ regcache_sync(adv7511->regmap); - if (adv7511->type == ADV7533 || adv7511->type == ADV7535) + if (adv7511->type == ADV7533 || adv7511->type == ADV7535) { adv7533_dsi_power_on(adv7511); + hikey970_hack_dpms(adv7511); + } adv7511->powered = true; } @@ -981,8 +1130,10 @@ static int adv7511_init_regulators(struct adv7511 *adv) adv->supplies[i].supply = supply_names[i]; ret = devm_regulator_bulk_get(dev, adv->num_supplies, adv->supplies); - if (ret) + if (ret) { + dev_err(dev, "Can't get all regulators\n"); return ret; + } return regulator_bulk_enable(adv->num_supplies, adv->supplies); } @@ -1022,6 +1173,14 @@ static const struct regmap_config adv7511_cec_regmap_config = { .volatile_reg = adv7511_cec_register_volatile, }; +static const struct regmap_config adv7533_packet_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + + .max_register = 0xff, + .cache_type = REGCACHE_RBTREE, +}; + static int adv7511_init_cec_regmap(struct adv7511 *adv) { int ret; @@ -1045,6 +1204,14 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv) goto err; } + adv->regmap_packet = devm_regmap_init_i2c(adv->i2c_packet, + &adv7533_packet_regmap_config); + if (IS_ERR(adv->regmap_packet)) { + ret = PTR_ERR(adv->regmap_packet); + goto err; + } + + return 0; err: i2c_unregister_device(adv->i2c_cec); @@ -1168,8 +1335,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) return ret; ret = adv7511_init_regulators(adv7511); - if (ret != -EPROBE_DEFER) { - dev_err(dev, "failed to init regulators\n"); + if (ret && ret != -EPROBE_DEFER) { + dev_err(dev, "failed to init regulators: %d\n", ret); return ret; } _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel