On 7/27/22 10:41, Thomas Zimmermann wrote:

[...]

quoted

quoted

+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.

I see. As Michal mentioned maybe someone more familiar with this platform
could shed some light about these but in any case that could be done later.

[...]

quoted

quoted

+
+	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.

Got it.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat