Re: [PATCH v4 3/3] drm: Add helper for simple display pipeline

["Noralf Trønnes" <noralf@tronnes.org>]

Den 17.05.2016 09:59, skrev Daniel Vetter:
> On Tue, May 17, 2016 at 10:46:51AM +0300, Ville Syrjälä wrote:
>> On Tue, May 17, 2016 at 09:05:01AM +0200, Daniel Vetter wrote:
>>> On Thu, May 12, 2016 at 09:36:14PM +0300, Ville Syrjälä wrote:
>>>> On Thu, May 12, 2016 at 08:25:23PM +0200, Noralf Trønnes wrote:
>>>>> Provides helper functions for drivers that have a simple display
>>>>> pipeline. Plane, crtc and encoder are collapsed into one entity.
>>>>>
>>>>> Cc: jsarha@ti.com
>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>> ---
>>>>>
>>>>> Changes since v3:
>>>>> - (struct drm_simple_display_pipe *)->funcs should be const
>>>>>
>>>>> Changes since v2:
>>>>> - Drop Kconfig knob DRM_KMS_HELPER
>>>>> - Expand documentation
>>>>>
>>>>> Changes since v1:
>>>>> - Add DOC header and add to gpu.tmpl
>>>>> - Fix docs: @funcs is optional, "negative error code",
>>>>>    "This hook is optional."
>>>>> - Add checks to drm_simple_kms_plane_atomic_check()
>>>>>
>>>>>   Documentation/DocBook/gpu.tmpl          |   6 +
>>>>>   drivers/gpu/drm/Makefile                |   2 +-
>>>>>   drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
>>>>>   include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
>>>>>   4 files changed, 309 insertions(+), 1 deletion(-)
>>>>>   create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
>>>>>   create mode 100644 include/drm/drm_simple_kms_helper.h
>>>>>
>>>>> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
>>>>> index 4a0c599..cf3f5a8 100644
>>>>> --- a/Documentation/DocBook/gpu.tmpl
>>>>> +++ b/Documentation/DocBook/gpu.tmpl
>>>>> @@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev)
>>>>>   !Edrivers/gpu/drm/drm_panel.c
>>>>>   !Pdrivers/gpu/drm/drm_panel.c drm panel
>>>>>       </sect2>
>>>>> +    <sect2>
>>>>> +      <title>Simple KMS Helper Reference</title>
>>>>> +!Iinclude/drm/drm_simple_kms_helper.h
>>>>> +!Edrivers/gpu/drm/drm_simple_kms_helper.c
>>>>> +!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview
>>>>> +    </sect2>
>>>>>     </sect1>
>>>>>
>>>>>     <!-- Internals: kms properties -->
>>>>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>>>>> index 2bd3e5a..31b85df5 100644
>>>>> --- a/drivers/gpu/drm/Makefile
>>>>> +++ b/drivers/gpu/drm/Makefile
>>>>> @@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
>>>>>
>>>>>   drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>>>>>   		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
>>>>> -		drm_kms_helper_common.o
>>>>> +		drm_kms_helper_common.o drm_simple_kms_helper.o
>>>>>
>>>>>   drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>>>>>   drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>>>>> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
>>>>> new file mode 100644
>>>>> index 0000000..d45417a
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
>>>>> @@ -0,0 +1,208 @@
>>>>> +/*
>>>>> + * Copyright (C) 2016 Noralf Trønnes
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>> + * it under the terms of the GNU General Public License as published by
>>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>>> + * (at your option) any later version.
>>>>> + */
>>>>> +
>>>>> +#include <drm/drmP.h>
>>>>> +#include <drm/drm_atomic.h>
>>>>> +#include <drm/drm_atomic_helper.h>
>>>>> +#include <drm/drm_crtc_helper.h>
>>>>> +#include <drm/drm_plane_helper.h>
>>>>> +#include <drm/drm_simple_kms_helper.h>
>>>>> +#include <linux/slab.h>
>>>>> +
>>>>> +/**
>>>>> + * DOC: overview
>>>>> + *
>>>>> + * This helper library provides helpers for drivers for simple display
>>>>> + * hardware.
>>>>> + *
>>>>> + * drm_simple_display_pipe_init() initializes a simple display pipeline
>>>>> + * which has only one full-screen scanout buffer feeding one output. The
>>>>> + * pipeline is represented by struct &drm_simple_display_pipe and binds
>>>>> + * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
>>>>> + * entity. Some flexibility for code reuse is provided through a separately
>>>>> + * allocated &drm_connector object and supporting optional &drm_bridge
>>>>> + * encoder drivers.
>>>>> + */
>>>>> +
>>>>> +static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
>>>>> +	.destroy = drm_encoder_cleanup,
>>>>> +};
>>>>> +
>>>>> +static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc)
>>>>> +{
>>>>> +	struct drm_simple_display_pipe *pipe;
>>>>> +
>>>>> +	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
>>>>> +	if (!pipe->funcs || !pipe->funcs->enable)
>>>>> +		return;
>>>>> +
>>>>> +	pipe->funcs->enable(pipe, crtc->state);
>>>>> +}
>>>>> +
>>>>> +static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc)
>>>>> +{
>>>>> +	struct drm_simple_display_pipe *pipe;
>>>>> +
>>>>> +	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
>>>>> +	if (!pipe->funcs || !pipe->funcs->disable)
>>>>> +		return;
>>>>> +
>>>>> +	pipe->funcs->disable(pipe);
>>>>> +}
>>>>> +
>>>>> +static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
>>>>> +	.disable = drm_simple_kms_crtc_disable,
>>>>> +	.enable = drm_simple_kms_crtc_enable,
>>>>> +};
>>>>> +
>>>>> +static const struct drm_crtc_funcs drm_simple_kms_crtc_funcs = {
>>>>> +	.reset = drm_atomic_helper_crtc_reset,
>>>>> +	.destroy = drm_crtc_cleanup,
>>>>> +	.set_config = drm_atomic_helper_set_config,
>>>>> +	.page_flip = drm_atomic_helper_page_flip,
>>>>> +	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>>>>> +	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>>>>> +};
>>>>> +
>>>>> +static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
>>>>> +					struct drm_plane_state *plane_state)
>>>>> +{
>>>>> +	struct drm_rect src = {
>>>>> +		.x1 = plane_state->src_x,
>>>>> +		.y1 = plane_state->src_y,
>>>>> +		.x2 = plane_state->src_x + plane_state->src_w,
>>>>> +		.y2 = plane_state->src_y + plane_state->src_h,
>>>>> +	};
>>>>> +	struct drm_rect dest = {
>>>>> +		.x1 = plane_state->crtc_x,
>>>>> +		.y1 = plane_state->crtc_y,
>>>>> +		.x2 = plane_state->crtc_x + plane_state->crtc_w,
>>>>> +		.y2 = plane_state->crtc_y + plane_state->crtc_h,
>>>>> +	};
>>>>> +	struct drm_rect clip = { 0 };
>>>>> +	struct drm_simple_display_pipe *pipe;
>>>>> +	struct drm_crtc_state *crtc_state;
>>>>> +	bool visible;
>>>>> +	int ret;
>>>>> +
>>>>> +	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
>>>>> +	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
>>>>> +							&pipe->crtc);
>>>>> +	if (crtc_state->enable != !!plane_state->crtc)
>>>>> +		return -EINVAL; /* plane must match crtc enable state */
>>>>> +
>>>>> +	if (!crtc_state->enable)
>>>>> +		return 0; /* nothing to check when disabling or disabled */
>>>>> +
>>>>> +	clip.x2 = crtc_state->adjusted_mode.hdisplay;
>>>>> +	clip.y2 = crtc_state->adjusted_mode.vdisplay;
>>>>> +	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
>>>>> +					    plane_state->fb,
>>>>> +					    &src, &dest, &clip,
>>>>> +					    DRM_PLANE_HELPER_NO_SCALING,
>>>>> +					    DRM_PLANE_HELPER_NO_SCALING,
>>>>> +					    false, true, &visible);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	if (!visible)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	if (!pipe->funcs || !pipe->funcs->check)
>>>>> +		return 0;
>>>>> +
>>>>> +	return pipe->funcs->check(pipe, plane_state, crtc_state);
>>>>> +}
>>>> What's anyone supposed to do with this when the clipped coordinates
>>>> aren't even passed/stored anywhere?
>>> It disallows positioning and scaling, so shouldn't ever need to have the
>>> clipped area?
>> You can still configure a larger area that gets clipped to the
>> fullscreen dimensions.
> Oh right. Noralf, sounds like we need to feed back the clipped rectangle.
> Probably best if we add clipped plane coordinates to drm_plane_state.

Both src and crtc or only src?

      if (!visible)
          return -EINVAL;
+
+    plane_state->src_x = src.x1;
+    plane_state->src_y = src.y1;
+    plane_state->src_w = drm_rect_width(&src);
+    plane_state->src_h = drm_rect_height(&src);
+
+    plane_state->crtc_x = dest.x1;
+    plane_state->crtc_y = dest.y1;
+    plane_state->crtc_w = drm_rect_width(&dest);
+    plane_state->crtc_h = drm_rect_height(&dest);

     if (!pipe->funcs || !pipe->funcs->check)
         return 0;

Noralf.