mbox series

[v2,00/10] drm: Add driver for PowerPC OF displays

Message ID 20220720142732.32041-1-tzimmermann@suse.de
Headers show
Series drm: Add driver for PowerPC OF displays | expand

Message

Thomas Zimmermann July 20, 2022, 2:27 p.m. UTC
(was: drm: Add driverof PowerPC OF displays)

PowerPC's Open Firmware offers a simple display buffer for graphics
output. Add ofdrm, a DRM driver for the device. As with the existing
simpledrm driver, the graphics hardware is pre-initialized by the
firmware. The driver only provides blitting, no actual DRM modesetting
is possible.

Version 2 of this patchset starts by cleaning up and refactoring
simpledrm, and moving some of the code in a helper library. These
functions are useful for ofdrm as well.

Patch 7 adds ofdrm, which has been significantly reworked since v1.
PCI is now optional and COMPILE_TEST is supported.

Patches 8 to 10 add support for color management. The code has been
taken from fbdev's offb. I have no hardware available for testing the
functionality. Qemu's stdvga apparently does not support gamma tables
in RGB modes. I verified that the color management code is executed
by running Gnome's night-mode settings, but the display's color tone
does not change.

Thomas Zimmermann (10):
  drm/simpledrm: Remove mem field from device structure
  drm/simpledrm: Inline device-init helpers
  drm/simpledrm: Remove pdev field from device structure
  drm/simpledrm: Compute framebuffer stride if not set
  drm/simpledrm: Convert to atomic helpers
  drm/simpledrm: Move some functionality into fwfb helper library
  drm/ofdrm: Add ofdrm for Open Firmware framebuffers
  drm/ofdrm: Add CRTC state
  drm/ofdrm: Add per-model device function
  drm/ofdrm: Support color management

 Documentation/gpu/drm-kms-helpers.rst |   12 +
 MAINTAINERS                           |    3 +
 drivers/gpu/drm/Kconfig               |    6 +
 drivers/gpu/drm/Makefile              |    3 +-
 drivers/gpu/drm/drm_fwfb_helper.c     |  301 ++++++
 drivers/gpu/drm/tiny/Kconfig          |   15 +
 drivers/gpu/drm/tiny/Makefile         |    1 +
 drivers/gpu/drm/tiny/ofdrm.c          | 1301 +++++++++++++++++++++++++
 drivers/gpu/drm/tiny/simpledrm.c      |  588 +++++------
 drivers/video/fbdev/Kconfig           |    1 +
 include/drm/drm_fwfb_helper.h         |   51 +
 11 files changed, 1949 insertions(+), 333 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_fwfb_helper.c
 create mode 100644 drivers/gpu/drm/tiny/ofdrm.c
 create mode 100644 include/drm/drm_fwfb_helper.h

Comments

Javier Martinez Canillas July 25, 2022, 2:48 p.m. UTC | #1
On 7/20/22 16:27, Thomas Zimmermann wrote:
> Remove the unused mem field from struct simpledrm_device.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Acked-by: Javier Martinez Canillas <javierm@redhat.com>
Javier Martinez Canillas July 25, 2022, 3:01 p.m. UTC | #2
Hello Thomas,

On 7/20/22 16:27, Thomas Zimmermann wrote:
> Inline the helpers for initializing the hardware FB, the memory
> management and the modesetting into the device-creation function.
> No functional changes.
>

Could you please elaborate in the commit message why this change is
desirable?  Without this additional context, this feels like going
backwards, since you are dropping few helpers that have quite self
contained code and making simpledrm_device_create() much larger.
Javier Martinez Canillas July 25, 2022, 3:02 p.m. UTC | #3
On 7/20/22 16:27, Thomas Zimmermann wrote:
> Replace the remaining uses of the field pdev by upcasts from the Linux
> device and remove the field.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Much better indeed.

