Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1395111 > unrolled thread
| Started by | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| First post | 2016-05-05 15:30 +0200 |
| Last post | 2016-05-06 15:20 +0200 |
| Articles | 15 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH 0/4] drm: Add various helpers for simple drivers Noralf Trønnes <noralf@tronnes.org> - 2016-05-05 15:30 +0200
[PATCH 2/4] drm: Make drm_encoder_helper_funcs optional Noralf Trønnes <noralf@tronnes.org> - 2016-05-05 15:30 +0200
Re: [PATCH 2/4] drm: Make drm_encoder_helper_funcs optional Daniel Vetter <daniel@ffwll.ch> - 2016-05-05 18:30 +0200
Re: [PATCH 2/4] drm: Make drm_encoder_helper_funcs optional Noralf Trønnes <noralf@tronnes.org> - 2016-05-09 21:20 +0200
Re: [PATCH 2/4] drm: Make drm_encoder_helper_funcs optional Daniel Vetter <daniel@ffwll.ch> - 2016-05-10 09:00 +0200
[PATCH 3/4] drm: Add helper for simple display pipeline Noralf Trønnes <noralf@tronnes.org> - 2016-05-05 15:30 +0200
Re: [PATCH 3/4] drm: Add helper for simple display pipeline Daniel Vetter <daniel@ffwll.ch> - 2016-05-05 18:50 +0200
Re: [PATCH 3/4] drm: Add helper for simple display pipeline Daniel Vetter <daniel@ffwll.ch> - 2016-05-09 16:50 +0200
Re: [PATCH 3/4] drm: Add helper for simple display pipeline Noralf Trønnes <noralf@tronnes.org> - 2016-05-09 20:40 +0200
Re: [PATCH 3/4] drm: Add helper for simple display pipeline Daniel Vetter <daniel@ffwll.ch> - 2016-05-10 09:00 +0200
Re: [PATCH 3/4] drm: Add helper for simple display pipeline Daniel Vetter <daniel@ffwll.ch> - 2016-05-11 00:40 +0200
[PATCH 1/4] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs() Noralf Trønnes <noralf@tronnes.org> - 2016-05-05 15:30 +0200
Re: [PATCH 1/4] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs() Daniel Vetter <daniel@ffwll.ch> - 2016-05-05 18:30 +0200
Re: [PATCH 1/4] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs() Noralf Trønnes <noralf@tronnes.org> - 2016-05-06 15:10 +0200
Re: [PATCH 1/4] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs() Daniel Vetter <daniel@ffwll.ch> - 2016-05-06 15:20 +0200
| From | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| Date | 2016-05-05 15:30 +0200 |
| Subject | [PATCH 0/4] drm: Add various helpers for simple drivers |
| Message-ID | <rvrvQ-MH-21@gated-at.bofh.it> |
This patchset adds various helpers that was originally part of the tinydrm patchset. Essentially it adds 3 functions: - drm_fb_cma_create_with_funcs() CMA backed framebuffer supporting a dirty() callback. - drm_simple_display_pipe_init() Plane, crtc and encoder are collapsed into one entity. - drm_panel_connector_create() Create a simple connector for a panel. Noralf Trønnes (4): drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs() drm: Make drm_encoder_helper_funcs optional drm: Add helper for simple display pipeline drm/panel: Add helper for simple panel connector drivers/gpu/drm/Kconfig | 7 ++ drivers/gpu/drm/Makefile | 2 + drivers/gpu/drm/drm_atomic_helper.c | 11 ++- drivers/gpu/drm/drm_crtc_helper.c | 41 +++++++-- drivers/gpu/drm/drm_fb_cma_helper.c | 29 ++++-- drivers/gpu/drm/drm_panel_helper.c | 117 ++++++++++++++++++++++++ drivers/gpu/drm/drm_simple_kms_helper.c | 157 ++++++++++++++++++++++++++++++++ drivers/gpu/drm/panel/Kconfig | 7 ++ include/drm/drm_fb_cma_helper.h | 3 + include/drm/drm_panel_helper.h | 20 ++++ include/drm/drm_simple_kms_helper.h | 88 ++++++++++++++++++ 11 files changed, 466 insertions(+), 16 deletions(-) create mode 100644 drivers/gpu/drm/drm_panel_helper.c create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c create mode 100644 include/drm/drm_panel_helper.h create mode 100644 include/drm/drm_simple_kms_helper.h -- 2.2.2
[toc] | [next] | [standalone]
| From | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| Date | 2016-05-05 15:30 +0200 |
| Subject | [PATCH 2/4] drm: Make drm_encoder_helper_funcs optional |
| Message-ID | <rvrvR-MH-33@gated-at.bofh.it> |
| In reply to | #1395111 |
Make drm_encoder_helper_funcs and it's functions optional to avoid
having dummy functions.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++++--
drivers/gpu/drm/drm_crtc_helper.c | 41 +++++++++++++++++++++++++++++--------
2 files changed, 42 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 92e11a2..03ea049 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -696,6 +696,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
continue;
funcs = encoder->helper_private;
+ if (!funcs)
+ continue;
DRM_DEBUG_ATOMIC("disabling [ENCODER:%d:%s]\n",
encoder->base.id, encoder->name);
@@ -711,7 +713,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
funcs->prepare(encoder);
else if (funcs->disable)
funcs->disable(encoder);
- else
+ else if (funcs->dpms)
funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
drm_bridge_post_disable(encoder->bridge);
@@ -859,6 +861,9 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
encoder = connector->state->best_encoder;
funcs = encoder->helper_private;
+ if (!funcs)
+ continue;
+
new_crtc_state = connector->state->crtc->state;
mode = &new_crtc_state->mode;
adjusted_mode = &new_crtc_state->adjusted_mode;
@@ -964,6 +969,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
encoder = connector->state->best_encoder;
funcs = encoder->helper_private;
+ if (!funcs)
+ continue;
DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
encoder->base.id, encoder->name);
@@ -976,7 +983,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
if (funcs->enable)
funcs->enable(encoder);
- else
+ else if (funcs->commit)
funcs->commit(encoder);
drm_bridge_enable(encoder->bridge);
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 66ca313..6e6ab38 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -170,11 +170,14 @@ drm_encoder_disable(struct drm_encoder *encoder)
{
const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
+ if (!encoder_funcs)
+ return;
+
drm_bridge_disable(encoder->bridge);
if (encoder_funcs->disable)
(*encoder_funcs->disable)(encoder);
- else
+ else if (encoder_funcs->dpms)
(*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF);
drm_bridge_post_disable(encoder->bridge);
@@ -248,6 +251,9 @@ drm_crtc_prepare_encoders(struct drm_device *dev)
drm_for_each_encoder(encoder, dev) {
encoder_funcs = encoder->helper_private;
+ if (!encoder_funcs)
+ continue;
+
/* Disable unused encoders */
if (encoder->crtc == NULL)
drm_encoder_disable(encoder);
@@ -326,6 +332,10 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
if (encoder->crtc != crtc)
continue;
+ encoder_funcs = encoder->helper_private;
+ if (!encoder_funcs)
+ continue;
+
ret = drm_bridge_mode_fixup(encoder->bridge,
mode, adjusted_mode);
if (!ret) {
@@ -360,11 +370,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
if (encoder->crtc != crtc)
continue;
+ encoder_funcs = encoder->helper_private;
+ if (!encoder_funcs)
+ continue;
+
drm_bridge_disable(encoder->bridge);
- encoder_funcs = encoder->helper_private;
/* Disable the encoders as the first thing we do. */
- encoder_funcs->prepare(encoder);
+ if (encoder_funcs->prepare)
+ encoder_funcs->prepare(encoder);
drm_bridge_post_disable(encoder->bridge);
}
@@ -385,11 +399,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
if (encoder->crtc != crtc)
continue;
+ encoder_funcs = encoder->helper_private;
+ if (!encoder_funcs)
+ continue;
+
DRM_DEBUG_KMS("[ENCODER:%d:%s] set [MODE:%d:%s]\n",
encoder->base.id, encoder->name,
mode->base.id, mode->name);
- encoder_funcs = encoder->helper_private;
- encoder_funcs->mode_set(encoder, mode, adjusted_mode);
+ if (encoder_funcs->mode_set)
+ encoder_funcs->mode_set(encoder, mode, adjusted_mode);
drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
}
@@ -402,10 +420,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
if (encoder->crtc != crtc)
continue;
+ encoder_funcs = encoder->helper_private;
+ if (!encoder_funcs)
+ continue;
+
drm_bridge_pre_enable(encoder->bridge);
- encoder_funcs = encoder->helper_private;
- encoder_funcs->commit(encoder);
+ if (encoder_funcs->commit)
+ encoder_funcs->commit(encoder);
drm_bridge_enable(encoder->bridge);
}
@@ -771,12 +793,15 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode)
struct drm_bridge *bridge = encoder->bridge;
const struct drm_encoder_helper_funcs *encoder_funcs;
+ encoder_funcs = encoder->helper_private;
+ if (!encoder_funcs)
+ return;
+
if (mode == DRM_MODE_DPMS_ON)
drm_bridge_pre_enable(bridge);
else
drm_bridge_disable(bridge);
- encoder_funcs = encoder->helper_private;
if (encoder_funcs->dpms)
encoder_funcs->dpms(encoder, mode);
--
2.2.2
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-05-05 18:30 +0200 |
| Subject | Re: [PATCH 2/4] drm: Make drm_encoder_helper_funcs optional |
| Message-ID | <rvuk2-3e2-15@gated-at.bofh.it> |
| In reply to | #1395113 |
On Thu, May 05, 2016 at 03:24:32PM +0200, Noralf Trønnes wrote:
> Make drm_encoder_helper_funcs and it's functions optional to avoid
> having dummy functions.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Please also update the kerneldoc and mention there that the enable/disable
hooks are optional. You can build the hmtl docs using
$ make htmldocs
to check the result. The vtables are documented in
include/drm/drm_helper_vtables.h
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++++--
> drivers/gpu/drm/drm_crtc_helper.c | 41 +++++++++++++++++++++++++++++--------
Hm, tbh I wouldn't bother at all with crtc helpers and just drop that
part. Old drivers should converted to atomic, new drivers should just use
atomic. No need to touch that code at all.
-Daniel
> 2 files changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 92e11a2..03ea049 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -696,6 +696,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> continue;
>
> funcs = encoder->helper_private;
> + if (!funcs)
> + continue;
>
> DRM_DEBUG_ATOMIC("disabling [ENCODER:%d:%s]\n",
> encoder->base.id, encoder->name);
> @@ -711,7 +713,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> funcs->prepare(encoder);
> else if (funcs->disable)
> funcs->disable(encoder);
> - else
> + else if (funcs->dpms)
> funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>
> drm_bridge_post_disable(encoder->bridge);
> @@ -859,6 +861,9 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>
> encoder = connector->state->best_encoder;
> funcs = encoder->helper_private;
> + if (!funcs)
> + continue;
> +
> new_crtc_state = connector->state->crtc->state;
> mode = &new_crtc_state->mode;
> adjusted_mode = &new_crtc_state->adjusted_mode;
> @@ -964,6 +969,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>
> encoder = connector->state->best_encoder;
> funcs = encoder->helper_private;
> + if (!funcs)
> + continue;
>
> DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
> encoder->base.id, encoder->name);
> @@ -976,7 +983,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>
> if (funcs->enable)
> funcs->enable(encoder);
> - else
> + else if (funcs->commit)
> funcs->commit(encoder);
>
> drm_bridge_enable(encoder->bridge);
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 66ca313..6e6ab38 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -170,11 +170,14 @@ drm_encoder_disable(struct drm_encoder *encoder)
> {
> const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
>
> + if (!encoder_funcs)
> + return;
> +
> drm_bridge_disable(encoder->bridge);
>
> if (encoder_funcs->disable)
> (*encoder_funcs->disable)(encoder);
> - else
> + else if (encoder_funcs->dpms)
> (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF);
>
> drm_bridge_post_disable(encoder->bridge);
> @@ -248,6 +251,9 @@ drm_crtc_prepare_encoders(struct drm_device *dev)
>
> drm_for_each_encoder(encoder, dev) {
> encoder_funcs = encoder->helper_private;
> + if (!encoder_funcs)
> + continue;
> +
> /* Disable unused encoders */
> if (encoder->crtc == NULL)
> drm_encoder_disable(encoder);
> @@ -326,6 +332,10 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> if (encoder->crtc != crtc)
> continue;
>
> + encoder_funcs = encoder->helper_private;
> + if (!encoder_funcs)
> + continue;
> +
> ret = drm_bridge_mode_fixup(encoder->bridge,
> mode, adjusted_mode);
> if (!ret) {
> @@ -360,11 +370,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> if (encoder->crtc != crtc)
> continue;
>
> + encoder_funcs = encoder->helper_private;
> + if (!encoder_funcs)
> + continue;
> +
> drm_bridge_disable(encoder->bridge);
>
> - encoder_funcs = encoder->helper_private;
> /* Disable the encoders as the first thing we do. */
> - encoder_funcs->prepare(encoder);
> + if (encoder_funcs->prepare)
> + encoder_funcs->prepare(encoder);
>
> drm_bridge_post_disable(encoder->bridge);
> }
> @@ -385,11 +399,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> if (encoder->crtc != crtc)
> continue;
>
> + encoder_funcs = encoder->helper_private;
> + if (!encoder_funcs)
> + continue;
> +
> DRM_DEBUG_KMS("[ENCODER:%d:%s] set [MODE:%d:%s]\n",
> encoder->base.id, encoder->name,
> mode->base.id, mode->name);
> - encoder_funcs = encoder->helper_private;
> - encoder_funcs->mode_set(encoder, mode, adjusted_mode);
> + if (encoder_funcs->mode_set)
> + encoder_funcs->mode_set(encoder, mode, adjusted_mode);
>
> drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
> }
> @@ -402,10 +420,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> if (encoder->crtc != crtc)
> continue;
>
> + encoder_funcs = encoder->helper_private;
> + if (!encoder_funcs)
> + continue;
> +
> drm_bridge_pre_enable(encoder->bridge);
>
> - encoder_funcs = encoder->helper_private;
> - encoder_funcs->commit(encoder);
> + if (encoder_funcs->commit)
> + encoder_funcs->commit(encoder);
>
> drm_bridge_enable(encoder->bridge);
> }
> @@ -771,12 +793,15 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode)
> struct drm_bridge *bridge = encoder->bridge;
> const struct drm_encoder_helper_funcs *encoder_funcs;
>
> + encoder_funcs = encoder->helper_private;
> + if (!encoder_funcs)
> + return;
> +
> if (mode == DRM_MODE_DPMS_ON)
> drm_bridge_pre_enable(bridge);
> else
> drm_bridge_disable(bridge);
>
> - encoder_funcs = encoder->helper_private;
> if (encoder_funcs->dpms)
> encoder_funcs->dpms(encoder, mode);
>
> --
> 2.2.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| Date | 2016-05-09 21:20 +0200 |
| Subject | Re: [PATCH 2/4] drm: Make drm_encoder_helper_funcs optional |
| Message-ID | <rwYSK-3gz-3@gated-at.bofh.it> |
| In reply to | #1395215 |
Den 05.05.2016 18:23, skrev Daniel Vetter:
> On Thu, May 05, 2016 at 03:24:32PM +0200, Noralf Trønnes wrote:
>> Make drm_encoder_helper_funcs and it's functions optional to avoid
>> having dummy functions.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> Please also update the kerneldoc and mention there that the enable/disable
> hooks are optional. You can build the hmtl docs using
AFAICT the kerneldoc already says that they are optional for atomic helpers:
struct drm_encoder_helper_funcs {
[...]
/**
* @disable:
[...]
* This hook is used both by legacy CRTC helpers and atomic
helpers.
* Atomic drivers don't need to implement it if there's no need to
* disable anything at the encoder level. To ensure that runtime PM
[...]
/**
* @enable:
[...]
* This hook is used only by atomic helpers, for symmetry with
@disable.
* Atomic drivers don't need to implement it if there's no need to
* enable anything at the encoder level. To ensure that runtime
PM handling
Noralf.
> $ make htmldocs
>
> to check the result. The vtables are documented in
> include/drm/drm_helper_vtables.h
>
>> ---
>> drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++++--
>> drivers/gpu/drm/drm_crtc_helper.c | 41 +++++++++++++++++++++++++++++--------
> Hm, tbh I wouldn't bother at all with crtc helpers and just drop that
> part. Old drivers should converted to atomic, new drivers should just use
> atomic. No need to touch that code at all.
> -Daniel
>
>> 2 files changed, 42 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 92e11a2..03ea049 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -696,6 +696,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>> continue;
>>
>> funcs = encoder->helper_private;
>> + if (!funcs)
>> + continue;
>>
>> DRM_DEBUG_ATOMIC("disabling [ENCODER:%d:%s]\n",
>> encoder->base.id, encoder->name);
>> @@ -711,7 +713,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>> funcs->prepare(encoder);
>> else if (funcs->disable)
>> funcs->disable(encoder);
>> - else
>> + else if (funcs->dpms)
>> funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>>
>> drm_bridge_post_disable(encoder->bridge);
>> @@ -859,6 +861,9 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>>
>> encoder = connector->state->best_encoder;
>> funcs = encoder->helper_private;
>> + if (!funcs)
>> + continue;
>> +
>> new_crtc_state = connector->state->crtc->state;
>> mode = &new_crtc_state->mode;
>> adjusted_mode = &new_crtc_state->adjusted_mode;
>> @@ -964,6 +969,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>>
>> encoder = connector->state->best_encoder;
>> funcs = encoder->helper_private;
>> + if (!funcs)
>> + continue;
>>
>> DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
>> encoder->base.id, encoder->name);
>> @@ -976,7 +983,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>>
>> if (funcs->enable)
>> funcs->enable(encoder);
>> - else
>> + else if (funcs->commit)
>> funcs->commit(encoder);
>>
>> drm_bridge_enable(encoder->bridge);
>> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
>> index 66ca313..6e6ab38 100644
>> --- a/drivers/gpu/drm/drm_crtc_helper.c
>> +++ b/drivers/gpu/drm/drm_crtc_helper.c
>> @@ -170,11 +170,14 @@ drm_encoder_disable(struct drm_encoder *encoder)
>> {
>> const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
>>
>> + if (!encoder_funcs)
>> + return;
>> +
>> drm_bridge_disable(encoder->bridge);
>>
>> if (encoder_funcs->disable)
>> (*encoder_funcs->disable)(encoder);
>> - else
>> + else if (encoder_funcs->dpms)
>> (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF);
>>
>> drm_bridge_post_disable(encoder->bridge);
>> @@ -248,6 +251,9 @@ drm_crtc_prepare_encoders(struct drm_device *dev)
>>
>> drm_for_each_encoder(encoder, dev) {
>> encoder_funcs = encoder->helper_private;
>> + if (!encoder_funcs)
>> + continue;
>> +
>> /* Disable unused encoders */
>> if (encoder->crtc == NULL)
>> drm_encoder_disable(encoder);
>> @@ -326,6 +332,10 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>> if (encoder->crtc != crtc)
>> continue;
>>
>> + encoder_funcs = encoder->helper_private;
>> + if (!encoder_funcs)
>> + continue;
>> +
>> ret = drm_bridge_mode_fixup(encoder->bridge,
>> mode, adjusted_mode);
>> if (!ret) {
>> @@ -360,11 +370,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>> if (encoder->crtc != crtc)
>> continue;
>>
>> + encoder_funcs = encoder->helper_private;
>> + if (!encoder_funcs)
>> + continue;
>> +
>> drm_bridge_disable(encoder->bridge);
>>
>> - encoder_funcs = encoder->helper_private;
>> /* Disable the encoders as the first thing we do. */
>> - encoder_funcs->prepare(encoder);
>> + if (encoder_funcs->prepare)
>> + encoder_funcs->prepare(encoder);
>>
>> drm_bridge_post_disable(encoder->bridge);
>> }
>> @@ -385,11 +399,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>> if (encoder->crtc != crtc)
>> continue;
>>
>> + encoder_funcs = encoder->helper_private;
>> + if (!encoder_funcs)
>> + continue;
>> +
>> DRM_DEBUG_KMS("[ENCODER:%d:%s] set [MODE:%d:%s]\n",
>> encoder->base.id, encoder->name,
>> mode->base.id, mode->name);
>> - encoder_funcs = encoder->helper_private;
>> - encoder_funcs->mode_set(encoder, mode, adjusted_mode);
>> + if (encoder_funcs->mode_set)
>> + encoder_funcs->mode_set(encoder, mode, adjusted_mode);
>>
>> drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
>> }
>> @@ -402,10 +420,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>> if (encoder->crtc != crtc)
>> continue;
>>
>> + encoder_funcs = encoder->helper_private;
>> + if (!encoder_funcs)
>> + continue;
>> +
>> drm_bridge_pre_enable(encoder->bridge);
>>
>> - encoder_funcs = encoder->helper_private;
>> - encoder_funcs->commit(encoder);
>> + if (encoder_funcs->commit)
>> + encoder_funcs->commit(encoder);
>>
>> drm_bridge_enable(encoder->bridge);
>> }
>> @@ -771,12 +793,15 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode)
>> struct drm_bridge *bridge = encoder->bridge;
>> const struct drm_encoder_helper_funcs *encoder_funcs;
>>
>> + encoder_funcs = encoder->helper_private;
>> + if (!encoder_funcs)
>> + return;
>> +
>> if (mode == DRM_MODE_DPMS_ON)
>> drm_bridge_pre_enable(bridge);
>> else
>> drm_bridge_disable(bridge);
>>
>> - encoder_funcs = encoder->helper_private;
>> if (encoder_funcs->dpms)
>> encoder_funcs->dpms(encoder, mode);
>>
>> --
>> 2.2.2
>>
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-05-10 09:00 +0200 |
| Subject | Re: [PATCH 2/4] drm: Make drm_encoder_helper_funcs optional |
| Message-ID | <rx9Oa-5u8-21@gated-at.bofh.it> |
| In reply to | #1397264 |
On Mon, May 09, 2016 at 09:19:22PM +0200, Noralf Trønnes wrote:
>
> Den 05.05.2016 18:23, skrev Daniel Vetter:
> >On Thu, May 05, 2016 at 03:24:32PM +0200, Noralf Trønnes wrote:
> >>Make drm_encoder_helper_funcs and it's functions optional to avoid
> >>having dummy functions.
> >>
> >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >Please also update the kerneldoc and mention there that the enable/disable
> >hooks are optional. You can build the hmtl docs using
>
> AFAICT the kerneldoc already says that they are optional for atomic helpers:
>
> struct drm_encoder_helper_funcs {
> [...]
> /**
> * @disable:
> [...]
> * This hook is used both by legacy CRTC helpers and atomic helpers.
> * Atomic drivers don't need to implement it if there's no need to
> * disable anything at the encoder level. To ensure that runtime PM
> [...]
> /**
> * @enable:
> [...]
> * This hook is used only by atomic helpers, for symmetry with
> @disable.
> * Atomic drivers don't need to implement it if there's no need to
> * enable anything at the encoder level. To ensure that runtime PM
> handling
Indeed, docs not matching the code. But with your patch here will soon ;-)
I'll apply this one to drm-misc.
-Daniel
>
> Noralf.
>
> >$ make htmldocs
> >
> >to check the result. The vtables are documented in
> >include/drm/drm_helper_vtables.h
> >
> >>---
> >> drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++++--
> >> drivers/gpu/drm/drm_crtc_helper.c | 41 +++++++++++++++++++++++++++++--------
> >Hm, tbh I wouldn't bother at all with crtc helpers and just drop that
> >part. Old drivers should converted to atomic, new drivers should just use
> >atomic. No need to touch that code at all.
> >-Daniel
> >
> >> 2 files changed, 42 insertions(+), 10 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>index 92e11a2..03ea049 100644
> >>--- a/drivers/gpu/drm/drm_atomic_helper.c
> >>+++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>@@ -696,6 +696,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> >> continue;
> >> funcs = encoder->helper_private;
> >>+ if (!funcs)
> >>+ continue;
> >> DRM_DEBUG_ATOMIC("disabling [ENCODER:%d:%s]\n",
> >> encoder->base.id, encoder->name);
> >>@@ -711,7 +713,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> >> funcs->prepare(encoder);
> >> else if (funcs->disable)
> >> funcs->disable(encoder);
> >>- else
> >>+ else if (funcs->dpms)
> >> funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> >> drm_bridge_post_disable(encoder->bridge);
> >>@@ -859,6 +861,9 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
> >> encoder = connector->state->best_encoder;
> >> funcs = encoder->helper_private;
> >>+ if (!funcs)
> >>+ continue;
> >>+
> >> new_crtc_state = connector->state->crtc->state;
> >> mode = &new_crtc_state->mode;
> >> adjusted_mode = &new_crtc_state->adjusted_mode;
> >>@@ -964,6 +969,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> >> encoder = connector->state->best_encoder;
> >> funcs = encoder->helper_private;
> >>+ if (!funcs)
> >>+ continue;
> >> DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
> >> encoder->base.id, encoder->name);
> >>@@ -976,7 +983,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> >> if (funcs->enable)
> >> funcs->enable(encoder);
> >>- else
> >>+ else if (funcs->commit)
> >> funcs->commit(encoder);
> >> drm_bridge_enable(encoder->bridge);
> >>diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> >>index 66ca313..6e6ab38 100644
> >>--- a/drivers/gpu/drm/drm_crtc_helper.c
> >>+++ b/drivers/gpu/drm/drm_crtc_helper.c
> >>@@ -170,11 +170,14 @@ drm_encoder_disable(struct drm_encoder *encoder)
> >> {
> >> const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
> >>+ if (!encoder_funcs)
> >>+ return;
> >>+
> >> drm_bridge_disable(encoder->bridge);
> >> if (encoder_funcs->disable)
> >> (*encoder_funcs->disable)(encoder);
> >>- else
> >>+ else if (encoder_funcs->dpms)
> >> (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF);
> >> drm_bridge_post_disable(encoder->bridge);
> >>@@ -248,6 +251,9 @@ drm_crtc_prepare_encoders(struct drm_device *dev)
> >> drm_for_each_encoder(encoder, dev) {
> >> encoder_funcs = encoder->helper_private;
> >>+ if (!encoder_funcs)
> >>+ continue;
> >>+
> >> /* Disable unused encoders */
> >> if (encoder->crtc == NULL)
> >> drm_encoder_disable(encoder);
> >>@@ -326,6 +332,10 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> >> if (encoder->crtc != crtc)
> >> continue;
> >>+ encoder_funcs = encoder->helper_private;
> >>+ if (!encoder_funcs)
> >>+ continue;
> >>+
> >> ret = drm_bridge_mode_fixup(encoder->bridge,
> >> mode, adjusted_mode);
> >> if (!ret) {
> >>@@ -360,11 +370,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> >> if (encoder->crtc != crtc)
> >> continue;
> >>+ encoder_funcs = encoder->helper_private;
> >>+ if (!encoder_funcs)
> >>+ continue;
> >>+
> >> drm_bridge_disable(encoder->bridge);
> >>- encoder_funcs = encoder->helper_private;
> >> /* Disable the encoders as the first thing we do. */
> >>- encoder_funcs->prepare(encoder);
> >>+ if (encoder_funcs->prepare)
> >>+ encoder_funcs->prepare(encoder);
> >> drm_bridge_post_disable(encoder->bridge);
> >> }
> >>@@ -385,11 +399,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> >> if (encoder->crtc != crtc)
> >> continue;
> >>+ encoder_funcs = encoder->helper_private;
> >>+ if (!encoder_funcs)
> >>+ continue;
> >>+
> >> DRM_DEBUG_KMS("[ENCODER:%d:%s] set [MODE:%d:%s]\n",
> >> encoder->base.id, encoder->name,
> >> mode->base.id, mode->name);
> >>- encoder_funcs = encoder->helper_private;
> >>- encoder_funcs->mode_set(encoder, mode, adjusted_mode);
> >>+ if (encoder_funcs->mode_set)
> >>+ encoder_funcs->mode_set(encoder, mode, adjusted_mode);
> >> drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
> >> }
> >>@@ -402,10 +420,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> >> if (encoder->crtc != crtc)
> >> continue;
> >>+ encoder_funcs = encoder->helper_private;
> >>+ if (!encoder_funcs)
> >>+ continue;
> >>+
> >> drm_bridge_pre_enable(encoder->bridge);
> >>- encoder_funcs = encoder->helper_private;
> >>- encoder_funcs->commit(encoder);
> >>+ if (encoder_funcs->commit)
> >>+ encoder_funcs->commit(encoder);
> >> drm_bridge_enable(encoder->bridge);
> >> }
> >>@@ -771,12 +793,15 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode)
> >> struct drm_bridge *bridge = encoder->bridge;
> >> const struct drm_encoder_helper_funcs *encoder_funcs;
> >>+ encoder_funcs = encoder->helper_private;
> >>+ if (!encoder_funcs)
> >>+ return;
> >>+
> >> if (mode == DRM_MODE_DPMS_ON)
> >> drm_bridge_pre_enable(bridge);
> >> else
> >> drm_bridge_disable(bridge);
> >>- encoder_funcs = encoder->helper_private;
> >> if (encoder_funcs->dpms)
> >> encoder_funcs->dpms(encoder, mode);
> >>--
> >>2.2.2
> >>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| Date | 2016-05-05 15:30 +0200 |
| Subject | [PATCH 3/4] drm: Add helper for simple display pipeline |
| Message-ID | <rvrvR-MH-39@gated-at.bofh.it> |
| In reply to | #1395111 |
Provides helper functions for drivers that have a simple display
pipeline. Plane, crtc and encoder are collapsed into one entity.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/gpu/drm/Kconfig | 7 ++
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_simple_kms_helper.c | 157 ++++++++++++++++++++++++++++++++
include/drm/drm_simple_kms_helper.h | 88 ++++++++++++++++++
4 files changed, 253 insertions(+)
create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
create mode 100644 include/drm/drm_simple_kms_helper.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 16e4c21..ff9f480 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -39,6 +39,13 @@ config DRM_KMS_HELPER
help
CRTC helpers for KMS drivers.
+config DRM_SIMPLE_KMS_HELPER
+ tristate
+ depends on DRM
+ select DRM_KMS_HELPER
+ help
+ Helpers for very simple KMS drivers.
+
config DRM_KMS_FB_HELPER
bool
depends on DRM_KMS_HELPER
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 43c2abf..7e99923 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -31,6 +31,7 @@ drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
+obj-$(CONFIG_DRM_SIMPLE_KMS_HELPER) += drm_simple_kms_helper.o
CFLAGS_drm_trace_points.o := -I$(src)
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..c8725bb
--- /dev/null
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -0,0 +1,157 @@
+/*
+ * 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>
+
+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 *pstate)
+{
+ struct drm_simple_display_pipe *pipe;
+ struct drm_crtc_state *cstate;
+
+ pipe = container_of(plane, struct drm_simple_display_pipe, plane);
+ if (!pipe->funcs || !pipe->funcs->check)
+ return 0;
+
+ cstate = drm_atomic_get_existing_crtc_state(pstate->state,
+ &pipe->crtc);
+
+ return pipe->funcs->check(pipe, pstate, cstate);
+}
+
+static void drm_simple_kms_plane_atomic_update(struct drm_plane *plane,
+ struct drm_plane_state *pstate)
+{
+ struct drm_simple_display_pipe *pipe;
+
+ pipe = container_of(plane, struct drm_simple_display_pipe, plane);
+ if (!pipe->funcs || !pipe->funcs->update)
+ return;
+
+ pipe->funcs->update(pipe, pstate);
+}
+
+static const struct drm_plane_helper_funcs drm_simple_kms_plane_helper_funcs = {
+ .atomic_check = drm_simple_kms_plane_atomic_check,
+ .atomic_update = drm_simple_kms_plane_atomic_update,
+};
+
+static const struct drm_plane_funcs drm_simple_kms_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .destroy = drm_plane_cleanup,
+ .reset = drm_atomic_helper_plane_reset,
+ .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+};
+
+/**
+ * drm_simple_display_pipe_init - Initialize a simple display pipeline
+ * @dev: DRM device
+ * @pipe: simple display pipe object to initialize
+ * @funcs: callbacks for the display pipe
+ * @formats: array of supported formats (%DRM_FORMAT_*)
+ * @format_count: number of elements in @formats
+ * @connector: connector to attach and register
+ *
+ * Sets up a display pipeline which consist of a really simple
+ * plane-crtc-encoder pipe coupled with the provided connector.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_simple_display_pipe_init(struct drm_device *dev,
+ struct drm_simple_display_pipe *pipe,
+ struct drm_simple_display_pipe_funcs *funcs,
+ const uint32_t *formats,
+ unsigned int format_count,
+ struct drm_connector *connector)
+{
+ struct drm_encoder *encoder = &pipe->encoder;
+ struct drm_plane *plane = &pipe->plane;
+ struct drm_crtc *crtc = &pipe->crtc;
+ int ret;
+
+ pipe->funcs = funcs;
+
+ drm_plane_helper_add(plane, &drm_simple_kms_plane_helper_funcs);
+ ret = drm_universal_plane_init(dev, plane, 0,
+ &drm_simple_kms_plane_funcs,
+ formats, format_count,
+ DRM_PLANE_TYPE_PRIMARY, NULL);
+ if (ret)
+ return ret;
+
+ drm_crtc_helper_add(crtc, &drm_simple_kms_crtc_helper_funcs);
+ ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL,
+ &drm_simple_kms_crtc_funcs, NULL);
+ if (ret)
+ return ret;
+
+ encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
+ ret = drm_encoder_init(dev, encoder, &drm_simple_kms_encoder_funcs,
+ DRM_MODE_ENCODER_NONE, NULL);
+ if (ret)
+ return ret;
+
+ ret = drm_mode_connector_attach_encoder(connector, encoder);
+ if (ret)
+ return ret;
+
+ return drm_connector_register(connector);
+}
+EXPORT_SYMBOL(drm_simple_display_pipe_init);
+
+MODULE_LICENSE("GPL");
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
new file mode 100644
index 0000000..69eac94
--- /dev/null
+++ b/include/drm/drm_simple_kms_helper.h
@@ -0,0 +1,88 @@
+/*
+ * 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.
+ */
+
+#ifndef __LINUX_DRM_SIMPLE_KMS_HELPER_H
+#define __LINUX_DRM_SIMPLE_KMS_HELPER_H
+
+struct drm_simple_display_pipe;
+
+/**
+ * struct drm_simple_display_pipe_funcs - helper operations for a simple
+ * display pipeline
+ */
+struct drm_simple_display_pipe_funcs {
+ /**
+ * @enable:
+ *
+ * This function should be used to enable the pipeline.
+ * It is called when the underlying crtc is enabled.
+ */
+ void (*enable)(struct drm_simple_display_pipe *pipe,
+ struct drm_crtc_state *crtc_state);
+ /**
+ * @disable:
+ *
+ * This function should be used to disable the pipeline.
+ * It is called when the underlying crtc is disabled.
+ */
+ void (*disable)(struct drm_simple_display_pipe *pipe);
+
+ /**
+ * @check:
+ *
+ * This function is called in the check phase of an atomic update,
+ * specifically when the underlying plane is checked.
+ *
+ * RETURNS:
+ *
+ * 0 on success, -EINVAL if the state or the transition can't be
+ * supported, -ENOMEM on memory allocation failure and -EDEADLK if an
+ * attempt to obtain another state object ran into a &drm_modeset_lock
+ * deadlock.
+ */
+ int (*check)(struct drm_simple_display_pipe *pipe,
+ struct drm_plane_state *plane_state,
+ struct drm_crtc_state *crtc_state);
+ /**
+ * @update:
+ *
+ * This function is called when the underlying plane state is updated.
+ */
+ void (*update)(struct drm_simple_display_pipe *pipe,
+ struct drm_plane_state *plane_state);
+};
+
+/**
+ * struct drm_simple_display_pipe - simple display pipeline
+ * @crtc: CRTC control structure
+ * @plane: Plane control structure
+ * @encoder: Encoder control structure
+ * @connector: Connector control structure
+ * @funcs: Pipeline control functions (optional)
+ *
+ * Simple display pipeline with plane, crtc and encoder collapsed into one
+ * entity.
+ */
+struct drm_simple_display_pipe {
+ struct drm_crtc crtc;
+ struct drm_plane plane;
+ struct drm_encoder encoder;
+ struct drm_connector *connector;
+
+ struct drm_simple_display_pipe_funcs *funcs;
+};
+
+int drm_simple_display_pipe_init(struct drm_device *dev,
+ struct drm_simple_display_pipe *pipe,
+ struct drm_simple_display_pipe_funcs *funcs,
+ const uint32_t *formats,
+ unsigned int format_count,
+ struct drm_connector *connector);
+
+#endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
--
2.2.2
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-05-05 18:50 +0200 |
| Subject | Re: [PATCH 3/4] drm: Add helper for simple display pipeline |
| Message-ID | <rvuDo-3mH-3@gated-at.bofh.it> |
| In reply to | #1395117 |
On Thu, May 05, 2016 at 03:24:33PM +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.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
When resubmitting a patch please always have a per-patch changelog of what
you've done. Otherwise you force reviewers to remember they looked at this
already, dig out the old mail and compare ;-)
> ---
> drivers/gpu/drm/Kconfig | 7 ++
> drivers/gpu/drm/Makefile | 1 +
> drivers/gpu/drm/drm_simple_kms_helper.c | 157 ++++++++++++++++++++++++++++++++
> include/drm/drm_simple_kms_helper.h | 88 ++++++++++++++++++
Please also add a new section (next to all the ones for the other kms
helpers) in Documentation/DocBook/gpu.tmpl and pull your kerneldoc in.
Please also add a short overview kernel doc section to the .c file (Using
the DOC: header) and also pull that into the overall gpu docs.
You can check the results using
$ make htmldocs
> 4 files changed, 253 insertions(+)
> create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
> create mode 100644 include/drm/drm_simple_kms_helper.h
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 16e4c21..ff9f480 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -39,6 +39,13 @@ config DRM_KMS_HELPER
> help
> CRTC helpers for KMS drivers.
>
> +config DRM_SIMPLE_KMS_HELPER
> + tristate
> + depends on DRM
> + select DRM_KMS_HELPER
> + help
> + Helpers for very simple KMS drivers.
> +
> config DRM_KMS_FB_HELPER
> bool
> depends on DRM_KMS_HELPER
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 43c2abf..7e99923 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -31,6 +31,7 @@ drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
>
> obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> +obj-$(CONFIG_DRM_SIMPLE_KMS_HELPER) += drm_simple_kms_helper.o
>
> CFLAGS_drm_trace_points.o := -I$(src)
>
> 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..c8725bb
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -0,0 +1,157 @@
> +/*
> + * 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>
> +
> +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 *pstate)
> +{
> + struct drm_simple_display_pipe *pipe;
> + struct drm_crtc_state *cstate;
> +
> + pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> + if (!pipe->funcs || !pipe->funcs->check)
> + return 0;
> +
> + cstate = drm_atomic_get_existing_crtc_state(pstate->state,
> + &pipe->crtc);
> +
> + return pipe->funcs->check(pipe, pstate, cstate);
> +}
> +
> +static void drm_simple_kms_plane_atomic_update(struct drm_plane *plane,
> + struct drm_plane_state *pstate)
> +{
> + struct drm_simple_display_pipe *pipe;
> +
> + pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> + if (!pipe->funcs || !pipe->funcs->update)
> + return;
> +
> + pipe->funcs->update(pipe, pstate);
> +}
> +
> +static const struct drm_plane_helper_funcs drm_simple_kms_plane_helper_funcs = {
> + .atomic_check = drm_simple_kms_plane_atomic_check,
> + .atomic_update = drm_simple_kms_plane_atomic_update,
> +};
> +
> +static const struct drm_plane_funcs drm_simple_kms_plane_funcs = {
> + .update_plane = drm_atomic_helper_update_plane,
> + .disable_plane = drm_atomic_helper_disable_plane,
> + .destroy = drm_plane_cleanup,
> + .reset = drm_atomic_helper_plane_reset,
> + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> +};
> +
> +/**
> + * drm_simple_display_pipe_init - Initialize a simple display pipeline
> + * @dev: DRM device
> + * @pipe: simple display pipe object to initialize
> + * @funcs: callbacks for the display pipe
> + * @formats: array of supported formats (%DRM_FORMAT_*)
> + * @format_count: number of elements in @formats
> + * @connector: connector to attach and register
> + *
> + * Sets up a display pipeline which consist of a really simple
> + * plane-crtc-encoder pipe coupled with the provided connector.
need to specify that @funcs is optional.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
"_negative_ error code"
> + */
> +int drm_simple_display_pipe_init(struct drm_device *dev,
> + struct drm_simple_display_pipe *pipe,
> + struct drm_simple_display_pipe_funcs *funcs,
> + const uint32_t *formats,
> + unsigned int format_count,
> + struct drm_connector *connector)
> +{
> + struct drm_encoder *encoder = &pipe->encoder;
> + struct drm_plane *plane = &pipe->plane;
> + struct drm_crtc *crtc = &pipe->crtc;
> + int ret;
> +
> + pipe->funcs = funcs;
> +
> + drm_plane_helper_add(plane, &drm_simple_kms_plane_helper_funcs);
> + ret = drm_universal_plane_init(dev, plane, 0,
> + &drm_simple_kms_plane_funcs,
> + formats, format_count,
> + DRM_PLANE_TYPE_PRIMARY, NULL);
> + if (ret)
> + return ret;
> +
> + drm_crtc_helper_add(crtc, &drm_simple_kms_crtc_helper_funcs);
> + ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL,
> + &drm_simple_kms_crtc_funcs, NULL);
> + if (ret)
> + return ret;
> +
> + encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
> + ret = drm_encoder_init(dev, encoder, &drm_simple_kms_encoder_funcs,
> + DRM_MODE_ENCODER_NONE, NULL);
> + if (ret)
> + return ret;
> +
> + ret = drm_mode_connector_attach_encoder(connector, encoder);
> + if (ret)
> + return ret;
> +
> + return drm_connector_register(connector);
> +}
> +EXPORT_SYMBOL(drm_simple_display_pipe_init);
> +
> +MODULE_LICENSE("GPL");
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> new file mode 100644
> index 0000000..69eac94
> --- /dev/null
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -0,0 +1,88 @@
> +/*
> + * 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.
> + */
> +
> +#ifndef __LINUX_DRM_SIMPLE_KMS_HELPER_H
> +#define __LINUX_DRM_SIMPLE_KMS_HELPER_H
> +
> +struct drm_simple_display_pipe;
> +
> +/**
> + * struct drm_simple_display_pipe_funcs - helper operations for a simple
> + * display pipeline
> + */
> +struct drm_simple_display_pipe_funcs {
> + /**
> + * @enable:
> + *
> + * This function should be used to enable the pipeline.
> + * It is called when the underlying crtc is enabled.
Please add boilerplate to all these hooks along the lines of:
"This hook is optional."
Besides that little bit of polish looks all good to me.
-Daniel
> + */
> + void (*enable)(struct drm_simple_display_pipe *pipe,
> + struct drm_crtc_state *crtc_state);
> + /**
> + * @disable:
> + *
> + * This function should be used to disable the pipeline.
> + * It is called when the underlying crtc is disabled.
> + */
> + void (*disable)(struct drm_simple_display_pipe *pipe);
> +
> + /**
> + * @check:
> + *
> + * This function is called in the check phase of an atomic update,
> + * specifically when the underlying plane is checked.
> + *
> + * RETURNS:
> + *
> + * 0 on success, -EINVAL if the state or the transition can't be
> + * supported, -ENOMEM on memory allocation failure and -EDEADLK if an
> + * attempt to obtain another state object ran into a &drm_modeset_lock
> + * deadlock.
> + */
> + int (*check)(struct drm_simple_display_pipe *pipe,
> + struct drm_plane_state *plane_state,
> + struct drm_crtc_state *crtc_state);
> + /**
> + * @update:
> + *
> + * This function is called when the underlying plane state is updated.
> + */
> + void (*update)(struct drm_simple_display_pipe *pipe,
> + struct drm_plane_state *plane_state);
> +};
> +
> +/**
> + * struct drm_simple_display_pipe - simple display pipeline
> + * @crtc: CRTC control structure
> + * @plane: Plane control structure
> + * @encoder: Encoder control structure
> + * @connector: Connector control structure
> + * @funcs: Pipeline control functions (optional)
> + *
> + * Simple display pipeline with plane, crtc and encoder collapsed into one
> + * entity.
> + */
> +struct drm_simple_display_pipe {
> + struct drm_crtc crtc;
> + struct drm_plane plane;
> + struct drm_encoder encoder;
> + struct drm_connector *connector;
> +
> + struct drm_simple_display_pipe_funcs *funcs;
> +};
> +
> +int drm_simple_display_pipe_init(struct drm_device *dev,
> + struct drm_simple_display_pipe *pipe,
> + struct drm_simple_display_pipe_funcs *funcs,
> + const uint32_t *formats,
> + unsigned int format_count,
> + struct drm_connector *connector);
> +
> +#endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
> --
> 2.2.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-05-09 16:50 +0200 |
| Subject | Re: [PATCH 3/4] drm: Add helper for simple display pipeline |
| Message-ID | <rwUFr-7AA-3@gated-at.bofh.it> |
| In reply to | #1395117 |
On Thu, May 05, 2016 at 03:24:33PM +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.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> +static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
> + struct drm_plane_state *pstate)
> +{
> + struct drm_simple_display_pipe *pipe;
> + struct drm_crtc_state *cstate;
> +
> + pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> + if (!pipe->funcs || !pipe->funcs->check)
> + return 0;
> +
> + cstate = drm_atomic_get_existing_crtc_state(pstate->state,
> + &pipe->crtc);
> +
> + return pipe->funcs->check(pipe, pstate, cstate);
> +}
Ok one thing I've missed here is that for most drivers this is way too
simple a check function, which means we'll end up with tons of duplicated
code. Things which the drm core allows, but simple pipelines all don't
really cope with:
- plane scaling
- disabling the plane without the crtc (i.e. scan out black)
- plane not sized to fill the entire hactive/vactive
There's a helper to do most of these checks for you -
drm_plane_helper_check_update. I think it'd be good to place a call for
that in here, before we call down into the driver's ->check callback. But
ofc before we return 0; we want these checks always done. And catch all
these things so that drivers never fall over this pitfall.
Noticed while discussing tilcdc atomic patches, since tilcdc could
probably use drm_simple_display_pipe too.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| Date | 2016-05-09 20:40 +0200 |
| Subject | Re: [PATCH 3/4] drm: Add helper for simple display pipeline |
| Message-ID | <rwYg1-2DJ-1@gated-at.bofh.it> |
| In reply to | #1397108 |
Den 09.05.2016 16:46, skrev Daniel Vetter:
> On Thu, May 05, 2016 at 03:24:33PM +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.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> +static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
>> + struct drm_plane_state *pstate)
>> +{
>> + struct drm_simple_display_pipe *pipe;
>> + struct drm_crtc_state *cstate;
>> +
>> + pipe = container_of(plane, struct drm_simple_display_pipe, plane);
>> + if (!pipe->funcs || !pipe->funcs->check)
>> + return 0;
>> +
>> + cstate = drm_atomic_get_existing_crtc_state(pstate->state,
>> + &pipe->crtc);
>> +
>> + return pipe->funcs->check(pipe, pstate, cstate);
>> +}
> Ok one thing I've missed here is that for most drivers this is way too
> simple a check function, which means we'll end up with tons of duplicated
> code. Things which the drm core allows, but simple pipelines all don't
> really cope with:
> - plane scaling
> - disabling the plane without the crtc (i.e. scan out black)
> - plane not sized to fill the entire hactive/vactive
>
> There's a helper to do most of these checks for you -
> drm_plane_helper_check_update. I think it'd be good to place a call for
> that in here, before we call down into the driver's ->check callback. But
> ofc before we return 0; we want these checks always done. And catch all
> these things so that drivers never fall over this pitfall.
Does this resemble what you're after? I'm just guessing here.
static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
struct drm_plane_state *pstate)
{
struct drm_rect src = {
.x1 = pstate->src_x,
.y1 = pstate->src_y,
.x2 = pstate->src_x + pstate->src_w,
.y2 = pstate->src_y + pstate->src_h,
};
struct drm_rect dest = {
.x1 = pstate->crtc_x,
.y1 = pstate->crtc_y,
.x2 = pstate->crtc_x + pstate->crtc_w,
.y2 = pstate->crtc_y + pstate->crtc_h,
};
struct drm_rect clip = { 0 };
struct drm_simple_display_pipe *pipe;
struct drm_crtc_state *cstate;
bool visible;
int ret;
pipe = container_of(plane, struct drm_simple_display_pipe, plane);
clip.x2 = pipe->crtc.mode.hdisplay;
clip.y2 = pipe->crtc.mode.vdisplay;
ret = drm_plane_helper_check_update(plane, &pipe->crtc, plane->fb,
&src, &dest, &clip,
DRM_PLANE_HELPER_NO_SCALING,
DRM_PLANE_HELPER_NO_SCALING,
false, false, &visible);
if (ret)
return ret;
/* How to handle !visible, is it even possible? */
if (!pipe->funcs || !pipe->funcs->check)
return 0;
cstate = drm_atomic_get_existing_crtc_state(pstate->state,
&pipe->crtc);
return pipe->funcs->check(pipe, pstate, cstate);
}
Noralf.
> Noticed while discussing tilcdc atomic patches, since tilcdc could
> probably use drm_simple_display_pipe too.
> -Daniel
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-05-10 09:00 +0200 |
| Subject | Re: [PATCH 3/4] drm: Add helper for simple display pipeline |
| Message-ID | <rx9Oa-5u8-19@gated-at.bofh.it> |
| In reply to | #1397253 |
On Mon, May 09, 2016 at 08:37:39PM +0200, Noralf Trønnes wrote:
>
> Den 09.05.2016 16:46, skrev Daniel Vetter:
> >On Thu, May 05, 2016 at 03:24:33PM +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.
> >>
> >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>+static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
> >>+ struct drm_plane_state *pstate)
> >>+{
> >>+ struct drm_simple_display_pipe *pipe;
> >>+ struct drm_crtc_state *cstate;
> >>+
> >>+ pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> >>+ if (!pipe->funcs || !pipe->funcs->check)
> >>+ return 0;
> >>+
> >>+ cstate = drm_atomic_get_existing_crtc_state(pstate->state,
> >>+ &pipe->crtc);
> >>+
> >>+ return pipe->funcs->check(pipe, pstate, cstate);
> >>+}
> >Ok one thing I've missed here is that for most drivers this is way too
> >simple a check function, which means we'll end up with tons of duplicated
> >code. Things which the drm core allows, but simple pipelines all don't
> >really cope with:
> >- plane scaling
> >- disabling the plane without the crtc (i.e. scan out black)
> >- plane not sized to fill the entire hactive/vactive
> >
> >There's a helper to do most of these checks for you -
> >drm_plane_helper_check_update. I think it'd be good to place a call for
> >that in here, before we call down into the driver's ->check callback. But
> >ofc before we return 0; we want these checks always done. And catch all
> >these things so that drivers never fall over this pitfall.
>
> Does this resemble what you're after? I'm just guessing here.
>
> static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
> struct drm_plane_state *pstate)
> {
> struct drm_rect src = {
> .x1 = pstate->src_x,
> .y1 = pstate->src_y,
> .x2 = pstate->src_x + pstate->src_w,
> .y2 = pstate->src_y + pstate->src_h,
> };
> struct drm_rect dest = {
> .x1 = pstate->crtc_x,
> .y1 = pstate->crtc_y,
> .x2 = pstate->crtc_x + pstate->crtc_w,
> .y2 = pstate->crtc_y + pstate->crtc_h,
> };
> struct drm_rect clip = { 0 };
Clip rect needs to be set to crtc_state->adjusted_mode.h/vdisplay, see
rockchip or armada. Otherwise you'll clip to nothing and always fail.
> struct drm_simple_display_pipe *pipe;
> struct drm_crtc_state *cstate;
> bool visible;
> int ret;
>
> pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> clip.x2 = pipe->crtc.mode.hdisplay;
> clip.y2 = pipe->crtc.mode.vdisplay;
> ret = drm_plane_helper_check_update(plane, &pipe->crtc, plane->fb,
> &src, &dest, &clip,
> DRM_PLANE_HELPER_NO_SCALING,
> DRM_PLANE_HELPER_NO_SCALING,
> false, false, &visible);
can_update_disabled = true should work, Only caveat is that you might get
a call to update the primary plane when the display is turned off.
Probably best though if we handle that in the simple pipe driver too. Just
call drm_atomic_helper_commit_planes with active_only = true.
> if (ret)
> return ret;
>
> /* How to handle !visible, is it even possible? */
if (!visible)
return -EINVAL;
You can't, so need to reject it.
>
> if (!pipe->funcs || !pipe->funcs->check)
> return 0;
>
> cstate = drm_atomic_get_existing_crtc_state(pstate->state,
> &pipe->crtc);
>
> return pipe->funcs->check(pipe, pstate, cstate);
> }
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-05-11 00:40 +0200 |
| Subject | Re: [PATCH 3/4] drm: Add helper for simple display pipeline |
| Message-ID | <rxotQ-3v7-11@gated-at.bofh.it> |
| In reply to | #1397736 |
On Tue, May 10, 2016 at 8:59 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> if (ret)
>> return ret;
>>
>> /* How to handle !visible, is it even possible? */
>
> if (!visible)
> return -EINVAL;
>
> You can't, so need to reject it.
Ok, on further thought I think we need a bit more here. We not just
need to make sure the plane is visible and not scaled or positioned,
but also that the plane is only enabled iff the crtc is.
So even before you call anything else you need to have this check:
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
already, calling check helpers and driver callbacks might only result
in confusion in such a case. */
Then continue with the remaining check logic that you have already,
with my coments in the earlier mail addressed.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| Date | 2016-05-05 15:30 +0200 |
| Subject | [PATCH 1/4] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs() |
| Message-ID | <rvrvS-MH-55@gated-at.bofh.it> |
| In reply to | #1395111 |
Add drm_fb_cma_create_with_funcs() for drivers that need to set the
dirty() callback.
Cc: laurent.pinchart@ideasonboard.com
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/gpu/drm/drm_fb_cma_helper.c | 29 +++++++++++++++++++++++------
include/drm/drm_fb_cma_helper.h | 3 +++
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 086f600..7165209 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -161,13 +161,16 @@ static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
}
/**
- * drm_fb_cma_create() - (struct drm_mode_config_funcs *)->fb_create callback function
+ * drm_fb_cma_create_with_funcs() - helper function for the
+ * &drm_mode_config_funcs ->fb_create
+ * callback function
*
- * If your hardware has special alignment or pitch requirements these should be
- * checked before calling this function.
+ * This can be used to set &drm_framebuffer_funcs for drivers that need the
+ * dirty() callback.
*/
-struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
- struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
+struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
+ struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
+ struct drm_framebuffer_funcs *funcs)
{
struct drm_fb_cma *fb_cma;
struct drm_gem_cma_object *objs[4];
@@ -204,7 +207,7 @@ struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
objs[i] = to_drm_gem_cma_obj(obj);
}
- fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, &drm_fb_cma_funcs);
+ fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, funcs);
if (IS_ERR(fb_cma)) {
ret = PTR_ERR(fb_cma);
goto err_gem_object_unreference;
@@ -217,6 +220,20 @@ err_gem_object_unreference:
drm_gem_object_unreference_unlocked(&objs[i]->base);
return ERR_PTR(ret);
}
+EXPORT_SYMBOL_GPL(drm_fb_cma_create_with_funcs);
+
+/**
+ * drm_fb_cma_create() - &drm_mode_config_funcs ->fb_create callback function
+ *
+ * If your hardware has special alignment or pitch requirements these should be
+ * checked before calling this function.
+ */
+struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
+ struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+ return drm_fb_cma_create_with_funcs(dev, file_priv, mode_cmd,
+ &drm_fb_cma_funcs);
+}
EXPORT_SYMBOL_GPL(drm_fb_cma_create);
/**
diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
index c6d9c9c..1f9a8bc 100644
--- a/include/drm/drm_fb_cma_helper.h
+++ b/include/drm/drm_fb_cma_helper.h
@@ -31,6 +31,9 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb);
int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
struct drm_file *file_priv, unsigned int *handle);
+struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
+ struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
+ struct drm_framebuffer_funcs *funcs);
struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd);
--
2.2.2
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-05-05 18:30 +0200 |
| Subject | Re: [PATCH 1/4] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs() |
| Message-ID | <rvuk2-3e2-31@gated-at.bofh.it> |
| In reply to | #1395120 |
On Thu, May 05, 2016 at 03:24:31PM +0200, Noralf Trønnes wrote:
> Add drm_fb_cma_create_with_funcs() for drivers that need to set the
> dirty() callback.
>
> Cc: laurent.pinchart@ideasonboard.com
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> drivers/gpu/drm/drm_fb_cma_helper.c | 29 +++++++++++++++++++++++------
> include/drm/drm_fb_cma_helper.h | 3 +++
> 2 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 086f600..7165209 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -161,13 +161,16 @@ static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
> }
>
> /**
> - * drm_fb_cma_create() - (struct drm_mode_config_funcs *)->fb_create callback function
> + * drm_fb_cma_create_with_funcs() - helper function for the
> + * &drm_mode_config_funcs ->fb_create
> + * callback function
> *
> - * If your hardware has special alignment or pitch requirements these should be
> - * checked before calling this function.
> + * This can be used to set &drm_framebuffer_funcs for drivers that need the
> + * dirty() callback.
Please reference the other function in your kerneldoc using
drm_fb_cma_create() syntax. That will create a hyperlink. With such sets
of functions it's always good to cross link them and explain exactly when
another one is more appropriate. E.g. here "If your driver does not need a
custom &drm_framebuffer_funcs then just use drm_fb_cma_create() directly."
Similar, but other way round for the existing one.
Again please check with make htmldocs that it all looks good.
Otherwise lgtm.
-Daniel
> */
> -struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> - struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
> +struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
> + struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
> + struct drm_framebuffer_funcs *funcs)
> {
> struct drm_fb_cma *fb_cma;
> struct drm_gem_cma_object *objs[4];
> @@ -204,7 +207,7 @@ struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> objs[i] = to_drm_gem_cma_obj(obj);
> }
>
> - fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, &drm_fb_cma_funcs);
> + fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, funcs);
> if (IS_ERR(fb_cma)) {
> ret = PTR_ERR(fb_cma);
> goto err_gem_object_unreference;
> @@ -217,6 +220,20 @@ err_gem_object_unreference:
> drm_gem_object_unreference_unlocked(&objs[i]->base);
> return ERR_PTR(ret);
> }
> +EXPORT_SYMBOL_GPL(drm_fb_cma_create_with_funcs);
> +
> +/**
> + * drm_fb_cma_create() - &drm_mode_config_funcs ->fb_create callback function
> + *
> + * If your hardware has special alignment or pitch requirements these should be
> + * checked before calling this function.
> + */
> +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> + struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> + return drm_fb_cma_create_with_funcs(dev, file_priv, mode_cmd,
> + &drm_fb_cma_funcs);
> +}
> EXPORT_SYMBOL_GPL(drm_fb_cma_create);
>
> /**
> diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
> index c6d9c9c..1f9a8bc 100644
> --- a/include/drm/drm_fb_cma_helper.h
> +++ b/include/drm/drm_fb_cma_helper.h
> @@ -31,6 +31,9 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb);
> int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
> struct drm_file *file_priv, unsigned int *handle);
>
> +struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
> + struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
> + struct drm_framebuffer_funcs *funcs);
> struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd);
>
> --
> 2.2.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| Date | 2016-05-06 15:10 +0200 |
| Subject | Re: [PATCH 1/4] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs() |
| Message-ID | <rvNG2-5Pa-33@gated-at.bofh.it> |
| In reply to | #1395217 |
Den 05.05.2016 18:27, skrev Daniel Vetter:
> On Thu, May 05, 2016 at 03:24:31PM +0200, Noralf Trønnes wrote:
>> Add drm_fb_cma_create_with_funcs() for drivers that need to set the
>> dirty() callback.
>>
>> Cc: laurent.pinchart@ideasonboard.com
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>> drivers/gpu/drm/drm_fb_cma_helper.c | 29 +++++++++++++++++++++++------
>> include/drm/drm_fb_cma_helper.h | 3 +++
>> 2 files changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
>> index 086f600..7165209 100644
>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> @@ -161,13 +161,16 @@ static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
>> }
>>
>> /**
>> - * drm_fb_cma_create() - (struct drm_mode_config_funcs *)->fb_create callback function
>> + * drm_fb_cma_create_with_funcs() - helper function for the
>> + * &drm_mode_config_funcs ->fb_create
>> + * callback function
>> *
>> - * If your hardware has special alignment or pitch requirements these should be
>> - * checked before calling this function.
>> + * This can be used to set &drm_framebuffer_funcs for drivers that need the
>> + * dirty() callback.
> Please reference the other function in your kerneldoc using
> drm_fb_cma_create() syntax. That will create a hyperlink. With such sets
> of functions it's always good to cross link them and explain exactly when
> another one is more appropriate. E.g. here "If your driver does not need a
> custom &drm_framebuffer_funcs then just use drm_fb_cma_create() directly."
>
> Similar, but other way round for the existing one.
>
> Again please check with make htmldocs that it all looks good.
Ok, I didn't understand this htmldocs stuff, I thought it picked up the docs
by magic or something.
It turns out that drm_fb_cma_helper isn't mentioned in gpu.tmpl so I have
to make a patch for that.
Is there an order to things where I should put it in gpu.tmpl?
(drm_simple_kms_helper also)
This is the current order:
Mode Setting Helper Functions
Atomic Modeset Helper Functions Reference
Modeset Helper Reference for Common Vtables
Legacy CRTC/Modeset Helper Functions Reference
Output Probing Helper Functions Reference
fbdev Helper Functions Reference
Display Port Helper Functions Reference
Display Port MST Helper Functions Reference
MIPI DSI Helper Functions Reference
EDID Helper Functions Reference
Rectangle Utilities Reference
Flip-work Helper Reference
HDMI Infoframes Helper Reference
Plane Helper Reference
Tile group
Bridges
And the code example I put in drm_fb_cma_helper DOC: looks
terrible, maybe it looks better in the intel augmented version?
Noralf.
> Otherwise lgtm.
> -Daniel
>
>> */
>> -struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>> - struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
>> +struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
>> + struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
>> + struct drm_framebuffer_funcs *funcs)
>> {
>> struct drm_fb_cma *fb_cma;
>> struct drm_gem_cma_object *objs[4];
>> @@ -204,7 +207,7 @@ struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>> objs[i] = to_drm_gem_cma_obj(obj);
>> }
>>
>> - fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, &drm_fb_cma_funcs);
>> + fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, funcs);
>> if (IS_ERR(fb_cma)) {
>> ret = PTR_ERR(fb_cma);
>> goto err_gem_object_unreference;
>> @@ -217,6 +220,20 @@ err_gem_object_unreference:
>> drm_gem_object_unreference_unlocked(&objs[i]->base);
>> return ERR_PTR(ret);
>> }
>> +EXPORT_SYMBOL_GPL(drm_fb_cma_create_with_funcs);
>> +
>> +/**
>> + * drm_fb_cma_create() - &drm_mode_config_funcs ->fb_create callback function
>> + *
>> + * If your hardware has special alignment or pitch requirements these should be
>> + * checked before calling this function.
>> + */
>> +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>> + struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
>> +{
>> + return drm_fb_cma_create_with_funcs(dev, file_priv, mode_cmd,
>> + &drm_fb_cma_funcs);
>> +}
>> EXPORT_SYMBOL_GPL(drm_fb_cma_create);
>>
>> /**
>> diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
>> index c6d9c9c..1f9a8bc 100644
>> --- a/include/drm/drm_fb_cma_helper.h
>> +++ b/include/drm/drm_fb_cma_helper.h
>> @@ -31,6 +31,9 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb);
>> int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
>> struct drm_file *file_priv, unsigned int *handle);
>>
>> +struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
>> + struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
>> + struct drm_framebuffer_funcs *funcs);
>> struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>> struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd);
>>
>> --
>> 2.2.2
>>
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-05-06 15:20 +0200 |
| Subject | Re: [PATCH 1/4] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs() |
| Message-ID | <rvNPI-5Xd-5@gated-at.bofh.it> |
| In reply to | #1395857 |
On Fri, May 06, 2016 at 03:01:37PM +0200, Noralf Trønnes wrote:
>
> Den 05.05.2016 18:27, skrev Daniel Vetter:
> >On Thu, May 05, 2016 at 03:24:31PM +0200, Noralf Trønnes wrote:
> >>Add drm_fb_cma_create_with_funcs() for drivers that need to set the
> >>dirty() callback.
> >>
> >>Cc: laurent.pinchart@ideasonboard.com
> >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>---
> >> drivers/gpu/drm/drm_fb_cma_helper.c | 29 +++++++++++++++++++++++------
> >> include/drm/drm_fb_cma_helper.h | 3 +++
> >> 2 files changed, 26 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> >>index 086f600..7165209 100644
> >>--- a/drivers/gpu/drm/drm_fb_cma_helper.c
> >>+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> >>@@ -161,13 +161,16 @@ static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
> >> }
> >> /**
> >>- * drm_fb_cma_create() - (struct drm_mode_config_funcs *)->fb_create callback function
> >>+ * drm_fb_cma_create_with_funcs() - helper function for the
> >>+ * &drm_mode_config_funcs ->fb_create
> >>+ * callback function
> >> *
> >>- * If your hardware has special alignment or pitch requirements these should be
> >>- * checked before calling this function.
> >>+ * This can be used to set &drm_framebuffer_funcs for drivers that need the
> >>+ * dirty() callback.
> >Please reference the other function in your kerneldoc using
> >drm_fb_cma_create() syntax. That will create a hyperlink. With such sets
> >of functions it's always good to cross link them and explain exactly when
> >another one is more appropriate. E.g. here "If your driver does not need a
> >custom &drm_framebuffer_funcs then just use drm_fb_cma_create() directly."
> >
> >Similar, but other way round for the existing one.
> >
> >Again please check with make htmldocs that it all looks good.
>
> Ok, I didn't understand this htmldocs stuff, I thought it picked up the docs
> by magic or something.
> It turns out that drm_fb_cma_helper isn't mentioned in gpu.tmpl so I have
> to make a patch for that.
Oh right, cma fb helpers aren't pulled int at all. Would be great if you
can fix that.
> Is there an order to things where I should put it in gpu.tmpl?
> (drm_simple_kms_helper also)
>
> This is the current order:
>
> Mode Setting Helper Functions
>
> Atomic Modeset Helper Functions Reference
> Modeset Helper Reference for Common Vtables
> Legacy CRTC/Modeset Helper Functions Reference
> Output Probing Helper Functions Reference
> fbdev Helper Functions Reference
> Display Port Helper Functions Reference
> Display Port MST Helper Functions Reference
> MIPI DSI Helper Functions Reference
> EDID Helper Functions Reference
> Rectangle Utilities Reference
> Flip-work Helper Reference
> HDMI Infoframes Helper Reference
> Plane Helper Reference
> Tile group
> Bridges
Just add it somewhere to this list of helpers where you think it fits.
We're not that structured yet.
> And the code example I put in drm_fb_cma_helper DOC: looks
> terrible, maybe it looks better in the intel augmented version?
Yeah, in upstream it's terrible. But if you base your patch on top of
drm-intel-nightly, or pull topic/kerneldoc in, and install asciidoc, then
it should be fairly pretty. Note: That asciidoc support is pretty hacked
up, it takes 10-20 minutes to build the gpu docs with that. We're working
on something much better.
-Daniel
>
> Noralf.
>
> >Otherwise lgtm.
> >-Daniel
> >
> >> */
> >>-struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> >>- struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
> >>+struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
> >>+ struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
> >>+ struct drm_framebuffer_funcs *funcs)
> >> {
> >> struct drm_fb_cma *fb_cma;
> >> struct drm_gem_cma_object *objs[4];
> >>@@ -204,7 +207,7 @@ struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> >> objs[i] = to_drm_gem_cma_obj(obj);
> >> }
> >>- fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, &drm_fb_cma_funcs);
> >>+ fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, funcs);
> >> if (IS_ERR(fb_cma)) {
> >> ret = PTR_ERR(fb_cma);
> >> goto err_gem_object_unreference;
> >>@@ -217,6 +220,20 @@ err_gem_object_unreference:
> >> drm_gem_object_unreference_unlocked(&objs[i]->base);
> >> return ERR_PTR(ret);
> >> }
> >>+EXPORT_SYMBOL_GPL(drm_fb_cma_create_with_funcs);
> >>+
> >>+/**
> >>+ * drm_fb_cma_create() - &drm_mode_config_funcs ->fb_create callback function
> >>+ *
> >>+ * If your hardware has special alignment or pitch requirements these should be
> >>+ * checked before calling this function.
> >>+ */
> >>+struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> >>+ struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
> >>+{
> >>+ return drm_fb_cma_create_with_funcs(dev, file_priv, mode_cmd,
> >>+ &drm_fb_cma_funcs);
> >>+}
> >> EXPORT_SYMBOL_GPL(drm_fb_cma_create);
> >> /**
> >>diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
> >>index c6d9c9c..1f9a8bc 100644
> >>--- a/include/drm/drm_fb_cma_helper.h
> >>+++ b/include/drm/drm_fb_cma_helper.h
> >>@@ -31,6 +31,9 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb);
> >> int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
> >> struct drm_file *file_priv, unsigned int *handle);
> >>+struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
> >>+ struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
> >>+ struct drm_framebuffer_funcs *funcs);
> >> struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> >> struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd);
> >>--
> >>2.2.2
> >>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web