Hi

Am 18.05.22 um 23:11 schrieb Mark Cave-Ayland:
> On 18/05/2022 19:30, Thomas Zimmermann wrote:
> 
>> Open Firmware provides basic display output via the 'display' node.
>> DT platform code already provides a device that represents the node's
>> framebuffer. Add a DRM driver for the device. The display mode and
>> color format is pre-initialized by the system's firmware. Runtime
>> modesetting via DRM is not possible. The display is useful during
>> early boot stages or as error fallback.
>>
>> Similar functionality is already provided by fbdev's offb driver,
>> which is insufficient for modern userspace. The old driver includes
>> support for BootX device tree, which can be found on old 32-bit
>> PowerPC Macintosh systems. If these are still in use, the
>> functionality can be added to ofdrm or implemented in a new
>> driver. As with simepldrm, the fbdev driver cannot be selected is
>> ofdrm is already enabled.
>>
>> Two noteable points about the driver:
>>
>>   * Reading the framebuffer aperture from the device tree is not
>> reliable on all systems. Ofdrm takes the heuristics and a comment
>> from offb to pick the correct range.
>>
>>   * No resource management may be tied to the underlying PCI device.
>> Otherwise the handover to the native driver will fail with a resource
>> conflict. PCI management is therefore done as part of the platform
>> device's cleanup.
>>
>> The driver has been tested on qemu's ppc64le emulation. The device
>> hand-over has been tested with bochs.
> 
> Thanks for working on this! Have you tried it on qemu-system-sparc and 
> qemu-system-sparc64 at all? At least under QEMU I'd expect it to work 
> for these platforms too, unless there is a particular dependency on PCI. 
> A couple of comments inline below:

I haven't tried Sparc, as the old offb driver only supports PPC.

I was already wondering why Sparc isn't supported. I wouldn't mind if we 
can add it.