Acked-by: Javier Martinez Canillas <javierm@redhat.com>
Javier Martinez Canillas July 25, 2022, 3:46 p.m. UTC | #4
On 7/20/22 16:27, Thomas Zimmermann wrote:
> Replace the simple-KMS helpers with the regular atomic helpers. The
> regular helpers are better architectured and therefore allow for easier
> code sharing among drivers. No functional changes.
>

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

But I've a question below...
 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/tiny/simpledrm.c | 283 ++++++++++++++++++++-----------
>  1 file changed, 180 insertions(+), 103 deletions(-)

[...]

> +static void simpledrm_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> +						struct drm_atomic_state *old_state)
> +{
> +	/*
> +	 * Always enabled; screen updates are performed by
> +	 * the primary plane's atomic_update function.
> +	 */
> +}
> +
> +static void simpledrm_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.
> +	 */
> +}

...do we really need to have these ? Can't we just not set them ?

> +
> +static const struct drm_crtc_helper_funcs simpledrm_crtc_helper_funcs = {
> +	.mode_valid = simpledrm_crtc_helper_mode_valid,
> +	.atomic_check = simpledrm_crtc_helper_atomic_check,
> +	.atomic_enable = simpledrm_crtc_helper_atomic_enable,
> +	.atomic_disable = simpledrm_crtc_helper_atomic_disable,
> +};
> +
looking at https://elixir.bootlin.com/linux/latest/source/include/drm/drm_modeset_helper_vtables.h#L703
that says the .atomic_{en,dis}able handlers are optional.
Javier Martinez Canillas July 26, 2022, 1:17 p.m. UTC | #5
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 ?

[...]

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

[...]

> +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 ?
Thomas Zimmermann July 27, 2022, 7:50 a.m. UTC | #6
Hi

Am 25.07.22 um 17:01 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 7/20/22 16:27, Thomas Zimmermann wrote:
>> Inline the helpers for initializing the hardware FB, the memory
>> management and the modesetting into the device-creation function.
>> No functional changes.
>>
> 
> Could you please elaborate in the commit message why this change is
> desirable?  Without this additional context, this feels like going
> backwards, since you are dropping few helpers that have quite self
> contained code and making simpledrm_device_create() much larger.

To clarify: I want to make the init code more easy to follow. These old 
init functions still had to be called in the right order as each 
possibly depends on settings from the others. It also feels like it's 
easier to extract common code for ofdrm. And the pipeline is static, so 
it doesn't require complex chains of helper calls. Having everything in 
one helper seems beneficial. (It's a trade-off, I know.)

Best regards
Thomas

>
Thomas Zimmermann July 27, 2022, 7:58 a.m. UTC | #7
Hi

Am 25.07.22 um 17:46 schrieb Javier Martinez Canillas:
> On 7/20/22 16:27, Thomas Zimmermann wrote:
>> Replace the simple-KMS helpers with the regular atomic helpers. The
>> regular helpers are better architectured and therefore allow for easier
>> code sharing among drivers. No functional changes.
>>
> 
> Acked-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> But I've a question below...
>   
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/tiny/simpledrm.c | 283 ++++++++++++++++++++-----------
>>   1 file changed, 180 insertions(+), 103 deletions(-)
> 
> [...]
> 
>> +static void simpledrm_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>> +						struct drm_atomic_state *old_state)
>> +{
>> +	/*
>> +	 * Always enabled; screen updates are performed by
>> +	 * the primary plane's atomic_update function.
>> +	 */
>> +}
>> +
>> +static void simpledrm_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.
>> +	 */
>> +}
> 
> ...do we really need to have these ? Can't we just not set them ?
> 
>> +
>> +static const struct drm_crtc_helper_funcs simpledrm_crtc_helper_funcs = {
>> +	.mode_valid = simpledrm_crtc_helper_mode_valid,
>> +	.atomic_check = simpledrm_crtc_helper_atomic_check,
>> +	.atomic_enable = simpledrm_crtc_helper_atomic_enable,
>> +	.atomic_disable = simpledrm_crtc_helper_atomic_disable,
>> +};
>> +
> looking at https://elixir.bootlin.com/linux/latest/source/include/drm/drm_modeset_helper_vtables.h#L703
> that says the .atomic_{en,dis}able handlers are optional.
> 

