Re: [PATCH 00/49] DRM driver for Hikey 970
Le mardi 25 août 2020 à 13:30 +0200, Mauro Carvalho Chehab a écrit : > Em Tue, 25 Aug 2020 05:29:29 +1000 > Dave Airlie escreveu: > > > On Thu, 20 Aug 2020 at 20:02, Laurent Pinchart > > wrote: > > > Hi Mauro, > > > > > > On Thu, Aug 20, 2020 at 09:03:26AM +0200, Mauro Carvalho Chehab wrote: > > > > Em Wed, 19 Aug 2020 12:52:06 -0700 John Stultz escreveu: > > > > > On Wed, Aug 19, 2020 at 8:31 AM Laurent Pinchart wrote: > > > > > > On Wed, Aug 19, 2020 at 05:21:20PM +0200, Sam Ravnborg wrote: > > > > > > > On Wed, Aug 19, 2020 at 01:45:28PM +0200, Mauro Carvalho Chehab > > > > > > > wrote: > > > > > > > > This patch series port the out-of-tree driver for Hikey 970 > > > > > > > > (which > > > > > > > > should also support Hikey 960) from the official 96boards tree: > > > > > > > > > > > > > > > >https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > > > > > > > > > > > > > > > Based on his history, this driver seems to be originally written > > > > > > > > for Kernel 4.4, and was later ported to Kernel 4.9. The original > > > > > > > > driver used to depend on ION (from Kernel 4.4) and had its own > > > > > > > > implementation for FB dev API. > > > > > > > > > > > > > > > > As I need to preserve the original history (with has patches > > > > > > > > from > > > > > > > > both HiSilicon and from Linaro), I'm starting from the original > > > > > > > > patch applied there. The remaining patches are incremental, > > > > > > > > and port this driver to work with upstream Kernel. > > > > > > > > > > > > > ... > > > > > > > > - 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; > > > > > ... > > > > > > > I do acknowledge you need to preserve history and all - > > > > > > > but this patchset is not easy to review. > > > > > > > > > > > > Why do we need to preserve history ? Adding relevant Signed-off-by > > > > > > and > > > > > > Co-developed-by should be enough, shouldn't it ? Having a public > > > > > > branch > > > > > > that contains the history is useful if anyone is interested, but I > > > > > > don't > > > > > > think it's required in mainline. > > > > > > > > > > Yea. I concur with Laurent here. I'm not sure what legal reasoning you > > > > > have on this but preserving the "absolute" history here is actively > > > > > detrimental for review and understanding of the patch set. > > > > > > > > > > Preserving Authorship, Signed-off-by lines and adding Co-developed-by > > > > > lines should be sufficient to provide both atribution credit and DCO > > > > > history. > > > > > > > > I'm not convinced that, from legal standpoint, folding things would > > > > be enough. See, there are at least 3 legal systems involved here > > > > among the different patch authors: > > > > > > > > - civil law; > > > > - common law; > > > > - customary law + common law. > > > > > > > > Merging stuff altogether from different law systems can be problematic, > > > > and trying to discuss this with experienced IP property lawyers will > > > > for sure take a lot of time and efforts. I also bet that different > > > > lawyers will have different opinions, because laws are subject to > > > > interpretation. With that matter I'm not aware of any court rules > > > > with regards to folded patches. So, it sounds to me that folding > > > > patches is something that has yet to be proofed in courts around > > > > the globe. > > > > > > > > At least for US legal system, it sounds that the Country of > > > > origin of a patch is relevant, as they have a concept of > > > > "national technology" that can be subject to export regulations. > > > > > > > > From my side, I really prefer to play safe and stay out of any such > > > > legal discussions. > > > > > > Let's be serious for a moment. If you think there are legal issues in > > > taking GPL-v2.0-only patches and squashing them while retaining > > > authorship information through tags, the Linux kernel if *full* of that. > > > You also routinely modify patches that you commit to the media subsystem > > > to fix "small issues". > > > > > > The country of origin argument makes no sense either, the kernel code > > > base if full of code coming from pretty much all country on the planet. > > > > > > Keeping the patches separate make this hard to review. Please squash > > > them. > > > > I'm inclined to agree with Laurent here. > > > > Patches submitted as GPL-v2 with DCO lines and author names/companies > > should be fine to be squashed and rearranged, > > as long as the DCO and Authorship is kept somewhere in the new patch > > that is applied. > > > > Review is more important here. > > Sorry, but I can't agree that review is more important than to be able > to properly indicate copyrights in a valid way at the legal systems that > it would apply ;-) Regardless of the
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro.
Laurent and I discussed this driver a little on irc.
Some highlights:
This parts could use register names:
+ writel(0x2, noc_dss_base + 0xc);
+ writel(0x2, noc_dss_base + 0x8c);
+ writel(0x2, noc_dss_base + 0x10c);
+ writel(0x2, noc_dss_base + 0x18c);
The two nodes in the DT for DPE and DSI uses overlapping range for reg
entries. It looks like a syscon node or some iommu thing is needed to do
this properly.
The chain will lok like this:
DPE -> DSI -> video mux -> {adv7533, panel}
But drm_bridge has not yet support for such non-linear setup.
The recommendation is to focus on the HDMI prat. Then we can later
come up with support for a video mux.
The video mux should have a dedicated node with one input node and two
output nodes. Which is also where the gpio should be.
The DSI node references two DPHY instances - should it be PHY driver(s)?
Does the DSI part contain one or two instances. Clocks looks duplicated.
Does the DPE and DSI share a lot of register blocks - or does it just
look like this from a first point of view?
You can read though the logs here:
https://people.freedesktop.org/~cbrill/dri-log/index.php
Could you please try to get back on some of the points above so we can
help you move forward in the right direction.
Thanks,
Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro
> Before posting the big patch series again, let me send the new
> version folded into a single patch.
>
> If you'd like to see the entire thing, I'm placing it here:
>
>
> https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/hikey970_v2/
Review 3/3
For next submission then to ease review it would be nice that the patch
is spilt up a little. Maybe something like:
- HW specific stuff in two patches (header fiels with register + setup
code)
- Display driver files
- DSI driver files
- build stuff (Makefile, Kconfig fragments)
So all splits on file level - which should be easy to do.
This will make it easier for more people to give feedback. It is a bit
much to walk through 4k loc or what the full size is.
And then we can also provide a-b or r-b for fragments so the reviewed
parts can be ignored for later reviews.
For the DSI parts I may be hit by the old "when you have a hammer then
everything looks like a nail syndrome".
In my case the hammer is the bridge interface.
Suggestions:
- Move encoder to display driver
- Move connector creation to display driver (using
drm_bridge_connector_init())
- Use drm_bridge interface for the DSI driver parts
- Maybe use the HDMI bridge driver as a chained driver - I did not look
to close on this
- Use the standard bridge interface to find the bridge and drop use of
the component framework
I hope that some other drm people chime in here.
Either to tell this is non-sense or confirm this is the way to go.
The above does not give any hint how to use the gpio_mux to shift
between the panel and HDMI. This logic needs to be in the display driver
as the bridge driver will only be used for HDMI - as I understand the
code. And I do not know how to do it.
Some details in the following that are not related to the above.
Sam
> diff --git a/drivers/staging/hikey9xx/gpu/kirin9xx_dw_drm_dsi.c
> b/drivers/staging/hikey9xx/gpu/kirin9xx_dw_drm_dsi.c
> new file mode 100644
> index ..a2eed961b7d5
> --- /dev/null
> +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_dw_drm_dsi.c
> @@ -0,0 +1,2126 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DesignWare MIPI DSI Host Controller v1.02 driver
> + *
> + * Copyright (c) 2016 Linaro Limited.
> + * Copyright (c) 2014-2016 Hisilicon Limited.
> + * Copyright (c) 2014-2020, Huawei Technologies Co., Ltd
> + *
> + * Author:
> + *
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Sort
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Sort
> +
> +#include "kirin9xx_dw_dsi_reg.h"
> +#include "kirin9xx_dpe.h"
> +#include "kirin9xx_drm_drv.h"
> +#include "kirin9xx_drm_dpe_utils.h"
Sort
> +
> +#define PHY_REF_CLK_RATE 1920
> +#define PHY_REF_CLK_PERIOD_PS(10 / (PHY_REF_CLK_RATE / 1000))
> +
> +#define encoder_to_dsi(encoder) \
> + container_of(encoder, struct dw_dsi, encoder)
> +#define host_to_dsi(host) \
> + container_of(host, struct dw_dsi, host)
> +#define connector_to_dsi(connector) \
> + container_of(connector, struct dw_dsi, connector)
Move the upcast next to the struct definition.
> +#define DSS_REDUCE(x)((x) > 0 ? ((x) - 1) : (x))
> +
> +enum dsi_output_client {
> + OUT_HDMI = 0,
> + OUT_PANEL,
> + OUT_MAX
> +};
> +
> +struct mipi_phy_params {
I did not check all fiels but I managed to find at least one unused
field. Cheack and delete what is unused.
> + u64 lane_byte_clk;
> + u32 clk_division;
> +
> + u32 clk_lane_lp2hs_time;
> + u32 clk_lane_hs2lp_time;
> + u32 data_lane_lp2hs_time;
> + u32 data_lane_hs2lp_time;
> + u32 clk2data_delay;
> + u32 data2clk_delay;
> +
> + u32 clk_pre_delay;
> + u32 clk_post_delay;
> + u32 clk_t_lpx;
> + u32 clk_t_hs_prepare;
> + u32 clk_t_hs_zero;
> + u32 clk_t_hs_trial;
> + u32 clk_t_wakeup;
> + u32 data_pre_delay;
> + u32 data_post_delay;
> + u32 data_t_lpx;
> + u32 data_t_hs_prepare;
> + u32 data_t_hs_zero;
> + u32 data_t_hs_trial;
> + u32 data_t_ta_go;
> + u32 data_t_ta_get;
> + u32 data_t_wakeup;
> +
> + u32 phy_stop_wait_time;
> +
> + u32 rg_vrefsel_vcm;
> + u32 rg_hstx_ckg_sel;
> + u32 rg_pll_fbd_div5f;
> + u32 rg_pll_fbd_div1f;
> + u32 rg_pll_fbd_2p;
> + u32 rg_pll_enbwt;
> + u32 rg_pll_fbd_p;
> + u32 rg_pll_fbd_s;
> + u32 rg_pll_pre_div1p;
> + u32 rg_pll_pre_p;
> + u32 rg_pll_vco_750m;
> + u32 rg_pll_lpf_rs;
> + u32 rg_pll_lpf_cs;
> + u32 rg_pll_enswc;
> + u32 rg_pll_chp;
> +
Some indent gone wrong here?
> + u32 pll_register_override; /* 0x1E[0] */
> + u32 pll_power_down; /* 0x1E[1] */
> + u32 rg_band_sel;/* 0x1E[2] */
> + u32 rg_phase_gen_en;/* 0x1E[3] */
> + u32 reload_sel;
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro, On Tue, 25 Aug 2020 at 12:30, Mauro Carvalho Chehab wrote: > Sorry, but I can't agree that review is more important than to be able > to properly indicate copyrights in a valid way at the legal systems that > it would apply ;-) The way to properly indicate copyright coverage is to insert a copyright statement in the file. This has been the accepted way of communicating copyright notices since approximately the dawn of time. The value of the 'author' field within a chain of git commits does not have privileged legal value. If what you were saying is true, it would be impossible for any project to copy code from any other project, unless they did git filter-branch and made sure to follow renames too. As others have noted, it would also be impossible for any patches to be developed collaboratively by different copyright holders, or for maintainers to apply changes. This is accepted community practice and has passed signoffs from a million different lawyers and copyright holders. If you wish to break with this and do something different, the onus is on you to provide the community with _specific_ legal advice; if this is accepted, the development model would have to drastically change in the presence of single pieces of code developed by multiple distinct copyright holders. Cheers, Daniel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro, On Tue, Aug 25, 2020 at 01:30:25PM +0200, Mauro Carvalho Chehab wrote: > Em Tue, 25 Aug 2020 05:29:29 +1000 > Dave Airlie escreveu: > > > On Thu, 20 Aug 2020 at 20:02, Laurent Pinchart > > wrote: > > > > > > Hi Mauro, > > > > > > On Thu, Aug 20, 2020 at 09:03:26AM +0200, Mauro Carvalho Chehab wrote: > > > > Em Wed, 19 Aug 2020 12:52:06 -0700 John Stultz escreveu: > > > > > On Wed, Aug 19, 2020 at 8:31 AM Laurent Pinchart wrote: > > > > > > On Wed, Aug 19, 2020 at 05:21:20PM +0200, Sam Ravnborg wrote: > > > > > > > On Wed, Aug 19, 2020 at 01:45:28PM +0200, Mauro Carvalho Chehab > > > > > > > wrote: > > > > > > > > This patch series port the out-of-tree driver for Hikey 970 > > > > > > > > (which > > > > > > > > should also support Hikey 960) from the official 96boards tree: > > > > > > > > > > > > > > > >https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > > > > > > > > > > > > > > > Based on his history, this driver seems to be originally written > > > > > > > > for Kernel 4.4, and was later ported to Kernel 4.9. The original > > > > > > > > driver used to depend on ION (from Kernel 4.4) and had its own > > > > > > > > implementation for FB dev API. > > > > > > > > > > > > > > > > As I need to preserve the original history (with has patches > > > > > > > > from > > > > > > > > both HiSilicon and from Linaro), I'm starting from the original > > > > > > > > patch applied there. The remaining patches are incremental, > > > > > > > > and port this driver to work with upstream Kernel. > > > > > > > > > > > > > ... > > > > > > > > - 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; > > > > > ... > > > > > > > I do acknowledge you need to preserve history and all - > > > > > > > but this patchset is not easy to review. > > > > > > > > > > > > Why do we need to preserve history ? Adding relevant Signed-off-by > > > > > > and > > > > > > Co-developed-by should be enough, shouldn't it ? Having a public > > > > > > branch > > > > > > that contains the history is useful if anyone is interested, but I > > > > > > don't > > > > > > think it's required in mainline. > > > > > > > > > > Yea. I concur with Laurent here. I'm not sure what legal reasoning you > > > > > have on this but preserving the "absolute" history here is actively > > > > > detrimental for review and understanding of the patch set. > > > > > > > > > > Preserving Authorship, Signed-off-by lines and adding Co-developed-by > > > > > lines should be sufficient to provide both atribution credit and DCO > > > > > history. > > > > > > > > I'm not convinced that, from legal standpoint, folding things would > > > > be enough. See, there are at least 3 legal systems involved here > > > > among the different patch authors: > > > > > > > > - civil law; > > > > - common law; > > > > - customary law + common law. > > > > > > > > Merging stuff altogether from different law systems can be problematic, > > > > and trying to discuss this with experienced IP property lawyers will > > > > for sure take a lot of time and efforts. I also bet that different > > > > lawyers will have different opinions, because laws are subject to > > > > interpretation. With that matter I'm not aware of any court rules > > > > with regards to folded patches. So, it sounds to me that folding > > > > patches is something that has yet to be proofed in courts around > > > > the globe. > > > > > > > > At least for US legal system, it sounds that the Country of > > > > origin of a patch is relevant, as they have a concept of > > > > "national technology" that can be subject to export regulations. > > > > > > > > From my side, I really prefer to play safe and stay out of any such > > > > legal discussions. > > > > > > Let's be serious for a moment. If you think there are legal issues in > > > taking GPL-v2.0-only patches and squashing them while retaining > > > authorship information through tags, the Linux kernel if *full* of that. > > > You also routinely modify patches that you commit to the media subsystem > > > to fix "small issues". > > > > > > The country of origin argument makes no sense either, the kernel code > > > base if full of code coming from pretty much all country on the planet. > > > > > > Keeping the patches separate make this hard to review. Please squash > > > them. > > > > I'm inclined to agree with Laurent here. > > > > Patches submitted as GPL-v2 with DCO lines and author names/companies > > should be fine to be squashed and rearranged, > > as long as the DCO and Authorship is kept somewhere in the new patch > > that is applied. > > > > Review is more important here. > > Sorry, but I can't agree that review is more important than to be able > to properly indicate copyrights in a valid way at the legal systems that > it would apply ;-) > > In any case,
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Tue, 25 Aug 2020 05:29:29 +1000 Dave Airlie escreveu: > On Thu, 20 Aug 2020 at 20:02, Laurent Pinchart > wrote: > > > > Hi Mauro, > > > > On Thu, Aug 20, 2020 at 09:03:26AM +0200, Mauro Carvalho Chehab wrote: > > > Em Wed, 19 Aug 2020 12:52:06 -0700 John Stultz escreveu: > > > > On Wed, Aug 19, 2020 at 8:31 AM Laurent Pinchart wrote: > > > > > On Wed, Aug 19, 2020 at 05:21:20PM +0200, Sam Ravnborg wrote: > > > > > > On Wed, Aug 19, 2020 at 01:45:28PM +0200, Mauro Carvalho Chehab > > > > > > wrote: > > > > > > > This patch series port the out-of-tree driver for Hikey 970 (which > > > > > > > should also support Hikey 960) from the official 96boards tree: > > > > > > > > > > > > > >https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > > > > > > > > > > > > > Based on his history, this driver seems to be originally written > > > > > > > for Kernel 4.4, and was later ported to Kernel 4.9. The original > > > > > > > driver used to depend on ION (from Kernel 4.4) and had its own > > > > > > > implementation for FB dev API. > > > > > > > > > > > > > > As I need to preserve the original history (with has patches from > > > > > > > both HiSilicon and from Linaro), I'm starting from the original > > > > > > > patch applied there. The remaining patches are incremental, > > > > > > > and port this driver to work with upstream Kernel. > > > > > > > > > > > ... > > > > > > > - 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; > > > > ... > > > > > > I do acknowledge you need to preserve history and all - > > > > > > but this patchset is not easy to review. > > > > > > > > > > Why do we need to preserve history ? Adding relevant Signed-off-by and > > > > > Co-developed-by should be enough, shouldn't it ? Having a public > > > > > branch > > > > > that contains the history is useful if anyone is interested, but I > > > > > don't > > > > > think it's required in mainline. > > > > > > > > Yea. I concur with Laurent here. I'm not sure what legal reasoning you > > > > have on this but preserving the "absolute" history here is actively > > > > detrimental for review and understanding of the patch set. > > > > > > > > Preserving Authorship, Signed-off-by lines and adding Co-developed-by > > > > lines should be sufficient to provide both atribution credit and DCO > > > > history. > > > > > > I'm not convinced that, from legal standpoint, folding things would > > > be enough. See, there are at least 3 legal systems involved here > > > among the different patch authors: > > > > > > - civil law; > > > - common law; > > > - customary law + common law. > > > > > > Merging stuff altogether from different law systems can be problematic, > > > and trying to discuss this with experienced IP property lawyers will > > > for sure take a lot of time and efforts. I also bet that different > > > lawyers will have different opinions, because laws are subject to > > > interpretation. With that matter I'm not aware of any court rules > > > with regards to folded patches. So, it sounds to me that folding > > > patches is something that has yet to be proofed in courts around > > > the globe. > > > > > > At least for US legal system, it sounds that the Country of > > > origin of a patch is relevant, as they have a concept of > > > "national technology" that can be subject to export regulations. > > > > > > From my side, I really prefer to play safe and stay out of any such > > > legal discussions. > > > > Let's be serious for a moment. If you think there are legal issues in > > taking GPL-v2.0-only patches and squashing them while retaining > > authorship information through tags, the Linux kernel if *full* of that. > > You also routinely modify patches that you commit to the media subsystem > > to fix "small issues". > > > > The country of origin argument makes no sense either, the kernel code > > base if full of code coming from pretty much all country on the planet. > > > > Keeping the patches separate make this hard to review. Please squash > > them. > > I'm inclined to agree with Laurent here. > > Patches submitted as GPL-v2 with DCO lines and author names/companies > should be fine to be squashed and rearranged, > as long as the DCO and Authorship is kept somewhere in the new patch > that is applied. > > Review is more important here. Sorry, but I can't agree that review is more important than to be able to properly indicate copyrights in a valid way at the legal systems that it would apply ;-) In any case, there's an easy way to make the code easy to review: I can write the patches against staging (where it is OK to submit preserving the history) and then add a final patch moving it out of staging. You can then just review the last patch, as it will contain the entire code on it. Another alternative, as I'm already doing with Sam,
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro
> Before posting the big patch series again, let me send the new
> version folded into a single patch.
Review 2/N
The way output_poll_changed is used to set gpio_mux to select between
the panel and the HDMI looks strange. But I do not know if there is a
more correct way to do it. Other DRM people would need to help
here if there is a better way to do it.
I looked briefly af suspend/resume.
I think this would benefit from using drm_mode_config_helper_suspend()
and drm_mode_config_helper_resume() but I did not finalize the anlyzis.
Other than this only some small details in the following.
Sam
> kirin9xx_drm_drv.c b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> new file mode 100644
> index ..61b65f8b1ace
> --- /dev/null
> +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> @@ -0,0 +1,277 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Hisilicon Kirin SoCs drm master driver
> + *
> + * Copyright (c) 2016 Linaro Limited.
> + * Copyright (c) 2014-2016 Hisilicon Limited.
> + * Copyright (c) 2014-2020, Huawei Technologies Co., Ltd
> + * Author:
> + *
> + *
> + */
> +
> +#include
> +#include
> +#include
Sort includes
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Sort includes
> +
> +#include "kirin9xx_dpe.h"
> +#include "kirin9xx_drm_drv.h"
> +
> +static int kirin_drm_kms_cleanup(struct drm_device *dev)
> +{
> + struct kirin_drm_private *priv = to_drm_private(dev);
> +
> + if (priv->fbdev)
> + priv->fbdev = NULL;
> +
> + drm_kms_helper_poll_fini(dev);
> + kirin9xx_dss_drm_cleanup(dev);
> +
> + return 0;
> +}
> +
> +static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
> +{
> + struct kirin_drm_private *priv = to_drm_private(dev);
> +
> + dsi_set_output_client(dev);
> +
> + drm_fb_helper_hotplug_event(priv->fbdev);
> +}
> +
> +static const struct drm_mode_config_funcs kirin_drm_mode_config_funcs = {
> + .fb_create = drm_gem_fb_create,
> + .output_poll_changed = kirin_fbdev_output_poll_changed,
> + .atomic_check = drm_atomic_helper_check,
> + .atomic_commit = drm_atomic_helper_commit,
> +};
> +
> +static int kirin_drm_kms_init(struct drm_device *dev)
> +{
> + long kirin_type;
> + int ret;
> +
> + dev_set_drvdata(dev->dev, dev);
> +
> + ret = drmm_mode_config_init(dev);
> + if (ret)
> + return ret;
> +
> + dev->mode_config.min_width = 0;
> + dev->mode_config.min_height = 0;
> + dev->mode_config.max_width = 2048;
> + dev->mode_config.max_height = 2048;
> + dev->mode_config.preferred_depth = 32;
> +
> + dev->mode_config.funcs = _drm_mode_config_funcs;
> +
> + /* display controller init */
> + kirin_type = (long)of_device_get_match_data(dev->dev);
> + if (WARN_ON(!kirin_type))
> + return -ENODEV;
> +
> + ret = dss_drm_init(dev, kirin_type);
> + if (ret)
> + return ret;
> +
> + /* bind and init sub drivers */
> + ret = component_bind_all(dev->dev, dev);
> + if (ret) {
> + drm_err(dev, "failed to bind all component.\n");
> + return ret;
> + }
> +
> + /* vblank init */
> + ret = drm_vblank_init(dev, dev->mode_config.num_crtc);
> + if (ret) {
> + drm_err(dev, "failed to initialize vblank.\n");
> + return ret;
> + }
> + /* with irq_enabled = true, we can use the vblank feature. */
> + dev->irq_enabled = true;
> +
> + /* reset all the states of crtc/plane/encoder/connector */
> + drm_mode_config_reset(dev);
> +
> + /* init kms poll for handling hpd */
> + drm_kms_helper_poll_init(dev);
> +
> + return 0;
> +}
> +
> +DEFINE_DRM_GEM_CMA_FOPS(kirin_drm_fops);
Move it to right above kirin_drm_driver where it is used
> +
> +static int kirin_drm_connectors_register(struct drm_device *dev)
> +{
> + struct drm_connector_list_iter conn_iter;
> + struct drm_connector *failed_connector;
> + struct drm_connector *connector;
> + int ret;
> +
> + mutex_lock(>mode_config.mutex);
> + drm_connector_list_iter_begin(dev, _iter);
> + drm_for_each_connector_iter(connector, _iter) {
> + ret = drm_connector_register(connector);
> + if (ret) {
> + failed_connector = connector;
> + goto err;
> + }
> + }
> + mutex_unlock(>mode_config.mutex);
> +
> + return 0;
> +
> +err:
> + drm_connector_list_iter_begin(dev, _iter);
> + drm_for_each_connector_iter(connector, _iter) {
> + if (failed_connector == connector)
> + break;
> + drm_connector_unregister(connector);
> + }
> + mutex_unlock(>mode_config.mutex);
> +
> + return ret;
> +}
> +
> +static struct drm_driver kirin_drm_driver = {
> + .driver_features= DRIVER_GEM | DRIVER_MODESET
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro > kirin9xx_fb_panel.h b/drivers/staging/hikey9xx/gpu/kirin9xx_fb_panel.h > new file mode 100644 > index ..a69c20470f1d > --- /dev/null > +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_fb_panel.h This file is not referenced and should be deleted. Sam ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
On Thu, 20 Aug 2020 at 20:02, Laurent Pinchart wrote: > > Hi Mauro, > > On Thu, Aug 20, 2020 at 09:03:26AM +0200, Mauro Carvalho Chehab wrote: > > Em Wed, 19 Aug 2020 12:52:06 -0700 John Stultz escreveu: > > > On Wed, Aug 19, 2020 at 8:31 AM Laurent Pinchart wrote: > > > > On Wed, Aug 19, 2020 at 05:21:20PM +0200, Sam Ravnborg wrote: > > > > > On Wed, Aug 19, 2020 at 01:45:28PM +0200, Mauro Carvalho Chehab wrote: > > > > > > This patch series port the out-of-tree driver for Hikey 970 (which > > > > > > should also support Hikey 960) from the official 96boards tree: > > > > > > > > > > > >https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > > > > > > > > > > > Based on his history, this driver seems to be originally written > > > > > > for Kernel 4.4, and was later ported to Kernel 4.9. The original > > > > > > driver used to depend on ION (from Kernel 4.4) and had its own > > > > > > implementation for FB dev API. > > > > > > > > > > > > As I need to preserve the original history (with has patches from > > > > > > both HiSilicon and from Linaro), I'm starting from the original > > > > > > patch applied there. The remaining patches are incremental, > > > > > > and port this driver to work with upstream Kernel. > > > > > > > > > ... > > > > > > - 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; > > > ... > > > > > I do acknowledge you need to preserve history and all - > > > > > but this patchset is not easy to review. > > > > > > > > Why do we need to preserve history ? Adding relevant Signed-off-by and > > > > Co-developed-by should be enough, shouldn't it ? Having a public branch > > > > that contains the history is useful if anyone is interested, but I don't > > > > think it's required in mainline. > > > > > > Yea. I concur with Laurent here. I'm not sure what legal reasoning you > > > have on this but preserving the "absolute" history here is actively > > > detrimental for review and understanding of the patch set. > > > > > > Preserving Authorship, Signed-off-by lines and adding Co-developed-by > > > lines should be sufficient to provide both atribution credit and DCO > > > history. > > > > I'm not convinced that, from legal standpoint, folding things would > > be enough. See, there are at least 3 legal systems involved here > > among the different patch authors: > > > > - civil law; > > - common law; > > - customary law + common law. > > > > Merging stuff altogether from different law systems can be problematic, > > and trying to discuss this with experienced IP property lawyers will > > for sure take a lot of time and efforts. I also bet that different > > lawyers will have different opinions, because laws are subject to > > interpretation. With that matter I'm not aware of any court rules > > with regards to folded patches. So, it sounds to me that folding > > patches is something that has yet to be proofed in courts around > > the globe. > > > > At least for US legal system, it sounds that the Country of > > origin of a patch is relevant, as they have a concept of > > "national technology" that can be subject to export regulations. > > > > From my side, I really prefer to play safe and stay out of any such > > legal discussions. > > Let's be serious for a moment. If you think there are legal issues in > taking GPL-v2.0-only patches and squashing them while retaining > authorship information through tags, the Linux kernel if *full* of that. > You also routinely modify patches that you commit to the media subsystem > to fix "small issues". > > The country of origin argument makes no sense either, the kernel code > base if full of code coming from pretty much all country on the planet. > > Keeping the patches separate make this hard to review. Please squash > them. I'm inclined to agree with Laurent here. Patches submitted as GPL-v2 with DCO lines and author names/companies should be fine to be squashed and rearranged, as long as the DCO and Authorship is kept somewhere in the new patch that is applied. Review is more important here. Dave. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro.
> Before posting the big patch series again, let me send the new
> version folded into a single patch.
Review 1/N
Lots of small details I missed last time.
A good thing is that there is an opportunity to delete som more code.
Sam
> diff --git a/drivers/staging/hikey9xx/gpu/Kconfig
> b/drivers/staging/hikey9xx/gpu/Kconfig
> new file mode 100644
> index ..8578ca953785
> --- /dev/null
> +++ b/drivers/staging/hikey9xx/gpu/Kconfig
> @@ -0,0 +1,10 @@
> +config DRM_HISI_KIRIN9XX
> + tristate "DRM Support for Hisilicon Kirin9xx series SoCs Platform"
> + depends on DRM && OF && ARM64
> + select DRM_KMS_HELPER
> + select DRM_GEM_CMA_HELPER
> + select DRM_KMS_CMA_HELPER
> + select DRM_MIPI_DSI
> + help
> + Choose this option if you have a HiSilicon Kirin960 or Kirin970.
> + If M is selected the module will be called kirin9xx-drm.
> diff --git a/drivers/staging/hikey9xx/gpu/Makefile
> b/drivers/staging/hikey9xx/gpu/Makefile
> new file mode 100644
> index ..5f7974a95367
> --- /dev/null
> +++ b/drivers/staging/hikey9xx/gpu/Makefile
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +kirin9xx-drm-y := kirin9xx_drm_drv.o \
> + kirin9xx_drm_dss.o \
> + kirin9xx_drm_dpe_utils.o \
> + kirin970_defs.o kirin960_defs.o \
> + kirin9xx_drm_overlay_utils.o
> +
> +obj-$(CONFIG_DRM_HISI_KIRIN9XX) += kirin9xx-drm.o kirin9xx_dw_drm_dsi.o
General comment which is true for many many Makefile's
I have never understood the love of '\'.
Something like this works equally well:
kirin9xx-drm-y := kirin9xx_drm_drv.o kirin9xx_drm_dss.o
kirin9xx-drm-y += kirin9xx_drm_dpe_utils.o kirin970_defs.o
kirin9xx-drm-y += kirin960_defs.o kirin9xx_drm_overlay_utils.o
obj-$(CONFIG_DRM_HISI_KIRIN9XX) += kirin9xx-drm.o kirin9xx_dw_drm_dsi.o
> diff --git a/drivers/staging/hikey9xx/gpu/kirin960_defs.c
> b/drivers/staging/hikey9xx/gpu/kirin960_defs.c
> new file mode 100644
> index ..c5e1ec03c818
> --- /dev/null
> +++ b/drivers/staging/hikey9xx/gpu/kirin960_defs.c
> @@ -0,0 +1,346 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2008-2011, Hisilicon Tech. Co., Ltd. All rights reserved.
> + * Copyright (c) 2008-2020, Huawei Technologies Co., Ltd
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "kirin9xx_drm_dpe_utils.h"
> +#include "kirin9xx_drm_drv.h"
> +#include "kirin960_dpe_reg.h"
All includes blocks should be sorted.
The list of include files looks far too large for this simple file.
Reduce to the relevant include files.
> +
> +static const u32
> kirin960_g_dss_module_base[DSS_CHN_MAX_DEFINE][MODULE_CHN_MAX] = {
> + {
> + /* D0 */
> + MIF_CH0_OFFSET,
> + AIF0_CH0_OFFSET,
> + AIF1_CH0_OFFSET,
> + MCTL_CTL_MUTEX_RCH0,
> + DSS_MCTRL_SYS_OFFSET + MCTL_RCH0_FLUSH_EN,
> + DSS_MCTRL_SYS_OFFSET + MCTL_RCH0_OV_OEN,
> + DSS_MCTRL_SYS_OFFSET + MCTL_RCH0_STARTY,
> + DSS_MCTRL_SYS_OFFSET + MCTL_MOD0_DBG,
> + DSS_RCH_D0_DMA_OFFSET,
> + DSS_RCH_D0_DFC_OFFSET,
> + 0,
> + 0,
> + 0,
> + 0,
> + 0,
> + 0,
> + DSS_RCH_D0_CSC_OFFSET,
> + }, {
...
> + },
> +};
> +
> +static const u32
> kirin960_g_dss_module_ovl_base[DSS_MCTL_IDX_MAX][MODULE_OVL_MAX] = {
> + {
> + DSS_OVL0_OFFSET,
> + DSS_MCTRL_CTL0_OFFSET
> + }, {
> + DSS_OVL1_OFFSET,
> + DSS_MCTRL_CTL1_OFFSET
> + }, {
> + DSS_OVL2_OFFSET,
> + DSS_MCTRL_CTL2_OFFSET,
> + }, {
> + DSS_OVL3_OFFSET,
> + DSS_MCTRL_CTL3_OFFSET,
> + }, {
> + 0,
> + DSS_MCTRL_CTL4_OFFSET,
> + }, {
> + 0,
> + DSS_MCTRL_CTL5_OFFSET,
> + }
> +};
> +
> +/* SCF_LUT_CHN coef_idx */
> +static const int kirin960_g_scf_lut_chn_coef_idx[DSS_CHN_MAX_DEFINE] = {
> + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1
> +};
> +
> +static const u32
> kirin960_g_dss_module_cap[DSS_CHN_MAX_DEFINE][MODULE_CAP_MAX] = {
> + /* D2 */
> + {0, 0, 1, 0, 0, 0, 1, 0, 0, 0, 1},
> + /* D3 */
> + {0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1},
> + /* V0 */
> + {0, 1, 1, 0, 1, 1, 1, 0, 0, 1, 1},
> + /* G0 */
> + {0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0},
> + /* V1 */
> + {0, 1, 1, 1, 0, 1, 1, 0, 1, 1, 1},
> + /* G1 */
> + {0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0},
> + /* D0 */
> + {0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1},
> + /* D1 */
> + {0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1},
> +
> + /* W0 */
> + {1, 0, 1, 0, 0, 0, 0, 1, 0, 1, 1},
> + /* W1 */
> + {1, 0, 1, 0, 0, 0, 0, 1, 0, 1, 1},
> +
> + /* V2 */
> + {0, 1, 1, 1, 0, 1, 1, 0, 1, 1, 1},
> + /* W2 */
>
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Fri, 21 Aug 2020 17:56:50 +0200 Sam Ravnborg escreveu: > Hi Mauro. > > On Fri, Aug 21, 2020 at 04:41:58PM +0200, Mauro Carvalho Chehab wrote: > > Another quick question: > > > > Em Wed, 19 Aug 2020 19:35:58 +0200 > > Sam Ravnborg escreveu: > > > > > > +#define DSS_REDUCE(x) ((x) > 0 ? ((x) - 1) : (x)) > > > Use generic macros for this? > > > > Do you know a generic macro similar to this? Or do you mean adding > > it to include/kernel.h? > > It looked like something there should be a macro for. > But I do not know one. > > And no, do not try to go the kernel.h route on this. > At least not until you see more than one user. Yeah, adding this to kernel.h just for a single usage is overkill. I would be expecting that a non-underflow decrement logic is something that would be used on other places at the Kernel, but identifying this pattern would require some time. Maybe Kernel janitors could write some coccinelle script to replace similar patterns like that into some macro in the future. Thanks, Mauro ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Mon, 24 Aug 2020 08:49:30 +0200
Mauro Carvalho Chehab escreveu:
> Hi John,
>
> Em Wed, 19 Aug 2020 20:28:44 -0700
> John Stultz escreveu:
>
>
> > That said even with the patches I've got on top of your series, I
> > still see a few issues:
> > 1) I'm seeing red-blue swap with your driver. I need to dig a bit to
> > see what the difference is, I know gralloc has a config option for
> > this, and maybe the version of the driver I'm carrying has it wrong?
>
> Maybe it is due to this:
>
> drivers/staging/hikey9xx/gpu/kirin9xx_drm_overlay_utils.c: hal_fmt
> = HISI_FB_PIXEL_FORMAT_BGRA_;/* dss_get_format(fb->pixel_format); */
>
> It sounds to me that someone added a hack hardcoding BGRA_ over
> there.
>
> Btw, I removed the hack, with:
>
>
> diff --git a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_overlay_utils.c
> b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_overlay_utils.c
> index a68db1a27bbf..ba64aae371e4 100644
> --- a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_overlay_utils.c
> +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_overlay_utils.c
> @@ -857,7 +857,7 @@ void hisi_fb_pan_display(struct drm_plane *plane)
> rect.right = src_w - 1;
> rect.top = 0;
> rect.bottom = src_h - 1;
> - hal_fmt = HISI_FB_PIXEL_FORMAT_BGRA_;/*
> dss_get_format(fb->pixel_format); */
> + hal_fmt = dss_get_format(fb->format->format);
>
> DRM_DEBUG_DRIVER("channel%d: src:(%d,%d, %dx%d) crtc:(%d,%d, %dx%d),
> rect(%d,%d,%d,%d),fb:%dx%d, pixel_format=%d, stride=%d, paddr=0x%x,
> bpp=%d.\n",
> chn_idx, src_x, src_y, src_w, src_h,
>
>
> And now red and blue are swapped on my HDMI screen too.
>
> I'll compare this part with your version, but I guess the bug is
> on this logic.
It sounds to me that the Hikey 960 version on your tree has some color
inversion hack, just for ARGB 32 bpp. See:
static const struct kirin_format dpe_formats[] = {
{ DRM_FORMAT_RGB565, DPE_RGB_565 },
{ DRM_FORMAT_BGR565, DPE_BGR_565 },
{ DRM_FORMAT_XRGB, DPE_RGBX_ },
{ DRM_FORMAT_XBGR, DPE_BGRX_ },
{ DRM_FORMAT_RGBA, DPE_RGBA_ },
{ DRM_FORMAT_BGRA, DPE_BGRA_ },
{ DRM_FORMAT_ARGB, DPE_BGRA_ },
{ DRM_FORMAT_ABGR, DPE_RGBA_ },
};
The last two lines are weird, as they're reverting the byte order,
If there's some endiannes issue (which the change from ARGB->ABGR
seems to imply), I would expect to have something similar for the
other formats as well.
I did some tests here: both FB and X11 sets bpp to 24 bits.
Trying to use "startx -- -depth 32" don't work:
"Default Screen Section" for depth/fbbpp 32/32
[ 129.250] (EE) modeset(0): Given depth (32) is not supported by the
driver
Which sounds weird, as the driver announces 32 bit formats.
I suspect that this could be related to the valid modes hack at
the driver.
Btw, there are some color shift also with --depth 16, but replacing
BGR <=> RGB didn't work.
Did you test the different bpp resolutions with the driver on your
tree? The enclosed patch makes 24 bits bpp work here.
Thanks,
Mauro
-
[PATCH] staging: kirin9xx/gpu: rework the colorspace mode setting logic
There are some problems when setting the fourcc KMS:
The original code hardcodes BGRA_. The real issue here seems
to be that the byte order is different than the one for Kirin 620.
Instead of addressing it, the origincla code just used a fixed
mode. A port of the Hikey 960 DPE driver based on Kernel 5.0,
found at:
https://git.linaro.org/people/john.stultz/android-dev.git/tree/drivers/gpu/drm/hisilicon/kirin/kirin_drm_dpe.c?h=dev/hikey960-mainline-WIP
contains a hack that swaps the byte order for 32-bits
ARGB/BRGR (see the last two lines):
{ DRM_FORMAT_RGB565, DPE_RGB_565 },
{ DRM_FORMAT_BGR565, DPE_BGR_565 },
{ DRM_FORMAT_XRGB, DPE_RGBX_ },
{ DRM_FORMAT_XBGR, DPE_BGRX_ },
{ DRM_FORMAT_RGBA, DPE_RGBA_ },
{ DRM_FORMAT_BGRA, DPE_BGRA_ },
{ DRM_FORMAT_ARGB, DPE_BGRA_ },
{ DRM_FORMAT_ABGR, DPE_RGBA_ },
But the same change was not applied to other modesets.
The Hikey 960 port was tested with AOSP, which seems to
be using ABGR format. Here, the chosen fourcc was
XBGR 32 bpp instead. I suspect that the original developer
also found a similar issue and decided to hardcode the
fourcc format.
That's said, currently the drivers uses some code instead of
tables in order to seek for the register settings. The version
from John Stultz tre for Kirin 960 that does a better approach
of using tables instead of code.
I opted to use the same code as the basis for the new logic,
as it makes easier to identify what the driver is actually doing.
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi John,
Em Wed, 19 Aug 2020 20:28:44 -0700
John Stultz escreveu:
> That said even with the patches I've got on top of your series, I
> still see a few issues:
> 1) I'm seeing red-blue swap with your driver. I need to dig a bit to
> see what the difference is, I know gralloc has a config option for
> this, and maybe the version of the driver I'm carrying has it wrong?
Maybe it is due to this:
drivers/staging/hikey9xx/gpu/kirin9xx_drm_overlay_utils.c: hal_fmt
= HISI_FB_PIXEL_FORMAT_BGRA_;/* dss_get_format(fb->pixel_format); */
It sounds to me that someone added a hack hardcoding BGRA_ over
there.
Btw, I removed the hack, with:
diff --git a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_overlay_utils.c
b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_overlay_utils.c
index a68db1a27bbf..ba64aae371e4 100644
--- a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_overlay_utils.c
+++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_overlay_utils.c
@@ -857,7 +857,7 @@ void hisi_fb_pan_display(struct drm_plane *plane)
rect.right = src_w - 1;
rect.top = 0;
rect.bottom = src_h - 1;
- hal_fmt = HISI_FB_PIXEL_FORMAT_BGRA_;/*
dss_get_format(fb->pixel_format); */
+ hal_fmt = dss_get_format(fb->format->format);
DRM_DEBUG_DRIVER("channel%d: src:(%d,%d, %dx%d) crtc:(%d,%d, %dx%d),
rect(%d,%d,%d,%d),fb:%dx%d, pixel_format=%d, stride=%d, paddr=0x%x, bpp=%d.\n",
chn_idx, src_x, src_y, src_w, src_h,
And now red and blue are swapped on my HDMI screen too.
I'll compare this part with your version, but I guess the bug is
on this logic.
Thanks,
Mauro
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
On Wed, 2020-08-19 at 22:48 +0200, Sam Ravnborg wrote: > And sometimes checkpatch is just wrong. I'm interested in examples for when checkpatch is "just wrong". ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro. On Fri, Aug 21, 2020 at 04:41:58PM +0200, Mauro Carvalho Chehab wrote: > Another quick question: > > Em Wed, 19 Aug 2020 19:35:58 +0200 > Sam Ravnborg escreveu: > > > > +#define DSS_REDUCE(x)((x) > 0 ? ((x) - 1) : (x)) > > Use generic macros for this? > > Do you know a generic macro similar to this? Or do you mean adding > it to include/kernel.h? It looked like something there should be a macro for. But I do not know one. And no, do not try to go the kernel.h route on this. At least not until you see more than one user. Sam ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro.
Thanks for the detailed feedabck.
Two comments in the following.
Sam
>
> > > + ctx->dss_pri_clk = devm_clk_get(dev, "clk_edc0");
> > > + if (!ctx->dss_pri_clk) {
> > > + DRM_ERROR("failed to parse dss_pri_clk\n");
> > > + return -ENODEV;
> > > + }
> ...
>
> > I had expected some of these could fail with a PROBE_DEFER.
> > Consider to use the newly introduced dev_probe_err()
>
> Yeah, getting clock lines can fail. I was unable to find dev_probe_err(),
> at least on Kernel 5.9-rc1. I saw this comment:
>
> https://lkml.org/lkml/2020/3/6/356
>
> It sounds it didn't reach upstream. Anyway, I add error handling for the
> the clk_get calls:
>
> ctx->dss_pri_clk = devm_clk_get(dev, "clk_edc0");
> ret = PTR_ERR_OR_ZERO(ctx->dss_pri_clk);
> if (ret == -EPROBE_DEFER) {
> return ret;
> } else if (ret) {
> DRM_ERROR("failed to parse dss_pri_clk: %d\n", ret);
> return ret;
> }
>
> This should be able to detect deferred probe, plus to warn
> about other errors.
I got the name wrong. It is named dev_err_probe(), and was introduced in -rc1.
> > Can the panel stuff be moved out and utilise drm_panel?
>
> I saw the code at drm_panel. The real issue here is that I can't
> test anything related to panel support, as I lack any hardware
> for testing. So, there's a high chance I may end breaking
> something while trying to do that.
I will try to take a look again when you post next revision.
Maybe we should update it and risk that is not works, so whenever
someone try to fix it they do so on top of an up-to-date implmentation.
Lets se and decide later.
Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Another quick question: Em Wed, 19 Aug 2020 19:35:58 +0200 Sam Ravnborg escreveu: > > +#define DSS_REDUCE(x) ((x) > 0 ? ((x) - 1) : (x)) > Use generic macros for this? Do you know a generic macro similar to this? Or do you mean adding it to include/kernel.h? There are the atomic sub ones, but doesn't make sense here. The closest one I found was min_not_zero(), but this would take two args. Btw, I agree that the name here is a bit odd... I would have called such macro as 'dec_not_zero()'. Thanks, Mauro ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Wed, 19 Aug 2020 19:35:58 +0200
Sam Ravnborg escreveu:
> Also a few high level comments:
Hi Sam,
Finally finished addressing the things you pointed, except by a few
ones:
- bridge bindings;
- use drm_foo() instead of DRM_foo() when possible.
A few answers to some of your comments.
> There is too much unused code present - please delete.
> I dod not think I spotted it all.
I'll took a look and dropped most of it. I kept just some
code there related to an optional IOMMU support. The code
there is commented out because they depend on a driver that,
after some upstream discussions, I'm opting to not sending
it. Basically, there are better ways to support the integrated
MMU for the GPU core.
I'added some FIXME and commented out the code, as I intend
to take another look on it in the future. Not top priority,
as the driver works without it, but it could affect driver's
performance.
> Some style issues - ask checkpatch --strict for help identifying
> these.
Ok. There aren't much reported by checkpatch, as I did lots of
checkpatch fixes already in this series. I ended keeping
some CamelCase for macros:
CHECK:CAMELCASE: Avoid CamelCase:
#143: FILE: drivers/staging/hikey9xx/gpu/kirin970_dpe_reg.h:143:
+#define SMMU_SMRx_P(0x1)
and a few other things that, IMHO, making checkpatch happier
would make things worse for humans ;-)
> Needs to be adapted to new bridge handling - see comments.
Ok.
> Move panel stuff to drm_panel (or maybe I got confsed so this was just
> bride stuff).
There used to have a separate panel driver. The DRM driver has
support for it, probably using some older binding model (although
I converted some things that don't work on upstream Kernels anymore).
The old panel driver is there at the history (patch 01/49), but
a later patch drops it. The main thing is that Hikey 970 board
doesn't come with any panel. Maybe are out there some daughter
boards providing a panel display, but I was unable to find it.
So, I ended dropping the panel driver, but keeping the main
driver's code to handle it, as someone could find it useful.
> Lots track a few times - so may have confused myself a few times.
>
> Many small comments - but general impression is good.
Good!
> Happy hacking!
Thanks!
> Sam
>
>
> > diff --git a/drivers/staging/hikey9xx/gpu/Kconfig
> > b/drivers/staging/hikey9xx/gpu/Kconfig
> > new file mode 100644
> > index ..957da13bcf81
> > --- /dev/null
> > +++ b/drivers/staging/hikey9xx/gpu/Kconfig
> > @@ -0,0 +1,22 @@
> > +config DRM_HISI_KIRIN9XX
> > + tristate "DRM Support for Hisilicon Kirin9xx series SoCs Platform"
> > + depends on DRM && OF && ARM64
> > + select DRM_KMS_HELPER
> > + select DRM_GEM_CMA_HELPER
> > + select DRM_KMS_CMA_HELPER
> > + select DRM_MIPI_DSI
> > + help
> > + Choose this option if you have a HiSilicon Kirin960 or Kirin970.
> > + If M is selected the module will be called kirin9xx-drm.
> > +
> > +config DRM_HISI_KIRIN970
> > + bool "Enable support for Hisilicon Kirin970"
> > + depends on DRM_MIPI_DSI
> Implied by DRM_HISI_KIRIN9XX, so not needed.
> > + depends on DRM_HISI_KIRIN9XX
> > + help
> > + Choose this option if you have a hisilicon Kirin chipsets(kirin970).
> > +
> > +config DRM_HISI_KIRIN9XX_DSI
> > + tristate
> > + depends on DRM_HISI_KIRIN9XX
> > + default y
> This is essential a copy of DRM_HISI_KIRIN9XX - so no need for this
> extra Kconfig variable.
The above are left-overs. I'm dropping everything, keeping just
DRM_HISI_KIRIN9XX. The driver now has support for both Kirin 960 and 970
at the same time, without needing an extra Kconfig.
> > + // D0
> Some people dislike the more readable c99 comments.
> I do not recall if coding style allows them
> Ask checkpatch --strict
It used to be forbidden. Linus changed his mind about c99 comments
some time ago. It is now allowed. More than that, using c99 comments is
now mandatory for SPDX on c files.
In any case, changing to /* */ is as easy as running this script:
for i in drivers/staging/hikey9xx/gpu/*.[ch]; do perl -ne 'if (! m,//
SPDX,) { s,//\s*(.*),/* \1 */,; }; print $_' -i $i; done
As I also prefer not using c99 comments, I applied a cleanup patch.
> Some of the enums I checked are not used.
Yeah, based on the size of the header files, when compared with
the size of the code, I suspect that lots of things there won't be
used. Cleaning this could take some time and may end removing
some bits that we could need in the future in order to address
existing problems at the driver.
So, I opted preserving them, at least on this initial
patchset. I intend to do a further cleanup when trying to
merge this driver with the one for Kirin 620. On a side node,
currently, I don't have a Kirin 620 board. So, I'm postponing
such task until I get one such boards, as I'm not willing
to take the risk of changing it without being able to test.
> If their members are used
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Sam, Em Wed, 19 Aug 2020 19:35:58 +0200 Sam Ravnborg escreveu: > > + ret = drm_bridge_attach(encoder, bridge, NULL, 0); > The bridge should be attached with the falg that tell the bridge NOT to > create a connector. > > The display driver shall created the connector. > > Please see how other drivers do this (but most driver uses the old > pattern so so look for drm_bridge_attach() with the flag argument. Not sure if I got what should be done here. >From what I've seen at the DRM code, one of the differences between the display engine for the first Hikey board (Kirin 620 based) and 960/970 is with regards to bridges. The first Hikey device doesn't use any external bridges: both panel and HDMI support are provided by the SoC. The Hikey 960 and 970 boards may either use an external bridge or not. They also have two output connectors: - The first one doesn't use an external bridge. It is used only together with an external daughter display panel board. It sounds that one such panels is this one: https://www.96boards.org/blog/linksprite-hikey-aosp/ I don't have any such board. The OOT driver came with one panel display, I didn't port such driver. - The second one uses an external bridge (adv7535) which is connected to the HDMI board's connector. As there's just one bridge, the driver uses this to find its OF data: struct device_node *bridge_node; bridge_node = of_graph_get_remote_port_parent(endpoint); dsi->bridge = of_drm_find_bridge(bridge_node); Basically, it doesn't call drm_bridge_add(), and doesn't declare any struct drm_bridge_funcs fops, as there's just one bridge that it is always there. - That's said, when I ported the code from Kernel 4.9, I fixed some broken things at the hotplug logic, trying to use other drivers with external bridges as examples. Yet, as you noticed, I ended using some older bridge model. The only other driver I found that doesn't use drm_bridge_add() and doesn't pass 0 as flags is this one: drivers/gpu/drm/omapdrm/omap_drv.c Is it a good example? What I see different there there is that it calls drm_bridge_attach() with: ret = drm_bridge_attach(pipe->encoder, pipe->output->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); Is adding this enough? Or should I do something else? Thanks, Mauro ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro.
Quick feedback below.
Sam
On Thu, Aug 20, 2020 at 05:13:22PM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 20 Aug 2020 16:48:08 +0200
> Sam Ravnborg escreveu:
>
> > Hi Mauro.
> >
> > On Thu, Aug 20, 2020 at 04:06:49PM +0200, Mauro Carvalho Chehab wrote:
> > > Em Wed, 19 Aug 2020 19:35:58 +0200
> > > Sam Ravnborg escreveu:
> > >
> > > I'm already handling the other comments from your review (I'll send a
> > > more complete comment about them after finishing),
> > If you get back only on things you do not understand or do not agree on
> > that would be fine. The rest should be visible in the changelog on the
> > updated patch - no need to do extra work here.
> >
> > > but I have a doubt what you meant about this:
> > >
> > > > +static int kirin_drm_bind(struct device *dev)
> > > > > +{
> > > > > + struct drm_driver *driver = _drm_driver;
> > > > > + struct drm_device *drm_dev;
> > > > > + struct kirin_drm_private *priv;
> > > > > + int ret;
> > > > > +
> > > > > + drm_dev = drm_dev_alloc(driver, dev);
> > > > > + if (!drm_dev)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + ret = kirin_drm_kms_init(drm_dev);
> > > > > + if (ret)
> > > > > + goto err_drm_dev_unref;
> > > > > +
> > > > > + ret = drm_dev_register(drm_dev, 0);
> > > > There is better ways to do this. See drm_drv.c for the code example.
> > >
> > > Not sure if I understood your comment here. The drm_drv.c example also
> > > calls
> > > drm_dev_register().
> >
> > This is indeed not obvious from my comments but what I wnated to say is
> > that the driver should embed drm_device in some struct,
> > maybe in "struct kirin_drm_private".
> >
> > This should also be part of the referenced example.
> >
> > I hope this clarifies it.
>
> Yeah. I was already doing those changes ;-)
>
> Something like the enclosed patch, right?
>
> Btw, I'm not sure if the error handling part is ok, as I didn't check
> what the devm stuff does at the subsystem.
>
> -
>
> On a separate question, I was unable to use the helper macros,
> as it sounds that there's no macro with this:
>
> .dumb_create= drm_gem_cma_dumb_create_internal,
>
> The existing DRM_GEM_CMA_VMAP_DRIVER_OPS uses, instead
> drm_gem_cma_dumb_create(). I'm not sure if this driver can use
> such function instead.
>From the documentation of drm_gem_cma_dumb_create_internal:
* It should not be used directly
* as their _driver.dumb_create callback.
I would expect drm_gem_cma_dumb_create() to be the right choice.
So you can go ahead with DRM_GEM_CMA_VMAP_DRIVER_OPS
(I hope I am right, not the are i know much about)
> staging: hikey9xx/gpu: use drm_managed interface
>
> Use a more modern design for the driver binding logic by
> using drm_managed and getting rid of drm->dev_private.
>
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> index c7736f4d74b7..600c89605cc0 100644
> --- a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> @@ -29,12 +29,13 @@
> #include
> #include
> #include
> +#include
>
> #include "kirin9xx_drm_drv.h"
>
> static int kirin_drm_kms_cleanup(struct drm_device *dev)
> {
> - struct kirin_drm_private *priv = dev->dev_private;
> + struct kirin_drm_private *priv = to_drm_private(dev);
> static struct kirin_dc_ops const *dc_ops;
>
> if (priv->fbdev)
> @@ -45,15 +46,13 @@ static int kirin_drm_kms_cleanup(struct drm_device *dev)
> drm_kms_helper_poll_fini(dev);
> dc_ops->cleanup(dev);
> drm_mode_config_cleanup(dev);
This should also be gone when you are using
drmm_mode_config_init()
> - devm_kfree(dev->dev, priv);
> - dev->dev_private = NULL;
>
> return 0;
> }
>
> static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
> {
> - struct kirin_drm_private *priv = dev->dev_private;
> + struct kirin_drm_private *priv = to_drm_private(dev);
>
> dsi_set_output_client(dev);
>
> @@ -69,18 +68,20 @@ static const struct drm_mode_config_funcs
> kirin_drm_mode_config_funcs = {
>
> static int kirin_drm_kms_init(struct drm_device *dev)
> {
> - struct kirin_drm_private *priv = dev->dev_private;
> + struct kirin_drm_private *priv = to_drm_private(dev);
It is assigned a few lines later.
> static struct kirin_dc_ops const *dc_ops;
> int ret;
>
> - priv = devm_kzalloc(dev->dev, sizeof(*priv), GFP_KERNEL);
> + priv = drmm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> return -ENOMEM;
OK, I am confused here.
This code allocates a struct kirin_drm_private.
But the calling function does the same.
What am I missing here? Coffee?
>
> dev->dev_private = priv;
> dev_set_drvdata(dev->dev, dev);
>
> -
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Thu, 20 Aug 2020 16:48:08 +0200
Sam Ravnborg escreveu:
> Hi Mauro.
>
> On Thu, Aug 20, 2020 at 04:06:49PM +0200, Mauro Carvalho Chehab wrote:
> > Em Wed, 19 Aug 2020 19:35:58 +0200
> > Sam Ravnborg escreveu:
> >
> > I'm already handling the other comments from your review (I'll send a
> > more complete comment about them after finishing),
> If you get back only on things you do not understand or do not agree on
> that would be fine. The rest should be visible in the changelog on the
> updated patch - no need to do extra work here.
>
> > but I have a doubt what you meant about this:
> >
> > > +static int kirin_drm_bind(struct device *dev)
> > > > +{
> > > > + struct drm_driver *driver = _drm_driver;
> > > > + struct drm_device *drm_dev;
> > > > + struct kirin_drm_private *priv;
> > > > + int ret;
> > > > +
> > > > + drm_dev = drm_dev_alloc(driver, dev);
> > > > + if (!drm_dev)
> > > > + return -ENOMEM;
> > > > +
> > > > + ret = kirin_drm_kms_init(drm_dev);
> > > > + if (ret)
> > > > + goto err_drm_dev_unref;
> > > > +
> > > > + ret = drm_dev_register(drm_dev, 0);
> > > There is better ways to do this. See drm_drv.c for the code example.
> >
> > Not sure if I understood your comment here. The drm_drv.c example also
> > calls
> > drm_dev_register().
>
> This is indeed not obvious from my comments but what I wnated to say is
> that the driver should embed drm_device in some struct,
> maybe in "struct kirin_drm_private".
>
> This should also be part of the referenced example.
>
> I hope this clarifies it.
Yeah. I was already doing those changes ;-)
Something like the enclosed patch, right?
Btw, I'm not sure if the error handling part is ok, as I didn't check
what the devm stuff does at the subsystem.
-
On a separate question, I was unable to use the helper macros,
as it sounds that there's no macro with this:
.dumb_create= drm_gem_cma_dumb_create_internal,
The existing DRM_GEM_CMA_VMAP_DRIVER_OPS uses, instead
drm_gem_cma_dumb_create(). I'm not sure if this driver can use
such function instead.
Thanks,
Mauro
staging: hikey9xx/gpu: use drm_managed interface
Use a more modern design for the driver binding logic by
using drm_managed and getting rid of drm->dev_private.
Signed-off-by: Mauro Carvalho Chehab
diff --git a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
index c7736f4d74b7..600c89605cc0 100644
--- a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
+++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
@@ -29,12 +29,13 @@
#include
#include
#include
+#include
#include "kirin9xx_drm_drv.h"
static int kirin_drm_kms_cleanup(struct drm_device *dev)
{
- struct kirin_drm_private *priv = dev->dev_private;
+ struct kirin_drm_private *priv = to_drm_private(dev);
static struct kirin_dc_ops const *dc_ops;
if (priv->fbdev)
@@ -45,15 +46,13 @@ static int kirin_drm_kms_cleanup(struct drm_device *dev)
drm_kms_helper_poll_fini(dev);
dc_ops->cleanup(dev);
drm_mode_config_cleanup(dev);
- devm_kfree(dev->dev, priv);
- dev->dev_private = NULL;
return 0;
}
static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
{
- struct kirin_drm_private *priv = dev->dev_private;
+ struct kirin_drm_private *priv = to_drm_private(dev);
dsi_set_output_client(dev);
@@ -69,18 +68,20 @@ static const struct drm_mode_config_funcs
kirin_drm_mode_config_funcs = {
static int kirin_drm_kms_init(struct drm_device *dev)
{
- struct kirin_drm_private *priv = dev->dev_private;
+ struct kirin_drm_private *priv = to_drm_private(dev);
static struct kirin_dc_ops const *dc_ops;
int ret;
- priv = devm_kzalloc(dev->dev, sizeof(*priv), GFP_KERNEL);
+ priv = drmm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
dev->dev_private = priv;
dev_set_drvdata(dev->dev, dev);
- drm_mode_config_init(dev);
+ ret = drmm_mode_config_init(dev);
+ if (ret)
+ return ret;
dev->mode_config.min_width = 0;
dev->mode_config.min_height = 0;
@@ -94,20 +95,20 @@ static int kirin_drm_kms_init(struct drm_device *dev)
dc_ops = of_device_get_match_data(dev->dev);
ret = dc_ops->init(dev);
if (ret)
- goto err_mode_config_cleanup;
+ return ret;
/* bind and init sub drivers */
ret = component_bind_all(dev->dev, dev);
if (ret) {
DRM_ERROR("failed to bind all component.\n");
- goto err_dc_cleanup;
+ return ret;
}
/* vblank init */
ret = drm_vblank_init(dev, dev->mode_config.num_crtc);
if (ret) {
DRM_ERROR("failed to initialize vblank.\n");
-
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro.
On Thu, Aug 20, 2020 at 04:06:49PM +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 19 Aug 2020 19:35:58 +0200
> Sam Ravnborg escreveu:
>
> I'm already handling the other comments from your review (I'll send a
> more complete comment about them after finishing),
If you get back only on things you do not understand or do not agree on
that would be fine. The rest should be visible in the changelog on the
updated patch - no need to do extra work here.
> but I have a doubt what you meant about this:
>
> > +static int kirin_drm_bind(struct device *dev)
> > > +{
> > > + struct drm_driver *driver = _drm_driver;
> > > + struct drm_device *drm_dev;
> > > + struct kirin_drm_private *priv;
> > > + int ret;
> > > +
> > > + drm_dev = drm_dev_alloc(driver, dev);
> > > + if (!drm_dev)
> > > + return -ENOMEM;
> > > +
> > > + ret = kirin_drm_kms_init(drm_dev);
> > > + if (ret)
> > > + goto err_drm_dev_unref;
> > > +
> > > + ret = drm_dev_register(drm_dev, 0);
> > There is better ways to do this. See drm_drv.c for the code example.
>
> Not sure if I understood your comment here. The drm_drv.c example also calls
> drm_dev_register().
This is indeed not obvious from my comments but what I wnated to say is
that the driver should embed drm_device in some struct,
maybe in "struct kirin_drm_private".
This should also be part of the referenced example.
I hope this clarifies it.
Sam
>
> The only difference is that it calls it inside driver_probe(), while
> on this driver, it uses:
>
> static const struct component_master_ops kirin_drm_ops = {
> .bind = kirin_drm_bind,
> .unbind = kirin_drm_unbind,
> };
>
> static int kirin_drm_platform_probe(struct platform_device *pdev)
> {
> ...
> drm_of_component_match_add(dev, , compare_of, remote);
> ...
> return component_master_add_with_match(dev, _drm_ops, match);
> }
>
> Are you meaning that I should get rid of this component API and call
> kirin_drm_bind() from kirin_drm_platform_probe()?
>
> Thanks,
> Mauro
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Wed, 19 Aug 2020 19:35:58 +0200
Sam Ravnborg escreveu:
I'm already handling the other comments from your review (I'll send a
more complete comment about them after finishing), but I have a doubt
what you meant about this:
> +static int kirin_drm_bind(struct device *dev)
> > +{
> > + struct drm_driver *driver = _drm_driver;
> > + struct drm_device *drm_dev;
> > + struct kirin_drm_private *priv;
> > + int ret;
> > +
> > + drm_dev = drm_dev_alloc(driver, dev);
> > + if (!drm_dev)
> > + return -ENOMEM;
> > +
> > + ret = kirin_drm_kms_init(drm_dev);
> > + if (ret)
> > + goto err_drm_dev_unref;
> > +
> > + ret = drm_dev_register(drm_dev, 0);
> There is better ways to do this. See drm_drv.c for the code example.
Not sure if I understood your comment here. The drm_drv.c example also calls
drm_dev_register().
The only difference is that it calls it inside driver_probe(), while
on this driver, it uses:
static const struct component_master_ops kirin_drm_ops = {
.bind = kirin_drm_bind,
.unbind = kirin_drm_unbind,
};
static int kirin_drm_platform_probe(struct platform_device *pdev)
{
...
drm_of_component_match_add(dev, , compare_of, remote);
...
return component_master_add_with_match(dev, _drm_ops, match);
}
Are you meaning that I should get rid of this component API and call
kirin_drm_bind() from kirin_drm_platform_probe()?
Thanks,
Mauro
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro, On Thu, Aug 20, 2020 at 09:03:26AM +0200, Mauro Carvalho Chehab wrote: > Em Wed, 19 Aug 2020 12:52:06 -0700 John Stultz escreveu: > > On Wed, Aug 19, 2020 at 8:31 AM Laurent Pinchart wrote: > > > On Wed, Aug 19, 2020 at 05:21:20PM +0200, Sam Ravnborg wrote: > > > > On Wed, Aug 19, 2020 at 01:45:28PM +0200, Mauro Carvalho Chehab wrote: > > > > > This patch series port the out-of-tree driver for Hikey 970 (which > > > > > should also support Hikey 960) from the official 96boards tree: > > > > > > > > > >https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > > > > > > > > > Based on his history, this driver seems to be originally written > > > > > for Kernel 4.4, and was later ported to Kernel 4.9. The original > > > > > driver used to depend on ION (from Kernel 4.4) and had its own > > > > > implementation for FB dev API. > > > > > > > > > > As I need to preserve the original history (with has patches from > > > > > both HiSilicon and from Linaro), I'm starting from the original > > > > > patch applied there. The remaining patches are incremental, > > > > > and port this driver to work with upstream Kernel. > > > > > > > ... > > > > > - 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; > > ... > > > > I do acknowledge you need to preserve history and all - > > > > but this patchset is not easy to review. > > > > > > Why do we need to preserve history ? Adding relevant Signed-off-by and > > > Co-developed-by should be enough, shouldn't it ? Having a public branch > > > that contains the history is useful if anyone is interested, but I don't > > > think it's required in mainline. > > > > Yea. I concur with Laurent here. I'm not sure what legal reasoning you > > have on this but preserving the "absolute" history here is actively > > detrimental for review and understanding of the patch set. > > > > Preserving Authorship, Signed-off-by lines and adding Co-developed-by > > lines should be sufficient to provide both atribution credit and DCO > > history. > > I'm not convinced that, from legal standpoint, folding things would > be enough. See, there are at least 3 legal systems involved here > among the different patch authors: > > - civil law; > - common law; > - customary law + common law. > > Merging stuff altogether from different law systems can be problematic, > and trying to discuss this with experienced IP property lawyers will > for sure take a lot of time and efforts. I also bet that different > lawyers will have different opinions, because laws are subject to > interpretation. With that matter I'm not aware of any court rules > with regards to folded patches. So, it sounds to me that folding > patches is something that has yet to be proofed in courts around > the globe. > > At least for US legal system, it sounds that the Country of > origin of a patch is relevant, as they have a concept of > "national technology" that can be subject to export regulations. > > From my side, I really prefer to play safe and stay out of any such > legal discussions. Let's be serious for a moment. If you think there are legal issues in taking GPL-v2.0-only patches and squashing them while retaining authorship information through tags, the Linux kernel if *full* of that. You also routinely modify patches that you commit to the media subsystem to fix "small issues". The country of origin argument makes no sense either, the kernel code base if full of code coming from pretty much all country on the planet. Keeping the patches separate make this hard to review. Please squash them. -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Wed, 19 Aug 2020 14:36:52 -0700 John Stultz escreveu: > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab > wrote: > > So, IMO, the best is to keep it on staging for a while, until those > > remaining bugs gets solved. > > > > I added this series, together with the regulator driver and > > a few other patches (including a hack to fix a Kernel 5.8 > > regression at WiFi ) at: > > > > > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master > > Sorry, one more small request: Could you create a branch that only has > the DRM driver changes in it? > > The reason I ask, is that since the HiKey960 isn't affected by the > majority of the problems you listed as motivation for going through > staging. So if we can validate that your tree works fine on HiKey960, > the series can be cleaned up and submitted properly upstream to enable > that SoC, and the outstanding 970 issues can be worked out afterwards > against mainline. Well, if support for HiKey 960 is OK, I guess what we can do is to not push the patch with DT bindings for hikey970. We should probably fix the color swap thing at the driver first. >From my side, provided that the history is preserved, I don't mind if this is merged: - via staging tree; - at dri-devel tree; - or having a the historic patchsets merged at /staging, with a follow up patch moving it from staging/ into /gpu/drm/. Thanks, Mauro ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Wed, 19 Aug 2020 20:28:44 -0700 John Stultz escreveu: > On Wed, Aug 19, 2020 at 7:01 PM John Stultz wrote: > > > > On Wed, Aug 19, 2020 at 2:36 PM John Stultz wrote: > > > > > > > > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab > > > wrote: > > > > So, IMO, the best is to keep it on staging for a while, until those > > > > remaining bugs gets solved. > > > > > > > > I added this series, together with the regulator driver and > > > > a few other patches (including a hack to fix a Kernel 5.8 > > > > regression at WiFi ) at: > > > > > > > > > > > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master > > > > > > > > > > Sorry, one more small request: Could you create a branch that only has > > > the DRM driver changes in it? > > > > > > The reason I ask, is that since the HiKey960 isn't affected by the > > > majority of the problems you listed as motivation for going through > > > staging. So if we can validate that your tree works fine on HiKey960, > > > the series can be cleaned up and submitted properly upstream to enable > > > that SoC, and the outstanding 970 issues can be worked out afterwards > > > against mainline. > > > > Just as a heads up, I tried testing your tree with my HiKey960, and > > after fixing the compat string inconsistency, the drivers seem to load > > properly. However the drm_hwcomposer seems to have some trouble with > > the driver: > > 01-01 00:12:41.456 345 345 E hwc-drm-display-compositor: Commit > > test failed for display 0, FIXME > > 01-01 00:12:41.456 345 345 E hwc-drm-two: Failed to apply the > > frame composition ret=-22 > > 01-01 00:12:41.456 351 351 E HWComposer: > > presentAndGetReleaseFences: present failed for display 0: BadParameter > > (4) > > > > I'll dig in a bit further as to why, but wanted to give you a heads up. > > Ok, I've mostly gotten it sorted out: > - You're missing a few color formats. > - And I re-discovered a crash that was already fixed in my tree. > > I'll send those patches in a few here. Thank you for the patches! I'll test them with Hikey 970 in order to be sure they're compatible also with such SoC. > > That said even with the patches I've got on top of your series, I > still see a few issues: > 1) I'm seeing red-blue swap with your driver. I need to dig a bit to > see what the difference is, I know gralloc has a config option for > this, and maybe the version of the driver I'm carrying has it wrong? There are some settings at adv7535 with regards to the colormap. The 4.9 fork of it has some different settings. Maybe it could be somehow related to it. I have here a Hikey 960, but didn't test it yet. > 2) Performance is noticeably worse. Whereas with my tree, I see close > to 60fps (that clk issue we mentioned earlier is why it's not exactly > 60) in most tests, but with yours it mostly hovers around 30some fps, > occasionally speeding up to 40 and then back down. That's weird, but it could be due to some settings related to CMA, IOMMU and/or AFBC. > Obviously with some work I suspect we'll be able to sort these out, > but I also do feel that the set you're starting with for upstreaming > is pretty old. The driver I'm carrying was heavily refactored around > 5.0 to share code with the existing kirin driver, in the hopes of > making usptreaming easier, and it seems a shame to throw that out and > focus your efforts on the older tree. > > But to be fair, I've not had time to upstream the driver myself, and > it's obviously your choice on how you spend your time. I am really > excited to see your efforts here, regardless of which driver you end > up pushing. On a quick look I've done, besides not having support for Hikey 970, the code on your tree seems to have less settings than the original one for Hikey 960. Yet, it should take some time to figure out what those extra settings are doing. Once I get this driver merged, and have USB support working fine[1], my plan is to compare the version from your tree, and compare with the one I have, in order to cleanup some stuff, check performance and do some other optimizations. - [1] this is a little OOT here: USB has been a challenge. Depending on the build, I'm getting an NMI interrupt error when the USB3 stack is loaded (usually at dwc3). The error is ESR_ELx_AET_UC. Unfortunately, it doesn't point to where this error is generated, making very hard to debug it. Thanks, Mauro ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Sam, Em Wed, 19 Aug 2020 22:48:00 +0200 Sam Ravnborg escreveu: > Hi Mauro. > > It seems my review comments failed to reach dri-devel - likely due to > the size of the mail. Probably. It reached here properly. > Link: > https://lore.kernel.org/linux-devicetree/20200819173558.ga3...@ravnborg.org/ > > I my review feedback I refer to checkpatch a few time. > For drivers/gpu/ we have some nice tooling support. > One thing our tooling does for us is running checkpatch every time > we apply a patch. > > checkpatch -q --emacs --strict --show-types > > So we expect patches to be more or less checkpatch --strict clean. > > "more or less" - as common sense also plays a role. > And sometimes checkpatch is just wrong. > > Just in case you wondered why checkpatch --strict was requested. We also use checkpatch --strict for media as a reference, ignoring the things that would make things worse during review :-) I'll run checkpatch here and ensure that the coding style issues will be properly addressed. Thanks, Mauro ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
On 2020-08-19 10:48 p.m., Sam Ravnborg wrote: > Hi Mauro. > > It seems my review comments failed to reach dri-devel - likely due to > the size of the mail. Right, some e-mails in this thread went through the dri-devel moderation queue due to their sizes. This mail of yours did as well, because you didn't trim the quoted text (hint, hint). -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Wed, 19 Aug 2020 12:52:06 -0700 John Stultz escreveu: > On Wed, Aug 19, 2020 at 8:31 AM Laurent Pinchart > wrote: > > On Wed, Aug 19, 2020 at 05:21:20PM +0200, Sam Ravnborg wrote: > > > On Wed, Aug 19, 2020 at 01:45:28PM +0200, Mauro Carvalho Chehab wrote: > > > > This patch series port the out-of-tree driver for Hikey 970 (which > > > > should also support Hikey 960) from the official 96boards tree: > > > > > > > >https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > > > > > > > Based on his history, this driver seems to be originally written > > > > for Kernel 4.4, and was later ported to Kernel 4.9. The original > > > > driver used to depend on ION (from Kernel 4.4) and had its own > > > > implementation for FB dev API. > > > > > > > > As I need to preserve the original history (with has patches from > > > > both HiSilicon and from Linaro), I'm starting from the original > > > > patch applied there. The remaining patches are incremental, > > > > and port this driver to work with upstream Kernel. > > > > > ... > > > > - 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; > ... > > > I do acknowledge you need to preserve history and all - > > > but this patchset is not easy to review. > > > > Why do we need to preserve history ? Adding relevant Signed-off-by and > > Co-developed-by should be enough, shouldn't it ? Having a public branch > > that contains the history is useful if anyone is interested, but I don't > > think it's required in mainline. > > Yea. I concur with Laurent here. I'm not sure what legal reasoning you > have on this but preserving the "absolute" history here is actively > detrimental for review and understanding of the patch set. > > Preserving Authorship, Signed-off-by lines and adding Co-developed-by > lines should be sufficient to provide both atribution credit and DCO > history. I'm not convinced that, from legal standpoint, folding things would be enough. See, there are at least 3 legal systems involved here among the different patch authors: - civil law; - common law; - customary law + common law. Merging stuff altogether from different law systems can be problematic, and trying to discuss this with experienced IP property lawyers will for sure take a lot of time and efforts. I also bet that different lawyers will have different opinions, because laws are subject to interpretation. With that matter I'm not aware of any court rules with regards to folded patches. So, it sounds to me that folding patches is something that has yet to be proofed in courts around the globe. At least for US legal system, it sounds that the Country of origin of a patch is relevant, as they have a concept of "national technology" that can be subject to export regulations. >From my side, I really prefer to play safe and stay out of any such legal discussions. Thanks, Mauro ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Wed, 19 Aug 2020 23:25:51 +0200 Sam Ravnborg escreveu: > Hi John. > > > > 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. > > There is a tendency to forget drivers in staging, and with the almost > constant refactoring that happens in the drm drivers we would end up > fixing this driver when a bot trigger an error. > So IMO we need very good reasons to go in via staging. My plan is to have this driver upstream for 5.10, and getting it out of staging by Kernel 5.11. So, I doubt that the DRM kAPIs would change a lot during those 2 Kernel cycles. In any case, I'm also fine to have a final patch at the end of this series moving it out of staging. The only thing that, IMHO, prevents it to be out of staging is the LDI underflow. Right now, if no input events reach the driver, DPMS will put the monitor to suspend, and it never returns back from life. I bet that, once we discover the root cause, the fix would be just a couple of lines, but identifying where the problem is can take a while. Thanks, Mauro ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
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
Re: [PATCH 00/49] DRM driver for Hikey 970
On Wed, Aug 19, 2020 at 7:01 PM John Stultz wrote: > > On Wed, Aug 19, 2020 at 2:36 PM John Stultz wrote: > > > > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab > > wrote: > > > So, IMO, the best is to keep it on staging for a while, until those > > > remaining bugs gets solved. > > > > > > I added this series, together with the regulator driver and > > > a few other patches (including a hack to fix a Kernel 5.8 > > > regression at WiFi ) at: > > > > > > > > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master > > > > Sorry, one more small request: Could you create a branch that only has > > the DRM driver changes in it? > > > > The reason I ask, is that since the HiKey960 isn't affected by the > > majority of the problems you listed as motivation for going through > > staging. So if we can validate that your tree works fine on HiKey960, > > the series can be cleaned up and submitted properly upstream to enable > > that SoC, and the outstanding 970 issues can be worked out afterwards > > against mainline. > > Just as a heads up, I tried testing your tree with my HiKey960, and > after fixing the compat string inconsistency, the drivers seem to load > properly. However the drm_hwcomposer seems to have some trouble with > the driver: > 01-01 00:12:41.456 345 345 E hwc-drm-display-compositor: Commit > test failed for display 0, FIXME > 01-01 00:12:41.456 345 345 E hwc-drm-two: Failed to apply the > frame composition ret=-22 > 01-01 00:12:41.456 351 351 E HWComposer: > presentAndGetReleaseFences: present failed for display 0: BadParameter > (4) > > I'll dig in a bit further as to why, but wanted to give you a heads up. Ok, I've mostly gotten it sorted out: - You're missing a few color formats. - And I re-discovered a crash that was already fixed in my tree. I'll send those patches in a few here. That said even with the patches I've got on top of your series, I still see a few issues: 1) I'm seeing red-blue swap with your driver. I need to dig a bit to see what the difference is, I know gralloc has a config option for this, and maybe the version of the driver I'm carrying has it wrong? 2) Performance is noticeably worse. Whereas with my tree, I see close to 60fps (that clk issue we mentioned earlier is why it's not exactly 60) in most tests, but with yours it mostly hovers around 30some fps, occasionally speeding up to 40 and then back down. Obviously with some work I suspect we'll be able to sort these out, but I also do feel that the set you're starting with for upstreaming is pretty old. The driver I'm carrying was heavily refactored around 5.0 to share code with the existing kirin driver, in the hopes of making usptreaming easier, and it seems a shame to throw that out and focus your efforts on the older tree. But to be fair, I've not had time to upstream the driver myself, and it's obviously your choice on how you spend your time. I am really excited to see your efforts here, regardless of which driver you end up pushing. thanks -john ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
On Wed, Aug 19, 2020 at 2:36 PM John Stultz wrote: > > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab > wrote: > > So, IMO, the best is to keep it on staging for a while, until those > > remaining bugs gets solved. > > > > I added this series, together with the regulator driver and > > a few other patches (including a hack to fix a Kernel 5.8 > > regression at WiFi ) at: > > > > > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master > > Sorry, one more small request: Could you create a branch that only has > the DRM driver changes in it? > > The reason I ask, is that since the HiKey960 isn't affected by the > majority of the problems you listed as motivation for going through > staging. So if we can validate that your tree works fine on HiKey960, > the series can be cleaned up and submitted properly upstream to enable > that SoC, and the outstanding 970 issues can be worked out afterwards > against mainline. Just as a heads up, I tried testing your tree with my HiKey960, and after fixing the compat string inconsistency, the drivers seem to load properly. However the drm_hwcomposer seems to have some trouble with the driver: 01-01 00:12:41.456 345 345 E hwc-drm-display-compositor: Commit test failed for display 0, FIXME 01-01 00:12:41.456 345 345 E hwc-drm-two: Failed to apply the frame composition ret=-22 01-01 00:12:41.456 351 351 E HWComposer: presentAndGetReleaseFences: present failed for display 0: BadParameter (4) I'll dig in a bit further as to why, but wanted to give you a heads up. thanks -john ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab wrote: > So, IMO, the best is to keep it on staging for a while, until those > remaining bugs gets solved. > > I added this series, together with the regulator driver and > a few other patches (including a hack to fix a Kernel 5.8 > regression at WiFi ) at: > > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master Sorry, one more small request: Could you create a branch that only has the DRM driver changes in it? The reason I ask, is that since the HiKey960 isn't affected by the majority of the problems you listed as motivation for going through staging. So if we can validate that your tree works fine on HiKey960, the series can be cleaned up and submitted properly upstream to enable that SoC, and the outstanding 970 issues can be worked out afterwards against mainline. thanks -john ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi John. > > 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. There is a tendency to forget drivers in staging, and with the almost constant refactoring that happens in the drm drivers we would end up fixing this driver when a bot trigger an error. So IMO we need very good reasons to go in via staging. Sam ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
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. >- 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). 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. Personally, I don't see the allow-list as a problematic short term solution, and again, not sure its worth pushing to staging for. >- 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). 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. >- 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. > 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. > I added this series, together with the regulator driver and > a few other patches (including a hack to fix a Kernel 5.8 > regression at WiFi ) at: > > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master > > > Chen Feng (1): > staging: hikey9xx: Add hisilicon DRM driver for hikey960/970 > > John Stultz (1): > staging: hikey9xx/gpu: port it to work with Kernel v4.9 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:" thanks -john ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
On Wed, Aug 19, 2020 at 8:31 AM Laurent Pinchart wrote: > On Wed, Aug 19, 2020 at 05:21:20PM +0200, Sam Ravnborg wrote: > > On Wed, Aug 19, 2020 at 01:45:28PM +0200, Mauro Carvalho Chehab wrote: > > > This patch series port the out-of-tree driver for Hikey 970 (which > > > should also support Hikey 960) from the official 96boards tree: > > > > > >https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > > > > > Based on his history, this driver seems to be originally written > > > for Kernel 4.4, and was later ported to Kernel 4.9. The original > > > driver used to depend on ION (from Kernel 4.4) and had its own > > > implementation for FB dev API. > > > > > > As I need to preserve the original history (with has patches from > > > both HiSilicon and from Linaro), I'm starting from the original > > > patch applied there. The remaining patches are incremental, > > > and port this driver to work with upstream Kernel. > > > ... > > > - 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; ... > > I do acknowledge you need to preserve history and all - > > but this patchset is not easy to review. > > Why do we need to preserve history ? Adding relevant Signed-off-by and > Co-developed-by should be enough, shouldn't it ? Having a public branch > that contains the history is useful if anyone is interested, but I don't > think it's required in mainline. Yea. I concur with Laurent here. I'm not sure what legal reasoning you have on this but preserving the "absolute" history here is actively detrimental for review and understanding of the patch set. Preserving Authorship, Signed-off-by lines and adding Co-developed-by lines should be sufficient to provide both atribution credit and DCO history. thanks -john ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
On Wed, Aug 19, 2020 at 05:21:20PM +0200, Sam Ravnborg wrote: > Hi Mauro. > > On Wed, Aug 19, 2020 at 01:45:28PM +0200, Mauro Carvalho Chehab wrote: > > This patch series port the out-of-tree driver for Hikey 970 (which > > should also support Hikey 960) from the official 96boards tree: > > > >https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > > > Based on his history, this driver seems to be originally written > > for Kernel 4.4, and was later ported to Kernel 4.9. The original > > driver used to depend on ION (from Kernel 4.4) and had its own > > implementation for FB dev API. > > > > As I need to preserve the original history (with has patches from > > both HiSilicon and from Linaro), I'm starting from the original > > patch applied there. The remaining patches are incremental, > > and port this driver to work with upstream Kernel. > > > > This driver doesn't depend on any firmware or on any special > > userspace code. It works as-is with both X11 and Wayland. > > > > 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; > >- 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; > >- 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; > >- Probably related to the previous issue, when the monitor > > suspends due to DPMS, it doesn't return back to life. > > > > So, IMO, the best is to keep it on staging for a while, until those > > remaining bugs gets solved. > > > > I added this series, together with the regulator driver and > > a few other patches (including a hack to fix a Kernel 5.8 > > regression at WiFi ) at: > > > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master > > > > > > Chen Feng (1): > > staging: hikey9xx: Add hisilicon DRM driver for hikey960/970 > > > > John Stultz (1): > > staging: hikey9xx/gpu: port it to work with Kernel v4.9 > > > > Liwei Cai (2): > > staging: hikey9xx/gpu: solve tearing issue of display > > staging: hikey9xx/gpu: resolve the performance issue by interrupt > > mechanism > > > > Mauro Carvalho Chehab (38): > > staging: hikey9xx/gpu: get rid of adv7535 fork > Very good - I was in my mind starting a rant why we needed a fork of > this driver, but I see it gets deleted again. > > I do acknowledge you need to preserve history and all - > but this patchset is not easy to review. Why do we need to preserve history ? Adding relevant Signed-off-by and Co-developed-by should be enough, shouldn't it ? Having a public branch that contains the history is useful if anyone is interested, but I don't think it's required in mainline. > Could you follow-up with a review-able set of patches as a follow-up > for this? > I spotted some wrong bridge handling in one patch but I do not know if > this got changed in a later patch. And I lost the motivation to go > looking for it. > > > staging: hikey9xx/gpu: rename the Kirin9xx namespace > > staging: hikey9xx/gpu: get rid of kirin9xx_fbdev.c > > staging: hikey9xx/gpu: get rid of some ifdefs > > staging: hikey9xx/gpu: rename the config option for Kirin970 > > staging: hikey9xx/gpu: change the includes to reflect upstream > > staging: hikey9xx/gpu: port driver to upstream kAPIs > > staging: hikey9xx/gpu: add a copy of set_reg() function there > > staging: hikey9xx/gpu: get rid of ION headers > > staging: hikey9xx/gpu: add support for using a reserved CMA memory > > staging: hikey9xx/gpu: cleanup encoder attach logic > > staging: hikey9xx/gpu: Change the logic which sets the burst mode > > staging: hikey9xx/gpu: fix the DRM setting logic > > staging: hikey9xx/gpu: do some code cleanups > > staging: hikey9xx/gpu: use default GEM_CMA fops > > staging: hikey9xx/gpu: place vblank enable/disable at the right place > > staging: hikey9xx/gpu: remove an uneeded hack > > staging: hikey9xx/gpu: add a possible implementation for > > atomic_disable > > staging: hikey9xx/gpu: register connector > > staging: hikey9xx/gpu: fix driver name > > staging: hikey9xx/gpu: get rid of iommu_format > > staging: hikey9xx/gpu: re-work the mode validation code > > staging: hikey9xx/gpu: add support for
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro. On Wed, Aug 19, 2020 at 01:45:28PM +0200, Mauro Carvalho Chehab wrote: > This patch series port the out-of-tree driver for Hikey 970 (which > should also support Hikey 960) from the official 96boards tree: > >https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > Based on his history, this driver seems to be originally written > for Kernel 4.4, and was later ported to Kernel 4.9. The original > driver used to depend on ION (from Kernel 4.4) and had its own > implementation for FB dev API. > > As I need to preserve the original history (with has patches from > both HiSilicon and from Linaro), I'm starting from the original > patch applied there. The remaining patches are incremental, > and port this driver to work with upstream Kernel. > > This driver doesn't depend on any firmware or on any special > userspace code. It works as-is with both X11 and Wayland. > > 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; >- 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; >- 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; >- Probably related to the previous issue, when the monitor > suspends due to DPMS, it doesn't return back to life. > > So, IMO, the best is to keep it on staging for a while, until those > remaining bugs gets solved. > > I added this series, together with the regulator driver and > a few other patches (including a hack to fix a Kernel 5.8 > regression at WiFi ) at: > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master > > > Chen Feng (1): > staging: hikey9xx: Add hisilicon DRM driver for hikey960/970 > > John Stultz (1): > staging: hikey9xx/gpu: port it to work with Kernel v4.9 > > Liwei Cai (2): > staging: hikey9xx/gpu: solve tearing issue of display > staging: hikey9xx/gpu: resolve the performance issue by interrupt > mechanism > > Mauro Carvalho Chehab (38): > staging: hikey9xx/gpu: get rid of adv7535 fork Very good - I was in my mind starting a rant why we needed a fork of this driver, but I see it gets deleted again. I do acknowledge you need to preserve history and all - but this patchset is not easy to review. Could you follow-up with a review-able set of patches as a follow-up for this? I spotted some wrong bridge handling in one patch but I do not know if this got changed in a later patch. And I lost the motivation to go looking for it. > staging: hikey9xx/gpu: rename the Kirin9xx namespace > staging: hikey9xx/gpu: get rid of kirin9xx_fbdev.c > staging: hikey9xx/gpu: get rid of some ifdefs > staging: hikey9xx/gpu: rename the config option for Kirin970 > staging: hikey9xx/gpu: change the includes to reflect upstream > staging: hikey9xx/gpu: port driver to upstream kAPIs > staging: hikey9xx/gpu: add a copy of set_reg() function there > staging: hikey9xx/gpu: get rid of ION headers > staging: hikey9xx/gpu: add support for using a reserved CMA memory > staging: hikey9xx/gpu: cleanup encoder attach logic > staging: hikey9xx/gpu: Change the logic which sets the burst mode > staging: hikey9xx/gpu: fix the DRM setting logic > staging: hikey9xx/gpu: do some code cleanups > staging: hikey9xx/gpu: use default GEM_CMA fops > staging: hikey9xx/gpu: place vblank enable/disable at the right place > staging: hikey9xx/gpu: remove an uneeded hack > staging: hikey9xx/gpu: add a possible implementation for > atomic_disable > staging: hikey9xx/gpu: register connector > staging: hikey9xx/gpu: fix driver name > staging: hikey9xx/gpu: get rid of iommu_format > staging: hikey9xx/gpu: re-work the mode validation code > staging: hikey9xx/gpu: add support for enable/disable ldo3 regulator > staging: hikey9xx/gpu: add SPMI headers > staging: hikey9xx/gpu: solve most coding style issues > staging: hikey9xx/gpu: don't use iommu code > staging: hikey9xx/gpu: add kirin9xx driver to the building system > staging: hikey9xx/gpu: get rid of typedefs > staging: hikey9xx/gpu: get rid of input/output macros > staging: hikey9xx/gpu: get rid of some unused data > staging: hikey9xx/gpu: place common definitions at kirin9xx_dpe.h > staging: hikey9xx/gpu: get
[PATCH 00/49] DRM driver for Hikey 970
This patch series port the out-of-tree driver for Hikey 970 (which should also support Hikey 960) from the official 96boards tree: https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 Based on his history, this driver seems to be originally written for Kernel 4.4, and was later ported to Kernel 4.9. The original driver used to depend on ION (from Kernel 4.4) and had its own implementation for FB dev API. As I need to preserve the original history (with has patches from both HiSilicon and from Linaro), I'm starting from the original patch applied there. The remaining patches are incremental, and port this driver to work with upstream Kernel. This driver doesn't depend on any firmware or on any special userspace code. It works as-is with both X11 and Wayland. 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; - 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; - 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; - Probably related to the previous issue, when the monitor suspends due to DPMS, it doesn't return back to life. So, IMO, the best is to keep it on staging for a while, until those remaining bugs gets solved. I added this series, together with the regulator driver and a few other patches (including a hack to fix a Kernel 5.8 regression at WiFi ) at: https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master Chen Feng (1): staging: hikey9xx: Add hisilicon DRM driver for hikey960/970 John Stultz (1): staging: hikey9xx/gpu: port it to work with Kernel v4.9 Liwei Cai (2): staging: hikey9xx/gpu: solve tearing issue of display staging: hikey9xx/gpu: resolve the performance issue by interrupt mechanism Mauro Carvalho Chehab (38): staging: hikey9xx/gpu: get rid of adv7535 fork staging: hikey9xx/gpu: rename the Kirin9xx namespace staging: hikey9xx/gpu: get rid of kirin9xx_fbdev.c staging: hikey9xx/gpu: get rid of some ifdefs staging: hikey9xx/gpu: rename the config option for Kirin970 staging: hikey9xx/gpu: change the includes to reflect upstream staging: hikey9xx/gpu: port driver to upstream kAPIs staging: hikey9xx/gpu: add a copy of set_reg() function there staging: hikey9xx/gpu: get rid of ION headers staging: hikey9xx/gpu: add support for using a reserved CMA memory staging: hikey9xx/gpu: cleanup encoder attach logic staging: hikey9xx/gpu: Change the logic which sets the burst mode staging: hikey9xx/gpu: fix the DRM setting logic staging: hikey9xx/gpu: do some code cleanups staging: hikey9xx/gpu: use default GEM_CMA fops staging: hikey9xx/gpu: place vblank enable/disable at the right place staging: hikey9xx/gpu: remove an uneeded hack staging: hikey9xx/gpu: add a possible implementation for atomic_disable staging: hikey9xx/gpu: register connector staging: hikey9xx/gpu: fix driver name staging: hikey9xx/gpu: get rid of iommu_format staging: hikey9xx/gpu: re-work the mode validation code staging: hikey9xx/gpu: add support for enable/disable ldo3 regulator staging: hikey9xx/gpu: add SPMI headers staging: hikey9xx/gpu: solve most coding style issues staging: hikey9xx/gpu: don't use iommu code staging: hikey9xx/gpu: add kirin9xx driver to the building system staging: hikey9xx/gpu: get rid of typedefs staging: hikey9xx/gpu: get rid of input/output macros staging: hikey9xx/gpu: get rid of some unused data staging: hikey9xx/gpu: place common definitions at kirin9xx_dpe.h staging: hikey9xx/gpu: get rid of DRM_HISI_KIRIN970 dts: hisilicon: hi3670.dtsi: add I2C settings dts: hikey970-pinctrl.dtsi: add missing pinctrl settings dt: hisilicon: add support for the PMIC found on Hikey 970 dts: add support for Hikey 970 DRM staging: hikey9xx/gpu: drop kirin9xx_pwm dt: display: Add binds for the DPE and DSI controller for Kirin 960/970 Xiubin Zhang (7): staging: hikey9xx/gpu: add support to hikey970 HDMI and panel staging: hikey9xx/gpu: Solve SR Cannot Display Problems. staging: hikey9xx/gpu: Solve HDMI compatibility Problem. staging: hikey9xx/gpu: Support MIPI DSI 3 lanes for hikey970. staging: hikey9xx/gpu: Solve SR test reset problem for hikey970. staging: hikey9xx/gpu: add debug
