Hi Am 26.07.22 um 15:17 schrieb Javier Martinez Canillas:
Hello Thomas, On 7/20/22 16:27, 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.I'm not familiar with OF display but the driver looks good to me. Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> I just have a few questions below. [...]+static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane, + struct drm_atomic_state *new_state) +{ + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane); + struct drm_crtc_state *new_crtc_state; + int ret; + + if (!new_plane_state->fb) + return 0; + + new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc); + + ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state, + DRM_PLANE_HELPER_NO_SCALING, + DRM_PLANE_HELPER_NO_SCALING, + false, false); + if (ret) + return ret; + + return 0; +}This seems to be exactly the same check than used in the simpledrm driver. Maybe could be moved to the fwfb helper library too ?
I've meanwhile dropped fwfb helpers. Color management requires specific code here, so there's no much to share anyway.
[...]+ +static void ofdrm_crtc_helper_atomic_disable(struct drm_crtc *crtc, + struct drm_atomic_state *old_state) +{ + /* + * Always enabled; disabling clears the screen in the + * primary plane's atomic_disable function. + */ +} +Same comment than for simpledrm, are these no-op helpers really needed ?
In simpledrm, I've meanwhile removed them. I'll do so here as well.
[...]+static const struct of_device_id ofdrm_of_match_display[] = { + { .compatible = "display", }, + { }, +}; +MODULE_DEVICE_TABLE(of, ofdrm_of_match_display); +I don't see a binding for this in Documentation/devicetree/bindings/display. Do we need one or it's that only required for FDT and not Open Firmware DT ?
No idea. The device is being created in drivers/of/platform.c. If offb didn't need these bindings, ofdrm probably won't need them either.
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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
