[PATCH v3 0/4] Add display support on the stm32f746-disco board

[PATCH v3 0/4] Add display support on the stm32f746-disco board

[Dario Binacchi]

The series adds support for the display on the stm32f746-disco board,
along with a generic patch that adds the "bpp" parameter to the stm-drm
module. The intention is to allow users to size, within certain limits,
the memory footprint required by the framebuffer.

Changes in v3:
- rename ltdc-pins-a-0 to ltdc-0.
- drop [4/6] dt-bindings: display: simple: add Rocktech RK043FN48H
  Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next):
  https://cgit.freedesktop.org/drm/drm-misc/commit/?id=c42a37a27c777d63961dd634a30f7c887949491a
- drop [5/6] drm/panel: simple: add support for Rocktech RK043FN48H panel
  Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next)
  https://cgit.freedesktop.org/drm/drm-misc/commit/?id=13cdd12a9f934158f4ec817cf048fcb4384aa9dc

Dario Binacchi (4):
  ARM: dts: stm32: add ltdc support on stm32f746 MCU
  ARM: dts: stm32: add pin map for LTDC on stm32f7
  ARM: dts: stm32: support display on stm32f746-disco board
  drm/stm: add an option to change FB bpp

 arch/arm/boot/dts/stm32f7-pinctrl.dtsi | 35 ++++++++++++++++++
 arch/arm/boot/dts/stm32f746-disco.dts  | 51 ++++++++++++++++++++++++++
 arch/arm/boot/dts/stm32f746.dtsi       | 10 +++++
 drivers/gpu/drm/stm/drv.c              |  8 +++-
 4 files changed, 103 insertions(+), 1 deletion(-)

-- 
2.32.0

[PATCH v3 1/4] ARM: dts: stm32: add ltdc support on stm32f746 MCU

[Dario Binacchi]

Add LTDC (Lcd-tft Display Controller) support.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---

(no changes since v1)

 arch/arm/boot/dts/stm32f746.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/stm32f746.dtsi b/arch/arm/boot/dts/stm32f746.dtsi
index dc868e6da40e..9c4ba0b7f239 100644
--- a/arch/arm/boot/dts/stm32f746.dtsi
+++ b/arch/arm/boot/dts/stm32f746.dtsi
@@ -507,6 +507,16 @@ pwm {
 			};
 		};
 
+		ltdc: display-controller@40016800 {
+			compatible = "st,stm32-ltdc";
+			reg = <0x40016800 0x200>;
+			interrupts = <88>, <89>;
+			resets = <&rcc STM32F7_APB2_RESET(LTDC)>;
+			clocks = <&rcc 1 CLK_LCD>;
+			clock-names = "lcd";
+			status = "disabled";
+		};
+
 		pwrcfg: power-config@40007000 {
 			compatible = "st,stm32-power-config", "syscon";
 			reg = <0x40007000 0x400>;
-- 
2.32.0

[PATCH v3 2/4] ARM: dts: stm32: add pin map for LTDC on stm32f7

[Dario Binacchi]

Add pin configurations for using LTDC (LCD-tft Display Controller) on
stm32f746-disco board.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

---

Changes in v3:
- rename ltdc-pins-a-0 to ltdc-0.

 arch/arm/boot/dts/stm32f7-pinctrl.dtsi | 35 ++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/arch/arm/boot/dts/stm32f7-pinctrl.dtsi b/arch/arm/boot/dts/stm32f7-pinctrl.dtsi
index 9f65403295ca..36dad40dd0c7 100644
--- a/arch/arm/boot/dts/stm32f7-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stm32f7-pinctrl.dtsi
@@ -365,6 +365,41 @@ pins2 {
 					bias-pull-up;
 				};
 			};
