lkml.org 
[lkml]   [2023]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [RFC PATCH] drm/simpledrm: Allow physical width and height configuration via DT
    From
    (cc: devicetree@vger.kernel.org, asahi@lists.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@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.

    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
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2023-03-26 23:47    [W:3.433 / U:0.080 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site