Den 11.05.2016 19:09, skrev Daniel Vetter:
> On Wed, May 11, 2016 at 06:09:22PM +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 at tronnes.org>
> Looks really nice, just a few suggestions for the kerneldoc. Would be
> awesome if we could get an ack on this from Jyri for tilcdc, but even
> without that I think I'll just pull in the next iteration. Still please cc
> him.
>
> Thanks, Daniel
Thanks for helping me with the docs.
[...]
>> +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;
> I think the logic above looks correct now, but might be worth checking
> with your driver that it doesn't let something silly through. I.e. a small
> test app that tries to reposition the primary plane, or tries to disable
> it while the crtc is on. We should have that somewhere in libdrm I think.
I hacked libdrm tests/kms/kms-universal-planes to position the plane
at (1,1) which failed (I added some debug output to the driver):
[ 885.906697] [drm:drm_atomic_set_fb_for_plane] Set [FB:25] for plane
state b84aec40
[ 885.906707] [drm:drm_atomic_check_only] checking b84aeec0
[ 885.906738] [drm:drm_plane_helper_check_update] Plane must cover
entire CRTC
[ 885.906748] [drm:drm_rect_debug_print] dst: 319x239+1+1
[ 885.906757] [drm:drm_rect_debug_print] clip: 320x240+0+0
[ 885.906765] drm_simple_kms_plane_atomic_check: ret=-22, visible=1
[ 885.906775] [drm:drm_atomic_helper_check_planes] [PLANE:19:plane-0]
atomic driver check failed
Then I changed the test to pass 0 for fb id which also failed:
[ 3144.599790] [drm:drm_atomic_set_fb_for_plane] Set [NOFB] for plane
state b87805c0
[ 3144.599799] [drm:drm_atomic_check_only] checking b8780d80
[ 3144.599816] [drm:drm_atomic_helper_check_planes] [PLANE:19:plane-0]
atomic driver check failed
Noralf.
