Re: [PATCH v2 6/6] drm: Add helper for simple display pipeline

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

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@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.