> 
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   MAINTAINERS                   |   1 +
>>   drivers/gpu/drm/tiny/Kconfig  |  12 +
>>   drivers/gpu/drm/tiny/Makefile |   1 +
>>   drivers/gpu/drm/tiny/ofdrm.c  | 748 ++++++++++++++++++++++++++++++++++
>>   drivers/video/fbdev/Kconfig   |   1 +
>>   5 files changed, 763 insertions(+)
>>   create mode 100644 drivers/gpu/drm/tiny/ofdrm.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 43d833273ae9..090cbe1aa5e3 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6395,6 +6395,7 @@ L:    dri-devel@lists.freedesktop.org
>>   S:    Maintained
>>   T:    git git://anongit.freedesktop.org/drm/drm-misc
>>   F:    drivers/gpu/drm/drm_aperture.c
>> +F:    drivers/gpu/drm/tiny/ofdrm.c
>>   F:    drivers/gpu/drm/tiny/simpledrm.c
>>   F:    include/drm/drm_aperture.h
>> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
>> index 627d637a1e7e..0bc54af42e7f 100644
>> --- a/drivers/gpu/drm/tiny/Kconfig
>> +++ b/drivers/gpu/drm/tiny/Kconfig
>> @@ -51,6 +51,18 @@ config DRM_GM12U320
>>        This is a KMS driver for projectors which use the GM12U320 chipset
>>        for video transfer over USB2/3, such as the Acer C120 mini 
>> projector.
>> +config DRM_OFDRM
>> +    tristate "Open Firmware display driver"
>> +    depends on DRM && MMU && PPC
>> +    select DRM_GEM_SHMEM_HELPER
>> +    select DRM_KMS_HELPER
>> +    help
>> +      DRM driver for Open Firmware framebuffers.
>> +
>> +      This driver assumes that the display hardware has been initialized
>> +      by the Open Firmware before the kernel boots. Scanout buffer, 
>> size,
>> +      and display format must be provided via device tree.
>> +
>>   config DRM_PANEL_MIPI_DBI
>>       tristate "DRM support for MIPI DBI compatible panels"
>>       depends on DRM && SPI
>> diff --git a/drivers/gpu/drm/tiny/Makefile 
>> b/drivers/gpu/drm/tiny/Makefile
>> index 1d9d6227e7ab..76dde89a044b 100644
>> --- a/drivers/gpu/drm/tiny/Makefile
>> +++ b/drivers/gpu/drm/tiny/Makefile
>> @@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_ARCPGU)        += arcpgu.o
>>   obj-$(CONFIG_DRM_BOCHS)            += bochs.o
>>   obj-$(CONFIG_DRM_CIRRUS_QEMU)        += cirrus.o
>>   obj-$(CONFIG_DRM_GM12U320)        += gm12u320.o
>> +obj-$(CONFIG_DRM_OFDRM)            += ofdrm.o
>>   obj-$(CONFIG_DRM_PANEL_MIPI_DBI)    += panel-mipi-dbi.o
>>   obj-$(CONFIG_DRM_SIMPLEDRM)        += simpledrm.o
>>   obj-$(CONFIG_TINYDRM_HX8357D)        += hx8357d.o
>> diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
>> new file mode 100644
>> index 000000000000..aca715b36179
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tiny/ofdrm.c
>> @@ -0,0 +1,748 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +#include <linux/pci.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include <drm/drm_aperture.h>
>> +#include <drm/drm_atomic_state_helper.h>
>> +#include <drm/drm_connector.h>
>> +#include <drm/drm_damage_helper.h>
>> +#include <drm/drm_device.h>
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_fb_helper.h>
>> +#include <drm/drm_format_helper.h>
>> +#include <drm/drm_gem_atomic_helper.h>
>> +#include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_gem_shmem_helper.h>
>> +#include <drm/drm_managed.h>
>> +#include <drm/drm_modeset_helper_vtables.h>
>> +#include <drm/drm_probe_helper.h>
>> +#include <drm/drm_simple_kms_helper.h>
>> +
>> +
>> +#define DRIVER_NAME    "ofdrm"
>> +#define DRIVER_DESC    "DRM driver for OF platform devices"
>> +#define DRIVER_DATE    "20220501"
>> +#define DRIVER_MAJOR    1
>> +#define DRIVER_MINOR    0
>> +
>> +/*
>> + * Assume a monitor resolution of 96 dpi to
>> + * get a somewhat reasonable screen size.
>> + */
>> +#define RES_MM(d)    \
>> +    (((d) * 254ul) / (96ul * 10ul))
>> +
>> +#define OFDRM_MODE(hd, vd)    \
>> +    DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd))
>> +
>> +/*
>> + * Helpers for display nodes
>> + */
>> +
>> +static int display_get_validated_int(struct drm_device *dev, const 
>> char *name, uint32_t value)
>> +{
>> +    if (value > INT_MAX) {
>> +        drm_err(dev, "invalid framebuffer %s of %u\n", name, value);
>> +        return -EINVAL;
>> +    }
>> +    return (int)value;
>> +}
>> +
>> +static int display_get_validated_int0(struct drm_device *dev, const 
>> char *name, uint32_t value)
>> +{
>> +    if (!value) {
>> +        drm_err(dev, "invalid framebuffer %s of %u\n", name, value);
>> +        return -EINVAL;
>> +    }
>> +    return display_get_validated_int(dev, name, value);
>> +}
>> +
>> +static const struct drm_format_info 
>> *display_get_validated_format(struct drm_device *dev,
>> +                                  u32 depth)
>> +{
>> +    const struct drm_format_info *info;
>> +    u32 format;
>> +
>> +    switch (depth) {
>> +    case 8:
>> +        format = drm_mode_legacy_fb_format(8, 8);
>> +        break;
>> +    case 15:
>> +    case 16:
>> +        format = drm_mode_legacy_fb_format(16, depth);
>> +        break;
>> +    case 32:
>> +        format = drm_mode_legacy_fb_format(32, 24);
>> +        break;
>> +    default:
>> +        drm_err(dev, "unsupported framebuffer depth %u\n", depth);
>> +        return ERR_PTR(-EINVAL);
>> +    }
>> +
>> +    info = drm_format_info(format);
>> +    if (!info) {
>> +        drm_err(dev, "cannot find framebuffer format for depth %u\n", 
>> depth);
>> +        return ERR_PTR(-EINVAL);
>> +    }
>> +
>> +    return info;
>> +}
>> +
>> +static int display_read_u32_of(struct drm_device *dev, struct 
>> device_node *of_node,
>> +                   const char *name, u32 *value)
>> +{
>> +    int ret = of_property_read_u32(of_node, name, value);
>> +
>> +    if (ret)
>> +        drm_err(dev, "cannot parse framebuffer %s: error %d\n", name, 
>> ret);
>> +    return ret;
>> +}
>> +
>> +static int display_get_width_of(struct drm_device *dev, struct 
>> device_node *of_node)
>> +{
>> +    u32 width;
>> +    int ret = display_read_u32_of(dev, of_node, "width", &width);
>> +
>> +    if (ret)
>> +        return ret;
>> +    return display_get_validated_int0(dev, "width", width);
>> +}
>> +
>> +static int display_get_height_of(struct drm_device *dev, struct 
>> device_node *of_node)
>> +{
>> +    u32 height;
>> +    int ret = display_read_u32_of(dev, of_node, "height", &height);
>> +
>> +    if (ret)
>> +        return ret;
>> +    return display_get_validated_int0(dev, "height", height);
>> +}
>> +
>> +static int display_get_linebytes_of(struct drm_device *dev, struct 
>> device_node *of_node)
>> +{
>> +    u32 linebytes;
>> +    int ret = display_read_u32_of(dev, of_node, "linebytes", 
>> &linebytes);
>> +
>> +    if (ret)
>> +        return ret;
>> +    return display_get_validated_int(dev, "linebytes", linebytes);
>> +}
>> +
>> +static const struct drm_format_info *display_get_format_of(struct 
>> drm_device *dev,
>> +                               struct device_node *of_node)
>> +{
>> +    u32 depth;
>> +    int ret = display_read_u32_of(dev, of_node, "depth", &depth);
>> +
>> +    if (ret)
>> +        return ERR_PTR(ret);
>> +    return display_get_validated_format(dev, depth);
>> +}
>> +
>> +static u64 display_get_address_of(struct drm_device *dev, struct 
>> device_node *of_node)
>> +{
>> +    u32 address;
>> +    int ret;
>> +
>> +    /*
>> +     * Not all devices provide an address property, it's not
>> +     * a bug if this fails. The driver will try to find the
>> +     * framebuffer base address from the device's memory regions.
>> +     */
>> +    ret = of_property_read_u32(of_node, "address", &address);
>> +    if (ret)
>> +        return OF_BAD_ADDR;
>> +
>> +    return address;
>> +}
>> +
>> +#if defined(CONFIG_PCI)
>> +static struct pci_dev *display_get_pci_dev_of(struct drm_device *dev, 
>> struct device_node *of_node)
>> +{
>> +    const __be32 *vendor_p, *device_p;
>> +    u32 vendor, device;
>> +    struct pci_dev *pcidev;
>> +
>> +    vendor_p = of_get_property(of_node, "vendor-id", NULL);
>> +    if (!vendor_p)
>> +        return ERR_PTR(-ENODEV);
>> +    vendor = be32_to_cpup(vendor_p);
>> +
>> +    device_p = of_get_property(of_node, "device-id", NULL);
>> +    if (!device_p)
>> +        return ERR_PTR(-ENODEV);
>> +    device = be32_to_cpup(device_p);
>> +
>> +    pcidev = pci_get_device(vendor, device, NULL);
>> +    if (!pcidev)
>> +        return ERR_PTR(-ENODEV);
>> +
>> +    return pcidev;
>> +}
>> +#else
>> +static struct pci_dev *display_get_pci_dev_of(struct drm_device *dev, 
>> struct device_node *of_node)
>> +{
>> +    return ERR_PTR(-ENODEV);
>> +}
>> +#endif
>> +
>> +/*
>> + * Open Firmware display device
>> + */
>> +
>> +struct ofdrm_device {
>> +    struct drm_device dev;
>> +    struct platform_device *pdev;
>> +
>> +    /* OF display settings */
>> +    struct drm_display_mode mode;
>> +    const struct drm_format_info *format;
>> +    unsigned int pitch;
>> +    resource_size_t fb_base;
>> +    resource_size_t fb_size;
>> +
>> +    /* memory management */
>> +    struct resource *mem;
>> +    void __iomem *screen_base;
>> +
>> +    /* modesetting */
>> +    uint32_t formats[8];
>> +    size_t nformats;
>> +    struct drm_connector connector;
>> +    struct drm_simple_display_pipe pipe;
>> +};
>> +
>> +static struct ofdrm_device *ofdrm_device_of_dev(struct drm_device *dev)
>> +{
>> +    return container_of(dev, struct ofdrm_device, dev);
>> +}
>> +
>> +/*
>> + *  OF display settings
>> + */
>> +
>> +static void ofdrm_pci_release(void *data)
>> +{
>> +    struct pci_dev *pcidev = data;
>> +
>> +    pci_disable_device(pcidev);
>> +}
>> +
>> +static struct drm_display_mode ofdrm_mode(unsigned int width, 
>> unsigned int height)
>> +{
>> +    struct drm_display_mode mode = { OFDRM_MODE(width, height) };
>> +
>> +    mode.clock = mode.hdisplay * mode.vdisplay * 60 / 1000 /* kHz */;
>> +    drm_mode_set_name(&mode);
>> +
>> +    return mode;
>> +}
>> +
>> +static struct resource *ofdrm_find_fb_resource(struct ofdrm_device 
>> *odev,
>> +                           struct resource *fb_res)
>> +{
>> +    struct platform_device *pdev = to_platform_device(odev->dev.dev);
>> +    struct resource *res, *max_res = NULL;
>> +    u32 i;
>> +
>> +    for (i = 0; pdev->num_resources; ++i) {
>> +        res = platform_get_resource(pdev, IORESOURCE_MEM, i);
>> +        if (!res)
>> +            break; /* all resources processed */
>> +        if (resource_size(res) < resource_size(fb_res))
>> +            continue; /* resource too small */
>> +        if (fb_res->start && resource_contains(res, fb_res))
>> +            return res; /* resource contains framebuffer */
>> +        if (!max_res || resource_size(res) > resource_size(max_res))
>> +            max_res = res; /* store largest resource as fallback */
>> +    }
>> +
>> +    return max_res;
>> +}
>> +
>> +static int ofdrm_device_init_fb(struct ofdrm_device *odev)
>> +{
>> +    struct drm_device *dev = &odev->dev;
>> +    struct platform_device *pdev = odev->pdev;
>> +    struct device_node *of_node = pdev->dev.of_node;
>> +    int width, height, linebytes;
>> +    u64 address;
>> +    const struct drm_format_info *format;
>> +    struct pci_dev *pcidev;
>> +    struct resource *res;
>> +    int ret;
>> +
>> +    width = display_get_width_of(dev, of_node);
>> +    if (width < 0)
>> +        return width;
>> +    height = display_get_height_of(dev, of_node);
>> +    if (height < 0)
>> +        return height;
>> +    linebytes = display_get_linebytes_of(dev, of_node);
>> +    if (linebytes < 0)
>> +        return linebytes;
>> +    format = display_get_format_of(dev, of_node);
>> +    if (IS_ERR(format))
>> +        return PTR_ERR(format);
>> +    address = display_get_address_of(dev, of_node);
>> +
>> +    /*
>> +     * Never use pcim_ or other managed helpers on the returned PCI
>> +     * device. Otherwise, probing the native driver will fail for
>> +     * resource conflicts. PCI-device management has to be tied to
>> +     * the lifetime of the platform device until the native driver
>> +     * takes over.
>> +     */
>> +    pcidev = display_get_pci_dev_of(dev, of_node);
>> +    if (!IS_ERR(pcidev)) {
>> +        ret = pci_enable_device(pcidev);
>> +        if (drm_WARN(dev, ret, "pci_enable_device(%s) failed: %d\n",
>> +                 dev_name(&pcidev->dev), ret))
>> +            return ret;
>> +        ret = devm_add_action_or_reset(&pdev->dev, ofdrm_pci_release, 
>> pcidev);
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>> +    odev->mode = ofdrm_mode(width, height);
>> +    odev->format = format;
>> +
>> +    if (linebytes)
>> +        odev->pitch = linebytes;
>> +    else
>> +        odev->pitch = width * format->cpp[0];
>> +
>> +    odev->fb_size = odev->pitch * height;
>> +
>> +    /*
>> +     * Try to figure out the address of the framebuffer. 
>> Unfortunately, Open
>> +     * Firmware doesn't provide a standard way to do so. All we can 
>> do is a
>> +     * dodgy heuristic that happens to work in practice.
> 
> This isn't quite true: there is a Forth value named "frame-buffer-adr" 
> which should be set to the physical screen base address for PCI and 
> non-PCI cards. It is mentioned in both the IEEE-1275 specification (page 
> 32) and according to OpenBIOS is also referenced by BootX: 
> https://mail.coreboot.org/pipermail/openbios/2011-August/006617.html.

I briefly looked at the patch and the spec document, but it looks like 
this is only for OF's Forth interface. (?)  The driver is build on top 
of device-tree values.

> 
>> +     * On most machines, the "address" property contains what we 
>> need, though
>> +     * not on Matrox cards found in IBM machines. What appears to 
>> give good
>> +     * results is to go through the PCI ranges and pick one that 
>> encloses the
>> +     * "address" property. If none match, we pick the largest.
>> +     */
> 
> In my experience so far "frame-buffer-adr" is the definitive way to get 
> the screen physical address, however you'll need to call the CIF 
> interface in order to get the value. The cases where I've seen an 
> "address" property in the display node is when the display has also been 
> mapped to a virtual address, which may or may not be the same as the 
> physical address. I suspect this optional virtual mapping may not be 
> present by the time the ofdrm driver loads?

It's all physical addresses.

At least on qemu's emulation, there's no 'frame-buffer-addr' device-tree 
value. Or is it a Sparc thing?

Best regards
Thomas


-- 
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