+
+
+			ltdc_pins_a: ltdc-0 {
+				pins {
+					pinmux = <STM32_PINMUX('E', 4, AF14)>, /* LCD_B0 */
+						 <STM32_PINMUX('G',12, AF9)>,  /* LCD_B4 */
+						 <STM32_PINMUX('I', 9, AF14)>, /* LCD_VSYNC */
+						 <STM32_PINMUX('I',10, AF14)>, /* LCD_HSYNC */
+						 <STM32_PINMUX('I',14, AF14)>, /* LCD_CLK */
+						 <STM32_PINMUX('I',15, AF14)>, /* LCD_R0 */
+						 <STM32_PINMUX('J', 0, AF14)>, /* LCD_R1 */
+						 <STM32_PINMUX('J', 1, AF14)>, /* LCD_R2 */
+						 <STM32_PINMUX('J', 2, AF14)>, /* LCD_R3 */
+						 <STM32_PINMUX('J', 3, AF14)>, /* LCD_R4 */
+						 <STM32_PINMUX('J', 4, AF14)>, /* LCD_R5 */
+						 <STM32_PINMUX('J', 5, AF14)>, /* LCD_R6 */
+						 <STM32_PINMUX('J', 6, AF14)>, /* LCD_R7 */
+						 <STM32_PINMUX('J', 7, AF14)>, /* LCD_G0 */
+						 <STM32_PINMUX('J', 8, AF14)>, /* LCD_G1 */
+						 <STM32_PINMUX('J', 9, AF14)>, /* LCD_G2 */
+						 <STM32_PINMUX('J',10, AF14)>, /* LCD_G3 */
+						 <STM32_PINMUX('J',11, AF14)>, /* LCD_G4 */
+						 <STM32_PINMUX('J',13, AF14)>, /* LCD_B1 */
+						 <STM32_PINMUX('J',14, AF14)>, /* LCD_B2 */
+						 <STM32_PINMUX('J',15, AF14)>, /* LCD_B3 */
+						 <STM32_PINMUX('K', 0, AF14)>, /* LCD_G5 */
+						 <STM32_PINMUX('K', 1, AF14)>, /* LCD_G6 */
+						 <STM32_PINMUX('K', 2, AF14)>, /* LCD_G7 */
+						 <STM32_PINMUX('K', 4, AF14)>, /* LCD_B5 */
+						 <STM32_PINMUX('K', 5, AF14)>, /* LCD_B6 */
+						 <STM32_PINMUX('K', 6, AF14)>, /* LCD_B7 */
+						 <STM32_PINMUX('K', 7, AF14)>; /* LCD_DE */
+					slew-rate = <2>;
+				};
+			};
 		};
 	};
 };
-- 
2.32.0

[PATCH v3 3/4] ARM: dts: stm32: support display on stm32f746-disco board

[Dario Binacchi]

Add support to Rocktech RK043FN48H display on stm32f746-disco board.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---

(no changes since v1)

 arch/arm/boot/dts/stm32f746-disco.dts | 51 +++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/arch/arm/boot/dts/stm32f746-disco.dts b/arch/arm/boot/dts/stm32f746-disco.dts
index c11616ed5fc6..cda423b6a874 100644
--- a/arch/arm/boot/dts/stm32f746-disco.dts
+++ b/arch/arm/boot/dts/stm32f746-disco.dts
@@ -60,10 +60,41 @@ memory@c0000000 {
 		reg = <0xC0000000 0x800000>;
 	};
 
+	reserved-memory {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		linux,cma {
+			compatible = "shared-dma-pool";
+			no-map;
+			size = <0x80000>;
+			linux,dma-default;
+		};
+	};
+
 	aliases {
 		serial0 = &usart1;
 	};
 
