Linux Tegra architecture development
 help / color / mirror / Atom feed
* Re: [PATCH v8 7/7] drm/panel-simple: Add missing connector type for some panels
From: Dmitry Osipenko @ 2020-06-20 13:19 UTC (permalink / raw)
  To: Laurent Pinchart, Sam Ravnborg
  Cc: Thierry Reding, Rob Herring, Frank Rowand,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20200620114934.GB5829-N3hz7ZxfLydczECFQUw77jytWr6r+dGw0E9HWUfgJXw@public.gmane.org>

20.06.2020 14:49, Laurent Pinchart пишет:
> Hi Sam and Dmitry,
> 
> On Sat, Jun 20, 2020 at 01:21:32PM +0200, Sam Ravnborg wrote:
>> On Thu, Jun 18, 2020 at 01:27:03AM +0300, Dmitry Osipenko wrote:
>>> The DRM panel bridge core requires connector type to be set up properly,
>>> otherwise it rejects the panel. The missing connector type problem popped
>>> up while I was trying to wrap CLAA070WP03XG panel into a DRM bridge in
>>> order to test whether panel's rotation property work properly using
>>> panel-simple driver on NVIDIA Tegra30 Nexus 7 tablet device, which uses
>>> CLAA070WP03XG display panel.
>>>
>>> The NVIDIA Tegra DRM driver recently gained DRM bridges support for the
>>> RGB output and now driver wraps directly-connected panels into DRM bridge.
>>> Hence all panels should have connector type set properly now, otherwise
>>> the panel's wrapping fails.
>>>
>>> This patch adds missing connector types for the LVDS panels that are found
>>> on NVIDIA Tegra devices:
>>>
>>>   1. AUO B101AW03
>>>   2. Chunghwa CLAA070WP03XG
>>>   3. Chunghwa CLAA101WA01A
>>>   4. Chunghwa CLAA101WB01
>>>   5. EDT ET057090DHU
>>>   6. Innolux N156BGE L21
>>>   7. Samsung LTN101NT05
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> Very good to have this fixed.
>> I went ahead and pushed this commit to drm-misc-next as it is really
>> independent from the rest of the series.
>>
>>> ---
>>>  drivers/gpu/drm/panel/panel-simple.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>>> index 6764ac630e22..9eb2dbb7bfa6 100644
>>> --- a/drivers/gpu/drm/panel/panel-simple.c
>>> +++ b/drivers/gpu/drm/panel/panel-simple.c
>>> @@ -687,6 +687,7 @@ static const struct panel_desc auo_b101aw03 = {
>>>  		.width = 223,
>>>  		.height = 125,
>>>  	},
>>> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> 
> Note that, for LVDS panels, the bus_format field is mandatory. This
> panel, for instance, according to
> http://www.vslcd.com/Specification/B101AW03%20V.0.pdf, uses
> MEDIA_BUS_FMT_RGB666_1X7X3_SPWG (see
> https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#v4l2-mbus-pixelcode).
> The panels below need to be investigated similarly.

Okay! I'll add the missing field in v9.

>>>  };
>>>  
>>>  static const struct display_timing auo_b101ean01_timing = {
>>> @@ -1340,6 +1341,7 @@ static const struct panel_desc chunghwa_claa070wp03xg = {
>>>  		.width = 94,
>>>  		.height = 150,
>>>  	},
>>> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>>>  };
>>>  
>>>  static const struct drm_display_mode chunghwa_claa101wa01a_mode = {
>>> @@ -1362,6 +1364,7 @@ static const struct panel_desc chunghwa_claa101wa01a = {
>>>  		.width = 220,
>>>  		.height = 120,
>>>  	},
>>> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>>>  };
>>>  
>>>  static const struct drm_display_mode chunghwa_claa101wb01_mode = {
>>> @@ -1384,6 +1387,7 @@ static const struct panel_desc chunghwa_claa101wb01 = {
>>>  		.width = 223,
>>>  		.height = 125,
>>>  	},
>>> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>>>  };
>>>  
>>>  static const struct drm_display_mode dataimage_scf0700c48ggu18_mode = {
>>> @@ -1573,6 +1577,7 @@ static const struct panel_desc edt_et057090dhu = {
>>>  	},
>>>  	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
>>>  	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
>>> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> 
> This contradicts .bus_format and .bus_flags that hint that the panel is
> a DPI panel, not an LVDS panel. According to
> https://www.lcdtek.co.uk/dwpdf/ET057090DHU-RoHS.pdf, this isn't an LVDS
> panel.
> 
> I'm worried enough research hasn't gone into this patch, and I'd prefer
> reverting it until we check each panel individually.

Hello Sam and Laurent,

Oops! Good catch! Indeed, I blindly set the LVDS type to all these
panels. Please revert this patch, I'll double check each panel and
prepare an updated version of this patch. Thank you very much for the
review!

^ permalink raw reply

* Re: [PATCH v8 7/7] drm/panel-simple: Add missing connector type for some panels
From: Laurent Pinchart @ 2020-06-20 11:49 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Dmitry Osipenko, Thierry Reding, Rob Herring, Frank Rowand,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20200620112132.GB16901-uyr5N9Q2VtJg9hUCZPvPmw@public.gmane.org>

Hi Sam and Dmitry,

On Sat, Jun 20, 2020 at 01:21:32PM +0200, Sam Ravnborg wrote:
> On Thu, Jun 18, 2020 at 01:27:03AM +0300, Dmitry Osipenko wrote:
> > The DRM panel bridge core requires connector type to be set up properly,
> > otherwise it rejects the panel. The missing connector type problem popped
> > up while I was trying to wrap CLAA070WP03XG panel into a DRM bridge in
> > order to test whether panel's rotation property work properly using
> > panel-simple driver on NVIDIA Tegra30 Nexus 7 tablet device, which uses
> > CLAA070WP03XG display panel.
> > 
> > The NVIDIA Tegra DRM driver recently gained DRM bridges support for the
> > RGB output and now driver wraps directly-connected panels into DRM bridge.
> > Hence all panels should have connector type set properly now, otherwise
> > the panel's wrapping fails.
> > 
> > This patch adds missing connector types for the LVDS panels that are found
> > on NVIDIA Tegra devices:
> > 
> >   1. AUO B101AW03
> >   2. Chunghwa CLAA070WP03XG
> >   3. Chunghwa CLAA101WA01A
> >   4. Chunghwa CLAA101WB01
> >   5. EDT ET057090DHU
> >   6. Innolux N156BGE L21
> >   7. Samsung LTN101NT05
> > 
> > Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Very good to have this fixed.
> I went ahead and pushed this commit to drm-misc-next as it is really
> independent from the rest of the series.
> 
> > ---
> >  drivers/gpu/drm/panel/panel-simple.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > index 6764ac630e22..9eb2dbb7bfa6 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -687,6 +687,7 @@ static const struct panel_desc auo_b101aw03 = {
> >  		.width = 223,
> >  		.height = 125,
> >  	},
> > +	.connector_type = DRM_MODE_CONNECTOR_LVDS,

Note that, for LVDS panels, the bus_format field is mandatory. This
panel, for instance, according to
http://www.vslcd.com/Specification/B101AW03%20V.0.pdf, uses
MEDIA_BUS_FMT_RGB666_1X7X3_SPWG (see
https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#v4l2-mbus-pixelcode).
The panels below need to be investigated similarly.

> >  };
> >  
> >  static const struct display_timing auo_b101ean01_timing = {
> > @@ -1340,6 +1341,7 @@ static const struct panel_desc chunghwa_claa070wp03xg = {
> >  		.width = 94,
> >  		.height = 150,
> >  	},
> > +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> >  };
> >  
> >  static const struct drm_display_mode chunghwa_claa101wa01a_mode = {
> > @@ -1362,6 +1364,7 @@ static const struct panel_desc chunghwa_claa101wa01a = {
> >  		.width = 220,
> >  		.height = 120,
> >  	},
> > +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> >  };
> >  
> >  static const struct drm_display_mode chunghwa_claa101wb01_mode = {
> > @@ -1384,6 +1387,7 @@ static const struct panel_desc chunghwa_claa101wb01 = {
> >  		.width = 223,
> >  		.height = 125,
> >  	},
> > +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> >  };
> >  
> >  static const struct drm_display_mode dataimage_scf0700c48ggu18_mode = {
> > @@ -1573,6 +1577,7 @@ static const struct panel_desc edt_et057090dhu = {
> >  	},
> >  	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
> >  	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
> > +	.connector_type = DRM_MODE_CONNECTOR_LVDS,

This contradicts .bus_format and .bus_flags that hint that the panel is
a DPI panel, not an LVDS panel. According to
https://www.lcdtek.co.uk/dwpdf/ET057090DHU-RoHS.pdf, this isn't an LVDS
panel.

I'm worried enough research hasn't gone into this patch, and I'd prefer
reverting it until we check each panel individually.

> >  };
> >  
> >  static const struct drm_display_mode edt_etm0700g0dh6_mode = {
> > @@ -2055,6 +2060,7 @@ static const struct panel_desc innolux_n156bge_l21 = {
> >  		.width = 344,
> >  		.height = 193,
> >  	},
> > +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> >  };
> >  
> >  static const struct drm_display_mode innolux_p120zdg_bf1_mode = {
> > @@ -3001,6 +3007,7 @@ static const struct panel_desc samsung_ltn101nt05 = {
> >  		.width = 223,
> >  		.height = 125,
> >  	},
> > +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> >  };
> >  
> >  static const struct drm_display_mode samsung_ltn140at29_301_mode = {

-- 
Regards,

Laurent Pinchart

^ permalink raw reply

* Re: [PATCH v8 7/7] drm/panel-simple: Add missing connector type for some panels
From: Sam Ravnborg @ 2020-06-20 11:21 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Thierry Reding, Laurent Pinchart, Rob Herring, Frank Rowand,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20200617222703.17080-8-digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Hi Dmitry

On Thu, Jun 18, 2020 at 01:27:03AM +0300, Dmitry Osipenko wrote:
> The DRM panel bridge core requires connector type to be set up properly,
> otherwise it rejects the panel. The missing connector type problem popped
> up while I was trying to wrap CLAA070WP03XG panel into a DRM bridge in
> order to test whether panel's rotation property work properly using
> panel-simple driver on NVIDIA Tegra30 Nexus 7 tablet device, which uses
> CLAA070WP03XG display panel.
> 
> The NVIDIA Tegra DRM driver recently gained DRM bridges support for the
> RGB output and now driver wraps directly-connected panels into DRM bridge.
> Hence all panels should have connector type set properly now, otherwise
> the panel's wrapping fails.
> 
> This patch adds missing connector types for the LVDS panels that are found
> on NVIDIA Tegra devices:
> 
>   1. AUO B101AW03
>   2. Chunghwa CLAA070WP03XG
>   3. Chunghwa CLAA101WA01A
>   4. Chunghwa CLAA101WB01
>   5. EDT ET057090DHU
>   6. Innolux N156BGE L21
>   7. Samsung LTN101NT05
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Very good to have this fixed.
I went ahead and pushed this commit to drm-misc-next as it is really
independent from the rest of the series.

	Sam

> ---
>  drivers/gpu/drm/panel/panel-simple.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 6764ac630e22..9eb2dbb7bfa6 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -687,6 +687,7 @@ static const struct panel_desc auo_b101aw03 = {
>  		.width = 223,
>  		.height = 125,
>  	},
> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>  };
>  
>  static const struct display_timing auo_b101ean01_timing = {
> @@ -1340,6 +1341,7 @@ static const struct panel_desc chunghwa_claa070wp03xg = {
>  		.width = 94,
>  		.height = 150,
>  	},
> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>  };
>  
>  static const struct drm_display_mode chunghwa_claa101wa01a_mode = {
> @@ -1362,6 +1364,7 @@ static const struct panel_desc chunghwa_claa101wa01a = {
>  		.width = 220,
>  		.height = 120,
>  	},
> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>  };
>  
>  static const struct drm_display_mode chunghwa_claa101wb01_mode = {
> @@ -1384,6 +1387,7 @@ static const struct panel_desc chunghwa_claa101wb01 = {
>  		.width = 223,
>  		.height = 125,
>  	},
> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>  };
>  
>  static const struct drm_display_mode dataimage_scf0700c48ggu18_mode = {
> @@ -1573,6 +1577,7 @@ static const struct panel_desc edt_et057090dhu = {
>  	},
>  	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
>  	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>  };
>  
>  static const struct drm_display_mode edt_etm0700g0dh6_mode = {
> @@ -2055,6 +2060,7 @@ static const struct panel_desc innolux_n156bge_l21 = {
>  		.width = 344,
>  		.height = 193,
>  	},
> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>  };
>  
>  static const struct drm_display_mode innolux_p120zdg_bf1_mode = {
> @@ -3001,6 +3007,7 @@ static const struct panel_desc samsung_ltn101nt05 = {
>  		.width = 223,
>  		.height = 125,
>  	},
> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>  };
>  
>  static const struct drm_display_mode samsung_ltn140at29_301_mode = {
> -- 
> 2.26.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply

* Re: [PATCH] [v3] dmaengine: tegra210-adma: Fix runtime PM imbalance on error
From: Jon Hunter @ 2020-06-20  9:53 UTC (permalink / raw)
  To: dinghao.liu-Y5EWUtBUdg4nDS1+zs4M5A
  Cc: kjlu-OJFnDUYgAso, Laxman Dewangan, Dan Williams, Vinod Koul,
	Thierry Reding, dmaengine-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <24ea1ef1.10213.172ca4d45be.Coremail.dinghao.liu-Y5EWUtBUdg4nDS1+zs4M5A@public.gmane.org>


On 19/06/2020 02:59, dinghao.liu-Y5EWUtBUdg4nDS1+zs4M5A@public.gmane.org wrote:
>>
>> Why noidle?
>>
> 
> _noidle is enough for fixing this bug. _sync may suspend
> the device beyond expectation.

In that case, then the other instance you are fixing with this patch is
not correct.

Jon

-- 
nvpublic

^ permalink raw reply

* Re: [PATCH 5.7 000/376] 5.7.5-rc1 review
From: Jon Hunter @ 2020-06-20  9:52 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
	akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
	linux-0h96xk9xTtrk1uMJSBkQmQ, shuah-DgEjT+Ai2ygdnm+yROfE0A,
	patches-ssFOTAMYnuFg9hUCZPvPmw,
	ben.hutchings-4yDnlxn2s6sWdaTGBSpHTA,
	lkft-triage-cunTk1MwBs8s++Sfvej+rw, stable-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra
In-Reply-To: <20200619141710.350494719-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>


On 19/06/2020 15:28, Greg Kroah-Hartman wrote:
> This is the start of the stable review cycle for the 5.7.5 release.
> There are 376 patches in this series, all will be posted as a response
> to this one.  If anyone has any issues with these being applied, please
> let me know.
> 
> Responses should be made by Sun, 21 Jun 2020 14:15:50 +0000.
> Anything received after that time might be too late.
> 
> The whole patch series can be found in one patch at:
> 	https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.7.5-rc1.gz
> or in the git tree and branch at:
> 	git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.7.y
> and the diffstat can be found below.
> 
> thanks,
> 
> greg k-h

All tests are passing for Tegra ...

Test results for stable-v5.7:
    11 builds:	11 pass, 0 fail
    26 boots:	26 pass, 0 fail
    56 tests:	56 pass, 0 fail

Linux version:	5.7.5-rc1-g19411dc6b061
Boards tested:	tegra124-jetson-tk1, tegra186-p2771-0000,
                tegra194-p2972-0000, tegra20-ventana,
                tegra210-p2371-2180, tegra210-p3450-0000,
                tegra30-cardhu-a04

Cheers
Jon

-- 
nvpublic

^ permalink raw reply

* Re: [PATCH 5.4 000/259] 5.4.48-rc2 review
From: Jon Hunter @ 2020-06-20  9:51 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-kernel
  Cc: torvalds, akpm, linux, shuah, patches, ben.hutchings, lkft-triage,
	stable, linux-tegra
In-Reply-To: <20200620082215.905874302@linuxfoundation.org>


On 20/06/2020 09:23, Greg Kroah-Hartman wrote:
> This is the start of the stable review cycle for the 5.4.48 release.
> There are 259 patches in this series, all will be posted as a response
> to this one.  If anyone has any issues with these being applied, please
> let me know.
> 
> Responses should be made by Mon, 22 Jun 2020 08:21:26 +0000.
> Anything received after that time might be too late.
> 
> The whole patch series can be found in one patch at:
> 	https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.4.48-rc2.gz
> or in the git tree and branch at:
> 	git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.4.y
> and the diffstat can be found below.
> 
> thanks,
> 
> greg k-h

All tests are passing for Tegra ...

Test results for stable-v5.4:
    11 builds:	11 pass, 0 fail
    26 boots:	26 pass, 0 fail
    56 tests:	56 pass, 0 fail

Linux version:	5.4.48-rc2-ga9a8b229b188
Boards tested:	tegra124-jetson-tk1, tegra186-p2771-0000,
                tegra194-p2972-0000, tegra20-ventana,
                tegra210-p2371-2180, tegra210-p3450-0000,
                tegra30-cardhu-a04

Cheers
Jon

-- 
nvpublic

^ permalink raw reply

* Re: [PATCH 4.19 000/265] 4.19.129-rc2 review
From: Jon Hunter @ 2020-06-20  9:51 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-kernel
  Cc: torvalds, akpm, linux, shuah, patches, ben.hutchings, lkft-triage,
	stable, linux-tegra
In-Reply-To: <20200620082214.928028424@linuxfoundation.org>


On 20/06/2020 09:22, Greg Kroah-Hartman wrote:
> This is the start of the stable review cycle for the 4.19.129 release.
> There are 265 patches in this series, all will be posted as a response
> to this one.  If anyone has any issues with these being applied, please
> let me know.
> 
> Responses should be made by Mon, 22 Jun 2020 08:21:23 +0000.
> Anything received after that time might be too late.
> 
> The whole patch series can be found in one patch at:
> 	https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.19.129-rc2.gz
> or in the git tree and branch at:
> 	git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.19.y
> and the diffstat can be found below.
> 
> thanks,
> 
> greg k-h

All tests are passing for Tegra ...

Test results for stable-v4.19:
    11 builds:	11 pass, 0 fail
    22 boots:	22 pass, 0 fail
    38 tests:	38 pass, 0 fail

Linux version:	4.19.129-rc2-g7e6addf7237f
Boards tested:	tegra124-jetson-tk1, tegra186-p2771-0000,
                tegra194-p2972-0000, tegra20-ventana,
                tegra210-p2371-2180, tegra30-cardhu-a04

Cheers
Jon

-- 
nvpublic

^ permalink raw reply

* Re: [PATCH 4.14 000/190] 4.14.185-rc1 review
From: Jon Hunter @ 2020-06-20  9:50 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
	akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
	linux-0h96xk9xTtrk1uMJSBkQmQ, shuah-DgEjT+Ai2ygdnm+yROfE0A,
	patches-ssFOTAMYnuFg9hUCZPvPmw,
	ben.hutchings-4yDnlxn2s6sWdaTGBSpHTA,
	lkft-triage-cunTk1MwBs8s++Sfvej+rw, stable-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra
In-Reply-To: <20200619141633.446429600-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>


On 19/06/2020 15:30, Greg Kroah-Hartman wrote:
> This is the start of the stable review cycle for the 4.14.185 release.
> There are 190 patches in this series, all will be posted as a response
> to this one.  If anyone has any issues with these being applied, please
> let me know.
> 
> Responses should be made by Sun, 21 Jun 2020 14:15:50 +0000.
> Anything received after that time might be too late.
> 
> The whole patch series can be found in one patch at:
> 	https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.14.185-rc1.gz
> or in the git tree and branch at:
> 	git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.14.y
> and the diffstat can be found below.
> 
> thanks,
> 
> greg k-h

All tests are passing for Tegra ...

Test results for stable-v4.14:
    8 builds:	8 pass, 0 fail
    16 boots:	16 pass, 0 fail
    30 tests:	30 pass, 0 fail

Linux version:	4.14.185-rc1-ge26bcff6a5af
Boards tested:	tegra124-jetson-tk1, tegra20-ventana,
                tegra210-p2371-2180, tegra30-cardhu-a04

Cheers
Jon

-- 
nvpublic

^ permalink raw reply

* Re: [PATCH 4.9 000/128] 4.9.228-rc1 review
From: Jon Hunter @ 2020-06-20  9:50 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-kernel
  Cc: torvalds, akpm, linux, shuah, patches, ben.hutchings, lkft-triage,
	stable, linux-tegra
In-Reply-To: <20200619141620.148019466@linuxfoundation.org>


On 19/06/2020 15:31, Greg Kroah-Hartman wrote:
> This is the start of the stable review cycle for the 4.9.228 release.
> There are 128 patches in this series, all will be posted as a response
> to this one.  If anyone has any issues with these being applied, please
> let me know.
> 
> Responses should be made by Sun, 21 Jun 2020 14:15:50 +0000.
> Anything received after that time might be too late.
> 
> The whole patch series can be found in one patch at:
> 	https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.9.228-rc1.gz
> or in the git tree and branch at:
> 	git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.9.y
> and the diffstat can be found below.
> 
> thanks,
> 
> greg k-h

All tests are passing for Tegra ...

Test results for stable-v4.9:
    8 builds:	8 pass, 0 fail
    16 boots:	16 pass, 0 fail
    30 tests:	30 pass, 0 fail

Linux version:	4.9.228-rc1-gd0cfa25033bf
Boards tested:	tegra124-jetson-tk1, tegra20-ventana,
                tegra210-p2371-2180, tegra30-cardhu-a04

Cheers
Jon

-- 
nvpublic

^ permalink raw reply

* Re: [PATCH 4.4 000/101] 4.4.228-rc1 review
From: Jon Hunter @ 2020-06-20  9:49 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
	akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
	linux-0h96xk9xTtrk1uMJSBkQmQ, shuah-DgEjT+Ai2ygdnm+yROfE0A,
	patches-ssFOTAMYnuFg9hUCZPvPmw,
	ben.hutchings-4yDnlxn2s6sWdaTGBSpHTA,
	lkft-triage-cunTk1MwBs8s++Sfvej+rw, stable-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra
In-Reply-To: <20200619141614.001544111-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>


On 19/06/2020 15:31, Greg Kroah-Hartman wrote:
> This is the start of the stable review cycle for the 4.4.228 release.
> There are 101 patches in this series, all will be posted as a response
> to this one.  If anyone has any issues with these being applied, please
> let me know.
> 
> Responses should be made by Sun, 21 Jun 2020 14:15:50 +0000.
> Anything received after that time might be too late.
> 
> The whole patch series can be found in one patch at:
> 	https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.4.228-rc1.gz
> or in the git tree and branch at:
> 	git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.4.y
> and the diffstat can be found below.
> 
> thanks,
> 
> greg k-h


All tests are passing for Tegra ...

Test results for stable-v4.4:
    6 builds:	6 pass, 0 fail
    12 boots:	12 pass, 0 fail
    25 tests:	25 pass, 0 fail

Linux version:	4.4.228-rc1-g2e99a284d540
Boards tested:	tegra124-jetson-tk1, tegra20-ventana,
                tegra30-cardhu-a04

Cheers
Jon

-- 
nvpublic

^ permalink raw reply

* Re: [PATCH 37/38] dt-bindings: pwm: Explicitly include pwm.yaml
From: Rob Herring @ 2020-06-19 18:05 UTC (permalink / raw)
  To: Thierry Reding; +Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, linux-tegra
In-Reply-To: <20200619074654.GD3704347@ulmo>

On Fri, Jun 19, 2020 at 1:47 AM Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> On Wed, Jun 17, 2020 at 08:51:40PM -0600, Rob Herring wrote:
> > On Fri, Jun 12, 2020 at 04:19:02PM +0200, Thierry Reding wrote:
> > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > >
> > > For PWM controller device tree bindings, make sure that they include the
> > > pwm.yaml controller core bindings explicitly. This prevents the tooling
> > > from matching on the $nodename pattern, which can falsely match things
> > > like pinmux nodes, etc.
> >
> > My preference here is to clean-up the mess that is pinmux nodes.
>
> Any suggestions on how to do that? Do you just want to rename the
> problematic nodes? Or do you want to introduce a standard naming scheme?

.*-pins$ is what I've been using/proposing. Doing that also helps
writing pinctrl schemas.

> As an example, I was running into the issue with this node:
>
>         pinmux@70000014 {
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&state_default>;
>
>                 state_default: pinmux {
>                         ...
>
>                         pwm-a-b {
>                                 nvidia,pins = "sdc";
>                                 nvidia,function = "pwm";
>                                 nvidia,tristate = <TEGRA_PIN_ENABLE>;
>                         };
>
>                         ...
>                 };
>         };
>
> My first instinct was to just add some sort of prefix to this, but then
> I realized that might not be the best option because there could be
> countless other nodes whose names might start with "pwm-" but that had
> nothing to do with PWM controllers whatsoever.
>
> You could for example have some node named "pwm-fan" and then these
> standard bindings will require that to be have a #pwm-cells property.

Pretty sure we only allow pwm@.* or pwm-[0-9a-f]+, so this would not match.

Plus shouldn't it be just 'fan' to be what the class is, not how it is
implemented/controlled.

> So I think the solution of only explicitly "activating" PWM controller
> bindings would work well in this particular case because it would only
> apply the bindings where explicitly requested. That way it doesn't
> matter what nodes are named.
>
> > This has the side effect of no longer checking pwm nodes that didn't
> > have explicit schema. Perhaps that's of somewhat limited value.
>
> There are two easy solutions to this: 1) convert all PWM bindings to
> YAML so that they have an explicit schema or 2) consider the presence of
> the #pwm-cells property as a marker that the node represents a PWM
> controller/provider, irrespective of the name. The latter would be much
> like gpio-controller or interrupt-controller, though less redundant.

