| Message ID | 1487100302-9445-2-git-send-email-john.stultz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 08:38:40PM +0100, Daniel Vetter 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 > - 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 Long ago I quickly looked at doing this for i915. IIRC the main complication was the normal vs. the crtc_ timings stored in the mode. I suppose one option would be to populate the crtc_ timings in .mode_valid() as well and otherwise just ignore the normal timings.
On Tue, Feb 14, 2017 at 11:45:54AM -0800, John Stultz wrote: > 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? 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. 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 :-) -Daniel
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 ... Cheers, Daniel
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. > 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. thanks -john
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. 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
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
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
Hi Daniel, On Tue, Feb 14, 2017 at 10:42:53PM +0100, Daniel Vetter wrote: > 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? I've had kind of the same issue on sun4i actually. The issue is that a bridge (through EDID) might report that some modes are supported, and the bridge has a chance to say whether or not it can support such resolutions and filter out the ones it cannot. However, components higher in the pipeline cannot do so. In my case, we had an RGB to HDMI that was definitely able to support up to 1080p60. However, our RGB encoder can only generate a pixel clock for up to ~720p60, and this is not really the bridge's fault, so it shouldn't be in the bridge driver. The same bridge attached to a different RGB encoder (even a different SoC from the same family in our case) will run just fine at 1080p. We currently end up letting the user choose a resolution we knew very well had not a chance to work. I came up with a similar solution than John's, but for the encoders (since our clocks are per-encoder), but unfortunately had no time to push it. Maxime
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
On Mon, Feb 20, 2017 at 2:32 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > 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. So... my ignorance here my have complicated things a bit. Xinliang can correct me here, if I get this wrong. :) So in the past, when we ran into this issue, we just hacked in a mode white list into the adv7511 bridge mode_valid check, as it was the easiest place. But as I understood it, the limitation came from logic in (what is now an early version of) the kirin ade driver. However, since kirin made it upstream, the code was refactored and I believe the specific code was moved to the dsi_calc_phy_rate() which is part of the dsi encoder, not the crtc code. So in my limited understanding I initially tried to add the mode_check logic in the ade crtc driver, but to me (and I could have this wrong again) it seems it really ought to be in the encoder (where we probably could do better then using a white list by doing the mode calculations and seeing if we get a value that matches what was requested). I'm not sure if that simplifies your proposal (possibly avoiding the crtc/encoder combo checks?), or if you think we still need the big grand solution? > - 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. So my mental map of the DRM code is all pretty hazy and limited here, but the above sounds reasonable. I'll try to trace through and better understand what specifically you're asking for and get something started here. Thanks again for the thoughts and feedback! -john
On Mon, Feb 27, 2017 at 10:03:20PM -0800, John Stultz wrote: > On Mon, Feb 20, 2017 at 2:32 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > 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. > > So... my ignorance here my have complicated things a bit. > > Xinliang can correct me here, if I get this wrong. :) > > So in the past, when we ran into this issue, we just hacked in a mode > white list into the adv7511 bridge mode_valid check, as it was the > easiest place. But as I understood it, the limitation came from logic > in (what is now an early version of) the kirin ade driver. > > However, since kirin made it upstream, the code was refactored and I > believe the specific code was moved to the dsi_calc_phy_rate() which > is part of the dsi encoder, not the crtc code. > > So in my limited understanding I initially tried to add the mode_check > logic in the ade crtc driver, but to me (and I could have this wrong > again) it seems it really ought to be in the encoder (where we > probably could do better then using a white list by doing the mode > calculations and seeing if we get a value that matches what was > requested). > > I'm not sure if that simplifies your proposal (possibly avoiding the > crtc/encoder combo checks?), or if you think we still need the big > grand solution? We'll need the grand plan for other drivers I think. Or at least that's what I'm trying to volunteer you for :-) If you just want to fix up your specific combo, then you can just hack up that driver. You don't need any core/helper changes for that. > > - 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. > > So my mental map of the DRM code is all pretty hazy and limited here, > but the above sounds reasonable. I'll try to trace through and better > understand what specifically you're asking for and get something > started here. Yeah, we're still lacking the nice overview graphs for the drm docs. I have them here, but the kbuild scripts need polish :( -Daniel
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(+)