+	backlight: backlight {
+		compatible = "gpio-backlight";
+		gpios = <&gpiok 3 GPIO_ACTIVE_HIGH>;
+		status = "okay";
+	};
+
+	panel_rgb: panel-rgb {
+		compatible = "rocktech,rk043fn48h";
+		backlight = <&backlight>;
+		enable-gpios = <&gpioi 12 GPIO_ACTIVE_HIGH>;
+		status = "okay";
+		port {
+			panel_in_rgb: endpoint {
+				remote-endpoint = <&ltdc_out_rgb>;
+			};
+		};
+	};
+
 	usbotg_hs_phy: usb-phy {
 		#phy-cells = <0>;
 		compatible = "usb-nop-xceiv";
@@ -99,6 +130,26 @@ &i2c1 {
 	status = "okay";
 };
 
+&dma1 {
+	status = "okay";
+};
+
+&dma2 {
+	status = "okay";
+};
+
+&ltdc {
+	pinctrl-0 = <&ltdc_pins_a>;
+	pinctrl-names = "default";
+	status = "okay";
+
+	port {
+		ltdc_out_rgb: endpoint {
+			remote-endpoint = <&panel_in_rgb>;
+		};
+	};
+};
+
 &sdio1 {
 	status = "okay";
 	vmmc-supply = <&mmc_vcard>;
-- 
2.32.0

[PATCH v3 4/4] drm/stm: add an option to change FB bpp

[Dario Binacchi]

Boards that use the STM32F{4,7} series have limited amounts of RAM. The
added parameter allows users to size, within certain limits, the memory
footprint required by the framebuffer.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

---

Changes in v3:
- drop [4/6] dt-bindings: display: simple: add Rocktech RK043FN48H
  Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next):
  https://cgit.freedesktop.org/drm/drm-misc/commit/?id=c42a37a27c777d63961dd634a30f7c887949491a
- drop [5/6] drm/panel: simple: add support for Rocktech RK043FN48H panel
  Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next)
  https://cgit.freedesktop.org/drm/drm-misc/commit/?id=13cdd12a9f934158f4ec817cf048fcb4384aa9dc

 drivers/gpu/drm/stm/drv.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
index 422220df7d8c..65be2b442a6a 100644
--- a/drivers/gpu/drm/stm/drv.c
+++ b/drivers/gpu/drm/stm/drv.c
@@ -30,6 +30,11 @@
 #define STM_MAX_FB_WIDTH	2048
 #define STM_MAX_FB_HEIGHT	2048 /* same as width to handle orientation */
 
+static uint stm_bpp = 16;
+
+MODULE_PARM_DESC(bpp, "bits-per-pixel (default: 16)");
+module_param_named(bpp, stm_bpp, uint, 0644);
+
 static const struct drm_mode_config_funcs drv_mode_config_funcs = {
 	.fb_create = drm_gem_fb_create,
 	.atomic_check = drm_atomic_helper_check,
@@ -93,6 +98,7 @@ static int drv_load(struct drm_device *ddev)
 	ddev->mode_config.min_height = 0;
 	ddev->mode_config.max_width = STM_MAX_FB_WIDTH;
 	ddev->mode_config.max_height = STM_MAX_FB_HEIGHT;
+	ddev->mode_config.preferred_depth = stm_bpp;
 	ddev->mode_config.funcs = &drv_mode_config_funcs;
 	ddev->mode_config.normalize_zpos = true;
 
@@ -203,7 +209,7 @@ static int stm_drm_platform_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_put;
 
-	drm_fbdev_dma_setup(ddev, 16);
+	drm_fbdev_dma_setup(ddev, stm_bpp);
 
 	return 0;
 
-- 
2.32.0

Re: [Linux-stm32] [PATCH v3 1/4] ARM: dts: stm32: add ltdc support on stm32f746 MCU

[Raphael Gallais-Pou]

On 6/9/23 08:20, Dario Binacchi wrote:
> Add LTDC (Lcd-tft Display Controller) support.
>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

Reviewed-by: Raphaël Gallais-Pou <raphael.gallais-pou@foss.st.com>

> ---
>
> (no changes since v1)
>
>  arch/arm/boot/dts/stm32f746.dtsi | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Re: [Linux-stm32] [PATCH v3 2/4] ARM: dts: stm32: add pin map for LTDC on stm32f7

[Raphael Gallais-Pou]

On 6/9/23 08:20, Dario Binacchi wrote:
> Add pin configurations for using LTDC (LCD-tft Display Controller) on
> stm32f746-disco board.
>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Reviewed-by: Raphaël Gallais-Pou <raphael.gallais-pou@foss.st.com>
>
> ---
>
> Changes in v3:
> - rename ltdc-pins-a-0 to ltdc-0.
>
>  arch/arm/boot/dts/stm32f7-pinctrl.dtsi | 35 ++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)