There's only 2 things we can generically check, #pwm-cells and the
node name. If we match on one, then we're really only checking the
other one. We could match on #pwm-cells presence and then check its
value is 2 or 3, but then we can do that without a select (i.e. always
apply the schema).

So I guess I'm convinced there's not much value here and we should
just do 1). Patches welcome. :) (BTW, I do think we should do some
mass conversions by class. That I think would be a bit more efficient
in both converting and reviewing. My calculation is something like 6
years to finish (3K bindings left and doing 100-150 a cycle).)

> We could even go as far as using #pwm-cells as the definitive marker and
> then require that it has a certain name, like we do for other types of
> nodes. I did a quick audit and came up with the following results. These
> are all the PWM controller nodes that I could find that don't follow the
> "^pwm(@.*)?$" pattern. The files are only one example of where I found
> them and there were often others that used the same pattern.
>
>  - arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi
>      - ec-pwm
>
>        It should be trivial to rename these to just "pwm" since I don't
>        see the cros-ec driver relying on the exact name.
>
>  - arch/arm/boot/dts/am5729-beagleboneai.dts
>      - stmpe_pwm
>
>        The stmpe MFD driver actually relies on this name, so not sure if
>        there's a lot we can do about that.

That's unfortunate...

The question is how do we allow this, but at the same time prevent more cases.

