| Message ID | 1408008267-11443-7-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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@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. 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
On Thu, Aug 14, 2014 at 05:26:27PM +0200, Daniel Vetter wrote: > 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@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. > > 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. We can't since we need to get past drm_crtc_check_viewport().
On Thu, Aug 14, 2014 at 05:26:27PM +0200, Daniel Vetter wrote: > 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@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. > > 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. My quick grep audit tells me the viewport check and drm_primary_helper_update() are the only places in the core that care. The latter is rather dubious anyway since the clipping should be done against the adjusted mode and not the user specified mode, so anyone using that is already dancing on thin ice. The other drivers are something I would not touch. Given how many places we had to frob in i915 I'm somewhat sure I'd not like what I see there and then any patch I might cook up would be too half assed to satisfy my quality standards anyway. As far as always filling the crtc->w,h always goes, I'm not sure that's the best idea anyway since we still need the "0 is special" handling. Well, we could fill them out, but then we definitely need a flag or something to indicate that they came from the mode and not the properties, so that we know whether we should overwrite them from with {h,v}display during a subsquent modeset or if they should keep their current value.
On Thu, Aug 14, 2014 at 5:45 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > My quick grep audit tells me the viewport check and > drm_primary_helper_update() are the only places in the core that care. > The latter is rather dubious anyway since the clipping should be done > against the adjusted mode and not the user specified mode, so anyone > using that is already dancing on thin ice. My understanding has always been that the requested mode is what userspace plans to feed into the pipe, and the adjusted mode is what actually lands in the sink. Yeah there's some hilarity in the vblank code which somehow presumes that the vblank counter works with the adjusted mode because that's what i915 needs. It's that fundamental assumption that we break by making the pipe_src stuff official and which is the part that freaks me out a bit. > The other drivers are something I would not touch. Given how many places > we had to frob in i915 I'm somewhat sure I'd not like what I see there > and then any patch I might cook up would be too half assed to satisfy my > quality standards anyway. Yeah, other drivers only need to be audited I think once they start supporting the pipe_src stuff. But I think the core+helpers should be able to cope properly. > As far as always filling the crtc->w,h always goes, I'm not sure that's > the best idea anyway since we still need the "0 is special" handling. > Well, we could fill them out, but then we definitely need a flag or > something to indicate that they came from the mode and not the > properties, so that we know whether we should overwrite them from > with {h,v}display during a subsquent modeset or if they should keep > their current value. Hm, I guess we can keep that implicit meaning, but then we need a small helper to get at the crtc viewport, e.g. drm_crtc_viewport_rect or so. That would also be an excellent place to document this trickery properly. Oh and: Such drm changes _really_ must be split out into separate prep patches cc: dri-devel. -Daniel
On Thu, Aug 14, 2014 at 06:07:44PM +0200, Daniel Vetter wrote: > On Thu, Aug 14, 2014 at 5:45 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > My quick grep audit tells me the viewport check and > > drm_primary_helper_update() are the only places in the core that care. > > The latter is rather dubious anyway since the clipping should be done > > against the adjusted mode and not the user specified mode, so anyone > > using that is already dancing on thin ice. > > My understanding has always been that the requested mode is what > userspace plans to feed into the pipe, and the adjusted mode is what > actually lands in the sink. Yeah there's some hilarity in the vblank > code which somehow presumes that the vblank counter works with the > adjusted mode because that's what i915 needs. The problem with clipping planes to the user size is that the driver is free to frob the mode a bit to line it up with hardware constraints. So what the user requested might be a few pixels off compared to what the hardware will end up using, and then if you configure the plane blindly using the coordinates clipped against the user size, the hardware may get somewhat upset. > > It's that fundamental assumption that we break by making the pipe_src > stuff official and which is the part that freaks me out a bit. Yeah there's definitely some dangers with these properties. The biggest being when one master sets the properties, and then another master which doesn't know anything about these properties takes over. The result might be a failed modeset an then the user doesn't see anything. This is one reason why I'd prefer that we'd maintain the state per-master. For fbdev the "reset everything to default" trick seems sufficient but I'm not sure I'd like to that to actual users. > > > The other drivers are something I would not touch. Given how many places > > we had to frob in i915 I'm somewhat sure I'd not like what I see there > > and then any patch I might cook up would be too half assed to satisfy my > > quality standards anyway. > > Yeah, other drivers only need to be audited I think once they start > supporting the pipe_src stuff. But I think the core+helpers should be > able to cope properly. > > > As far as always filling the crtc->w,h always goes, I'm not sure that's > > the best idea anyway since we still need the "0 is special" handling. > > Well, we could fill them out, but then we definitely need a flag or > > something to indicate that they came from the mode and not the > > properties, so that we know whether we should overwrite them from > > with {h,v}display during a subsquent modeset or if they should keep > > their current value. > > Hm, I guess we can keep that implicit meaning, but then we need a > small helper to get at the crtc viewport, e.g. drm_crtc_viewport_rect > or so. That would also be an excellent place to document this trickery > properly. Yeah such small helpers is what I had in mind when suggesting this originally. > Oh and: Such drm changes _really_ must be split out into separate prep > patches cc: dri-devel. Indeed.
On Thu, Aug 14, 2014 at 6:45 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Aug 14, 2014 at 06:07:44PM +0200, Daniel Vetter wrote: >> On Thu, Aug 14, 2014 at 5:45 PM, Ville Syrjälä >> <ville.syrjala@linux.intel.com> wrote: >> > My quick grep audit tells me the viewport check and >> > drm_primary_helper_update() are the only places in the core that care. >> > The latter is rather dubious anyway since the clipping should be done >> > against the adjusted mode and not the user specified mode, so anyone >> > using that is already dancing on thin ice. >> >> My understanding has always been that the requested mode is what >> userspace plans to feed into the pipe, and the adjusted mode is what >> actually lands in the sink. Yeah there's some hilarity in the vblank >> code which somehow presumes that the vblank counter works with the >> adjusted mode because that's what i915 needs. > > The problem with clipping planes to the user size is that the driver is > free to frob the mode a bit to line it up with hardware constraints. So > what the user requested might be a few pixels off compared to what the > hardware will end up using, and then if you configure the plane > blindly using the coordinates clipped against the user size, the > hardware may get somewhat upset. Hm, I've thought we didn't do that yet, but only frobbed the adjusted mode to make it fit our requirements for e.g interlaced stuff. I don't think it would be good if we start doing that since there's no way for userspace to be aware of the resulting single-pixel border of garbage. _If_ we need to do that I think we should frob modes when adding them to the connector mode list. >> It's that fundamental assumption that we break by making the pipe_src >> stuff official and which is the part that freaks me out a bit. > > Yeah there's definitely some dangers with these properties. The biggest > being when one master sets the properties, and then another master which > doesn't know anything about these properties takes over. The result > might be a failed modeset an then the user doesn't see anything. > > This is one reason why I'd prefer that we'd maintain the state > per-master. For fbdev the "reset everything to default" trick seems > sufficient but I'm not sure I'd like to that to actual users. Yeah, this is unsolved territory. Not sure whether attaching the state to the master should be helpful, or whether we should care at all. For a generic distro we should boot-up sane-ish, and userspace can always grab the full property picture and restore that if all else fails. The fbcon restoring is really just for developers imo. >> > The other drivers are something I would not touch. Given how many places >> > we had to frob in i915 I'm somewhat sure I'd not like what I see there >> > and then any patch I might cook up would be too half assed to satisfy my >> > quality standards anyway. >> >> Yeah, other drivers only need to be audited I think once they start >> supporting the pipe_src stuff. But I think the core+helpers should be >> able to cope properly. >> >> > As far as always filling the crtc->w,h always goes, I'm not sure that's >> > the best idea anyway since we still need the "0 is special" handling. >> > Well, we could fill them out, but then we definitely need a flag or >> > something to indicate that they came from the mode and not the >> > properties, so that we know whether we should overwrite them from >> > with {h,v}display during a subsquent modeset or if they should keep >> > their current value. >> >> Hm, I guess we can keep that implicit meaning, but then we need a >> small helper to get at the crtc viewport, e.g. drm_crtc_viewport_rect >> or so. That would also be an excellent place to document this trickery >> properly. > > Yeah such small helpers is what I had in mind when suggesting this > originally. > >> Oh and: Such drm changes _really_ must be split out into separate prep >> patches cc: dri-devel. > > Indeed. Sounds like we have a rough plan. -Daniel
On Thu, Aug 14, 2014 at 07:36:13PM +0200, Daniel Vetter wrote: > On Thu, Aug 14, 2014 at 6:45 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Thu, Aug 14, 2014 at 06:07:44PM +0200, Daniel Vetter wrote: > >> On Thu, Aug 14, 2014 at 5:45 PM, Ville Syrjälä > >> <ville.syrjala@linux.intel.com> wrote: > >> > My quick grep audit tells me the viewport check and > >> > drm_primary_helper_update() are the only places in the core that care. > >> > The latter is rather dubious anyway since the clipping should be done > >> > against the adjusted mode and not the user specified mode, so anyone > >> > using that is already dancing on thin ice. > >> > >> My understanding has always been that the requested mode is what > >> userspace plans to feed into the pipe, and the adjusted mode is what > >> actually lands in the sink. Yeah there's some hilarity in the vblank > >> code which somehow presumes that the vblank counter works with the > >> adjusted mode because that's what i915 needs. > > > > The problem with clipping planes to the user size is that the driver is > > free to frob the mode a bit to line it up with hardware constraints. So > > what the user requested might be a few pixels off compared to what the > > hardware will end up using, and then if you configure the plane > > blindly using the coordinates clipped against the user size, the > > hardware may get somewhat upset. > > Hm, I've thought we didn't do that yet, but only frobbed the adjusted > mode to make it fit our requirements for e.g interlaced stuff. I don't > think it would be good if we start doing that since there's no way for > userspace to be aware of the resulting single-pixel border of garbage. > > _If_ we need to do that I think we should frob modes when adding them > to the connector mode list. Sure but the user can supply any mode, doesn't have to be on any list. And the only sane rule for the frobbing would be that you can (slightly) reduce hdisp/vdisp but never expand them so that there will never be any extra garbage exposed (and the FB might not be big enough anyway). But even reducing hdisp/vdisp by one pixel can be enough to anger the hardware if a plane then extends one pixel into the blanking. This isn't much of a problem for i915 though. The hardware is generally good enough to not need it. Double wide and (s)dvo/lvds gang mode are the only exception that comes to mind. Even there we just need to make pipe src width even, but still that's something we have to account when clipping planes. On older hardware there were generally more restrictions eg. some legacy baggage from VGA days which required horizontal timings to be multiples of 8. I also "fondly" remember much more magic timing restrictions in certain pieces hardware which were something close to "if (foo*bar % this == that) frob else don't". IMO these kinds of restrictions are too magic to make rejecting the mode an option, so frobbing is the lesser of two evils.
On Thu, Aug 14, 2014 at 7:58 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > Sure but the user can supply any mode, doesn't have to be on any list. > And the only sane rule for the frobbing would be that you can (slightly) > reduce hdisp/vdisp but never expand them so that there will never be any > extra garbage exposed (and the FB might not be big enough anyway). But > even reducing hdisp/vdisp by one pixel can be enough to anger the > hardware if a plane then extends one pixel into the blanking. > > This isn't much of a problem for i915 though. The hardware is generally > good enough to not need it. Double wide and (s)dvo/lvds gang mode are > the only exception that comes to mind. Even there we just need to make > pipe src width even, but still that's something we have to account > when clipping planes. > > On older hardware there were generally more restrictions eg. some > legacy baggage from VGA days which required horizontal timings to > be multiples of 8. I also "fondly" remember much more magic timing > restrictions in certain pieces hardware which were something close > to "if (foo*bar % this == that) frob else don't". IMO these kinds of > restrictions are too magic to make rejecting the mode an option, > so frobbing is the lesser of two evils. Imo the mode list we provide should be reasonable for everyone, and if you start to add your own modes then I expect the user to do that adjusting for us. Nowadays there should be very few cases where we don't provide decent mode lists and where it's not a super-special embedded thing where you need to configure everything yourself anyway. So I don't think we should ever adjust the input region for a crtc. -Daniel
On Thu, Aug 14, 2014 at 08:33:23PM +0200, Daniel Vetter wrote: > On Thu, Aug 14, 2014 at 7:58 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > Sure but the user can supply any mode, doesn't have to be on any list. > > And the only sane rule for the frobbing would be that you can (slightly) > > reduce hdisp/vdisp but never expand them so that there will never be any > > extra garbage exposed (and the FB might not be big enough anyway). But > > even reducing hdisp/vdisp by one pixel can be enough to anger the > > hardware if a plane then extends one pixel into the blanking. > > > > This isn't much of a problem for i915 though. The hardware is generally > > good enough to not need it. Double wide and (s)dvo/lvds gang mode are > > the only exception that comes to mind. Even there we just need to make > > pipe src width even, but still that's something we have to account > > when clipping planes. > > > > On older hardware there were generally more restrictions eg. some > > legacy baggage from VGA days which required horizontal timings to > > be multiples of 8. I also "fondly" remember much more magic timing > > restrictions in certain pieces hardware which were something close > > to "if (foo*bar % this == that) frob else don't". IMO these kinds of > > restrictions are too magic to make rejecting the mode an option, > > so frobbing is the lesser of two evils. > > Imo the mode list we provide should be reasonable for everyone, and if > you start to add your own modes then I expect the user to do that > adjusting for us. Nowadays there should be very few cases where we > don't provide decent mode lists and where it's not a super-special > embedded thing where you need to configure everything yourself anyway. > So I don't think we should ever adjust the input region for a crtc. That's fine for decent hardware. But if/when I write a driver for old junk I'm probably going to frob no matter what anyone says :)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index f09b752..328efac 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -5169,3 +5169,10 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, supported_rotations); } EXPORT_SYMBOL(drm_mode_create_rotation_property); + +struct drm_property *drm_mode_create_crtc_size_property(struct drm_device *dev) +{ + return drm_property_create_range(dev, 0, "crtc size", + 0, UINT_MAX); +} +EXPORT_SYMBOL(drm_mode_create_crtc_size_property); diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 63d7b8e..6eb1949 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -307,6 +307,13 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper) struct drm_crtc *crtc = mode_set->crtc; int ret; + if (dev->mode_config.crtc_size_property) { + crtc->crtc_w = crtc->crtc_h = 0; + drm_object_property_set_value(&crtc->base, + dev->mode_config.crtc_size_property, + 0); + } + if (crtc->funcs->cursor_set) { ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0); if (ret) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a75d1a0..7c417fc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10183,6 +10183,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, pipe_config->pipe_src_w = pipe_config->requested_mode.crtc_hdisplay; pipe_config->pipe_src_h = pipe_config->requested_mode.crtc_vdisplay; + /* Set the Pipe Src values as per the crtc size property values */ + if (crtc->crtc_w && crtc->crtc_h) { + pipe_config->pipe_src_w = crtc->crtc_w; + pipe_config->pipe_src_h = crtc->crtc_h; + } + encoder_retry: /* Ensure the port clock defaults are reset when retrying. */ pipe_config->port_clock = 0; @@ -11438,11 +11444,67 @@ out_config: return ret; } +static int intel_crtc_set_property(struct drm_crtc *crtc, + struct drm_property *prop, + uint64_t val) +{ + struct drm_device *dev = crtc->dev; + int ret = -ENOENT; + + if (prop == dev->mode_config.crtc_size_property) { + int old_width, old_height; + int new_width = (int)((val >> 16) & 0xffff); + int new_height = (int)(val & 0xffff); + const struct drm_framebuffer *fb = crtc->primary->fb; + + if ((new_width == crtc->crtc_w) && + (new_height == crtc->crtc_h)) + return 0; + + /* Check if property is resetted, so just return */ + if ((new_width == 0) && (new_height) == 0) { + crtc->crtc_w = crtc->crtc_h = 0; + /* FIXME, Is modeset required here ?. Actually the + current FB may not be compatible with the mode */ + return 0; + } else if ((new_width == 0) || (new_height == 0)) + return -EINVAL; + + /* 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; + } + + old_width = crtc->crtc_w; + old_height = crtc->crtc_h; + + crtc->crtc_w = new_width; + crtc->crtc_h = new_height; + + /* Currently do a modeset always, this will also + * enable & configure the Panel fitter accordingly */ + ret = intel_crtc_restore_mode(crtc); + if (ret) { + crtc->crtc_w = old_width; + crtc->crtc_h = old_height; + } + } + + return ret; +} + static const struct drm_crtc_funcs intel_crtc_funcs = { .gamma_set = intel_crtc_gamma_set, .set_config = intel_crtc_set_config, .destroy = intel_crtc_destroy, .page_flip = intel_crtc_page_flip, + .set_property = intel_crtc_set_property, }; static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv, @@ -11963,6 +12025,16 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe); + + if (!dev->mode_config.crtc_size_property) + dev->mode_config.crtc_size_property = + drm_mode_create_crtc_size_property(dev); + + if (dev->mode_config.crtc_size_property) + drm_object_attach_property(&intel_crtc->base.base, + dev->mode_config.crtc_size_property, + 0); + return; fail: diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 0375d75..b409003 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -354,6 +354,11 @@ struct drm_crtc { bool invert_dimensions; int x, y; + + /* Gets set only through the crtc_size property */ + int crtc_w; + int crtc_h; + const struct drm_crtc_funcs *funcs; /* CRTC gamma size for reporting to userspace */ @@ -826,6 +831,7 @@ struct drm_mode_config { struct drm_property *path_property; struct drm_property *plane_type_property; struct drm_property *rotation_property; + struct drm_property *crtc_size_property; /* DVI-I properties */ struct drm_property *dvi_i_subconnector_property; @@ -1137,6 +1143,7 @@ extern int drm_format_vert_chroma_subsampling(uint32_t format); extern const char *drm_get_format_name(uint32_t format); extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, unsigned int supported_rotations); +extern struct drm_property *drm_mode_create_crtc_size_property(struct drm_device *dev); extern unsigned int drm_rotation_simplify(unsigned int rotation, unsigned int supported_rotations);