On 7/27/22 10:41, Thomas Zimmermann wrote:
[...]
>>
>>> +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.
[...]
>>> +
>>> + 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