>  - arch/arm/boot/dts/armada-38x.dtsi
>      - gpio@...
>
>        This is both a GPIO and PWM controller, so can't really do much
>        about the name.

In general, we need some solution for the more than 1 function nodes.
Combo clock and reset controllers are a common one.

>  - arch/arm/boot/dts/at91-kizbox.dts
>      - pwm
>
>        Actually also matches the pattern because the '@.*' part is
>        optional.
>
>  - arch/arm/boot/dts/at91sam9n12.dtsi
>      - hlcdc-pwm
>
>        The MFD driver matches on the compatible string, so we should be
>        able to just rename this to "pwm".
>
>  - arch/arm/boot/dts/da850.dtsi
>     - ecap@...
>
>       No matching on the name as far as I can tell, so we should be able
>       to rename this 'pwm@...'.
>
>  - arch/arm/boot/dts/logicpd-torpedo-baseboard.dtsi
>     - dmtimer-pwm
>
>       Could probably be renamed 'pwm'.
>
>  - arch/arm/boot/dts/lpc32xx.dtsi
>     - mpwm@...
>
>       Could probably be renamed 'pwm'.
>
>  - arch/arm/boot/dts/motorola-mapphone-common.dtsi
>     - dmtimer-pwm-*
>
>       Maybe these should be renamed 'pwm@*' instead?
>
>  - arch/arm/boot/dts/s3c24xx.dtsi
>     - timer@...
>
>       This is a variant similar to dmtimer-pwm above and is driven by a
>       timer that can run in PWM mode. I think this is the same category
>       as the GPIO/PWM controller hybrid above.
>
>       Not much we can do about the name.
>
>  - arch/arm/boot/dts/stm32f429.dtsi
>     - pwm
>
>       Matches the pattern.
>
>  - arch/arm/boot/dts/twl4030.dtsi
>     - pwm
>
>       Matches the pattern.
>
>     - pwmled
>
>       Perhaps both of the above should be named 'pwm@*'? There doesn't
>       seem to be any matching on the name.
>
> For many of the above it should be possible to rename them. But then we
> will always have exceptions where we can't do that because then it might
> conflict with other bindings.
>
> Two interesting things I gathered from the above are that:
>
>   1) nothing in the above actually matches the pwm-* variant that's part
>      of the current pattern defined in pwm.yaml and which is causing the
>      problem for the pinmux nodes, so an easy solution would be to
>      simply drop that part of the pattern since it is useless anyway.
>
>   2) There are actually quite a few PWM controllers that currently are
>      not checked because of the name matching. Now I haven't actually
>      checked the reverse, i.e. to see if all nodes matching the pattern
>      actually have a #pwm-cells property, but given that we miss a
>      number of controller because they don't match the pattern makes me
>      think that that aspect isn't actually very helpful.
>
> All of the above makes me think even more that we should just abandon
> the idea of matching on the names for PWM controller because in some
> instances we can't change the name for backwards-compatibility or
> because the names would then conflict with other bindings.

Yes. :(

Maybe node name checks can be an optional thing to enable until we
come up with a more general way to opt-in/out of specific checks.

Rob

^ permalink raw reply

* [PATCH v7 31/36] staging: tegra-vde: fix common struct sg_table related issues
From: Marek Szyprowski @ 2020-06-19 10:36 UTC (permalink / raw)
  To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	linaro-mm-sig-cunTk1MwBs8s++Sfvej+rw,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, Bartlomiej Zolnierkiewicz,
	David Airlie, Greg Kroah-Hartman,
	linux-media-u79uwXL29TY76Z2rM5mHXA, Jonathan Hunter,
	Thierry Reding, Daniel Vetter, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	Dmitry Osipenko, Mauro Carvalho Chehab, Robin Murphy,
	Christoph Hellwig,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20200619103636.11974-1-m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.

To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Reviewed-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/staging/media/tegra-vde/iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/tegra-vde/iommu.c b/drivers/staging/media/tegra-vde/iommu.c
index 6af863d92123..adf8dc7ee25c 100644
--- a/drivers/staging/media/tegra-vde/iommu.c
+++ b/drivers/staging/media/tegra-vde/iommu.c
@@ -36,8 +36,8 @@ int tegra_vde_iommu_map(struct tegra_vde *vde,
 
 	addr = iova_dma_addr(&vde->iova, iova);
 
-	size = iommu_map_sg(vde->domain, addr, sgt->sgl, sgt->nents,
-			    IOMMU_READ | IOMMU_WRITE);
+	size = iommu_map_sgtable(vde->domain, addr, sgt,
+				 IOMMU_READ | IOMMU_WRITE);
 	if (!size) {
 		__free_iova(&vde->iova, iova);
 		return -ENXIO;
-- 
2.17.1

^ permalink raw reply related

* [PATCH v7 26/36] drm: host1x: fix common struct sg_table related issues
From: Marek Szyprowski @ 2020-06-19 10:36 UTC (permalink / raw)
  To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	linaro-mm-sig-cunTk1MwBs8s++Sfvej+rw,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Bartlomiej Zolnierkiewicz, David Airlie, Thierry Reding,
	Daniel Vetter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Robin Murphy,
	Christoph Hellwig,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20200619103636.11974-1-m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.

To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/gpu/host1x/job.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index a10643aa89aa..4832b57f10c4 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -166,11 +166,9 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 				goto unpin;
 			}
 
-			err = dma_map_sg(dev, sgt->sgl, sgt->nents, dir);
-			if (!err) {
-				err = -ENOMEM;
+			err = dma_map_sgtable(dev, sgt, dir, 0);
+			if (err)
 				goto unpin;
-			}
 
 			job->unpins[job->num_unpins].dev = dev;
 			job->unpins[job->num_unpins].dir = dir;
@@ -217,7 +215,7 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 		}
 
 		if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && host->domain) {
-			for_each_sg(sgt->sgl, sg, sgt->nents, j)
+			for_each_sgtable_sg(sgt, sg, j)
 				gather_size += sg->length;
 			gather_size = iova_align(&host->iova, gather_size);
 
@@ -229,9 +227,9 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 				goto unpin;
 			}
 
-			err = iommu_map_sg(host->domain,
+			err = iommu_map_sgtable(host->domain,
 					iova_dma_addr(&host->iova, alloc),
-					sgt->sgl, sgt->nents, IOMMU_READ);
+					sgt, IOMMU_READ);
 			if (err == 0) {
 				__free_iova(&host->iova, alloc);
 				err = -EINVAL;
@@ -241,12 +239,9 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 			job->unpins[job->num_unpins].size = gather_size;
 			phys_addr = iova_dma_addr(&host->iova, alloc);
 		} else if (sgt) {
-			err = dma_map_sg(host->dev, sgt->sgl, sgt->nents,
-					 DMA_TO_DEVICE);
-			if (!err) {
-				err = -ENOMEM;
+			err = dma_map_sgtable(host->dev, sgt, DMA_TO_DEVICE, 0);
+			if (err)
 				goto unpin;
-			}
 
 			job->unpins[job->num_unpins].dir = DMA_TO_DEVICE;
 			job->unpins[job->num_unpins].dev = host->dev;
@@ -647,8 +642,7 @@ void host1x_job_unpin(struct host1x_job *job)
 		}
 
 		if (unpin->dev && sgt)
-			dma_unmap_sg(unpin->dev, sgt->sgl, sgt->nents,
-				     unpin->dir);
+			dma_unmap_sgtable(unpin->dev, sgt, unpin->dir, 0);
 
 		host1x_bo_unpin(dev, unpin->bo, sgt);
 		host1x_bo_put(unpin->bo);
-- 
2.17.1

^ permalink raw reply related

* [PATCH v7 20/36] drm: tegra: fix common struct sg_table related issues
From: Marek Szyprowski @ 2020-06-19 10:36 UTC (permalink / raw)
  To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	linaro-mm-sig-cunTk1MwBs8s++Sfvej+rw,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Bartlomiej Zolnierkiewicz, David Airlie, Jonathan Hunter,
	Thierry Reding, Daniel Vetter, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	Robin Murphy, Christoph Hellwig,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20200619103636.11974-1-m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.

To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/gpu/drm/tegra/gem.c   | 27 ++++++++++-----------------
 drivers/gpu/drm/tegra/plane.c | 15 +++++----------
 2 files changed, 15 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
