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

[Noralf TrÃnnes]

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