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