The code also looks like we don't need the helpers. I mostly added them 
for the comments they contain, but I can also add those next to 
simpledrm_crtc_helper_funcs.

Best regards
Thomas
Javier Martinez Canillas July 27, 2022, 9:30 a.m. UTC | #8
Hello Thomas,

On 7/27/22 09:50, Thomas Zimmermann wrote:
> Hi
> 
> Am 25.07.22 um 17:01 schrieb Javier Martinez Canillas:
>> Hello Thomas,
>>
>> On 7/20/22 16:27, Thomas Zimmermann wrote:
>>> Inline the helpers for initializing the hardware FB, the memory
>>> management and the modesetting into the device-creation function.
>>> No functional changes.
>>>
>>
>> Could you please elaborate in the commit message why this change is
>> desirable?  Without this additional context, this feels like going
>> backwards, since you are dropping few helpers that have quite self
>> contained code and making simpledrm_device_create() much larger.
> 
> To clarify: I want to make the init code more easy to follow. These old 
> init functions still had to be called in the right order as each > possibly depends on settings from the others. It also feels like it's 
> easier to extract common code for ofdrm. And the pipeline is static, so 
> it doesn't require complex chains of helper calls. Having everything in 
> one helper seems beneficial. (It's a trade-off, I know.)
>

I see. That makes sense to me. Could you please add the explanation to
the commit message ? And feel free to add my Acked-by for this one too.
Michael Ellerman July 28, 2022, 11:13 a.m. UTC | #9
Thomas Zimmermann <tzimmermann@suse.de> writes:
> (was: drm: Add driverof PowerPC OF displays)
>
> PowerPC's Open Firmware offers a simple display buffer for graphics
> output. Add ofdrm, a DRM driver for the device. As with the existing
> simpledrm driver, the graphics hardware is pre-initialized by the
> firmware. The driver only provides blitting, no actual DRM modesetting
> is possible.

Hi Thomas,

I tried to test this on a 32-bit ppc Mac Mini but didn't have much luck.
But I'm probably doing something wrong because I'm a graphics noob.

The machine normally uses CONFIG_DRM_RADEON, so I turned that off, and
turned DRM_OFDRM on.

When I boot I get boot messages but only one screen worth, the messages
don't scroll at all, which is unusual. But I'm not sure if that's
related to ofdrm or something else.

The machine does come up, I can login via SSH. Is there some way to
start X to exercise the driver from an SSH login?

cheers
Michal Suchanek July 28, 2022, 11:31 a.m. UTC | #10
Hello,

On Thu, Jul 28, 2022 at 09:13:59PM +1000, Michael Ellerman wrote:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> > (was: drm: Add driverof PowerPC OF displays)
> >
> > PowerPC's Open Firmware offers a simple display buffer for graphics
> > output. Add ofdrm, a DRM driver for the device. As with the existing
> > simpledrm driver, the graphics hardware is pre-initialized by the
> > firmware. The driver only provides blitting, no actual DRM modesetting
> > is possible.
> 
> Hi Thomas,
> 
> I tried to test this on a 32-bit ppc Mac Mini but didn't have much luck.
> But I'm probably doing something wrong because I'm a graphics noob.
> 
> The machine normally uses CONFIG_DRM_RADEON, so I turned that off, and
> turned DRM_OFDRM on.
> 
> When I boot I get boot messages but only one screen worth, the messages
> don't scroll at all, which is unusual. But I'm not sure if that's
> related to ofdrm or something else.

A somewhat interesting datapoint might be how this works with offb.

> The machine does come up, I can login via SSH. Is there some way to
> start X to exercise the driver from an SSH login?

The startx script provided by distribution usually works.

It's basically a very convoluted way to do something like

X :0&
DISPLAY=:0 xterm&

Thanks

Michal
Thomas Zimmermann Sept. 21, 2022, 11:41 a.m. UTC | #11
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