From: Zhao Junwang zhjwpku@gmail.com
Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Zhao Junwang zhjwpku@gmail.com --- drivers/gpu/drm/drm_atomic_helper.c | 55 +++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 62 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 536ae4d..3d94ff8 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1336,6 +1336,61 @@ void drm_atomic_helper_swap_state(struct drm_device *dev, EXPORT_SYMBOL(drm_atomic_helper_swap_state);
/** + * drm_atomic_helper_plane_check_update + * @plane: plane object to update + * @state: drm plane state + * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point + * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point + * @can_position: is it legal to position the plane such that it + * doesn't cover the entire crtc? This will generally + * only be false for primary planes. + * @can_update_disabled: can the plane be updated while the crtc + * is disabled? + * + * Provide a default plane check update handler + * + * RETURNS: + * Zero on success, error code on failure + */ +int drm_atomic_helper_plane_check_update(struct drm_plane *plane, + struct drm_plane_state *state, + int min_scale, + int max_scale, + bool can_position, + bool can_update_disabled, + bool *visible) +{ + struct drm_crtc *crtc = state->crtc; + struct drm_framebuffer *fb = state->fb; + + if (!fb) + return 0; + + struct drm_rect src = { + .x1 = state->src_x, + .y1 = state->src_y, + .x2 = state->src_x + state->src_w, + .y2 = state->src_y + state->src_h, + }; + struct drm_rect dest = { + .x1 = state->crtc_x, + .y1 = state->crtc_y, + .x2 = state->crtc_x + state->crtc_w, + .y2 = state->crtc_y + state->crtc_h, + }; + const struct drm_rect clip = { + .x2 = crtc->mode.hdisplay, + .y2 = crtc->mode.vdisplay, + }; + + return drm_plane_helper_check_update(plane, crtc, fb, + &src, &dest, &clip, + min_scale, max_scale, + can_position, can_update_disabled, visible); +} +EXPORT_SYMBOL(drm_atomic_helper_plane_check_update); + +/** * drm_atomic_helper_update_plane - Helper for primary plane update using atomic * @plane: plane object to update * @crtc: owning CRTC of owning plane diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index cc1fee8..5305a01 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -64,6 +64,13 @@ void drm_atomic_helper_swap_state(struct drm_device *dev, struct drm_atomic_state *state);
/* implementations for legacy interfaces */ +int drm_atomic_helper_plane_check_update(struct drm_plane *plane, + struct drm_plane_state *state, + int min_scale, + int max_scale, + bool can_position, + bool can_update_disabled, + bool *visible); int drm_atomic_helper_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb,
On Mon, Jul 13, 2015 at 08:21:32AM +0800, John Hunter wrote:
From: Zhao Junwang zhjwpku@gmail.com
A bit of text to motivate this would be good - i.e. why is this useful, how did it blow up in the bochs conversion?
Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Zhao Junwang zhjwpku@gmail.com
drivers/gpu/drm/drm_atomic_helper.c | 55 +++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 62 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 536ae4d..3d94ff8 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1336,6 +1336,61 @@ void drm_atomic_helper_swap_state(struct drm_device *dev, EXPORT_SYMBOL(drm_atomic_helper_swap_state);
/**
- drm_atomic_helper_plane_check_update
- @plane: plane object to update
- @state: drm plane state
- @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
- @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
- @can_position: is it legal to position the plane such that it
doesn't cover the entire crtc? This will generally
only be false for primary planes.
- @can_update_disabled: can the plane be updated while the crtc
is disabled?
- Provide a default plane check update handler
It's not a default handler since the signatures don't match. Also I think we should mention the legacy plane helper function here too. What about
This provides a helper to be used in a driver's plane atomic_check callback. It is the atomic equivalent of drm_plane_helper_check_update() and has the exact same semantics, except that it looks at the atomit CRTC state in the atomic update instead of legacy state directly embedded in struct &drm_crtc.
Also adding a comment to drm_plane_helper_check_update's kerneldoc would be good, like this:
Note: When converting over to atomic drivers you need to switch over to using drm_atomic_helper_plane_check_update() since only that correctly checks atomic state - this function here only looks at legacy state and hence will check against stale values in atomic updates.
- RETURNS:
- Zero on success, error code on failure
- */
+int drm_atomic_helper_plane_check_update(struct drm_plane *plane,
struct drm_plane_state *state,int min_scale,int max_scale,bool can_position,bool can_update_disabled,bool *visible)+{
- struct drm_crtc *crtc = state->crtc;
- struct drm_framebuffer *fb = state->fb;
- if (!fb)
return 0;- struct drm_rect src = {
.x1 = state->src_x,.y1 = state->src_y,.x2 = state->src_x + state->src_w,.y2 = state->src_y + state->src_h,- };
- struct drm_rect dest = {
.x1 = state->crtc_x,.y1 = state->crtc_y,.x2 = state->crtc_x + state->crtc_w,.y2 = state->crtc_y + state->crtc_h,- };
- const struct drm_rect clip = {
.x2 = crtc->mode.hdisplay,.y2 = crtc->mode.vdisplay,
crtc->mode is legacy state - we need to look at crtc_state here instead.
- };
- return drm_plane_helper_check_update(plane, crtc, fb,
&src, &dest, &clip,min_scale, max_scale,can_position, can_update_disabled, visible);
This looks at crtc->enabled (which is also legacy state) for can_update_disabled. If we want to reuse this function we need to check can_updated_disabled here and it unconditionally to true when calling the plane helpers.
Cheers, Daniel
+} +EXPORT_SYMBOL(drm_atomic_helper_plane_check_update);
+/**
- drm_atomic_helper_update_plane - Helper for primary plane update using atomic
- @plane: plane object to update
- @crtc: owning CRTC of owning plane
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index cc1fee8..5305a01 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -64,6 +64,13 @@ void drm_atomic_helper_swap_state(struct drm_device *dev, struct drm_atomic_state *state);
/* implementations for legacy interfaces */ +int drm_atomic_helper_plane_check_update(struct drm_plane *plane,
struct drm_plane_state *state,int min_scale,int max_scale,bool can_position,bool can_update_disabled,bool *visible);int drm_atomic_helper_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, -- 1.7.10.4
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 13-07-15 om 02:21 schreef John Hunter:
From: Zhao Junwang zhjwpku@gmail.com
Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Zhao Junwang zhjwpku@gmail.com
drivers/gpu/drm/drm_atomic_helper.c | 55 +++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 62 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 536ae4d..3d94ff8 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1336,6 +1336,61 @@ void drm_atomic_helper_swap_state(struct drm_device *dev, EXPORT_SYMBOL(drm_atomic_helper_swap_state);
/**
- drm_atomic_helper_plane_check_update
- @plane: plane object to update
== plane_state->plane, so can be removed
- @state: drm plane state
rename to plane_state
- @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
- @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
- @can_position: is it legal to position the plane such that it
doesn't cover the entire crtc? This will generally
only be false for primary planes.
- @can_update_disabled: can the plane be updated while the crtc
is disabled?
If you look carefully at drm_plane_helper_check_update, can_update_disabled will be a noop, so remove this parameter.
plane_state->crtc != NULL iff plane_state->fb != NULL
Op 13-07-15 om 09:12 schreef Maarten Lankhorst:
Op 13-07-15 om 02:21 schreef John Hunter:
From: Zhao Junwang zhjwpku@gmail.com
Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Zhao Junwang zhjwpku@gmail.com
drivers/gpu/drm/drm_atomic_helper.c | 55 +++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 62 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 536ae4d..3d94ff8 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1336,6 +1336,61 @@ void drm_atomic_helper_swap_state(struct drm_device *dev, EXPORT_SYMBOL(drm_atomic_helper_swap_state);
/**
- drm_atomic_helper_plane_check_update
- @plane: plane object to update
== plane_state->plane, so can be removed
- @state: drm plane state
rename to plane_state
- @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
- @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
- @can_position: is it legal to position the plane such that it
doesn't cover the entire crtc? This will generally
only be false for primary planes.
- @can_update_disabled: can the plane be updated while the crtc
is disabled?
If you look carefully at drm_plane_helper_check_update, can_update_disabled will be a noop, so remove this parameter.
plane_state->crtc != NULL iff plane_state->fb != NULL
Oops, should check harder before I hit send. You can disable a crtc with planes attached, but the clip will be bogus in that case. When !crtc->enable the clip will be set { 0, 0, 0, 0 }, hiding the plane.
There's still no need for can_update_disabled in the atomic world though, the plane state will be properly recalculated during a modeset. If you don't want to update a plane while a crtc is disabled, just don't update it in your commit function.
So we should remove the can_update_disabled parameter, and set it to true in the drm_plane_helper_check_update callback?
I am still learning the atomic world, pls forgive if I don't understand it well
On Mon, Jul 13, 2015 at 3:17 PM, Maarten Lankhorst < maarten.lankhorst@linux.intel.com> wrote:
Op 13-07-15 om 09:12 schreef Maarten Lankhorst:
Op 13-07-15 om 02:21 schreef John Hunter:
From: Zhao Junwang zhjwpku@gmail.com
Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Zhao Junwang zhjwpku@gmail.com
drivers/gpu/drm/drm_atomic_helper.c | 55
+++++++++++++++++++++++++++++++++++
include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 62 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 536ae4d..3d94ff8 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1336,6 +1336,61 @@ void drm_atomic_helper_swap_state(struct
drm_device *dev,
EXPORT_SYMBOL(drm_atomic_helper_swap_state);
/**
- drm_atomic_helper_plane_check_update
- @plane: plane object to update
== plane_state->plane, so can be removed
- @state: drm plane state
rename to plane_state
- @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
- @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
- @can_position: is it legal to position the plane such that it
doesn't cover the entire crtc? This will generally
only be false for primary planes.
- @can_update_disabled: can the plane be updated while the crtc
is disabled?
If you look carefully at drm_plane_helper_check_update,
can_update_disabled will be a noop,
so remove this parameter.
plane_state->crtc != NULL iff plane_state->fb != NULL
Oops, should check harder before I hit send. You can disable a crtc with planes attached, but the clip will be bogus in that case. When !crtc->enable the clip will be set { 0, 0, 0, 0 }, hiding the plane.
There's still no need for can_update_disabled in the atomic world though, the plane state will be properly recalculated during a modeset. If you don't want to update a plane while a crtc is disabled, just don't update it in your commit function.
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
John Hunter -
Maarten Lankhorst