[RFC PATCH] drm/simpledrm: Allow physical width and height configuration via DT
Rayyan Ansari
rayyan at ansari.sh
Fri Jan 20 17:25:45 UTC 2023
On 19/01/2023 10:01, Janne Grunau wrote:
> Hej,
>
> adding devicetree at vger.kernel.org and asahi at lists.linux.dev to cc:, the
> former for the obvious devictree/bindings related questions,
> asahi for the prospect of supporting high DPI displays during early boot
> and in u-boot.
>
> On 2023-01-18 18:48:17 +0000, Rayyan Ansari wrote:
>> 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 at 3200000 {
>> 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.
>>
>> 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?
>
> u32 is the appropriate type. The device tree describes the hardware and
> not the data types used in a "random" linux driver/subsystem. 65m is
> probably enough for all practical purposes but u32 is the better choice.
> Documentation/devicetree/bindings/display/panel/panel-common.yaml
> already specifies "height-mm" and "width-mm" and all device tree files
> using this binding code the properties as u32.
>
Okay, will change.
> We probably do not want add height and width properties to the
> simple-framebuffer node directly. At least for the static case I would
> expect that it duplicates information already present in a panel node.
> For that case parsing the panel dimensions via a phandle reference to
> that panel node would be preferred.
In my case, there is currently no panel driver. The interface I
mentioned (Phosh) is running on the simpledrm driver.
Here is my Lumia running this interface:
https://wiki.postmarketos.org/images/c/c3/Lumia_735_Phosh.png
>
> I'm not sure if it worth considering the dynamic case. The bootloader
> may be able to provide dimensions of HDMI, DP, ... connected displays
> from the EDID. In that case "height-mm" and "width-mm" properties would
> make sense.
>
> The existing panel drivers seem to ignore the u32 -> u16 conversion
> problem.
>
>> - 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.
>
> I think the code would be more readable if width_mm and height_mm would
> be calculated outside of DRM_MODE_INIT if they are zero.
>
>> - 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.
>
> Documentation/devicetree/bindings/display/simple-framebuffer.yaml needs
> to be updates to list and document the properties added to the node.
>
>> ---
>> 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,
>> + const char *name, u16 *value)
>> +{
>> + int ret = of_property_read_u16(of_node, name, value);
>> + if (ret)
>> + value = 0;
>> +}
>> +
>> 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;
>> +}
>
> I don't think it makes sense to have these two mostly identical wrapper
> functions. Please pass the name of the property as parameter. It could
> make sense to have a function to both height and width. I think we
> should ignore both height and width if one fails to parse or is 0.
> That could of course also be done in simpledrm_mode() for example like:
>
> | if (!width_mm || !height_mm) {
> | width_mm = DRM_MODE_RES_MM(width, 96ul);
> | height_mm = DRM_MODE_RES_MM(height, 96ul);
> | }
>
I based this on the way the pixel height and width is fetched from DT
(simplefb_get_width_of and simplefb_get_height_of) but changing it to
one function makes sense.
>> /*
>> * 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
>> + * 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))
>> + )
>> };
>>
>> 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;
>
> these need to be initialized to 0 otherwise they may end up used
> unitialized if pd is not NULL.
>
Noted.
>> 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);
>> }
>>
>> - sdev->mode = simpledrm_mode(width, height);
>> + sdev->mode = simpledrm_mode(width, height, width_mm, height_mm);
>> sdev->format = format;
>> sdev->pitch = stride;
>
> Janne
Thanks for the feedback! I'll write the documentation and improve on the
points mentioned to make this mainline-ready.
--
Rayyan Ansari
https://ansari.sh
More information about the dri-devel
mailing list