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 <tzimmerm...@suse.de> ---Reviewed-by: Javier Martinez Canillas <javi...@redhat.com> [...]+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
OpenPGP_signature
Description: OpenPGP digital signature
