Re: [PATCH v2 10/10] drm/ofdrm: Support color management
From: Javier Martinez Canillas <javierm@redhat.com>
Date: 2022-07-26 14:02:13
Also in:
dri-devel, linuxppc-dev
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@suse.de> ---
Reviewed-by: Javier Martinez Canillas <javierm@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. [...]
quoted hunk ↗ jump to hunk
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 ? -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat