| Message ID | c8d27538-4f05-40ad-d371-adc958a39f89@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 21, 2017 at 02:29:12PM +0100, Jose Abreu wrote: > ++ Daniel > > ++ Dave > > > On 21-04-2017 13:59, Jose Abreu wrote: > > Hi All, > > > > > > Is there any callback available, at the crtc level, that can > > validate the probed modes before making them available for > > userspace? I mean: I know that there is connector->mode_valid(), > > but I couldn't find any equivalent crtc->mode_valid(). I found > > mode_fixup() but this is called after giving the mode to > > userspace, which is not what I need. > > > > For reference here is the situation I'm facing: > > > > ARCPGU is a DRM driver that uses ADV7511 as connector/bridge. The > > PGU encodes raw video into a stream that ADV can understands and > > in order to do this it needs a pixel clock. Currently this pixel > > clock value is fixed, so technically arcpgu driver supports only > > one video mode. There is a patch currently on discussion that > > adds more supported frequencies for arcpgu, but there are still > > some frequencies that will not be supported. This is NOT a > > limitation in ADV7511, so it doesn't make sense to limit the > > available modes in adv, we could instead limit the modes in the > > crtc level (i.e. the pgu). > > > > In order to know if a mode can be displayed or not it is as > > simple as call clk_round_rate() and check if the returned > > frequency is the same. But in order to do this I need some sort > > of callback that is called at the crtc level and before > > delivering modes to userspace. > > > > I believe this would be a good addition to arcpgu because this > > way we wouldn't "fool" userspace by delivering modes that will > > fail in atomic check. The user may still specify manually a mode, > > this will still fail in atomic check, but the difference is that > > this mode will not be in the probed list. > > > > > > Best regards, > > > > Jose Miguel Abreu > > > > With this simple patch I could fix my problem, what do you think? > Is this ok to be added? I guess some connectors may not have the > crtc linked at probbing stage but this is handled. > > From 252b7cb5ad9999eaf16be95988d17468eea2895b Mon Sep 17 00:00:00 > 2001 > Message-Id: > <252b7cb5ad9999eaf16be95988d17468eea2895b.1492781127.git.joabreu@synopsys.com> > From: Jose Abreu <joabreu@synopsys.com> > Date: Fri, 21 Apr 2017 14:24:16 +0100 > Subject: [PATCH] drm: Introduce crtc->mode_valid() callback > > Introduce a new crtc callback which validates the probbed modes, > just like connector->mode_valid() does. > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > --- > drivers/gpu/drm/arc/arcpgu_crtc.c | 14 ++++++++++++++ > drivers/gpu/drm/drm_probe_helper.c | 12 ++++++++++++ > include/drm/drm_modeset_helper_vtables.h | 3 +++ > 3 files changed, 29 insertions(+) > > diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c > b/drivers/gpu/drm/arc/arcpgu_crtc.c > index 7130b04..e43e8d6 100644 > --- a/drivers/gpu/drm/arc/arcpgu_crtc.c > +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c > @@ -63,6 +63,19 @@ static void arc_pgu_set_pxl_fmt(struct > drm_crtc *crtc) > .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > }; > > +enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc, > + struct drm_display_mode *mode) > +{ > + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > + long rate, clk_rate = mode->clock * 1000; > + > + rate = clk_round_rate(arcpgu->clk, clk_rate); > + if (rate != clk_rate) > + return MODE_NOCLOCK; > + > + return MODE_OK; > +} > + > static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc) > { > struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > @@ -157,6 +170,7 @@ static void arc_pgu_crtc_atomic_begin(struct > drm_crtc *crtc, > } > > static const struct drm_crtc_helper_funcs > arc_pgu_crtc_helper_funcs = { > + .mode_valid = arc_pgu_crtc_mode_valid, > .mode_set = drm_helper_crtc_mode_set, > .mode_set_base = drm_helper_crtc_mode_set_base, > .mode_set_nofb = arc_pgu_crtc_mode_set_nofb, > diff --git a/drivers/gpu/drm/drm_probe_helper.c > b/drivers/gpu/drm/drm_probe_helper.c > index cf8f012..6c9af88 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -233,6 +233,8 @@ int > drm_helper_probe_single_connector_modes(struct drm_connector > *connector, > struct drm_display_mode *mode; > const struct drm_connector_helper_funcs *connector_funcs = > connector->helper_private; > + struct drm_crtc *crtc = NULL; > + const struct drm_crtc_helper_funcs *crtc_funcs = NULL; > int count = 0; > int mode_flags = 0; > bool verbose_prune = true; > @@ -242,6 +244,13 @@ int > drm_helper_probe_single_connector_modes(struct drm_connector > *connector, > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, > connector->name); > + > + if (connector->encoder && connector->encoder->crtc && Basing this decision on whichever crtc might happen to be driving the connector at the time is not what we want. This would need to loop over all the crtc that could potentially drive the connector, and reject the mode only if all the crtcs rejected it. > + connector->encoder->crtc->helper_private) { > + crtc = connector->encoder->crtc; > + crtc_funcs = crtc->helper_private; > + } > + > /* set all old modes to the stale state */ > list_for_each_entry(mode, &connector->modes, head) > mode->status = MODE_STALE; > @@ -338,6 +347,9 @@ 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); > + > + if (mode->status == MODE_OK && crtc && > crtc_funcs->mode_valid) > + mode->status = crtc_funcs->mode_valid(crtc, mode); > } > > prune: > diff --git a/include/drm/drm_modeset_helper_vtables.h > b/include/drm/drm_modeset_helper_vtables.h > index 69c3974..776c9d0 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -104,6 +104,9 @@ struct drm_crtc_helper_funcs { > */ > void (*commit)(struct drm_crtc *crtc); > > + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, > + struct drm_display_mode *mode); > + > /** > * @mode_fixup: > * > -- > 1.9.1 > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Apr 21, 2017 at 02:29:12PM +0100, Jose Abreu wrote: > ++ Daniel > > ++ Dave > > > On 21-04-2017 13:59, Jose Abreu wrote: > > Hi All, > > > > > > Is there any callback available, at the crtc level, that can > > validate the probed modes before making them available for > > userspace? I mean: I know that there is connector->mode_valid(), > > but I couldn't find any equivalent crtc->mode_valid(). I found > > mode_fixup() but this is called after giving the mode to > > userspace, which is not what I need. > > > > For reference here is the situation I'm facing: > > > > ARCPGU is a DRM driver that uses ADV7511 as connector/bridge. The > > PGU encodes raw video into a stream that ADV can understands and > > in order to do this it needs a pixel clock. Currently this pixel > > clock value is fixed, so technically arcpgu driver supports only > > one video mode. There is a patch currently on discussion that > > adds more supported frequencies for arcpgu, but there are still > > some frequencies that will not be supported. This is NOT a > > limitation in ADV7511, so it doesn't make sense to limit the > > available modes in adv, we could instead limit the modes in the > > crtc level (i.e. the pgu). > > > > In order to know if a mode can be displayed or not it is as > > simple as call clk_round_rate() and check if the returned > > frequency is the same. But in order to do this I need some sort > > of callback that is called at the crtc level and before > > delivering modes to userspace. > > > > I believe this would be a good addition to arcpgu because this > > way we wouldn't "fool" userspace by delivering modes that will > > fail in atomic check. The user may still specify manually a mode, > > this will still fail in atomic check, but the difference is that > > this mode will not be in the probed list. This is a faq, and your patch isn't the solution. Adding John (who made a different spin on this, differently broken). I guess I to write a todo.rst entry about this ... -Daniel > > > > > > Best regards, > > > > Jose Miguel Abreu > > > > With this simple patch I could fix my problem, what do you think? > Is this ok to be added? I guess some connectors may not have the > crtc linked at probbing stage but this is handled. > > From 252b7cb5ad9999eaf16be95988d17468eea2895b Mon Sep 17 00:00:00 > 2001 > Message-Id: > <252b7cb5ad9999eaf16be95988d17468eea2895b.1492781127.git.joabreu@synopsys.com> > From: Jose Abreu <joabreu@synopsys.com> > Date: Fri, 21 Apr 2017 14:24:16 +0100 > Subject: [PATCH] drm: Introduce crtc->mode_valid() callback > > Introduce a new crtc callback which validates the probbed modes, > just like connector->mode_valid() does. > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > --- > drivers/gpu/drm/arc/arcpgu_crtc.c | 14 ++++++++++++++ > drivers/gpu/drm/drm_probe_helper.c | 12 ++++++++++++ > include/drm/drm_modeset_helper_vtables.h | 3 +++ > 3 files changed, 29 insertions(+) > > diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c > b/drivers/gpu/drm/arc/arcpgu_crtc.c > index 7130b04..e43e8d6 100644 > --- a/drivers/gpu/drm/arc/arcpgu_crtc.c > +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c > @@ -63,6 +63,19 @@ static void arc_pgu_set_pxl_fmt(struct > drm_crtc *crtc) > .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > }; > > +enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc, > + struct drm_display_mode *mode) > +{ > + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > + long rate, clk_rate = mode->clock * 1000; > + > + rate = clk_round_rate(arcpgu->clk, clk_rate); > + if (rate != clk_rate) > + return MODE_NOCLOCK; > + > + return MODE_OK; > +} > + > static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc) > { > struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > @@ -157,6 +170,7 @@ static void arc_pgu_crtc_atomic_begin(struct > drm_crtc *crtc, > } > > static const struct drm_crtc_helper_funcs > arc_pgu_crtc_helper_funcs = { > + .mode_valid = arc_pgu_crtc_mode_valid, > .mode_set = drm_helper_crtc_mode_set, > .mode_set_base = drm_helper_crtc_mode_set_base, > .mode_set_nofb = arc_pgu_crtc_mode_set_nofb, > diff --git a/drivers/gpu/drm/drm_probe_helper.c > b/drivers/gpu/drm/drm_probe_helper.c > index cf8f012..6c9af88 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -233,6 +233,8 @@ int > drm_helper_probe_single_connector_modes(struct drm_connector > *connector, > struct drm_display_mode *mode; > const struct drm_connector_helper_funcs *connector_funcs = > connector->helper_private; > + struct drm_crtc *crtc = NULL; > + const struct drm_crtc_helper_funcs *crtc_funcs = NULL; > int count = 0; > int mode_flags = 0; > bool verbose_prune = true; > @@ -242,6 +244,13 @@ int > drm_helper_probe_single_connector_modes(struct drm_connector > *connector, > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, > connector->name); > + > + if (connector->encoder && connector->encoder->crtc && > + connector->encoder->crtc->helper_private) { > + crtc = connector->encoder->crtc; > + crtc_funcs = crtc->helper_private; > + } > + > /* set all old modes to the stale state */ > list_for_each_entry(mode, &connector->modes, head) > mode->status = MODE_STALE; > @@ -338,6 +347,9 @@ 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); > + > + if (mode->status == MODE_OK && crtc && > crtc_funcs->mode_valid) > + mode->status = crtc_funcs->mode_valid(crtc, mode); > } > > prune: > diff --git a/include/drm/drm_modeset_helper_vtables.h > b/include/drm/drm_modeset_helper_vtables.h > index 69c3974..776c9d0 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -104,6 +104,9 @@ struct drm_crtc_helper_funcs { > */ > void (*commit)(struct drm_crtc *crtc); > > + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, > + struct drm_display_mode *mode); > + > /** > * @mode_fixup: > * > -- > 1.9.1 > >
diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c index 7130b04..e43e8d6 100644 --- a/drivers/gpu/drm/arc/arcpgu_crtc.c +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c @@ -63,6 +63,19 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc) .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, }; +enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc, + struct drm_display_mode *mode) +{ + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); + long rate, clk_rate = mode->clock * 1000; + + rate = clk_round_rate(arcpgu->clk, clk_rate); + if (rate != clk_rate) + return MODE_NOCLOCK; + + return MODE_OK; +} + static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc) { struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); @@ -157,6 +170,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, } static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = { + .mode_valid = arc_pgu_crtc_mode_valid, .mode_set = drm_helper_crtc_mode_set, .mode_set_base = drm_helper_crtc_mode_set_base, .mode_set_nofb = arc_pgu_crtc_mode_set_nofb, diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index cf8f012..6c9af88 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -233,6 +233,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, struct drm_display_mode *mode; const struct drm_connector_helper_funcs *connector_funcs = connector->helper_private; + struct drm_crtc *crtc = NULL; + const struct drm_crtc_helper_funcs *crtc_funcs = NULL; int count = 0; int mode_flags = 0; bool verbose_prune = true; @@ -242,6 +244,13 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name); + + if (connector->encoder && connector->encoder->crtc && + connector->encoder->crtc->helper_private) { + crtc = connector->encoder->crtc; + crtc_funcs = crtc->helper_private; + } + /* set all old modes to the stale state */ list_for_each_entry(mode, &connector->modes, head) mode->status = MODE_STALE; @@ -338,6 +347,9 @@ 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); + + if (mode->status == MODE_OK && crtc && crtc_funcs->mode_valid) + mode->status = crtc_funcs->mode_valid(crtc, mode); } prune: diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 69c3974..776c9d0 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -104,6 +104,9 @@ struct drm_crtc_helper_funcs { */ void (*commit)(struct drm_crtc *crtc); + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, + struct drm_display_mode *mode); + /** * @mode_fixup: *