index 723df142a981..01d94befab11 100644
--- a/drivers/gpu/drm/tegra/gem.c
+++ b/drivers/gpu/drm/tegra/gem.c
@@ -98,8 +98,8 @@ static struct sg_table *tegra_bo_pin(struct device *dev, struct host1x_bo *bo,
 		 * the SG table needs to be copied to avoid overwriting any
 		 * other potential users of the original SG table.
 		 */
-		err = sg_alloc_table_from_sg(sgt, obj->sgt->sgl, obj->sgt->nents,
-					     GFP_KERNEL);
+		err = sg_alloc_table_from_sg(sgt, obj->sgt->sgl,
+					     obj->sgt->orig_nents, GFP_KERNEL);
 		if (err < 0)
 			goto free;
 	} else {
@@ -196,8 +196,7 @@ static int tegra_bo_iommu_map(struct tegra_drm *tegra, struct tegra_bo *bo)
 
 	bo->iova = bo->mm->start;
 
-	bo->size = iommu_map_sg(tegra->domain, bo->iova, bo->sgt->sgl,
-				bo->sgt->nents, prot);
+	bo->size = iommu_map_sgtable(tegra->domain, bo->iova, bo->sgt, prot);
 	if (!bo->size) {
 		dev_err(tegra->drm->dev, "failed to map buffer\n");
 		err = -ENOMEM;
@@ -264,8 +263,7 @@ static struct tegra_bo *tegra_bo_alloc_object(struct drm_device *drm,
 static void tegra_bo_free(struct drm_device *drm, struct tegra_bo *bo)
 {
 	if (bo->pages) {
-		dma_unmap_sg(drm->dev, bo->sgt->sgl, bo->sgt->nents,
-			     DMA_FROM_DEVICE);
+		dma_unmap_sgtable(drm->dev, bo->sgt, DMA_FROM_DEVICE, 0);
 		drm_gem_put_pages(&bo->gem, bo->pages, true, true);
 		sg_free_table(bo->sgt);
 		kfree(bo->sgt);
@@ -290,12 +288,9 @@ static int tegra_bo_get_pages(struct drm_device *drm, struct tegra_bo *bo)
 		goto put_pages;
 	}
 
-	err = dma_map_sg(drm->dev, bo->sgt->sgl, bo->sgt->nents,
-			 DMA_FROM_DEVICE);
-	if (err == 0) {
-		err = -EFAULT;
+	err = dma_map_sgtable(drm->dev, bo->sgt, DMA_FROM_DEVICE, 0);
+	if (err)
 		goto free_sgt;
-	}
 
 	return 0;
 
@@ -571,7 +566,7 @@ tegra_gem_prime_map_dma_buf(struct dma_buf_attachment *attach,
 			goto free;
 	}
 
-	if (dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir) == 0)
+	if (dma_map_sgtable(attach->dev, sgt, dir, 0))
 		goto free;
 
 	return sgt;
@@ -590,7 +585,7 @@ static void tegra_gem_prime_unmap_dma_buf(struct dma_buf_attachment *attach,
 	struct tegra_bo *bo = to_tegra_bo(gem);
 
 	if (bo->pages)
-		dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+		dma_unmap_sgtable(attach->dev, sgt, dir, 0);
 
 	sg_free_table(sgt);
 	kfree(sgt);
@@ -609,8 +604,7 @@ static int tegra_gem_prime_begin_cpu_access(struct dma_buf *buf,
 	struct drm_device *drm = gem->dev;
 
 	if (bo->pages)
-		dma_sync_sg_for_cpu(drm->dev, bo->sgt->sgl, bo->sgt->nents,
-				    DMA_FROM_DEVICE);
+		dma_sync_sgtable_for_cpu(drm->dev, bo->sgt, DMA_FROM_DEVICE);
 
 	return 0;
 }
@@ -623,8 +617,7 @@ static int tegra_gem_prime_end_cpu_access(struct dma_buf *buf,
 	struct drm_device *drm = gem->dev;
 
 	if (bo->pages)
-		dma_sync_sg_for_device(drm->dev, bo->sgt->sgl, bo->sgt->nents,
-				       DMA_TO_DEVICE);
+		dma_sync_sgtable_for_device(drm->dev, bo->sgt, DMA_TO_DEVICE);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/tegra/plane.c b/drivers/gpu/drm/tegra/plane.c
index 9ccfb56e9b01..0d2ef1662a39 100644
--- a/drivers/gpu/drm/tegra/plane.c
+++ b/drivers/gpu/drm/tegra/plane.c
@@ -130,12 +130,9 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct tegra_plane_state *state)
 		}
 
 		if (sgt) {
-			err = dma_map_sg(dc->dev, sgt->sgl, sgt->nents,
-					 DMA_TO_DEVICE);
-			if (err == 0) {
-				err = -ENOMEM;
+			err = dma_map_sgtable(dc->dev, sgt, DMA_TO_DEVICE, 0);
+			if (err)
 				goto unpin;
-			}
 
 			/*
 			 * The display controller needs contiguous memory, so
@@ -143,7 +140,7 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct tegra_plane_state *state)
 			 * map its SG table to a single contiguous chunk of
 			 * I/O virtual memory.
 			 */
-			if (err > 1) {
+			if (sgt->nents > 1) {
 				err = -EINVAL;
 				goto unpin;
 			}
@@ -165,8 +162,7 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct tegra_plane_state *state)
 		struct sg_table *sgt = state->sgt[i];
 
 		if (sgt)
-			dma_unmap_sg(dc->dev, sgt->sgl, sgt->nents,
-				     DMA_TO_DEVICE);
+			dma_unmap_sgtable(dc->dev, sgt, DMA_TO_DEVICE, 0);
 
 		host1x_bo_unpin(dc->dev, &bo->base, sgt);
 		state->iova[i] = DMA_MAPPING_ERROR;
@@ -185,8 +181,7 @@ static void tegra_dc_unpin(struct tegra_dc *dc, struct tegra_plane_state *state)
 		struct sg_table *sgt = state->sgt[i];
 
 		if (sgt)
-			dma_unmap_sg(dc->dev, sgt->sgl, sgt->nents,
-				     DMA_TO_DEVICE);
+			dma_unmap_sgtable(dc->dev, sgt, DMA_TO_DEVICE, 0);
 
 		host1x_bo_unpin(dc->dev, &bo->base, sgt);
 		state->iova[i] = DMA_MAPPING_ERROR;
-- 
2.17.1

^ permalink raw reply related

* Re: [PATCH 07/38] dt-bindings: display: tegra: Convert to json-schema
From: Thierry Reding @ 2020-06-19  8:08 UTC (permalink / raw)
  To: Rob Herring; +Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, linux-tegra
In-Reply-To: <CAL_JsqLfpSgJxRMNTFdAsSEVOTU6a7bzD8v8Sg1LPXHdgEmdAQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 8752 bytes --]

On Thu, Jun 18, 2020 at 09:23:58AM -0600, Rob Herring wrote:
> On Thu, Jun 18, 2020 at 8:16 AM Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> > On Wed, Jun 17, 2020 at 05:13:26PM -0600, Rob Herring wrote:
> > > On Fri, Jun 12, 2020 at 04:18:32PM +0200, Thierry Reding wrote:
> > > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > >
> > > > Convert the Tegra host1x controller bindings from the free-form text
> > > > format to json-schema.
> > > >
> > > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > ---
> > > >  .../display/tegra/nvidia,tegra20-host1x.txt   |  516 ------
> > > >  .../display/tegra/nvidia,tegra20-host1x.yaml  | 1418 +++++++++++++++++
> > > >  2 files changed, 1418 insertions(+), 516 deletions(-)
> > > >  delete mode 100644 Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
> > > >  create mode 100644 Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
> 
> [...]
> 
> > > > +  - if:
> > > > +      properties:
> > > > +        compatible:
> > > > +          contains:
> > > > +            enum:
> > > > +              - nvidia,tegra124-host1x
> > > > +              - nvidia,tegra210-host1x
> > > > +              - nvidia,tegra186-host1x
> > > > +              - nvidia,tegra194-host1x
> > > > +    then:
> > > > +      patternProperties:
> > > > +        "^sor@[0-9a-f]+$":
> > > > +          description: |
> > > > +            The Serial Output Resource (SOR) can be used to drive HDMI, LVDS,
> > > > +            eDP and DP outputs.
> > > > +
> > > > +            See ../pinctrl/nvidia,tegra124-dpaux-padctl.txt for information
> > > > +            regarding the DPAUX pad controller bindings.
> > > > +          type: object
> > > > +          properties:
> > > > +            # required
> > > > +            compatible:
> > > > +              oneOf:
> > > > +                - const: nvidia,tegra124-sor
> > > > +                - items:
> > > > +                    - const: nvidia,tegra132-sor
> > > > +                    - const: nvidia,tegra124-sor
> > > > +                - const: nvidia,tegra210-sor
> > > > +                - const: nvidia,tegra210-sor1
> > > > +                - const: nvidia,tegra186-sor
> > > > +                - const: nvidia,tegra186-sor1
> > > > +                - const: nvidia,tegra194-sor
> > > > +
> > > > +            reg:
> > > > +              maxItems: 1
> > > > +
> > > > +            interrupts:
> > > > +              maxItems: 1
> > > > +
> > > > +            resets:
> > > > +              items:
> > > > +                - description: module reset
> > > > +
> > > > +            reset-names:
> > > > +              items:
> > > > +                - const: sor
> > > > +
> > > > +            status:
> > > > +              $ref: "/schemas/dt-core.yaml#/properties/status"
> > >
> > > 'status' should never need to be listed.
> >
> > This seems to be needed at least when I try to validate against a single
> > binding, like so:
> >
> >         $ make DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml dtbs_check
> >
> > I assume that that somehow prevents the tooling from looking at any of
> > the other bindings, which in turn then causes status and other standard
> > properties to never be defined and then it flags them as extra and
> > causes a failure.
> 
> I'm surprised using DT_SCHEMA_FILES makes a difference. I'm guessing
> that has your 'unevaluatedProperties' support. If so, that means
> there's an unintended side effect that any common schema property
> becomes always allowed. That's good for 'status' and 'phandle', but
> not so much for 'reg', '*-gpios, '*-names', etc.

I don't think that's an unintended side-effect. If the property
validates against a schema it counts as evaluated, doesn't it? In order
to avoid that we would somehow have to restrict which schemas contribute
to the evaluatedProperties annotation and I don't think there's a way to
do that because we don't know which out of all the schemas is relevant.

> > I think I've even seen this trigger on dt_binding_check if I happened to
> > have status in there. Now, you've mentioned elsewhere that we shouldn't
> > use "status" in examples, so that would work around this. However, I
> > think I've seen this happen as well in examples that referenced some
> > node via phandle, and then dt_binding_check would emit an error about
> > phandle being undefined.
> >
> > Perhaps this is a problem with the tooling? Should we instruct the
> > scripts to always include the core schema even if we're only testing a
> > single YAML file via DT_SCHEMA_FILES?
> 
> The purpose of DT_SCHEMA_FILES is to see warnings just from that
> schema file. If the core schema was warning free, we could add that,
> but it's not. Plus that wouldn't solve the problem here. 'status' and
> 'phandle' are added to each schema by the tooling (along with other
> things), not by another schema file (well, they are in another schema
> file, but they are added to each schema so that 'additionalProperties:
> false' works).
> 
> This is certainly a limitation in the tooling in that what you have is
> a bit different from the expected form. Generally it is expected that
> everything is defined under the top-level 'properties' and then any
> 'if/then' schema only add further constraints. However, you have the
> child nodes only defined under an if/then. We could fix that, but I'm
> not sure I want to. IMO, extensive use of if/then is a sign the schema
> should be split up. More on that below.

Okay, I see your point.

> > > > +            pinctrl-names: true
> > > > +            phandle:
> > > > +              $ref: "/schemas/types.yaml#/definitions/uint32"
> > >
> > > 'phandle' shouldn't need to be listed.
> > >
> > > > +
> > > > +          patternProperties:
> > > > +            "^pinctrl-[0-9]+$": true
> > >
> > > pinctrl properties are automatically added, but maybe not if under an
> > > 'if' schema. Really, I think probably either this should be split
> > > into multiple schema files or all of these child nodes should be
> > > described at the top-level. I'm not sure it's really important to define
> > > which set of child nodes belong or not for each chip.
> >
> > I'm not too worried about the set of child nodes for each chip, but I
> > think having this all in one file underlines the importance of the
> > hierarchy. If these were discrete bindings for each of the compatible
> > strings it'd be easy for someone to create them as standalone nodes in
> > device tree, but that's not something that would work. All of these
> > devices are children of host1x and they do depend on host1x for a lot
> > of the functionality, so the hierarchy must be respected.
> 
> I'm not saying don't describe the hierarchy.
> 
> The first option is 1 host1x schema file per SoC (roughly) and the
> 'host1x' parent node would be duplicated in each one. That doesn't
> worry me too much as it's all standard properties and not that many of
> them. Though you could have a common 'host1x-bus.yaml' just describing
> the parent node properties that each <soc>-host1x.yaml references.
> 
> The 2nd option is keep this as a single file, but just move every
> child node definition under the top-level 'patternProperties'. This
> option has the limitation that you can't enforce which child nodes are
> valid per SoC.

Okay, I'll give the first option a try and see where I end up.

> > > I'm stopping there. I think the rest is more of the same comments.
> >
> > I've made a pass over the whole file and fixed the issues that you
> > pointed out above in other places.
> >
> > Sounds like the biggest remaining issue is with the duplicated standard
> > properties. I'm not a huge fan of giving up on doing the right thing
> > because the tooling can't deal with it. I think we should fix the
> > tooling to do the right thing. So if there's something in the core DT
> > schema then it should apply regardless of what mode we run in. Much of
> > the above issues should go away once that's fixed.
> >
> > Any thoughts on making some of the schema files "always included"? I
> > haven't looked at this side of the tooling at all yet, so I'm not sure
> > how difficult that would be, but if you're okay with it conceptually I
> > can take a closer look.
> 
> Hopefully, it's clear why that doesn't help here. But don't worry,
> there's plenty of other work to do on the tooling. :)

Yes, I think I understand now.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* Re: [PATCH 37/38] dt-bindings: pwm: Explicitly include pwm.yaml
From: Thierry Reding @ 2020-06-19  7:46 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200618025140.GB3378010@bogus>

[-- Attachment #1: Type: text/plain, Size: 5776 bytes --]

On Wed, Jun 17, 2020 at 08:51:40PM -0600, Rob Herring wrote:
> On Fri, Jun 12, 2020 at 04:19:02PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > For PWM controller device tree bindings, make sure that they include the
> > pwm.yaml controller core bindings explicitly. This prevents the tooling
> > from matching on the $nodename pattern, which can falsely match things
> > like pinmux nodes, etc.
> 
> My preference here is to clean-up the mess that is pinmux nodes.

Any suggestions on how to do that? Do you just want to rename the
problematic nodes? Or do you want to introduce a standard naming scheme?
As an example, I was running into the issue with this node:

	pinmux@70000014 {
		pinctrl-names = "default";
		pinctrl-0 = <&state_default>;

		state_default: pinmux {
			...

			pwm-a-b {
				nvidia,pins = "sdc";
				nvidia,function = "pwm";
				nvidia,tristate = <TEGRA_PIN_ENABLE>;
			};

			...
		};
	};

My first instinct was to just add some sort of prefix to this, but then
I realized that might not be the best option because there could be
countless other nodes whose names might start with "pwm-" but that had
nothing to do with PWM controllers whatsoever.

You could for example have some node named "pwm-fan" and then these
standard bindings will require that to be have a #pwm-cells property.

So I think the solution of only explicitly "activating" PWM controller
bindings would work well in this particular case because it would only
apply the bindings where explicitly requested. That way it doesn't
matter what nodes are named.

> This has the side effect of no longer checking pwm nodes that didn't 
> have explicit schema. Perhaps that's of somewhat limited value.

There are two easy solutions to this: 1) convert all PWM bindings to
YAML so that they have an explicit schema or 2) consider the presence of
the #pwm-cells property as a marker that the node represents a PWM
controller/provider, irrespective of the name. The latter would be much
like gpio-controller or interrupt-controller, though less redundant.

We could even go as far as using #pwm-cells as the definitive marker and
then require that it has a certain name, like we do for other types of
nodes. I did a quick audit and came up with the following results. These
are all the PWM controller nodes that I could find that don't follow the
"^pwm(@.*)?$" pattern. The files are only one example of where I found
them and there were often others that used the same pattern.

 - arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi
     - ec-pwm

       It should be trivial to rename these to just "pwm" since I don't
       see the cros-ec driver relying on the exact name.

 - arch/arm/boot/dts/am5729-beagleboneai.dts
     - stmpe_pwm

       The stmpe MFD driver actually relies on this name, so not sure if
       there's a lot we can do about that.

 - arch/arm/boot/dts/armada-38x.dtsi
     - gpio@...

       This is both a GPIO and PWM controller, so can't really do much
       about the name.

 - arch/arm/boot/dts/at91-kizbox.dts
     - pwm

       Actually also matches the pattern because the '@.*' part is
       optional.

 - arch/arm/boot/dts/at91sam9n12.dtsi
     - hlcdc-pwm

       The MFD driver matches on the compatible string, so we should be
       able to just rename this to "pwm".

 - arch/arm/boot/dts/da850.dtsi
    - ecap@...

      No matching on the name as far as I can tell, so we should be able
      to rename this 'pwm@...'.

 - arch/arm/boot/dts/logicpd-torpedo-baseboard.dtsi
    - dmtimer-pwm

      Could probably be renamed 'pwm'.

 - arch/arm/boot/dts/lpc32xx.dtsi
    - mpwm@...

      Could probably be renamed 'pwm'.

 - arch/arm/boot/dts/motorola-mapphone-common.dtsi
    - dmtimer-pwm-*

      Maybe these should be renamed 'pwm@*' instead?

 - arch/arm/boot/dts/s3c24xx.dtsi
    - timer@...

      This is a variant similar to dmtimer-pwm above and is driven by a
      timer that can run in PWM mode. I think this is the same category
      as the GPIO/PWM controller hybrid above.

      Not much we can do about the name.

 - arch/arm/boot/dts/stm32f429.dtsi
    - pwm

      Matches the pattern.

 - arch/arm/boot/dts/twl4030.dtsi
    - pwm

      Matches the pattern.

    - pwmled

      Perhaps both of the above should be named 'pwm@*'? There doesn't
      seem to be any matching on the name.

For many of the above it should be possible to rename them. But then we
will always have exceptions where we can't do that because then it might
conflict with other bindings.

Two interesting things I gathered from the above are that:

  1) nothing in the above actually matches the pwm-* variant that's part
     of the current pattern defined in pwm.yaml and which is causing the
     problem for the pinmux nodes, so an easy solution would be to
     simply drop that part of the pattern since it is useless anyway.

  2) There are actually quite a few PWM controllers that currently are
     not checked because of the name matching. Now I haven't actually
     checked the reverse, i.e. to see if all nodes matching the pattern
     actually have a #pwm-cells property, but given that we miss a
     number of controller because they don't match the pattern makes me
     think that that aspect isn't actually very helpful.

All of the above makes me think even more that we should just abandon
the idea of matching on the names for PWM controller because in some
instances we can't change the name for backwards-compatibility or
because the names would then conflict with other bindings.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* Re: [PATCH 28/38] dt-bindings: phy: tegra-xusb: Convert to json-schema
From: Thierry Reding @ 2020-06-19  6:47 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200618023826.GB3343853@bogus>

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

On Wed, Jun 17, 2020 at 08:38:26PM -0600, Rob Herring wrote:
> On Fri, Jun 12, 2020 at 04:18:53PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > Convert the Tegra XUSB pad controller bindings from free-form text
> > format to json-schema.
> > 
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  .../phy/nvidia,tegra124-xusb-padctl.txt       |  778 -----
> >  .../phy/nvidia,tegra124-xusb-padctl.yaml      | 2800 +++++++++++++++++
> >  2 files changed, 2800 insertions(+), 778 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> >  create mode 100644 Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.yaml
> 
> This one I definitely think should be split up into multiple schema 
> files. There's very little shared.

Okay, I think I can do that. It's going to be a little annoying because
then there's going to be at least four bindings to check, but I don't
think it'll be all that bad. Once the bindings are merged I think I'll
end up mostly running dtbs_check anyway, in which case the number of
files doesn't really matter.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* Re: [PATCH 26/38] dt-bindings: pci: iommu: Convert to json-schema
From: Thierry Reding @ 2020-06-19  6:45 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200618023457.GA3343853@bogus>

[-- Attachment #1: Type: text/plain, Size: 862 bytes --]

On Wed, Jun 17, 2020 at 08:34:57PM -0600, Rob Herring wrote:
> On Fri, Jun 12, 2020 at 04:18:51PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > Convert the PCI IOMMU device tree bindings from free-form text format to
> > json-schema.
> > 
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  .../devicetree/bindings/pci/pci-iommu.txt     | 171 ------------------
> >  .../devicetree/bindings/pci/pci-iommu.yaml    | 168 +++++++++++++++++
> >  2 files changed, 168 insertions(+), 171 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/pci/pci-iommu.txt
> >  create mode 100644 Documentation/devicetree/bindings/pci/pci-iommu.yaml
> 
> This needs to come before you use it.

Good point, I'll reorder it.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* Re: [PATCH 06/38] dt-bindings: display: tegra: Document display-hub
From: Thierry Reding @ 2020-06-19  6:45 UTC (permalink / raw)
  To: Rob Herring; +Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, linux-tegra
In-Reply-To: <CAL_JsqKgJa0iEfnJO9KfS=sB23bT0E0szB7SwShh2kxWEsxgdA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 21326 bytes --]

On Thu, Jun 18, 2020 at 12:17:36PM -0600, Rob Herring wrote:
> On Thu, Jun 18, 2020 at 4:27 AM Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> > On Wed, Jun 17, 2020 at 04:55:06PM -0600, Rob Herring wrote:
> > > On Fri, Jun 12, 2020 at 04:18:31PM +0200, Thierry Reding wrote:
> > > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > >
> > > > Tegra186 and later have an additional component in the display pipeline
> > > > called the display hub. Document the bindings which were missing.
> > >
> > > I'd rather this be after the conversion or I'm reviewing it twice.
> >
> > Okay, I'll reorder the patches accordingly.
> >
> > > >
> > > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > ---
> > > >  .../display/tegra/nvidia,tegra20-host1x.txt   | 50 +++++++++++++++++++
> > > >  1 file changed, 50 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
> > > > index 47319214b5f6..2cf3cc4893da 100644
> > > > --- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
> > > > +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
> > > > @@ -297,6 +297,56 @@ of the following host1x client modules:
> > > >    - reset-names: Must include the following entries:
> > > >      - vic
> > > >
> > > > +- display-hub: display controller hub
> > > > +  Required properties:
> > > > +  - compatible: "nvidia,tegra<chip>-display"
> > > > +  - reg: Physical base address and length of the controller's registers.
> > > > +  - interrupts: The interrupt outputs from the controller.
> > > > +  - clocks: Must contain an entry for each entry in clock-names.
> > > > +    See ../clocks/clock-bindings.txt for details.
> > > > +  - clock-names: Must include the following entries:
> > > > +    - disp
> > > > +    - dsc
> > > > +    - hub
> > > > +  - resets: Must contain an entry for each entry in reset-names.
> > > > +    See ../reset/reset.txt for details.
> > > > +  - reset-names: Must include the following entries:
> > > > +    - misc
> > > > +    - wgrp0
> > > > +    - wgrp1
> > > > +    - wgrp2
> > > > +    - wgrp3
> > > > +    - wgrp4
> > > > +    - wgrp5
> > > > +  - power-domains: A list of phandle and specifiers identifying the power
> > > > +    domains that the display hub is part of.
> > > > +  - ranges: Range of registers used for the display controllers.
> > > > +
> > > > +  Each subnode of the display hub represents one of the display controllers
> > > > +  available:
> > > > +
> > > > +  - display: display controller
> > > > +    - compatible: "nvidia,tegra<chip>-dc"
> > > > +    - reg: Physical base address and length of the controller's registers.
> > > > +    - interrupts: The interrupt outputs from the controller.
> > > > +    - clocks: Must contain an entry for each entry in clock-names.
> > > > +      See ../clocks/clock-bindings.txt for details.
> > > > +    - clock-names: Must include the following entries:
> > > > +      - dc
> > > > +    - resets: Must contain an entry for each entry in reset-names.
> > > > +      See ../reset/reset.txt for details.
> > > > +    - reset-names: Must include the following entries:
> > > > +      - dc
> > > > +    - power-domains: A list of phandle and specifiers that identify the power
> > > > +      domains that this display controller is part of.
> > > > +    - iommus: A phandle and specifier identifying the SMMU master interface of
> > > > +      this display controller.
> > > > +    - nvidia,outputs: A list of phandles of outputs that this display
> > > > +      controller can drive.
> > >
> > > Seems like an OF graph should describe this?
> >
> > The above documents the current state of affairs. I don't recall exactly
> > why we never merged the bindings, but we've been using this
> > nvidia,outputs property for almost three years now. Changing this would
> > break ABI, although I guess you could say that since this was never
> > documented it can't be ABI. Still, changing this is going to cause old
> > device trees to fail with new kernels. Unless of course if we add some
> > backwards-compatibility mechanism in the driver. But in that case, what
> > exactly do we gain by switching to an OF graph?
> 
> Probably nothing at this point. More I was just curious how we ended
> up with something different.

So does that mean yes or no? Do you want me to proceed with what's
currently used or should I switch to the OF graph version?

> > Historically, I think nvidia,outputs was introduced before OF graphs
> > were "a thing", at least in DRM. According to the git log, the helpers
> > for graphs were introduced a couple of years before nvidia,outputs was
> > used, but I guess they must not have been widespread enough for me to
> > have been aware of them.
> 
> There was a period display subsystem bindings were pretty much un-reviewed...
> 
> > Anyway, irrespective of the compatibility issues, I tried to use an OF
> > graph to describe this and here's what I came up with:
> >
> > --- >8 ---
> >  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 170 ++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/tegra/dc.c               |  15 +--
> >  drivers/gpu/drm/tegra/dc.h               |   1 -
> >  drivers/gpu/drm/tegra/output.c           |  12 +--
> >  4 files changed, 172 insertions(+), 26 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > index 58100fb9cd8b..a3dcf2437976 100644
> > --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > @@ -994,8 +994,38 @@ display@15200000 {
> >                                 power-domains = <&bpmp TEGRA186_POWER_DOMAIN_DISP>;
> >                                 iommus = <&smmu TEGRA186_SID_NVDISPLAY>;
> >
> > -                               nvidia,outputs = <&dsia &dsib &sor0 &sor1>;
> >                                 nvidia,head = <0>;
> > +
> > +                               ports {
> > +                                       #address-cells = <1>;
> > +                                       #size-cells = <0>;
> > +
> > +                                       dc0_out: port@0 {
> > +                                               #address-cells = <1>;
> > +                                               #size-cells = <0>;
> > +                                               reg = <0>;
> > +
> > +                                               dc0_out_dsia: endpoint@0 {
> > +                                                       reg = <0>;
> > +                                                       remote-endpoint = <&dsia_in_dc0>;
> > +                                               };
> > +
> > +                                               dc0_out_dsib: endpoint@1 {
> > +                                                       reg = <1>;
> > +                                                       remote-endpoint = <&dsib_in_dc0>;
> > +                                               };
> > +
> > +                                               dc0_out_sor0: endpoint@2 {
> > +                                                       reg = <2>;
> > +                                                       remote-endpoint = <&sor0_in_dc0>;
> > +                                               };
> > +
> > +                                               dc0_out_sor1: endpoint@3 {
> > +                                                       reg = <3>;
> > +                                                       remote-endpoint = <&sor1_in_dc0>;
> > +                                               };
> > +                                       };
> > +                               };
> >                         };
> >
> >                         display@15210000 {
> > @@ -1010,8 +1040,38 @@ display@15210000 {
> >                                 power-domains = <&bpmp TEGRA186_POWER_DOMAIN_DISPB>;
> >                                 iommus = <&smmu TEGRA186_SID_NVDISPLAY>;
> >
> > -                               nvidia,outputs = <&dsia &dsib &sor0 &sor1>;
> >                                 nvidia,head = <1>;
> > +
> > +                               ports {
> > +                                       #address-cells = <1>;
> > +                                       #size-cells = <0>;
> > +
> > +                                       dc1_out: port@0 {
> > +                                               #address-cells = <1>;
> > +                                               #size-cells = <0>;
> > +                                               reg = <0>;
> > +
> > +                                               dc1_out_dsia: endpoint@0 {
> > +                                                       reg = <0>;
> > +                                                       remote-endpoint = <&dsia_in_dc1>;
> > +                                               };
> > +
> > +                                               dc1_out_dsib: endpoint@1 {
> > +                                                       reg = <1>;
> > +                                                       remote-endpoint = <&dsib_in_dc1>;
> > +                                               };
> > +
> > +                                               dc1_out_sor0: endpoint@2 {
> > +                                                       reg = <2>;
> > +                                                       remote-endpoint = <&sor0_in_dc1>;
> > +                                               };
> > +
> > +                                               dc1_out_sor1: endpoint@3 {
> > +                                                       reg = <3>;
> > +                                                       remote-endpoint = <&sor1_in_dc1>;
> > +                                               };
> > +                                       };
> > +                               };
> >                         };
> >
> >                         display@15220000 {
> > @@ -1026,8 +1086,28 @@ display@15220000 {
> >                                 power-domains = <&bpmp TEGRA186_POWER_DOMAIN_DISPC>;
> >                                 iommus = <&smmu TEGRA186_SID_NVDISPLAY>;
> >
> > -                               nvidia,outputs = <&sor0 &sor1>;
> >                                 nvidia,head = <2>;
> > +
> > +                               ports {
> > +                                       #address-cells = <1>;
> > +                                       #size-cells = <0>;
> > +
> > +                                       dc2_out: port@0 {
> > +                                               #address-cells = <1>;
> > +                                               #size-cells = <0>;
> > +                                               reg = <0>;
> > +
> > +                                               dc2_out_sor0: endpoint@0 {
> > +                                                       reg = <0>;
> > +                                                       remote-endpoint = <&sor0_in_dc2>;
> > +                                               };
> > +
> > +                                               dc2_out_sor1: endpoint@1 {
> > +                                                       reg = <1>;
> > +                                                       remote-endpoint = <&sor1_in_dc2>;
> > +                                               };
> > +                                       };
> > +                               };
> >                         };
> >                 };
> >
> > @@ -1044,6 +1124,25 @@ dsia: dsi@15300000 {
> >                         status = "disabled";
> >
> >                         power-domains = <&bpmp TEGRA186_POWER_DOMAIN_DISP>;
> > +
> > +                       ports {
> > +                               #address-cells = <1>;
> > +                               #size-cells = <0>;
> > +
> > +                               dsia_in: port@0 {
> > +                                       #address-cells = <1>;
> > +                                       #size-cells = <0>;
> > +                                       reg = <0>;
> > +
> > +                                       dsia_in_dc0: endpoint@0 {
> > +                                               remote-endpoint = <&dc0_out_dsia>;
> > +                                       };
> > +
> > +                                       dsia_in_dc1: endpoint@1 {
> > +                                               remote-endpoint = <&dc1_out_dsia>;
> > +                                       };
> > +                               };
> > +                       };
> >                 };
> >
> >                 vic@15340000 {
> > @@ -1072,6 +1171,25 @@ dsib: dsi@15400000 {
> >                         status = "disabled";
> >
> >                         power-domains = <&bpmp TEGRA186_POWER_DOMAIN_DISP>;
> > +
> > +                       ports {
> > +                               #address-cells = <1>;
> > +                               #size-cells = <0>;
> > +
> > +                               dsib_in: port@0 {
> > +                                       #address-cells = <1>;
> > +                                       #size-cells = <0>;
> > +                                       reg = <0>;
> > +
> > +                                       dsib_in_dc0: endpoint@0 {
> > +                                               remote-endpoint = <&dc0_out_dsib>;
> > +                                       };
> > +
> > +                                       dsib_in_dc1: endpoint@1 {
> > +                                               remote-endpoint = <&dc1_out_dsib>;
> > +                                       };
> > +                               };
> > +                       };
> >                 };
> >
> >                 sor0: sor@15540000 {
> > @@ -1096,6 +1214,29 @@ sor0: sor@15540000 {
> >
> >                         power-domains = <&bpmp TEGRA186_POWER_DOMAIN_DISP>;
> >                         nvidia,interface = <0>;
> > +
> > +                       ports {
> > +                               #address-cells = <1>;
> > +                               #size-cells = <0>;
> > +
> > +                               sor0_in: port@0 {
> > +                                       #address-cells = <1>;
> > +                                       #size-cells = <0>;
> > +                                       reg = <0>;
> > +
> > +                                       sor0_in_dc0: endpoint@0 {
> > +                                               remote-endpoint = <&dc0_out_sor0>;
> > +                                       };
> > +
> > +                                       sor0_in_dc1: endpoint@1 {
> > +                                               remote-endpoint = <&dc1_out_sor0>;
> > +                                       };
> > +
> > +                                       sor0_in_dc2: endpoint@2 {
> > +                                               remote-endpoint = <&dc2_out_sor0>;
> > +                                       };
> > +                               };
> > +                       };
> >                 };
> >
> >                 sor1: sor@15580000 {
> > @@ -1120,6 +1261,29 @@ sor1: sor@15580000 {
> >
> >                         power-domains = <&bpmp TEGRA186_POWER_DOMAIN_DISP>;
> >                         nvidia,interface = <1>;
> > +
> > +                       ports {
> > +                               #address-cells = <1>;
> > +                               #size-cells = <0>;
> > +
> > +                               sor1_in: port@0 {
> > +                                       #address-cells = <1>;
> > +                                       #size-cells = <0>;
> > +                                       reg = <0>;
> > +
> > +                                       sor1_in_dc0: endpoint@0 {
> > +                                               remote-endpoint = <&dc0_out_sor1>;
> > +                                       };
> > +
> > +                                       sor1_in_dc1: endpoint@1 {
> > +                                               remote-endpoint = <&dc1_out_sor1>;
> > +                                       };
> > +
> > +                                       sor1_in_dc2: endpoint@2 {
> > +                                               remote-endpoint = <&dc2_out_sor1>;
> > +                                       };
> > +                               };
> > +                       };
> >                 };
> >
> >                 dpaux: dpaux@155c0000 {
> > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > index 04d6848d19fc..4adb64c083c8 100644
> > --- a/drivers/gpu/drm/tegra/dc.c
> > +++ b/drivers/gpu/drm/tegra/dc.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/iommu.h>
> >  #include <linux/module.h>
> >  #include <linux/of_device.h>
> > +#include <linux/of_graph.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/reset.h>
> >
> > @@ -86,19 +87,6 @@ static inline void tegra_plane_writel(struct tegra_plane *plane, u32 value,
> >         tegra_dc_writel(plane->dc, value, tegra_plane_offset(plane, offset));
> >  }
> >
> > -bool tegra_dc_has_output(struct tegra_dc *dc, struct device *dev)
> > -{
> > -       struct device_node *np = dc->dev->of_node;
> > -       struct of_phandle_iterator it;
> > -       int err;
> > -
> > -       of_for_each_phandle(&it, err, np, "nvidia,outputs", NULL, 0)
> > -               if (it.node == dev->of_node)
> > -                       return true;
> > -
> > -       return false;
> > -}
> > -
> >  /*
> >   * Double-buffered registers have two copies: ASSEMBLY and ACTIVE. When the
> >   * *_ACT_REQ bits are set the ASSEMBLY copy is latched into the ACTIVE copy.
> > @@ -2061,6 +2049,7 @@ static int tegra_dc_init(struct host1x_client *client)
> >         if (err < 0)
> >                 goto cleanup;
> >
> > +       dc->base.port = of_graph_get_port_by_id(dc->dev->of_node, 0);
> >         drm_crtc_helper_add(&dc->base, &tegra_crtc_helper_funcs);
> >
> >         /*
> > diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
> > index 3d8ddccd758f..9e4ae77e6270 100644
> > --- a/drivers/gpu/drm/tegra/dc.h
> > +++ b/drivers/gpu/drm/tegra/dc.h
> > @@ -144,7 +144,6 @@ struct tegra_dc_window {
> >  };
> >
> >  /* from dc.c */
> > -bool tegra_dc_has_output(struct tegra_dc *dc, struct device *dev);
> >  void tegra_dc_commit(struct tegra_dc *dc);
> >  int tegra_dc_state_setup_clock(struct tegra_dc *dc,
> >                                struct drm_crtc_state *crtc_state,
> > diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
> > index e36e5e7c2f69..b09935cdf397 100644
> > --- a/drivers/gpu/drm/tegra/output.c
> > +++ b/drivers/gpu/drm/tegra/output.c
> > @@ -5,6 +5,7 @@
> >   */
> >
> >  #include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_of.h>
> >  #include <drm/drm_panel.h>
> >  #include <drm/drm_simple_kms_helper.h>
> >
> > @@ -229,16 +230,9 @@ void tegra_output_find_possible_crtcs(struct tegra_output *output,
> >                                       struct drm_device *drm)
> >  {
> >         struct device *dev = output->dev;
> > -       struct drm_crtc *crtc;
> > -       unsigned int mask = 0;
> > -
> > -       drm_for_each_crtc(crtc, drm) {
> > -               struct tegra_dc *dc = to_tegra_dc(crtc);
> > -
> > -               if (tegra_dc_has_output(dc, dev))
> > -                       mask |= drm_crtc_mask(crtc);
> > -       }
> > +       u32 mask;
> >
> > +       mask = drm_of_find_possible_crtcs(drm, dev->of_node);
> >         if (mask == 0) {
> >                 dev_warn(dev, "missing output definition for heads in DT\n");
> >                 mask = 0x3;
> > --- >8 ---
> >
> > I do see the benefit of using standard bindings where available, but in
> > this case I think that's hardly an improvement over the current binding,
> > even though it's undocumented.
> >
> > > > +    - nvidia,head: The number of the display controller head. This is used to
> > > > +      setup the various types of output to receive video data from the given
> > > > +      head.
> > >
> > > Not really clear what this is...
> >
> > This is the same as for the display controller in older Tegra devices.
> > The value is the index of the display controller head, or the instance
> > number of the IP, if that's any clearer. We need this in some places
> > for register programming. We can't always safely derive it in some
> > other way.
> 
> Index, humm. I'll pretend I didn't ask...

Do you have a better suggestion? If we break ABI for the OF graph thing
maybe we should role this ABI break in at the same time. This should be
safe to do on Tegra because I'm not aware of any devices that will boot
with a DTB from a read-only location.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* Re: Re: [PATCH] [v3] dmaengine: tegra210-adma: Fix runtime PM imbalance on error
From: dinghao.liu-Y5EWUtBUdg4nDS1+zs4M5A @ 2020-06-19  1:59 UTC (permalink / raw)
  To: Jon Hunter
  Cc: kjlu-OJFnDUYgAso, Laxman Dewangan, Dan Williams, Vinod Koul,
	Thierry Reding, dmaengine-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <9f7684d9-7a75-497d-db1c-75cf0991a072-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

> 
> Why noidle?
> 

_noidle is enough for fixing this bug. _sync may suspend
the device beyond expectation.

Regards,
Dinghao

^ permalink raw reply

* Re: [PATCH] [v3] dmaengine: tegra210-adma: Fix runtime PM imbalance on error
From: Jon Hunter @ 2020-06-18 19:32 UTC (permalink / raw)
  To: Dinghao Liu, kjlu-OJFnDUYgAso
  Cc: Laxman Dewangan, Dan Williams, Vinod Koul, Thierry Reding,
	dmaengine-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200618105727.14669-1-dinghao.liu-Y5EWUtBUdg4nDS1+zs4M5A@public.gmane.org>


On 18/06/2020 11:57, Dinghao Liu wrote:
> pm_runtime_get_sync() increments the runtime PM usage counter even
> when it returns an error code. Thus a pairing decrement is needed on
> the error handling path to keep the counter balanced.
> 
> Signed-off-by: Dinghao Liu <dinghao.liu-Y5EWUtBUdg4nDS1+zs4M5A@public.gmane.org>
> ---
> 
> Changelog:
> 
> v2: - Merge two patches that fix runtime PM imbalance in
>       tegra_adma_probe() and tegra_adma_alloc_chan_resources()
>       respectively.
> 
> v3: - Use pm_runtime_put_noidle() instead of pm_runtime_put_sync()
>       in tegra_adma_alloc_chan_resources().
> ---
>  drivers/dma/tegra210-adma.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c
> index db58d7e4f9fe..bfa8800dfb4c 100644
> --- a/drivers/dma/tegra210-adma.c
> +++ b/drivers/dma/tegra210-adma.c
> @@ -658,6 +658,7 @@ static int tegra_adma_alloc_chan_resources(struct dma_chan *dc)
>  
>  	ret = pm_runtime_get_sync(tdc2dev(tdc));
>  	if (ret < 0) {
> +		pm_runtime_put_noidle(tdc2dev(tdc));

Why noidle?

Jon

-- 
nvpublic

^ permalink raw reply

* Re: [PATCH 06/38] dt-bindings: display: tegra: Document display-hub
From: Rob Herring @ 2020-06-18 18:17 UTC (permalink / raw)
  To: Thierry Reding; +Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, linux-tegra
In-Reply-To: <20200618102736.GA3659851@ulmo>

On Thu, Jun 18, 2020 at 4:27 AM Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> On Wed, Jun 17, 2020 at 04:55:06PM -0600, Rob Herring wrote:
> > On Fri, Jun 12, 2020 at 04:18:31PM +0200, Thierry Reding wrote:
> > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > >
> > > Tegra186 and later have an additional component in the display pipeline
> > > called the display hub. Document the bindings which were missing.
> >
> > I'd rather this be after the conversion or I'm reviewing it twice.
>
> Okay, I'll reorder the patches accordingly.
>
> > >
> > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  .../display/tegra/nvidia,tegra20-host1x.txt   | 50 +++++++++++++++++++
> > >  1 file changed, 50 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
> > > index 47319214b5f6..2cf3cc4893da 100644
> > > --- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
> > > +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
> > > @@ -297,6 +297,56 @@ of the following host1x client modules:
> > >    - reset-names: Must include the following entries:
> > >      - vic
> > >
> > > +- display-hub: display controller hub
> > > +  Required properties:
> > > +  - compatible: "nvidia,tegra<chip>-display"
> > > +  - reg: Physical base address and length of the controller's registers.
> > > +  - interrupts: The interrupt outputs from the controller.
> > > +  - clocks: Must contain an entry for each entry in clock-names.
> > > +    See ../clocks/clock-bindings.txt for details.
> > > +  - clock-names: Must include the following entries:
> > > +    - disp
> > > +    - dsc
> > > +    - hub
> > > +  - resets: Must contain an entry for each entry in reset-names.
> > > +    See ../reset/reset.txt for details.
> > > +  - reset-names: Must include the following entries:
> > > +    - misc
> > > +    - wgrp0
> > > +    - wgrp1
> > > +    - wgrp2
> > > +    - wgrp3
> > > +    - wgrp4
> > > +    - wgrp5
> > > +  - power-domains: A list of phandle and specifiers identifying the power
> > > +    domains that the display hub is part of.
> > > +  - ranges: Range of registers used for the display controllers.
> > > +
> > > +  Each subnode of the display hub represents one of the display controllers
> > > +  available:
> > > +
> > > +  - display: display controller
> > > +    - compatible: "nvidia,tegra<chip>-dc"
> > > +    - reg: Physical base address and length of the controller's registers.
> > > +    - interrupts: The interrupt outputs from the controller.
> > > +    - clocks: Must contain an entry for each entry in clock-names.
> > > +      See ../clocks/clock-bindings.txt for details.
> > > +    - clock-names: Must include the following entries:
> > > +      - dc
> > > +    - resets: Must contain an entry for each entry in reset-names.
> > > +      See ../reset/reset.txt for details.
> > > +    - reset-names: Must include the following entries:
> > > +      - dc
> > > +    - power-domains: A list of phandle and specifiers that identify the power
> > > +      domains that this display controller is part of.
> > > +    - iommus: A phandle and specifier identifying the SMMU master interface of
> > > +      this display controller.
> > > +    - nvidia,outputs: A list of phandles of outputs that this display
> > > +      controller can drive.
> >
> > Seems like an OF graph should describe this?
>
> The above documents the current state of affairs. I don't recall exactly
> why we never merged the bindings, but we've been using this
> nvidia,outputs property for almost three years now. Changing this would
> break ABI, although I guess you could say that since this was never
> documented it can't be ABI. Still, changing this is going to cause old
> device trees to fail with new kernels. Unless of course if we add some
> backwards-compatibility mechanism in the driver. But in that case, what
> exactly do we gain by switching to an OF graph?

Probably nothing at this point. More I was just curious how we ended
up with something different.

> Historically, I think nvidia,outputs was introduced before OF graphs
> were "a thing", at least in DRM. According to the git log, the helpers
> for graphs were introduced a couple of years before nvidia,outputs was
> used, but I guess they must not have been widespread enough for me to
> have been aware of them.

There was a period display subsystem bindings were pretty much un-reviewed...

> Anyway, irrespective of the compatibility issues, I tried to use an OF
> graph to describe this and here's what I came up with:
>
> --- >8 ---
>  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 170 ++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/tegra/dc.c               |  15 +--
>  drivers/gpu/drm/tegra/dc.h               |   1 -
>  drivers/gpu/drm/tegra/output.c           |  12 +--
>  4 files changed, 172 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> index 58100fb9cd8b..a3dcf2437976 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> @@ -994,8 +994,38 @@ display@15200000 {
>                                 power-domains = <&bpmp TEGRA186_POWER_DOMAIN_DISP>;
>                                 iommus = <&smmu TEGRA186_SID_NVDISPLAY>;
>
> -                               nvidia,outputs = <&dsia &dsib &sor0 &sor1>;
>                                 nvidia,head = <0>;
> +
> +                               ports {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +
> +                                       dc0_out: port@0 {
> +                                               #address-cells = <1>;
> +                                               #size-cells = <0>;
> +                                               reg = <0>;
> +
> +                                               dc0_out_dsia: endpoint@0 {
> +                                                       reg = <0>;
> +                                                       remote-endpoint = <&dsia_in_dc0>;
> +                                               };
> +
> +                                               dc0_out_dsib: endpoint@1 {
> +                                                       reg = <1>;
> +                                                       remote-endpoint = <&dsib_in_dc0>;
> +                                               };
> +
> +                                               dc0_out_sor0: endpoint@2 {
> +                                                       reg = <2>;
> +                                                       remote-endpoint = <&sor0_in_dc0>;
> +                                               };
> +
> +                                               dc0_out_sor1: endpoint@3 {
> +                                                       reg = <3>;
> +                                                       remote-endpoint = <&sor1_in_dc0>;
> +                                               };
> +                                       };
> +                               };
>                         };
>
>                         display@15210000 {
> @@ -1010,8 +1040,38 @@ display@15210000 {
>                                 power-domains = <&bpmp TEGRA186_POWER_DOMAIN_DISPB>;
>                                 iommus = <&smmu TEGRA186_SID_NVDISPLAY>;
>
> -                               nvidia,outputs = <&dsia &dsib &sor0 &sor1>;
>                                 nvidia,head = <1>;
> +
> +                               ports {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +
> +                                       dc1_out: port@0 {
> +                                               #address-cells = <1>;
> +                                               #size-cells = <0>;
> +                                               reg = <0>;
> +
> +                                               dc1_out_dsia: endpoint@0 {
> +                                                       reg = <0>;
> +                                                       remote-endpoint = <&dsia_in_dc1>;
> +                                               };
> +
> +                                               dc1_out_dsib: endpoint@1 {
> +                                                       reg = <1>;
> +                                                       remote-endpoint = <&dsib_in_dc1>;
> +                                               };
> +
> +                                               dc1_out_sor0: endpoint@2 {
> +                                                       reg = <2>;
> +                                                       remote-endpoint = <&sor0_in_dc1>;
> +                                               };
> +
> +                                               dc1_out_sor1: endpoint@3 {
> +                                                       reg = <3>;
> +                                                       remote-endpoint = <&sor1_in_dc1>;
> +                                               };
> +                                       };
> +                               };
>                         };
>
>                         display@15220000 {
> @@ -1026,8 +1086,28 @@ display@15220000 {
>                                 power-domains = <&bpmp TEGRA186_POWER_DOMAIN_DISPC>;
>                                 iommus = <&smmu TEGRA186_SID_NVDISPLAY>;
>
> -                               nvidia,outputs = <&sor0 &sor1>;
>                                 nvidia,head = <2>;
> +
> +                               ports {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +
> +                                       dc2_out: port@0 {
> +                                               #address-cells = <1>;
> +                                               #size-cells = <0>;
> +                                               reg = <0>;
> +
> +                                               dc2_out_sor0: endpoint@0 {
> +                                                       reg = <0>;
> +                                                       remote-endpoint = <&sor0_in_dc2>;
> +                                               };
> +
> +                                               dc2_out_sor1: endpoint@1 {
> +                                                       reg = <1>;
> +                                                       remote-endpoint = <&sor1_in_dc2>;
> +                                               };
> +                                       };
> +                               };
>                         };
>                 };
>
> @@ -1044,6 +1124,25 @@ dsia: dsi@15300000 {
>                         status = "disabled";
>
>                         power-domains = <&bpmp TEGRA186_POWER_DOMAIN_DISP>;
> +
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               dsia_in: port@0 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <0>;
> +
> +                                       dsia_in_dc0: endpoint@0 {
> +                                               remote-endpoint = <&dc0_out_dsia>;
> +                                       };
> +
> +                                       dsia_in_dc1: endpoint@1 {
> +                                               remote-endpoint = <&dc1_out_dsia>;
> +                                       };
> +                               };
> +                       };
>                 };
>
>                 vic@15340000 {
> @@ -1072,6 +1171,25 @@ dsib: dsi@15400000 {
>                         status = "disabled";
>
>                         power-domains = <&bpmp TEGRA186_POWER_DOMAIN_DISP>;
> +
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               dsib_in: port@0 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <0>;
> +
> +                                       dsib_in_dc0: endpoint@0 {
> +                                               remote-endpoint = <&dc0_out_dsib>;
> +                                       };
> +
> +                                       dsib_in_dc1: endpoint@1 {
> +                                               remote-endpoint = <&dc1_out_dsib>;
> +                                       };
> +                               };
> +                       };
>                 };
>
>                 sor0: sor@15540000 {
> @@ -1096,6 +1214,29 @@ sor0: sor@15540000 {
>
>                         power-domains = <&bpmp TEGRA186_POWER_DOMAIN_DISP>;
>                         nvidia,interface = <0>;
> +
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               sor0_in: port@0 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <0>;
> +
> +                                       sor0_in_dc0: endpoint@0 {
> +                                               remote-endpoint = <&dc0_out_sor0>;
> +                                       };
> +
> +                                       sor0_in_dc1: endpoint@1 {
> +                                               remote-endpoint = <&dc1_out_sor0>;
> +                                       };
> +
> +                                       sor0_in_dc2: endpoint@2 {
> +                                               remote-endpoint = <&dc2_out_sor0>;
> +                                       };
> +                               };
> +                       };
>                 };
>
>                 sor1: sor@15580000 {
> @@ -1120,6 +1261,29 @@ sor1: sor@15580000 {
>
>                         power-domains = <&bpmp TEGRA186_POWER_DOMAIN_DISP>;
>                         nvidia,interface = <1>;
> +
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               sor1_in: port@0 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <0>;
> +
> +                                       sor1_in_dc0: endpoint@0 {
> +                                               remote-endpoint = <&dc0_out_sor1>;
> +                                       };
> +
> +                                       sor1_in_dc1: endpoint@1 {
> +                                               remote-endpoint = <&dc1_out_sor1>;
> +                                       };
> +
> +                                       sor1_in_dc2: endpoint@2 {
> +                                               remote-endpoint = <&dc2_out_sor1>;
> +                                       };
> +                               };
> +                       };
>                 };
>
>                 dpaux: dpaux@155c0000 {
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 04d6848d19fc..4adb64c083c8 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -10,6 +10,7 @@
>  #include <linux/iommu.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> +#include <linux/of_graph.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/reset.h>
>
> @@ -86,19 +87,6 @@ static inline void tegra_plane_writel(struct tegra_plane *plane, u32 value,
>         tegra_dc_writel(plane->dc, value, tegra_plane_offset(plane, offset));
>  }
>
> -bool tegra_dc_has_output(struct tegra_dc *dc, struct device *dev)
> -{
> -       struct device_node *np = dc->dev->of_node;
> -       struct of_phandle_iterator it;
> -       int err;
> -
> -       of_for_each_phandle(&it, err, np, "nvidia,outputs", NULL, 0)
> -               if (it.node == dev->of_node)
> -                       return true;
> -
> -       return false;
> -}
> -
>  /*
>   * Double-buffered registers have two copies: ASSEMBLY and ACTIVE. When the
>   * *_ACT_REQ bits are set the ASSEMBLY copy is latched into the ACTIVE copy.
> @@ -2061,6 +2049,7 @@ static int tegra_dc_init(struct host1x_client *client)
>         if (err < 0)
>                 goto cleanup;
>
> +       dc->base.port = of_graph_get_port_by_id(dc->dev->of_node, 0);
>         drm_crtc_helper_add(&dc->base, &tegra_crtc_helper_funcs);
>
>         /*
> diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
> index 3d8ddccd758f..9e4ae77e6270 100644
> --- a/drivers/gpu/drm/tegra/dc.h
> +++ b/drivers/gpu/drm/tegra/dc.h
> @@ -144,7 +144,6 @@ struct tegra_dc_window {
>  };
>
>  /* from dc.c */
> -bool tegra_dc_has_output(struct tegra_dc *dc, struct device *dev);
>  void tegra_dc_commit(struct tegra_dc *dc);
>  int tegra_dc_state_setup_clock(struct tegra_dc *dc,
>                                struct drm_crtc_state *crtc_state,
> diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
> index e36e5e7c2f69..b09935cdf397 100644
> --- a/drivers/gpu/drm/tegra/output.c
> +++ b/drivers/gpu/drm/tegra/output.c
> @@ -5,6 +5,7 @@
>   */
>
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
>  #include <drm/drm_simple_kms_helper.h>
>
> @@ -229,16 +230,9 @@ void tegra_output_find_possible_crtcs(struct tegra_output *output,
>                                       struct drm_device *drm)
>  {
>         struct device *dev = output->dev;
> -       struct drm_crtc *crtc;
> -       unsigned int mask = 0;
> -
> -       drm_for_each_crtc(crtc, drm) {
> -               struct tegra_dc *dc = to_tegra_dc(crtc);
> -
> -               if (tegra_dc_has_output(dc, dev))
> -                       mask |= drm_crtc_mask(crtc);
> -       }
> +       u32 mask;
>
> +       mask = drm_of_find_possible_crtcs(drm, dev->of_node);
>         if (mask == 0) {
>                 dev_warn(dev, "missing output definition for heads in DT\n");
>                 mask = 0x3;
> --- >8 ---
>
> I do see the benefit of using standard bindings where available, but in
> this case I think that's hardly an improvement over the current binding,
> even though it's undocumented.
>
> > > +    - nvidia,head: The number of the display controller head. This is used to
> > > +      setup the various types of output to receive video data from the given
> > > +      head.
> >
> > Not really clear what this is...
>
> This is the same as for the display controller in older Tegra devices.
> The value is the index of the display controller head, or the instance
> number of the IP, if that's any clearer. We need this in some places
> for register programming. We can't always safely derive it in some
> other way.

Index, humm. I'll pretend I didn't ask...

Rob

^ permalink raw reply

* [PATCH v6 30/36] staging: tegra-vde: fix common struct sg_table related issues
From: Marek Szyprowski @ 2020-06-18 15:39 UTC (permalink / raw)
  To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	linaro-mm-sig-cunTk1MwBs8s++Sfvej+rw,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, Bartlomiej Zolnierkiewicz,
	David Airlie, Greg Kroah-Hartman,
	linux-media-u79uwXL29TY76Z2rM5mHXA, Jonathan Hunter,
	Thierry Reding, Daniel Vetter, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	Dmitry Osipenko, Mauro Carvalho Chehab, Robin Murphy,
	Christoph Hellwig,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20200618153956.29558-1-m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.

To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Reviewed-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/staging/media/tegra-vde/iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/tegra-vde/iommu.c b/drivers/staging/media/tegra-vde/iommu.c
index 6af863d92123..adf8dc7ee25c 100644
--- a/drivers/staging/media/tegra-vde/iommu.c
+++ b/drivers/staging/media/tegra-vde/iommu.c
@@ -36,8 +36,8 @@ int tegra_vde_iommu_map(struct tegra_vde *vde,
 
 	addr = iova_dma_addr(&vde->iova, iova);
 
-	size = iommu_map_sg(vde->domain, addr, sgt->sgl, sgt->nents,
-			    IOMMU_READ | IOMMU_WRITE);
+	size = iommu_map_sgtable(vde->domain, addr, sgt,
+				 IOMMU_READ | IOMMU_WRITE);
 	if (!size) {
 		__free_iova(&vde->iova, iova);
 		return -ENXIO;
-- 
2.17.1

^ permalink raw reply related

* [PATCH v6 25/36] drm: host1x: fix common struct sg_table related issues
From: Marek Szyprowski @ 2020-06-18 15:39 UTC (permalink / raw)
  To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	linaro-mm-sig-cunTk1MwBs8s++Sfvej+rw,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Bartlomiej Zolnierkiewicz, David Airlie, Thierry Reding,
	Daniel Vetter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Robin Murphy,
	Christoph Hellwig,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20200618153956.29558-1-m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.

To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/gpu/host1x/job.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index a10643aa89aa..4832b57f10c4 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -166,11 +166,9 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 				goto unpin;
 			}
 
-			err = dma_map_sg(dev, sgt->sgl, sgt->nents, dir);
-			if (!err) {
-				err = -ENOMEM;
+			err = dma_map_sgtable(dev, sgt, dir, 0);
+			if (err)
 				goto unpin;
-			}
 
 			job->unpins[job->num_unpins].dev = dev;
 			job->unpins[job->num_unpins].dir = dir;
@@ -217,7 +215,7 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 		}
 
 		if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && host->domain) {
-			for_each_sg(sgt->sgl, sg, sgt->nents, j)
+			for_each_sgtable_sg(sgt, sg, j)
 				gather_size += sg->length;
 			gather_size = iova_align(&host->iova, gather_size);
 
@@ -229,9 +227,9 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 				goto unpin;
 			}
 
-			err = iommu_map_sg(host->domain,
+			err = iommu_map_sgtable(host->domain,
 					iova_dma_addr(&host->iova, alloc),
-					sgt->sgl, sgt->nents, IOMMU_READ);
+					sgt, IOMMU_READ);
 			if (err == 0) {
 				__free_iova(&host->iova, alloc);
 				err = -EINVAL;
@@ -241,12 +239,9 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 			job->unpins[job->num_unpins].size = gather_size;
 			phys_addr = iova_dma_addr(&host->iova, alloc);
 		} else if (sgt) {
-			err = dma_map_sg(host->dev, sgt->sgl, sgt->nents,
-					 DMA_TO_DEVICE);
-			if (!err) {
-				err = -ENOMEM;
+			err = dma_map_sgtable(host->dev, sgt, DMA_TO_DEVICE, 0);
+			if (err)
 				goto unpin;
-			}
 
 			job->unpins[job->num_unpins].dir = DMA_TO_DEVICE;
 			job->unpins[job->num_unpins].dev = host->dev;
@@ -647,8 +642,7 @@ void host1x_job_unpin(struct host1x_job *job)
 		}
 
 		if (unpin->dev && sgt)
-			dma_unmap_sg(unpin->dev, sgt->sgl, sgt->nents,
-				     unpin->dir);
+			dma_unmap_sgtable(unpin->dev, sgt, unpin->dir, 0);
 
 		host1x_bo_unpin(dev, unpin->bo, sgt);
 		host1x_bo_put(unpin->bo);
-- 
2.17.1

^ permalink raw reply related

* [PATCH v6 20/36] drm: tegra: fix common struct sg_table related issues
From: Marek Szyprowski @ 2020-06-18 15:39 UTC (permalink / raw)
  To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	linaro-mm-sig-cunTk1MwBs8s++Sfvej+rw,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
	Bartlomiej Zolnierkiewicz,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, David Airlie,
	Daniel Vetter, Thierry Reding, Jonathan Hunter,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200618153956.29558-1-m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.

To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/gpu/drm/tegra/gem.c   | 27 ++++++++++-----------------
 drivers/gpu/drm/tegra/plane.c | 15 +++++----------
 2 files changed, 15 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
index 723df142a981..01d94befab11 100644
--- a/drivers/gpu/drm/tegra/gem.c
+++ b/drivers/gpu/drm/tegra/gem.c
@@ -98,8 +98,8 @@ static struct sg_table *tegra_bo_pin(struct device *dev, struct host1x_bo *bo,
 		 * the SG table needs to be copied to avoid overwriting any
 		 * other potential users of the original SG table.
 		 */
-		err = sg_alloc_table_from_sg(sgt, obj->sgt->sgl, obj->sgt->nents,
-					     GFP_KERNEL);
+		err = sg_alloc_table_from_sg(sgt, obj->sgt->sgl,
+					     obj->sgt->orig_nents, GFP_KERNEL);
 		if (err < 0)
 			goto free;
 	} else {
@@ -196,8 +196,7 @@ static int tegra_bo_iommu_map(struct tegra_drm *tegra, struct tegra_bo *bo)
 
 	bo->iova = bo->mm->start;
 
-	bo->size = iommu_map_sg(tegra->domain, bo->iova, bo->sgt->sgl,
-				bo->sgt->nents, prot);
+	bo->size = iommu_map_sgtable(tegra->domain, bo->iova, bo->sgt, prot);
 	if (!bo->size) {
 		dev_err(tegra->drm->dev, "failed to map buffer\n");
 		err = -ENOMEM;
@@ -264,8 +263,7 @@ static struct tegra_bo *tegra_bo_alloc_object(struct drm_device *drm,
 static void tegra_bo_free(struct drm_device *drm, struct tegra_bo *bo)
 {
 	if (bo->pages) {
-		dma_unmap_sg(drm->dev, bo->sgt->sgl, bo->sgt->nents,
-			     DMA_FROM_DEVICE);
+		dma_unmap_sgtable(drm->dev, bo->sgt, DMA_FROM_DEVICE, 0);
 		drm_gem_put_pages(&bo->gem, bo->pages, true, true);
 		sg_free_table(bo->sgt);
 		kfree(bo->sgt);
@@ -290,12 +288,9 @@ static int tegra_bo_get_pages(struct drm_device *drm, struct tegra_bo *bo)
 		goto put_pages;
 	}
 
-	err = dma_map_sg(drm->dev, bo->sgt->sgl, bo->sgt->nents,
-			 DMA_FROM_DEVICE);
-	if (err == 0) {
-		err = -EFAULT;
+	err = dma_map_sgtable(drm->dev, bo->sgt, DMA_FROM_DEVICE, 0);
+	if (err)
 		goto free_sgt;
-	}
 
 	return 0;
 
@@ -571,7 +566,7 @@ tegra_gem_prime_map_dma_buf(struct dma_buf_attachment *attach,
 			goto free;
 	}
 
-	if (dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir) == 0)
+	if (dma_map_sgtable(attach->dev, sgt, dir, 0))
 		goto free;
 
 	return sgt;
@@ -590,7 +585,7 @@ static void tegra_gem_prime_unmap_dma_buf(struct dma_buf_attachment *attach,
 	struct tegra_bo *bo = to_tegra_bo(gem);
 
 	if (bo->pages)
-		dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+		dma_unmap_sgtable(attach->dev, sgt, dir, 0);
 
 	sg_free_table(sgt);
 	kfree(sgt);
@@ -609,8 +604,7 @@ static int tegra_gem_prime_begin_cpu_access(struct dma_buf *buf,
 	struct drm_device *drm = gem->dev;
 
 	if (bo->pages)
-		dma_sync_sg_for_cpu(drm->dev, bo->sgt->sgl, bo->sgt->nents,
-				    DMA_FROM_DEVICE);
+		dma_sync_sgtable_for_cpu(drm->dev, bo->sgt, DMA_FROM_DEVICE);
 
 	return 0;
 }
@@ -623,8 +617,7 @@ static int tegra_gem_prime_end_cpu_access(struct dma_buf *buf,
 	struct drm_device *drm = gem->dev;
 
 	if (bo->pages)
-		dma_sync_sg_for_device(drm->dev, bo->sgt->sgl, bo->sgt->nents,
-				       DMA_TO_DEVICE);
+		dma_sync_sgtable_for_device(drm->dev, bo->sgt, DMA_TO_DEVICE);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/tegra/plane.c b/drivers/gpu/drm/tegra/plane.c
index 9ccfb56e9b01..0d2ef1662a39 100644
--- a/drivers/gpu/drm/tegra/plane.c
+++ b/drivers/gpu/drm/tegra/plane.c
@@ -130,12 +130,9 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct tegra_plane_state *state)
 		}
 
 		if (sgt) {
-			err = dma_map_sg(dc->dev, sgt->sgl, sgt->nents,
-					 DMA_TO_DEVICE);
-			if (err == 0) {
-				err = -ENOMEM;
+			err = dma_map_sgtable(dc->dev, sgt, DMA_TO_DEVICE, 0);
+			if (err)
 				goto unpin;
-			}
 
 			/*
 			 * The display controller needs contiguous memory, so
@@ -143,7 +140,7 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct tegra_plane_state *state)
 			 * map its SG table to a single contiguous chunk of
 			 * I/O virtual memory.
 			 */
-			if (err > 1) {
+			if (sgt->nents > 1) {
 				err = -EINVAL;
 				goto unpin;
 			}
@@ -165,8 +162,7 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct tegra_plane_state *state)
 		struct sg_table *sgt = state->sgt[i];
 
 		if (sgt)
-			dma_unmap_sg(dc->dev, sgt->sgl, sgt->nents,
-				     DMA_TO_DEVICE);
+			dma_unmap_sgtable(dc->dev, sgt, DMA_TO_DEVICE, 0);
 
 		host1x_bo_unpin(dc->dev, &bo->base, sgt);
 		state->iova[i] = DMA_MAPPING_ERROR;
@@ -185,8 +181,7 @@ static void tegra_dc_unpin(struct tegra_dc *dc, struct tegra_plane_state *state)
 		struct sg_table *sgt = state->sgt[i];
 
 		if (sgt)
-			dma_unmap_sg(dc->dev, sgt->sgl, sgt->nents,
-				     DMA_TO_DEVICE);
+			dma_unmap_sgtable(dc->dev, sgt, DMA_TO_DEVICE, 0);
 
 		host1x_bo_unpin(dc->dev, &bo->base, sgt);
 		state->iova[i] = DMA_MAPPING_ERROR;
-- 
2.17.1

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox