On Tue, May 2, 2017 at 11:29 AM, Jose Abreu Jose.Abreu@synopsys.com wrote:
On 02-05-2017 09:48, Daniel Vetter wrote:
On Wed, Apr 26, 2017 at 11:48:34AM +0100, Jose Abreu wrote:
Some crtc's may have restrictions in the mode they can display. In this patch a new callback (crtc->mode_valid()) is introduced that is called at the same stage of connector->mode_valid() callback.
This shall be implemented if the crtc has some sort of restriction so that we don't probe modes that will fail in the commit() stage. For example: A given crtc may be responsible to set a clock value. If the clock can not produce all the values for the available modes then this callback can be used to restrict the number of probbed modes to only the ones that can be displayed.
If the crtc does not implement the callback then the behaviour will remain the same. Also, for a given set of crtcs that can be bound to the connector, if at least one can display the mode then the mode will be probbed.
Signed-off-by: Jose Abreu joabreu@synopsys.com Cc: Carlos Palminha palminha@synopsys.com Cc: Alexey Brodkin abrodkin@synopsys.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Dave Airlie airlied@linux.ie
Not sure this is useful, since you still have to duplicate the exact same check into your ->mode_fixup hook. That seems to make things even more confusing.
Yeah, in arcpgu I had to duplicate the code in ->atomic_check.
Also this doesn't update the various kerneldoc comments. For the existing hooks. Since this topic causes so much confusion, I don't think a half-solution will help, but has some good potential to make things worse.
I only documented the callback in drm_modeset_helper_vtables.h.
Despite all of this, I think it doesn't makes sense delivering modes to userspace which can never be used.
This is really annoying in arcpgu. Imagine: I try to use mpv to play a video, the full set of modes from EDID were probed so if I just start mpv it will pick the native mode of the TV instead of the one that is supported, so mpv will fail to play. I know the value of clock which will work (so I know what mode shall be used), but a normal user which is not aware of the HW will have to cycle through the list of modes and try them all until it hits one that works. Its really boring.
For the modes that user specifies manually there is nothing we can do, but we should not trick users into thinking that a given mode is supported when it will always fail at commit.
Yes, you are supposed to filter these out in ->mode_valid. But my stance is that only adding a half-baked support for a new callback to the core isn't going to make life easier for drivers, it will just add to the confusion. There's already piles of docs for both @mode_valid and @mode_fixup hooks explaining this, I don't want to make the documentation even more complex. And half-baked crtc checking is _much_ easier to implement in the driver directly (e.g. i915 checks for crtc constraints since forever, as do the other big x86 drivers).
So all taken together, if we add a ->mode_valid to crtcs, then imo we should do it right and actually make life easier for drivers. A good proof would be if your patch would allow us to drop a lot of the lenghty language from the @mode_valid hooks. -Daniel