| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9acf95f1-0bce-62c7-b524-4eac408b4207@suse.de>
Date: Thu, 19 Jan 2023 11:44:06 +0100
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Rayyan Ansari <rayyan@...ari.sh>, dri-devel@...ts.freedesktop.org
Cc: Javier Martinez Canillas <javierm@...hat.com>,
linux-kernel@...r.kernel.org,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE"
<devicetree@...r.kernel.org>, asahi@...ts.linux.dev
Subject: Re: [RFC PATCH] drm/simpledrm: Allow physical width and height
configuration via DT
(cc: devicetree@...r.kernel.org, asahi@...ts.linux.dev)
Hi,
thanks for the patch. I already wondered if the DPI value should be
configurable in some way.
Am 18.01.23 um 19:48 schrieb Rayyan Ansari:
> Hello,
> The following draft patch adds support for configuring the
> height-mm and width-mm DRM properties in the simpledrm driver
> via devicetree.
> This is useful to get proper scaling in UIs such as Phosh.
> An example of using this property is this, taken from my local tree:
>
> framebuffer0: framebuffer@...0000 {
> compatible = "simple-framebuffer";
> reg = <0x3200000 0x800000>;
> format = "a8r8g8b8";
> width = <720>;
> height = <1280>;
> stride = <(720 * 4)>;
> width-mm = /bits/ 16 <58>;
> height-mm = /bits/ 16 <103>;
>
> clocks = <&mmcc MDSS_AHB_CLK>,
> <&mmcc MDSS_AXI_CLK>,
> <&mmcc MDSS_BYTE0_CLK>,
> <&mmcc MDSS_MDP_CLK>,
> <&mmcc MDSS_PCLK0_CLK>,
> <&mmcc MDSS_VSYNC_CLK>;
> power-domains = <&mmcc MDSS_GDSC>;
> };
>
> I have tested this on my Lumia 735, and it does indeed
> allow Phosh to scale correctly on the screen.
Is this something that is already supported by some device, or just a
pet project of yours?
>
> However, I would like to get some feedback before I write the
> documentation.
> - What data type should be used?
> The width_mm and height_mm properties of the drm_display_mode
> struct are defined as u16. I have also made the devicetree
> properties as the u16 type, but this requires specifying
> "/bits/ 16" before the value. Should u32 be used instead to get
> rid of this? If so, how could the conversion from u32->u16 be
> handled?
I'd use 32 bits in the DT, just like the other properties.
> - Style?
> I have split the arguments to the DRM_MODE_INIT macro across
> multiple lines to increase readability. I'm not sure if this
> is the correct style though.
> - Anything else?
> This is my first time writing code for a Linux driver, so I
> would be grateful if you have any suggestions for improvements.
>
> Thanks,
> Rayyan.
> ---
> drivers/gpu/drm/tiny/simpledrm.c | 49 +++++++++++++++++++++++++++-----
> 1 file changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 162eb44dcba8..92109f870b35 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -116,6 +116,15 @@ simplefb_get_format_pd(struct drm_device *dev,
> return simplefb_get_validated_format(dev, pd->format);
> }
>
> +static void
> +simplefb_read_u16_of_optional(struct drm_device *dev, struct device_node *of_node,
Maybe call it simplefb_read_u16_of_opt()
> + const char *name, u16 *value)
The alignment looks off.
> +{
> + int ret = of_property_read_u16(of_node, name, value);
> + if (ret)
> + value = 0;
You mean *value = 0 ?
I think we should be stricter here. Look at the docs at [1]. A result of
0 means success and -EINVAL means that the property does not exist. We
should still report errors for the other errno codes.
Something like
ret = of_property_read_u16()
if (ret) {
if(ret == -EINVAL) {
*value = 0;
ret= 0;
} else {
drm_err(dev, "simplefb: cannot parse framebuffer %s:
"error %d\n", name, ret);
}
}
return ret;
[1] https://elixir.bootlin.com/linux/latest/source/include/linux/of.h#L1202
> +}
> +
> static int
> simplefb_read_u32_of(struct drm_device *dev, struct device_node *of_node,
> const char *name, u32 *value)
> @@ -184,6 +193,21 @@ simplefb_get_format_of(struct drm_device *dev, struct device_node *of_node)
> return simplefb_get_validated_format(dev, format);
> }
>
> +static u16
> +simplefb_get_width_mm_of(struct drm_device *dev, struct device_node *of_node)
> +{
> + u16 width_mm;
> + simplefb_read_u16_of_optional(dev, of_node, "width-mm", &width_mm);
> + return width_mm;
> +}
> +
> +static u16
> +simplefb_get_height_mm_of(struct drm_device *dev, struct device_node *of_node)
> +{
> + u16 height_mm;
> + simplefb_read_u16_of_optional(dev, of_node, "height-mm", &height_mm);
> + return height_mm;
> +}
> /*
> * Simple Framebuffer device
> */
> @@ -599,16 +623,24 @@ static const struct drm_mode_config_funcs simpledrm_mode_config_funcs = {
> */
>
> static struct drm_display_mode simpledrm_mode(unsigned int width,
> - unsigned int height)
> + unsigned int height,
> + u16 width_mm,
> + u16 height_mm)
> {
> /*
> - * Assume a monitor resolution of 96 dpi to
> - * get a somewhat reasonable screen size.
> + * Assume a monitor resolution of 96 dpi if physical
> + * dimensions are not specified to get a somewhat reasonable
Please move 'dimensions' to the previous line to make it more pleasant
to the eyes.
> + * screen size.
> */
> +
> const struct drm_display_mode mode = {
> - DRM_MODE_INIT(60, width, height,
> - DRM_MODE_RES_MM(width, 96ul),
> - DRM_MODE_RES_MM(height, 96ul))
> + DRM_MODE_INIT(
> + 60,
> + width,
> + height,
> + (width_mm ? width_mm : DRM_MODE_RES_MM(width, 96ul)),
> + (height_mm ? height_mm : DRM_MODE_RES_MM(height, 96ul))
> + )
The coding style is awkward and the ?: doesn't belong here. Please see
further below.
> };
>
> return mode;
> @@ -622,6 +654,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> struct simpledrm_device *sdev;
> struct drm_device *dev;
> int width, height, stride;
> + u16 width_mm, height_mm;
Init those two variables to zero.
> const struct drm_format_info *format;
> struct resource *res, *mem;
> void __iomem *screen_base;
> @@ -676,6 +709,8 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> format = simplefb_get_format_of(dev, of_node);
> if (IS_ERR(format))
> return ERR_CAST(format);
> + width_mm = simplefb_get_width_mm_of(dev, of_node);
> + height_mm = simplefb_get_height_mm_of(dev, of_node);
> } else {
> drm_err(dev, "no simplefb configuration found\n");
> return ERR_PTR(-ENODEV);
> @@ -686,7 +721,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> return ERR_PTR(-EINVAL);
> }
Just like with the framebuffer stride, here's the place to detect
default values. So at this point, do something like
if (!width_mm)
width_mm = DRM_MODE_RES_MM(width, 96ul);
if (!height_mm)
height_mm = DRM_MODE_RES_MM(height, 96ul);
And pass the initialized physical dimensions to simpldrm_mode(). You can
move the comment in simpledrm_mode() before the branches.
Best regards
Thomas
>
> - sdev->mode = simpledrm_mode(width, height);
> + sdev->mode = simpledrm_mode(width, height, width_mm, height_mm);
> sdev->format = format;
> sdev->pitch = stride;
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)
Powered by blists - more mailing lists