Re: [PATCH v2 07/10] drm/ofdrm: Add ofdrm for Open Firmware framebuffers

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 2878 bytes --]

Hi

Am 26.07.22 um 15:17 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 7/20/22 16:27, Thomas Zimmermann wrote:
>> Open Firmware provides basic display output via the 'display' node.
>> DT platform code already provides a device that represents the node's
>> framebuffer. Add a DRM driver for the device. The display mode and
>> color format is pre-initialized by the system's firmware. Runtime
>> modesetting via DRM is not possible. The display is useful during
>> early boot stages or as error fallback.
>>
> 
> I'm not familiar with OF display but the driver looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> I just have a few questions below.
> 
> [...]
> 
>> +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_crtc_state *new_crtc_state;
>> +	int ret;
>> +
>> +	if (!new_plane_state->fb)
>> +		return 0;
>> +
>> +	new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc);
>> +
>> +	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  false, false);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
> 
> This seems to be exactly the same check than used in the simpledrm driver.
> Maybe could be moved to the fwfb helper library too ?

I've meanwhile dropped fwfb helpers. Color management requires specific 
code here, so there's no much to share anyway.

> 
> [...]
> 
>> +
>> +static void ofdrm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
>> +					     struct drm_atomic_state *old_state)
>> +{
>> +	/*
>> +	 * Always enabled; disabling clears the screen in the
>> +	 * primary plane's atomic_disable function.
>> +	 */
>> +}
>> +
> 
> Same comment than for simpledrm, are these no-op helpers really needed ?

In simpledrm, I've meanwhile removed them. I'll do so here as well.

> 
> [...]
> 
>> +static const struct of_device_id ofdrm_of_match_display[] = {
>> +	{ .compatible = "display", },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, ofdrm_of_match_display);
>> +
> 
> I don't see a binding for this in Documentation/devicetree/bindings/display.
> Do we need one or it's that only required for FDT and not Open Firmware DT ?
> 

No idea. The device is being created in drivers/of/platform.c. If offb 
didn't need these bindings, ofdrm probably won't need them either.

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]