Hi Am 26.07.22 um 15:49 schrieb Javier Martinez Canillas:
On 7/20/22 16:27, Thomas Zimmermann wrote:Support the CRTC's color-management property and implement each model's palette support. The OF hardware has different methods of setting the palette. The respective code has been taken from fbdev's offb and refactored into per-model device functions. The device functions integrate this functionality into the overall modesetting. As palette handling is a CRTC property that depends on the primary plane's color format, the plane's atomic_check helper now updates the format field in ofdrm's custom CRTC state. The CRTC's atomic_flush helper updates the palette for the format as needed. Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx> ---Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> [...]+static void __iomem *ofdrm_mach64_cmap_ioremap(struct ofdrm_device *odev, + struct device_node *of_node, + u64 fb_base) +{ + struct drm_device *dev = &odev->dev; + u64 address; + void __iomem *cmap_base; + + address = fb_base & 0xff000000ul; + address += 0x7ff000; +It would be good to know where these addresses are coming from. Maybe some constant macros or a comment ? Same for the other places where addresses and offsets are used.
I have no idea where these values come from. I took them from offb. And I suspect that some of these CMAP helpers could be further merged if only it was clear where the numbers come from. But as i don't have the equipment for testing, I took most of this literally as-is from offb.
[...]static struct ofdrm_crtc_state *to_ofdrm_crtc_state(struct drm_crtc_state *base) @@ -376,10 +735,12 @@ 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_framebuffer *new_fb = new_plane_state->fb; struct drm_crtc_state *new_crtc_state; + struct ofdrm_crtc_state *new_ofdrm_crtc_state; int ret;- if (!new_plane_state->fb)+ if (!new_fb) return 0;new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc);@@ -391,6 +752,14 @@ static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane, if (ret) return ret;+ if (!new_plane_state->visible)+ return 0; + + new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc); + + new_ofdrm_crtc_state = to_ofdrm_crtc_state(new_crtc_state); + new_ofdrm_crtc_state->format = new_fb->format; +Ah, I understand now why you didn't factor out the .atomic_check callbacks for the two drivers in a fwfb helper. Maybe you can also add a comment to mention that this updates the format so the CRTC palette can be applied in the .atomic_flush callback ?
Yeah, this code is one reason for not sharing atomic_check in fwfb. The other reason is that the fwfb code is only a wrapper around the atomic helpers with little extra value. I did have such fwfb helpers a some point, but removed them.
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
