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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Aug 14, 2014 at 04:42:01PM +0200, Daniel Vetter wrote:
> 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

We don't use it in i915. If we do that's a bug. All the relevant places
should be loooking at pipe_src_{w,h}. In the core the viewport check
should be about the only place that cares about this stuff.

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

We shouldn't just magically change the user specified mode, we need it
to stay intact for a subsequent modeset so that we can start the
adjusted_mode frobbery fresh next time around. It also seems weird to
report back a different mode to userspace than what the user provided.
What you suggest was exactly the previous approach and I NAKed it.

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

We don't have sufficient checks in the modeset path. The
drm_crtc_check_viewport() call is in drm_mode_setcrtc() which is too
high up in the stack to affect modesets originating from property
changes. That being said we should definitely use drm_crtc_check_viewport()
here instead of hand rolling the exacty same thing in i915.

And to avoid having to touch too much code, drm_crtc_check_viewport()
should encapsulate the logic to fall back to mode->{h,v}display when
crtc->{w,h} are zero.

> 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC