On Thu, Aug 14, 2014 at 06:06:58PM +0300, Ville Syrjälä wrote:
> 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@xxxxxxxxx wrote:
> > > From: Akash Goel <akash.goel@xxxxxxxxx>
> > > + /* 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.
Well within i915, but not anywhere in the drm core or helper code since
they're simply not aware of pipe_src_w/h. E.g. the plane helper code also
uses crtc->mode.h/vdisplay.
Changing that basic drm assumption is really invasive imo.
> > 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.
Oh I'm fully aware that it's in the leagues of cross hacks ;-) But doing
the crtc->src_w/h thing correctly rolled out across the entire drm
subsystem will be a big chunk more work than just adding new variables to
struct drm_crtc which are only used in i915.
> > 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.
I guess then it needs to be moved down eventually.
> 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.
Actually you get to do a full drm audit anyway, since there's more than
check_viewport. They should all switch to crtc->src_w/h, and we should
fill that out by default in the crtc helpers. Imo stuffing magic new stuff
into the core only used by one driver isn't good, if we go this route we
should do it properly.
Or we ditch the struct drm_crtc change and pull it all into i915.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
