| Message ID | 1487100302-9445-2-git-send-email-john.stultz@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add mode_valid drm_crtc_helper_funcs for HiKey | expand |
On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@linaro.org> wrote: > Currently, on the hikey board, we have the adv7511 bridge wired > up to the kirin ade drm driver. Unfortunately, the kirin ade > core cannot generate accurate byteclocks for all pixel clock > values. > > Thus if a mode clock is selected that we cannot calculate a > matching byteclock, the device will boot with a blank screen. > > Unfortunately, currently the only place we can properly check > potential modes for this issue in the connector mode_valid > helper. Again, hikey uses the adv7511 bridge, which is shared > between a number of different devices, so its improper to put > restrictions caused by the kirin drm driver in the adv7511 > logic. > > So this patch tries to correct for that, by adding some > infrastructure so that the drm_crtc_helper_funcs can optionally > implement a mode_valid check, so that the probe helpers can > check to make sure there are not any restrictions at the crtc > level as well. > > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: David Airlie <airlied@linux.ie> > Cc: Rob Clark <robdclark@gmail.com> > Cc: Xinliang Liu <xinliang.liu@linaro.org> > Cc: Xinliang Liu <z.liuxinliang@hisilicon.com> > Cc: Rongrong Zou <zourongrong@gmail.com> > Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> > Cc: Chen Feng <puck.chen@hisilicon.com> > Cc: Archit Taneja <architt@codeaurora.org> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: John Stultz <john.stultz@linaro.org> So I'm going to be super-annoying here and ask for a complete solution. This here is defacto what ever driver already does (or has too), but it doesn't really solve the overall issue of having entirely separate validation paths for probe and atomic_check paths. I think if we wan to solve this, we need to solve this properly, with a generic solution. That would mean: - still in helpers, to make it all opt-int - covers crtc and encoders and bridges - allows you to implement the current mode_valid in terms of the new stuff (maybe as a default hook) - allows you to implement the current assortment of mode_fixup and/or atomic_check in terms of the new stuff, or at least to not have to duplicate logic in there Docs for all this, especially updating all the warnings on how to use the existing hooks correctly. I think just pushing this specific case into the helpers, without rethinking the overall big picture, isn't gaining us all that much. For just this I'd say just put it into drivers, until we have some good clue about how the helpers should look like (maybe this time is the time? ...). Cheers, Daniel > --- > drivers/gpu/drm/drm_probe_helper.c | 24 ++++++++++++++++++++++++ > include/drm/drm_modeset_helper_vtables.h | 26 ++++++++++++++++++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index cf8f012..a808348 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -170,6 +170,28 @@ drm_connector_detect(struct drm_connector *connector, bool force) > connector_status_connected; > } > > +static enum drm_mode_status > +drm_connector_check_crtc_modes(struct drm_connector *connector, > + struct drm_display_mode *mode) > +{ > + struct drm_device *dev = connector->dev; > + const struct drm_crtc_helper_funcs *crtc_funcs; > + struct drm_crtc *c; > + > + if (mode->status != MODE_OK) > + return mode->status; > + > + /* Check all the crtcs on a connector to make sure the mode is valid */ > + drm_for_each_crtc(c, dev) { > + crtc_funcs = c->helper_private; > + if (crtc_funcs && crtc_funcs->mode_valid) > + mode->status = crtc_funcs->mode_valid(c, mode); > + if (mode->status != MODE_OK) > + break; > + } > + return mode->status; > +} > + > /** > * drm_helper_probe_single_connector_modes - get complete set of display modes > * @connector: connector to probe > @@ -338,6 +360,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, > if (mode->status == MODE_OK && connector_funcs->mode_valid) > mode->status = connector_funcs->mode_valid(connector, > mode); > + > + mode->status = drm_connector_check_crtc_modes(connector, mode); > } > > prune: > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index 69c3974..53ca0e4 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -105,6 +105,32 @@ struct drm_crtc_helper_funcs { > void (*commit)(struct drm_crtc *crtc); > > /** > + * @mode_valid: > + * > + * Callback to validate a mode for a crtc, irrespective of the > + * specific display configuration. > + * > + * This callback is used by the probe helpers to filter the mode list > + * (which is usually derived from the EDID data block from the sink). > + * See e.g. drm_helper_probe_single_connector_modes(). > + * > + * NOTE: > + * > + * This only filters the mode list supplied to userspace in the > + * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and > + * ask the kernel to use them. It this case the atomic helpers or legacy > + * CRTC helpers will not call this function. Drivers therefore must > + * still fully validate any mode passed in in a modeset request. > + * > + * RETURNS: > + * > + * Either MODE_OK or one of the failure reasons in enum > + * &drm_mode_status. > + */ > + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, > + struct drm_display_mode *mode); > + > + /** > * @mode_fixup: > * > * This callback is used to validate a mode. The parameter mode is the > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Feb 14, 2017 at 11:38 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@linaro.org> wrote: >> Currently, on the hikey board, we have the adv7511 bridge wired >> up to the kirin ade drm driver. Unfortunately, the kirin ade >> core cannot generate accurate byteclocks for all pixel clock >> values. >> >> Thus if a mode clock is selected that we cannot calculate a >> matching byteclock, the device will boot with a blank screen. >> >> Unfortunately, currently the only place we can properly check >> potential modes for this issue in the connector mode_valid >> helper. Again, hikey uses the adv7511 bridge, which is shared >> between a number of different devices, so its improper to put >> restrictions caused by the kirin drm driver in the adv7511 >> logic. >> >> So this patch tries to correct for that, by adding some >> infrastructure so that the drm_crtc_helper_funcs can optionally >> implement a mode_valid check, so that the probe helpers can >> check to make sure there are not any restrictions at the crtc >> level as well. >> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: Sean Paul <seanpaul@chromium.org> >> Cc: David Airlie <airlied@linux.ie> >> Cc: Rob Clark <robdclark@gmail.com> >> Cc: Xinliang Liu <xinliang.liu@linaro.org> >> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com> >> Cc: Rongrong Zou <zourongrong@gmail.com> >> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> >> Cc: Chen Feng <puck.chen@hisilicon.com> >> Cc: Archit Taneja <architt@codeaurora.org> >> Cc: dri-devel@lists.freedesktop.org >> Signed-off-by: John Stultz <john.stultz@linaro.org> > > So I'm going to be super-annoying here and ask for a complete > solution. This here is defacto what ever driver already does (or has > too), but it doesn't really solve the overall issue of having entirely > separate validation paths for probe and atomic_check paths. I think if > we wan to solve this, we need to solve this properly, with a generic > solution. That would mean: > - still in helpers, to make it all opt-int Just to be clear, I believe what I proposed is opt-in, but I assume you want that in addition to the following, right? > - covers crtc and encoders and bridges So you'd like similar mode_valid() calls in the crtc/encoder/bridge helpers? right? > - allows you to implement the current mode_valid in terms of the new > stuff (maybe as a default hook) This bit I'm not sure I'm following. The current drm_connector's mode_valid in terms of a new mode_valid call that also looks at crtc/encoder/bridges? Or do you mean something else? > - allows you to implement the current assortment of mode_fixup and/or > atomic_check in terms of the new stuff, or at least to not have to > duplicate logic in there This is over my head, so I'll have to research to better understand. > Docs for all this, especially updating all the warnings on how to use > the existing hooks correctly. That's fair. > I think just pushing this specific case into the helpers, without > rethinking the overall big picture, isn't gaining us all that much. > For just this I'd say just put it into drivers, until we have some Not following here. What do you mean by "put it into drivers"? Where? thanks -john
On Tue, Feb 14, 2017 at 01:03:27PM -0800, John Stultz wrote: > On Tue, Feb 14, 2017 at 12:22 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, Feb 14, 2017 at 11:45:54AM -0800, John Stultz wrote: > >> > >> Not following here. What do you mean by "put it into drivers"? Where? > > > > In your driver callback for ->mode_valid, call into a shared function to > > validate CRTC limits. Which you also call from the crtc's ->mode_fixup > > function. > > So bascially have the adv7511 driver's mode_valid() have a special > callback to the kirin (and freedreno, and whatever other) drm driver > to check the modes? Or I guess the drm driver that uses that bridge > should register something w/ the adv7511 code? > > Part of my confusion here is that the main issue is that its not just > one driver I'm dealing with, and they're all really just tied together > via device tree, so I'm not sure how to special case it in just one of > the drivers. This sounds you want to fix this for bridges, but your patch only adds a crtc callback? > > In short my complain here is that this is only a partial solution of the > > larger problem, specific for your driver. That kind of code is better put > > into drivers, until we have a clear understanding to type up something > > complete in the helpers. Shared code is imo overrated :-) > > Yea, apologies for my not seeing the larger problem at first (its > still a bit hazy, really), part of this submission is just trying to > get something to throw darts at and figure out the right thing. > > But I'll try to figure out another approach here. Latest kerneldoc in drm-tip should explain this, not sure you didn't spot it or looked at an old kernel version. For drm docs, _always_ look at drm-tip, they're moving real fast :-) https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#modeset-helper-reference-for-common-vtables See e.g. drm_crtc_helpers_funcs->mode_fixup docs, it's there. There's explanations sprinkled at various places (mode_valid, plus the different helper funcs), probably good to first hunt these all down. Then read a bunch of driver callbacks so you have a better idea of the patterns of checks, otoh _lots_ of kms drivers get this wrong :( Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Tue, Feb 14, 2017 at 01:07:21PM -0800, John Stultz wrote: > On Tue, Feb 14, 2017 at 12:32 PM, Daniel Stone <daniel@fooishbar.org> wrote: > > Hi John, > > > > On 14 February 2017 at 19:25, John Stultz <john.stultz@linaro.org> wrote: > >> +static enum drm_mode_status > >> +drm_connector_check_crtc_modes(struct drm_connector *connector, > >> + struct drm_display_mode *mode) > >> +{ > >> + struct drm_device *dev = connector->dev; > >> + const struct drm_crtc_helper_funcs *crtc_funcs; > >> + struct drm_crtc *c; > >> + > >> + if (mode->status != MODE_OK) > >> + return mode->status; > >> + > >> + /* Check all the crtcs on a connector to make sure the mode is valid */ > >> + drm_for_each_crtc(c, dev) { > >> + crtc_funcs = c->helper_private; > >> + if (crtc_funcs && crtc_funcs->mode_valid) > >> + mode->status = crtc_funcs->mode_valid(c, mode); > >> + if (mode->status != MODE_OK) > >> + break; > >> + } > >> + return mode->status; > >> +} > > > > Hm, that's unfortunate: it limits the mode list for every connector, > > to those which are supported by every single CRTC. So if you have one > > CRTC serving low-res LVDS, and another serving higher-res HDMI, > > suddenly you can't get bigger modes on HDMI. The idea seems sound > > enough, but a little more nuance might be good ... > > Yea. That is not my intent at all I'm just trying to get the drm_crtc > attached to the connector that we're getting the EDID mode lines from. > I had tried going connector->encoder->crtc, but at the time this is > called, the encoder is null. So Rob suggested the for_each_crtc(), and > I guess I mistook that for being each crtc on the connector. > > Thanks for pointing out this issue. From Daniel's feedback it looks > like I need to start over from scratch though, so little worry this > implementation will go much further. Well your idea was somewhat right, but logic inverted. In ->mode_valid we need to check whether any encoder/crtc combo could support the mode. Which means you need to reject it only when there's no encoder/crtc combo that could support the mode (you reject it if there's only one crtc which can't handle it). On the ->mode_fixup/atomic_check callbacks otoh we need to reject the mode when it's not suitable for the current chain (as described in the atomic states). That little difference is why this is not an entirely trivial problem, and yes there's lots of hw out there where this matters. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Tue, Feb 14, 2017 at 4:49 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Feb 14, 2017 at 01:07:21PM -0800, John Stultz wrote: >> On Tue, Feb 14, 2017 at 12:32 PM, Daniel Stone <daniel@fooishbar.org> wrote: >> > Hi John, >> > >> > On 14 February 2017 at 19:25, John Stultz <john.stultz@linaro.org> wrote: >> >> +static enum drm_mode_status >> >> +drm_connector_check_crtc_modes(struct drm_connector *connector, >> >> + struct drm_display_mode *mode) >> >> +{ >> >> + struct drm_device *dev = connector->dev; >> >> + const struct drm_crtc_helper_funcs *crtc_funcs; >> >> + struct drm_crtc *c; >> >> + >> >> + if (mode->status != MODE_OK) >> >> + return mode->status; >> >> + >> >> + /* Check all the crtcs on a connector to make sure the mode is valid */ >> >> + drm_for_each_crtc(c, dev) { >> >> + crtc_funcs = c->helper_private; >> >> + if (crtc_funcs && crtc_funcs->mode_valid) >> >> + mode->status = crtc_funcs->mode_valid(c, mode); >> >> + if (mode->status != MODE_OK) >> >> + break; >> >> + } >> >> + return mode->status; >> >> +} >> > >> > Hm, that's unfortunate: it limits the mode list for every connector, >> > to those which are supported by every single CRTC. So if you have one >> > CRTC serving low-res LVDS, and another serving higher-res HDMI, >> > suddenly you can't get bigger modes on HDMI. The idea seems sound >> > enough, but a little more nuance might be good ... >> >> Yea. That is not my intent at all I'm just trying to get the drm_crtc >> attached to the connector that we're getting the EDID mode lines from. >> I had tried going connector->encoder->crtc, but at the time this is >> called, the encoder is null. So Rob suggested the for_each_crtc(), and >> I guess I mistook that for being each crtc on the connector. >> >> Thanks for pointing out this issue. From Daniel's feedback it looks >> like I need to start over from scratch though, so little worry this >> implementation will go much further. > > Well your idea was somewhat right, but logic inverted. In ->mode_valid we > need to check whether any encoder/crtc combo could support the mode. Which > means you need to reject it only when there's no encoder/crtc combo that > could support the mode (you reject it if there's only one crtc which can't > handle it). sorry, I was probably not expressing my idea to John very well on IRC, but yeah, the idea was for this to only reject modes that are impossible for all CRTCs (so a bit different than the case that the atomic_check callbacks would be validating) and btw, yeah, this is specifically about fixing things for bridges or situations where the connector is shared across multiple drivers. It isn't really something we can solve in-driver. Maybe driver provided callbacks to the bridge would do the trick, but that seemed a bit weird. The simple idea was to give the bridge a way to figure out things that were completely unpossible and let the driver figure out how to make the things that are possible work somehow. BR, -R
I thought about this some more, I think what we need to fix this mess properly is: - mode_valid helper callbacks for crtc, encoder, bridges, with the same interface as for connectors. - calling all these new mode_valid hooks from the probe helpers, but with the restriction that we only reject a mode if all possible crtc/encoders combos reject it. We need to filter by possible_encoders/crtcs for these checks. Bridges have a fixed routing to their encoder, so those are easy. - add calls to mode_valid in the atomic helpers, right before we call mode_fixup or atomic_check in drm_atomic_helper_check_modesets. - convert drivers to move code from mode_fixup into mode_valid wherever possible, to make sure we can share as much of the check logic between probe and atomic comit code. - update docs for all the hooks, plus update the overview sections accordingly. I think this should give us a good approximation of nirvana. For I long time I thought we could get away without adding mode_valid everywhere, but in the probe paths we really can't fake the adjusted_mode (and other atomic state), so adding dedicated hooks which are called from both places is probably the only option. -Daniel On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@linaro.org> wrote: > Currently, on the hikey board, we have the adv7511 bridge wired > up to the kirin ade drm driver. Unfortunately, the kirin ade > core cannot generate accurate byteclocks for all pixel clock > values. > > Thus if a mode clock is selected that we cannot calculate a > matching byteclock, the device will boot with a blank screen. > > Unfortunately, currently the only place we can properly check > potential modes for this issue in the connector mode_valid > helper. Again, hikey uses the adv7511 bridge, which is shared > between a number of different devices, so its improper to put > restrictions caused by the kirin drm driver in the adv7511 > logic. > > So this patch tries to correct for that, by adding some > infrastructure so that the drm_crtc_helper_funcs can optionally > implement a mode_valid check, so that the probe helpers can > check to make sure there are not any restrictions at the crtc > level as well. > > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: David Airlie <airlied@linux.ie> > Cc: Rob Clark <robdclark@gmail.com> > Cc: Xinliang Liu <xinliang.liu@linaro.org> > Cc: Xinliang Liu <z.liuxinliang@hisilicon.com> > Cc: Rongrong Zou <zourongrong@gmail.com> > Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> > Cc: Chen Feng <puck.chen@hisilicon.com> > Cc: Archit Taneja <architt@codeaurora.org> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > drivers/gpu/drm/drm_probe_helper.c | 24 ++++++++++++++++++++++++ > include/drm/drm_modeset_helper_vtables.h | 26 ++++++++++++++++++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index cf8f012..a808348 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -170,6 +170,28 @@ drm_connector_detect(struct drm_connector *connector, bool force) > connector_status_connected; > } > > +static enum drm_mode_status > +drm_connector_check_crtc_modes(struct drm_connector *connector, > + struct drm_display_mode *mode) > +{ > + struct drm_device *dev = connector->dev; > + const struct drm_crtc_helper_funcs *crtc_funcs; > + struct drm_crtc *c; > + > + if (mode->status != MODE_OK) > + return mode->status; > + > + /* Check all the crtcs on a connector to make sure the mode is valid */ > + drm_for_each_crtc(c, dev) { > + crtc_funcs = c->helper_private; > + if (crtc_funcs && crtc_funcs->mode_valid) > + mode->status = crtc_funcs->mode_valid(c, mode); > + if (mode->status != MODE_OK) > + break; > + } > + return mode->status; > +} > + > /** > * drm_helper_probe_single_connector_modes - get complete set of display modes > * @connector: connector to probe > @@ -338,6 +360,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, > if (mode->status == MODE_OK && connector_funcs->mode_valid) > mode->status = connector_funcs->mode_valid(connector, > mode); > + > + mode->status = drm_connector_check_crtc_modes(connector, mode); > } > > prune: > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index 69c3974..53ca0e4 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -105,6 +105,32 @@ struct drm_crtc_helper_funcs { > void (*commit)(struct drm_crtc *crtc); > > /** > + * @mode_valid: > + * > + * Callback to validate a mode for a crtc, irrespective of the > + * specific display configuration. > + * > + * This callback is used by the probe helpers to filter the mode list > + * (which is usually derived from the EDID data block from the sink). > + * See e.g. drm_helper_probe_single_connector_modes(). > + * > + * NOTE: > + * > + * This only filters the mode list supplied to userspace in the > + * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and > + * ask the kernel to use them. It this case the atomic helpers or legacy > + * CRTC helpers will not call this function. Drivers therefore must > + * still fully validate any mode passed in in a modeset request. > + * > + * RETURNS: > + * > + * Either MODE_OK or one of the failure reasons in enum > + * &drm_mode_status. > + */ > + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, > + struct drm_display_mode *mode); > + > + /** > * @mode_fixup: > * > * This callback is used to validate a mode. The parameter mode is the > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index cf8f012..a808348 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -170,6 +170,28 @@ drm_connector_detect(struct drm_connector *connector, bool force) connector_status_connected; } +static enum drm_mode_status +drm_connector_check_crtc_modes(struct drm_connector *connector, + struct drm_display_mode *mode) +{ + struct drm_device *dev = connector->dev; + const struct drm_crtc_helper_funcs *crtc_funcs; + struct drm_crtc *c; + + if (mode->status != MODE_OK) + return mode->status; + + /* Check all the crtcs on a connector to make sure the mode is valid */ + drm_for_each_crtc(c, dev) { + crtc_funcs = c->helper_private; + if (crtc_funcs && crtc_funcs->mode_valid) + mode->status = crtc_funcs->mode_valid(c, mode); + if (mode->status != MODE_OK) + break; + } + return mode->status; +} + /** * drm_helper_probe_single_connector_modes - get complete set of display modes * @connector: connector to probe @@ -338,6 +360,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, if (mode->status == MODE_OK && connector_funcs->mode_valid) mode->status = connector_funcs->mode_valid(connector, mode); + + mode->status = drm_connector_check_crtc_modes(connector, mode); } prune: diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 69c3974..53ca0e4 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -105,6 +105,32 @@ struct drm_crtc_helper_funcs { void (*commit)(struct drm_crtc *crtc); /** + * @mode_valid: + * + * Callback to validate a mode for a crtc, irrespective of the + * specific display configuration. + * + * This callback is used by the probe helpers to filter the mode list + * (which is usually derived from the EDID data block from the sink). + * See e.g. drm_helper_probe_single_connector_modes(). + * + * NOTE: + * + * This only filters the mode list supplied to userspace in the + * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and + * ask the kernel to use them. It this case the atomic helpers or legacy + * CRTC helpers will not call this function. Drivers therefore must + * still fully validate any mode passed in in a modeset request. + * + * RETURNS: + * + * Either MODE_OK or one of the failure reasons in enum + * &drm_mode_status. + */ + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, + struct drm_display_mode *mode); + + /** * @mode_fixup: * * This callback is used to validate a mode. The parameter mode is the
Currently, on the hikey board, we have the adv7511 bridge wired up to the kirin ade drm driver. Unfortunately, the kirin ade core cannot generate accurate byteclocks for all pixel clock values. Thus if a mode clock is selected that we cannot calculate a matching byteclock, the device will boot with a blank screen. Unfortunately, currently the only place we can properly check potential modes for this issue in the connector mode_valid helper. Again, hikey uses the adv7511 bridge, which is shared between a number of different devices, so its improper to put restrictions caused by the kirin drm driver in the adv7511 logic. So this patch tries to correct for that, by adding some infrastructure so that the drm_crtc_helper_funcs can optionally implement a mode_valid check, so that the probe helpers can check to make sure there are not any restrictions at the crtc level as well. Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Sean Paul <seanpaul@chromium.org> Cc: David Airlie <airlied@linux.ie> Cc: Rob Clark <robdclark@gmail.com> Cc: Xinliang Liu <xinliang.liu@linaro.org> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com> Cc: Rongrong Zou <zourongrong@gmail.com> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> Cc: Chen Feng <puck.chen@hisilicon.com> Cc: Archit Taneja <architt@codeaurora.org> Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz <john.stultz@linaro.org> --- drivers/gpu/drm/drm_probe_helper.c | 24 ++++++++++++++++++++++++ include/drm/drm_modeset_helper_vtables.h | 26 ++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) -- 2.7.4