Hello Thomas,
On 5/18/22 20:30, Thomas Zimmermann wrote:
>
> +config DRM_OFDRM
> + tristate "Open Firmware display driver"
> + depends on DRM && MMU && PPC
Shouldn't depend on OF? I mean, is a DRM driver for Open Firmware after all :)
I know that the old drivers/video/fbdev/offb.c doesn't, but I think that is a
but and should `depends on OF || COMPILE_TEST`
> +
> +/*
> + * 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);
> +}
> +
These two helpers are the same that we already have in simpledrm.c, maybe could
include a preparatory patch that moves to drivers/gpu/drm/drm_format_helper.c
and make them public for drivers to use ? Or maybe even as static inline in
include/drm/drm_format_helper.h ?
> +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:
I think is customary now to add /* fall through */ here to silence GCC warns ?
> +}
> +
> +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;
> +}
> +
[snip]
> +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;
> +}
> +
All these helpers seems to be quite generic and something that other OF
drivers could benefit from. Maybe add them to drivers/gpu/drm/drm_of.c ?
> +#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
> +
Unsure about this one, I don't see other display driver using a "vendor-id"
or "device-id" when looking at Documentation/devicetree/bindings/, so guess
this one would have to remain in the driver and not in a helper library.
But since you have #ifdefery here, maybe would be cleaner to have that stub
defined as static inline in include/drm/drm_of.h ?
> +static struct ofdrm_device *ofdrm_device_of_dev(struct drm_device *dev)
> +{
> + return container_of(dev, struct ofdrm_device, dev);
> +}
> +
> +/*
> + * OF display settings
> + */
> +
This seems like another candidate to move to the include/drm/drm_of.h header.
> +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 */;
Maybe a comment here about the clock value chosen ?
> + drm_mode_set_name(&mode);
> +
> + return mode;
> +}
> +
[snip]
> +
> + /*
> + * 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.
> + */
Ah, was this the issue that you mentioned the other day? How interesting.
> +/*
> + * Support all formats of OF display and maybe more; in order
> + * of preference. The display's update function will do any
> + * conversion necessary.
> + *
> + * TODO: Add blit helpers for remaining formats and uncomment
> + * constants.
> + */
> +static const uint32_t ofdrm_default_formats[] = {
> + DRM_FORMAT_XRGB8888,
> + DRM_FORMAT_RGB565,
> + //DRM_FORMAT_XRGB1555,
Wonder if makes sense to keep this commented and the TODO, why not
leave that format from first version and just do it as follow-up ?
> +static const struct drm_connector_funcs ofdrm_connector_funcs = {
> + .reset = drm_atomic_helper_connector_reset,
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .destroy = drm_connector_cleanup,
> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
All of the callbacks used comes from helper libraries so I maybe we
could have a macro or something to set those ? It's the same set that
are used in simpledrm so it would make sense to have them defined in
the same place.
> +static const struct drm_mode_config_funcs ofdrm_mode_config_funcs = {
> + .fb_create = drm_gem_fb_create_with_dirty,
> + .atomic_check = drm_atomic_helper_check,
> + .atomic_commit = drm_atomic_helper_commit,
> +};
> +
Same for these. We could also have a macro to define this for both
simpledrm and ofdrm.
> +static const uint32_t *ofdrm_device_formats(struct ofdrm_device *odev, size_t *nformats_out)
> +{
> + struct drm_device *dev = &odev->dev;
> + size_t i;
> +
> + if (odev->nformats)
> + goto out; /* don't rebuild list on recurring calls */
> +
Nice optimization to cache this.
> + /*
> + * TODO: The ofdrm driver converts framebuffers to the native
> + * format when copying them to device memory. If there are more
> + * formats listed than supported by the driver, the native format
> + * is not supported by the conversion helpers. Therefore *only*
> + * support the native format and add a conversion helper ASAP.
> + */
> + if (drm_WARN_ONCE(dev, i != odev->nformats,
> + "format conversion helpers required for %p4cc",
> + &odev->format->format)) {
> + odev->nformats = 1;
> + }
> +
Interesting. Did you find some formats that were not supported ?
The rest of the patch looks good to me, thanks a lot for writing this.
Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
