From: Javier Martinez Canillas <javierm@redhat.com> To: Thomas Zimmermann <tzimmermann@suse.de>, airlied@linux.ie, daniel@ffwll.ch, deller@gmx.de, maxime@cerno.tech, sam@ravnborg.org, msuchanek@suse.de, mpe@ellerman.id.au, benh@kernel.crashing.org, paulus@samba.org Cc: dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH 2/2] drm/tiny: Add ofdrm for Open Firmware framebuffers Date: Fri, 20 May 2022 08:19:11 +0200 [thread overview] Message-ID: <84a550c3-dcef-17ac-0ae5-666cdf0fb67e@redhat.com> (raw) In-Reply-To: <20220518183006.14548-3-tzimmermann@suse.de> 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@redhat.com> -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
WARNING: multiple messages have this Message-ID (diff)
From: Javier Martinez Canillas <javierm@redhat.com> To: Thomas Zimmermann <tzimmermann@suse.de>, airlied@linux.ie, daniel@ffwll.ch, deller@gmx.de, maxime@cerno.tech, sam@ravnborg.org, msuchanek@suse.de, mpe@ellerman.id.au, benh@kernel.crashing.org, paulus@samba.org Cc: linux-fbdev@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 2/2] drm/tiny: Add ofdrm for Open Firmware framebuffers Date: Fri, 20 May 2022 08:19:11 +0200 [thread overview] Message-ID: <84a550c3-dcef-17ac-0ae5-666cdf0fb67e@redhat.com> (raw) In-Reply-To: <20220518183006.14548-3-tzimmermann@suse.de> 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@redhat.com> -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
next prev parent reply other threads:[~2022-05-20 6:19 UTC|newest] Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-18 18:30 [PATCH 0/2] drm: Add driverof PowerPC OF displays Thomas Zimmermann 2022-05-18 18:30 ` Thomas Zimmermann 2022-05-18 18:30 ` [PATCH 1/2] MAINTAINERS: Broaden scope of simpledrm entry Thomas Zimmermann 2022-05-18 18:30 ` Thomas Zimmermann 2022-05-20 5:18 ` Javier Martinez Canillas 2022-05-20 5:18 ` Javier Martinez Canillas 2022-05-18 18:30 ` [PATCH 2/2] drm/tiny: Add ofdrm for Open Firmware framebuffers Thomas Zimmermann 2022-05-18 18:30 ` Thomas Zimmermann 2022-05-18 18:51 ` Michal Suchánek 2022-05-18 18:51 ` Michal Suchánek 2022-05-18 18:51 ` Michal Suchánek 2022-05-19 7:11 ` Geert Uytterhoeven 2022-05-19 7:11 ` Geert Uytterhoeven 2022-05-19 7:11 ` Geert Uytterhoeven 2022-05-19 7:27 ` Thomas Zimmermann 2022-05-19 7:27 ` Thomas Zimmermann 2022-05-19 7:27 ` Thomas Zimmermann 2022-05-21 2:49 ` Benjamin Herrenschmidt 2022-05-21 2:49 ` Benjamin Herrenschmidt 2022-05-21 2:49 ` Benjamin Herrenschmidt 2022-05-25 16:45 ` Thomas Zimmermann 2022-05-25 16:45 ` Thomas Zimmermann 2022-05-25 16:45 ` Thomas Zimmermann 2022-05-27 0:17 ` Benjamin Herrenschmidt 2022-05-27 0:17 ` Benjamin Herrenschmidt 2022-05-27 0:17 ` Benjamin Herrenschmidt 2022-05-27 0:19 ` Benjamin Herrenschmidt 2022-05-27 0:19 ` Benjamin Herrenschmidt 2022-05-27 0:19 ` Benjamin Herrenschmidt 2022-05-21 1:35 ` Benjamin Herrenschmidt 2022-05-21 1:35 ` Benjamin Herrenschmidt 2022-05-21 1:35 ` Benjamin Herrenschmidt 2022-05-18 21:11 ` Mark Cave-Ayland 2022-05-19 6:34 ` Michal Suchánek 2022-05-19 6:34 ` Michal Suchánek 2022-05-19 6:34 ` Michal Suchánek 2022-05-19 7:39 ` Thomas Zimmermann 2022-05-20 6:19 ` Javier Martinez Canillas [this message] 2022-05-20 6:19 ` Javier Martinez Canillas 2022-05-22 19:35 ` Thomas Zimmermann 2022-05-22 19:35 ` Thomas Zimmermann 2022-05-25 3:15 ` Benjamin Herrenschmidt 2022-05-25 3:15 ` Benjamin Herrenschmidt
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=84a550c3-dcef-17ac-0ae5-666cdf0fb67e@redhat.com \ --to=javierm@redhat.com \ --cc=airlied@linux.ie \ --cc=benh@kernel.crashing.org \ --cc=daniel@ffwll.ch \ --cc=deller@gmx.de \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-fbdev@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=maxime@cerno.tech \ --cc=mpe@ellerman.id.au \ --cc=msuchanek@suse.de \ --cc=paulus@samba.org \ --cc=sam@ravnborg.org \ --cc=tzimmermann@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.