Re: [PATCH 6/6] drm/i915: New drm crtc property for varying the Crtc size

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Aug 14, 2014 at 02:54:27PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> +		/* Check if the current FB is compatible with new requested
> +		   Pipesrc values by the User */
> +		if (new_width > fb->width ||
> +			new_height > fb->height ||
> +			crtc->x > fb->width - new_width ||
> +			crtc->y > fb->height - new_height) {
> +			DRM_DEBUG_KMS("New Pipe Src values %dx%d is incompatible with current fb size & viewport %ux%u+%d+%d\n",
> +			      new_width, new_height, fb->width, fb->height, crtc->x, crtc->y);
> +			return -EINVAL;
> +		}

I think this part here is the achilles heel of your approach. Right now we
use crtc->mode.hdisplay/vdisplay in a lot of places both in i915 and the
drm core to make sure that the primary plane fb, sprite fb and positioning
and cursor placement all make sense.

If my understanding of the pipe src rectangle is correct (please correct
me if I'm wrong) we should switch _all_ these checks to instead look at
the new crtc_w/h field. Even worse that we need to change drm core code
and as a result of that all drm drivers. Awful lot of code to check, test
and for i915 validate with i-g-t testcases.

Now the solution thus far (for the normal panel fitter operation) is that
the logical input mode for the crtc ends up in crtc->config.mode and as a
copy in crtc->mode. And the actual output mode is in
crtc->config.adjusted_mode.

Our modeset code already takes care of copying crtc->config.mode to
crtc->mode, so we only need to concern ourselfs with crtc->config.mode. If
we'd copy the pipe_src_w/h values back into it the existing code would
still be able to check all the sprite/cursor/fb constraints.

So the flow on a modeset (and that's what we'll end up calling from the
set_property function too) is:

1. Userspace requested mode goes into pipe_config->mode.
2. We make a copy into pipe_config->adjusted_mode and frob it more.
3. crtc compute_config code notices the special pipe src window, and
adjusts pipe_config->mode plus computes the panel fitter configuration.

If all that checks out we continue with the modeset sequence.

4. We store the new pipe_config into crtc->config.
5. Actual hw register writes for the modeset change happens.
6. We copy crtc->config.mode into crtc->mode so that drm core and helper
functions can validate fb/sprite/cursors again.

The result would be that the set_property function here would do _no_
argument checking, but would instead fully rely upon the modeset sequence
to compute the desired configuration. And if it fails it would restore the
old configuration like you already do.

Now test coverage: I think we need a few i-g-ts that provoke this, i.e.
which set the pipe_src != requested mode and then place cursor/sprite/fb
to validate that the checking is still correct.

Aside: In the shiny new world of atomic display updates we always need to
have similar procedures i.e.

1. Store state without much checking.
2. Run full modeset machinery to compute the new resulting state.
3. Check that, and only if that checks out, commit it.

If we duplicate special cases of these checks all over, then we'll have an
unmaintainable mess in shorter order. C.f. compare the discussion about
rotation properties.

Thoughts, better ideas and issues with this plan?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch