[PATCH 2/2] drm/tiny: Add ofdrm for Open Firmware framebuffers
Thomas Zimmermann
tzimmermann at suse.de
Mon May 23 05:35:17 AEST 2022
Hi Javier
Am 20.05.22 um 08:19 schrieb Javier Martinez Canillas:
> 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`
I have to look for possible pitfalls, but that seems to makes sense.
>
>> +
>> +/*
>> + * 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 ?
To me it seems that these helpers are so small that they really don't
need to be shared. If anything, we could add them directly to the OF
module. Something like of_property_read_u32_range() that takes additions
min/max values.
>
>> +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 ?
There should be no warnings if multiple case statements are directly
next to each other.
>
>> +}
>> +
>> +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 ?
No point in exporting them AFAICT. They look like the code in simpledrm,
but they are specific to this single driver. In this context 'OF' is not
OF in general, but the specific generic display of PPC's OF device tree.
No other DRM driver should use this.
>
>> +#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.
That was one of the problems, but not the one I mentioned. :) That one
was related to the setting of preferred_color_depth.
>
>
>> +/*
>> + * 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.
Even better, I meanwhile made a little helper library for the whole
modesetting pipeline and implemented simpledrm and ofdrm on top of it.
It only needs the information about the native buffer and does the rest.
>
>> +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 ?
We still don't support XRGB1555. If the native buffer uses this format,
we'd have no conversion helper. In this case, we rely on userspace/fbcon
to use the native format exclusively. (BTW, I asked one of my coworkers
to implement XRGB1555 to make her familiar with DRM. That's why I
haven't sent a patch yet.)
>
> The rest of the patch looks good to me, thanks a lot for writing this.
>
> Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>
>
Thanks a lot.
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20220522/3b90ee98/attachment-0001.sig>
More information about the Linuxppc-dev
mailing list