Re: [Linux-stm32] [PATCH v3 3/4] ARM: dts: stm32: support display on stm32f746-disco board

[Raphael Gallais-Pou]

On 6/9/23 08:20, Dario Binacchi wrote:
> Add support to Rocktech RK043FN48H display on stm32f746-disco board.
>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Reviewed-by: Raphaël Gallais-Pou <raphael.gallais-pou@foss.st.com>
> ---
>
> (no changes since v1)
>
>  arch/arm/boot/dts/stm32f746-disco.dts | 51 +++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)

Re: [PATCH v3 4/4] drm/stm: add an option to change FB bpp

[Philippe CORNU]

On 6/9/23 08:20, Dario Binacchi wrote:
> Boards that use the STM32F{4,7} series have limited amounts of RAM. The
> added parameter allows users to size, within certain limits, the memory
> footprint required by the framebuffer.
> 
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> 
> ---
> 
> Changes in v3:
> - drop [4/6] dt-bindings: display: simple: add Rocktech RK043FN48H
>    Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next):
>    https://cgit.freedesktop.org/drm/drm-misc/commit/?id=c42a37a27c777d63961dd634a30f7c887949491a
> - drop [5/6] drm/panel: simple: add support for Rocktech RK043FN48H panel
>    Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next)
>    https://cgit.freedesktop.org/drm/drm-misc/commit/?id=13cdd12a9f934158f4ec817cf048fcb4384aa9dc
> 
>   drivers/gpu/drm/stm/drv.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> index 422220df7d8c..65be2b442a6a 100644
> --- a/drivers/gpu/drm/stm/drv.c
> +++ b/drivers/gpu/drm/stm/drv.c
> @@ -30,6 +30,11 @@
>   #define STM_MAX_FB_WIDTH	2048
>   #define STM_MAX_FB_HEIGHT	2048 /* same as width to handle orientation */
>   
> +static uint stm_bpp = 16;
> +
> +MODULE_PARM_DESC(bpp, "bits-per-pixel (default: 16)");
> +module_param_named(bpp, stm_bpp, uint, 0644);
> +
>   static const struct drm_mode_config_funcs drv_mode_config_funcs = {
>   	.fb_create = drm_gem_fb_create,
>   	.atomic_check = drm_atomic_helper_check,
> @@ -93,6 +98,7 @@ static int drv_load(struct drm_device *ddev)
>   	ddev->mode_config.min_height = 0;
>   	ddev->mode_config.max_width = STM_MAX_FB_WIDTH;
>   	ddev->mode_config.max_height = STM_MAX_FB_HEIGHT;
> +	ddev->mode_config.preferred_depth = stm_bpp;
>   	ddev->mode_config.funcs = &drv_mode_config_funcs;
>   	ddev->mode_config.normalize_zpos = true;
>   
> @@ -203,7 +209,7 @@ static int stm_drm_platform_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto err_put;
>   
> -	drm_fbdev_dma_setup(ddev, 16);
> +	drm_fbdev_dma_setup(ddev, stm_bpp);
>   
>   	return 0;
>   

Acked-by: Philippe Cornu <philippe.cornu@foss.st.com>
Many thanks,
Philippe :-)

Re: [PATCH v3 4/4] drm/stm: add an option to change FB bpp

[Michael Nazzareno Trimarchi]

Hi

On Tue, Jun 13, 2023 at 4:41 PM Philippe CORNU
<philippe.cornu@foss.st.com> wrote:
>
>
>
> On 6/9/23 08:20, Dario Binacchi wrote:
> > Boards that use the STM32F{4,7} series have limited amounts of RAM. The
> > added parameter allows users to size, within certain limits, the memory
> > footprint required by the framebuffer.
> >
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >
> > ---
> >
> > Changes in v3:
> > - drop [4/6] dt-bindings: display: simple: add Rocktech RK043FN48H
> >    Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next):
> >    https://cgit.freedesktop.org/drm/drm-misc/commit/?id=c42a37a27c777d63961dd634a30f7c887949491a
> > - drop [5/6] drm/panel: simple: add support for Rocktech RK043FN48H panel
> >    Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next)
> >    https://cgit.freedesktop.org/drm/drm-misc/commit/?id=13cdd12a9f934158f4ec817cf048fcb4384aa9dc
> >
> >   drivers/gpu/drm/stm/drv.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> > index 422220df7d8c..65be2b442a6a 100644
> > --- a/drivers/gpu/drm/stm/drv.c
> > +++ b/drivers/gpu/drm/stm/drv.c
> > @@ -30,6 +30,11 @@
> >   #define STM_MAX_FB_WIDTH    2048
> >   #define STM_MAX_FB_HEIGHT   2048 /* same as width to handle orientation */
> >
> > +static uint stm_bpp = 16;
> > +
> > +MODULE_PARM_DESC(bpp, "bits-per-pixel (default: 16)");
> > +module_param_named(bpp, stm_bpp, uint, 0644);
> > +
> >   static const struct drm_mode_config_funcs drv_mode_config_funcs = {
> >       .fb_create = drm_gem_fb_create,
> >       .atomic_check = drm_atomic_helper_check,
> > @@ -93,6 +98,7 @@ static int drv_load(struct drm_device *ddev)
> >       ddev->mode_config.min_height = 0;
> >       ddev->mode_config.max_width = STM_MAX_FB_WIDTH;
> >       ddev->mode_config.max_height = STM_MAX_FB_HEIGHT;
> > +     ddev->mode_config.preferred_depth = stm_bpp;
> >       ddev->mode_config.funcs = &drv_mode_config_funcs;
> >       ddev->mode_config.normalize_zpos = true;
> >
> > @@ -203,7 +209,7 @@ static int stm_drm_platform_probe(struct platform_device *pdev)
> >       if (ret)
> >               goto err_put;
> >
> > -     drm_fbdev_dma_setup(ddev, 16);
> > +     drm_fbdev_dma_setup(ddev, stm_bpp);
> >
> >       return 0;
> >
>
> Acked-by: Philippe Cornu <philippe.cornu@foss.st.com>
> Many thanks,
> Philippe :-)
>

According to the latest review on usb patchset: "Please do not add new
module parameters, this is not the 1990's anymore.
We have per-device settings everywhere, this makes that impossible.
Just use a DT value, if it is wrong, then fix the DT value!  No need to
have the kernel override it, that's not what DT files are for."

I think it makes more sense to have dts parameters. Should maybe apply here too

Michael

Re: [PATCH v3 4/4] drm/stm: add an option to change FB bpp

[Raphael Gallais-Pou]

