Hi

Am 26.07.22 um 15:49 schrieb Javier Martinez Canillas:

On 7/20/22 16:27, Thomas Zimmermann wrote:

quoted

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@suse.de>
---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

[...]

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.

[...]

quoted

  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