On 6/13/23 16:52, Michael Nazzareno Trimarchi wrote:
> Hi
>
> On Tue, Jun 13, 2023 at 4:41 PM Philippe CORNU
> <philippe.cornu@foss.st.com> wrote:
>>
>>
>> On 6/9/23 08:20, Dario Binacchi wrote:
>>> Boards that use the STM32F{4,7} series have limited amounts of RAM. The
>>> added parameter allows users to size, within certain limits, the memory
>>> footprint required by the framebuffer.
>>>
>>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>>>
>>> ---
>>>
>>> Changes in v3:
>>> - drop [4/6] dt-bindings: display: simple: add Rocktech RK043FN48H
>>>    Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next):
>>>    https://cgit.freedesktop.org/drm/drm-misc/commit/?id=c42a37a27c777d63961dd634a30f7c887949491a
>>> - drop [5/6] drm/panel: simple: add support for Rocktech RK043FN48H panel
>>>    Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next)
>>>    https://cgit.freedesktop.org/drm/drm-misc/commit/?id=13cdd12a9f934158f4ec817cf048fcb4384aa9dc
>>>
>>>   drivers/gpu/drm/stm/drv.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
>>> index 422220df7d8c..65be2b442a6a 100644
>>> --- a/drivers/gpu/drm/stm/drv.c
>>> +++ b/drivers/gpu/drm/stm/drv.c
>>> @@ -30,6 +30,11 @@
>>>   #define STM_MAX_FB_WIDTH    2048
>>>   #define STM_MAX_FB_HEIGHT   2048 /* same as width to handle orientation */
>>>
>>> +static uint stm_bpp = 16;
>>> +
>>> +MODULE_PARM_DESC(bpp, "bits-per-pixel (default: 16)");
>>> +module_param_named(bpp, stm_bpp, uint, 0644);
>>> +
>>>   static const struct drm_mode_config_funcs drv_mode_config_funcs = {
>>>       .fb_create = drm_gem_fb_create,
>>>       .atomic_check = drm_atomic_helper_check,
>>> @@ -93,6 +98,7 @@ static int drv_load(struct drm_device *ddev)
>>>       ddev->mode_config.min_height = 0;
>>>       ddev->mode_config.max_width = STM_MAX_FB_WIDTH;
>>>       ddev->mode_config.max_height = STM_MAX_FB_HEIGHT;
>>> +     ddev->mode_config.preferred_depth = stm_bpp;
>>>       ddev->mode_config.funcs = &drv_mode_config_funcs;
>>>       ddev->mode_config.normalize_zpos = true;
>>>
>>> @@ -203,7 +209,7 @@ static int stm_drm_platform_probe(struct platform_device *pdev)
>>>       if (ret)
>>>               goto err_put;
>>>
>>> -     drm_fbdev_dma_setup(ddev, 16);
>>> +     drm_fbdev_dma_setup(ddev, stm_bpp);
>>>
>>>       return 0;
>>>
>> Acked-by: Philippe Cornu <philippe.cornu@foss.st.com>
>> Many thanks,
>> Philippe :-)
>>
> According to the latest review on usb patchset: "Please do not add new
> module parameters, this is not the 1990's anymore.
> We have per-device settings everywhere, this makes that impossible.
> Just use a DT value, if it is wrong, then fix the DT value!  No need to
> have the kernel override it, that's not what DT files are for."


I actually am conflicted about this idea, but I still think that here the best
option would be to put a device-tree property.

In which context here the module parameters could be used ? I think a module
parameter would be quite troublesome for userspace applications in that case.


Raphaël

>
> I think it makes more sense to have dts parameters. Should maybe apply here too
>
> Michael

Re: [PATCH v3 4/4] drm/stm: add an option to change FB bpp

[Philippe CORNU]

On 6/13/23 17:26, Raphael Gallais-Pou wrote:
> 
> On 6/13/23 16:52, Michael Nazzareno Trimarchi wrote:
>> Hi
>>
>> On Tue, Jun 13, 2023 at 4:41 PM Philippe CORNU
>> <philippe.cornu@foss.st.com> wrote:
>>>
>>>
>>> On 6/9/23 08:20, Dario Binacchi wrote:
>>>> Boards that use the STM32F{4,7} series have limited amounts of RAM. The
>>>> added parameter allows users to size, within certain limits, the memory
>>>> footprint required by the framebuffer.
>>>>
>>>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>>>>
>>>> ---
>>>>
>>>> Changes in v3:
>>>> - drop [4/6] dt-bindings: display: simple: add Rocktech RK043FN48H
>>>>     Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next):
>>>>     https://cgit.freedesktop.org/drm/drm-misc/commit/?id=c42a37a27c777d63961dd634a30f7c887949491a
>>>> - drop [5/6] drm/panel: simple: add support for Rocktech RK043FN48H panel
>>>>     Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next)
>>>>     https://cgit.freedesktop.org/drm/drm-misc/commit/?id=13cdd12a9f934158f4ec817cf048fcb4384aa9dc
>>>>
>>>>    drivers/gpu/drm/stm/drv.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
>>>> index 422220df7d8c..65be2b442a6a 100644
>>>> --- a/drivers/gpu/drm/stm/drv.c
>>>> +++ b/drivers/gpu/drm/stm/drv.c
>>>> @@ -30,6 +30,11 @@
>>>>    #define STM_MAX_FB_WIDTH    2048
>>>>    #define STM_MAX_FB_HEIGHT   2048 /* same as width to handle orientation */
>>>>
>>>> +static uint stm_bpp = 16;
>>>> +
>>>> +MODULE_PARM_DESC(bpp, "bits-per-pixel (default: 16)");
>>>> +module_param_named(bpp, stm_bpp, uint, 0644);
>>>> +
>>>>    static const struct drm_mode_config_funcs drv_mode_config_funcs = {
>>>>        .fb_create = drm_gem_fb_create,
>>>>        .atomic_check = drm_atomic_helper_check,
>>>> @@ -93,6 +98,7 @@ static int drv_load(struct drm_device *ddev)
>>>>        ddev->mode_config.min_height = 0;
>>>>        ddev->mode_config.max_width = STM_MAX_FB_WIDTH;
>>>>        ddev->mode_config.max_height = STM_MAX_FB_HEIGHT;
>>>> +     ddev->mode_config.preferred_depth = stm_bpp;
>>>>        ddev->mode_config.funcs = &drv_mode_config_funcs;
>>>>        ddev->mode_config.normalize_zpos = true;
>>>>
>>>> @@ -203,7 +209,7 @@ static int stm_drm_platform_probe(struct platform_device *pdev)
>>>>        if (ret)
>>>>                goto err_put;
>>>>
>>>> -     drm_fbdev_dma_setup(ddev, 16);
>>>> +     drm_fbdev_dma_setup(ddev, stm_bpp);
>>>>
>>>>        return 0;
>>>>
>>> Acked-by: Philippe Cornu <philippe.cornu@foss.st.com>
>>> Many thanks,
>>> Philippe :-)
>>>
>> According to the latest review on usb patchset: "Please do not add new
>> module parameters, this is not the 1990's anymore.
>> We have per-device settings everywhere, this makes that impossible.
>> Just use a DT value, if it is wrong, then fix the DT value!  No need to
>> have the kernel override it, that's not what DT files are for."
> 
> 
> I actually am conflicted about this idea, but I still think that here the best
> option would be to put a device-tree property.
> 
> In which context here the module parameters could be used ? I think a module
> parameter would be quite troublesome for userspace applications in that case.
> 
> 
> Raphaël
> 
>>
>> I think it makes more sense to have dts parameters. Should maybe apply here too
>>
>> Michael

Hi Raphaël & Michael,

Many thanks for your comments.

Dario's usage of this stm driver is STM32 MCUs (STM32F4 & F7...) where, 
sometimes, old userland fbdev-based applications are used, and I imagine 
it is maybe "easier" to use a module parameter (through the kernel 
command line or whatever...) in these use cases (even if using dt is 
always better and not that complex).

Moreover, as I did not find any drm drivers with drm_fbdev_dma_setup() 
using a dt property "as example" (but always hard-coded value), then I 
decided to not block this proposal :)

Thanks to your feedback, I am reconsidering my position. And sorry 
Dario, hope you understand it will take more time for reviewing your patch.

Does anyone have an opinion to share on this point?

Many thanks,
Philippe :-)