From: Daniel Vetter <daniel@ffwll.ch> To: Jose Abreu <Jose.Abreu@synopsys.com> Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, "Carlos Palminha" <CARLOS.PALMINHA@synopsys.com>, "Alexey Brodkin" <Alexey.Brodkin@synopsys.com>, "Ville Syrjälä" <ville.syrjala@linux.intel.com>, "Daniel Vetter" <daniel.vetter@ffwll.ch>, "Dave Airlie" <airlied@linux.ie> Subject: Re: [PATCH 1/2] drm: Introduce crtc->mode_valid() callback Date: Tue, 2 May 2017 10:48:07 +0200 [thread overview] Message-ID: <20170502084807.k7ioo7a5j33xtnpj@phenom.ffwll.local> (raw) In-Reply-To: <3e1dba0cd5fc053e56f1c9c94d0cb8789ecca4ae.1493203049.git.joabreu@synopsys.com> 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. 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. -Daniel > --- > drivers/gpu/drm/drm_probe_helper.c | 44 ++++++++++++++++++++++++++++++++ > include/drm/drm_modeset_helper_vtables.h | 26 +++++++++++++++++++ > 2 files changed, 70 insertions(+) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index 1b0c14a..61eac30 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -80,6 +80,46 @@ > return MODE_OK; > } > > +static enum drm_mode_status drm_mode_validate_connector_crtc( > + struct drm_connector *connector, > + struct drm_display_mode *mode) > +{ > + const struct drm_crtc_helper_funcs *crtc_funcs = NULL; > + enum drm_mode_status mode_status = MODE_ERROR; > + struct drm_device *dev = connector->dev; > + struct drm_encoder *encoder; > + struct drm_crtc *crtc; > + bool callback_found = false; > + int i; > + > + for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) { > + encoder = drm_encoder_find(dev, connector->encoder_ids[i]); > + > + if (!encoder) > + continue; > + > + drm_for_each_crtc(crtc, dev) { > + crtc_funcs = crtc->helper_private; > + > + if (!drm_encoder_crtc_ok(encoder, crtc)) > + continue; > + if (!crtc_funcs || !crtc_funcs->mode_valid) > + continue; > + > + /* MODE_OK=0 and default mode_status=MODE_ERROR=-1 > + * so if at least one crtc accepts the mode we get > + * MODE_OK */ > + mode_status &= crtc_funcs->mode_valid(crtc, mode); > + callback_found |= true; > + } > + } > + > + /* We can reach here without calling mode_valid if there is no > + * registered crtc with this callback, lets return MODE_OK in this > + * case */ > + return callback_found ? mode_status : MODE_OK; > +} > + > static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) > { > struct drm_cmdline_mode *cmdline_mode; > @@ -431,6 +471,10 @@ 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) > + mode->status = drm_mode_validate_connector_crtc( > + connector, mode); > } > > prune: > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index c01c328..59fffba 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -106,6 +106,32 @@ struct drm_crtc_helper_funcs { > void (*commit)(struct drm_crtc *crtc); > > /** > + * @mode_valid: > + * > + * This callback should be implemented if the crtc has some sort of > + * restriction in the modes it can display. 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. > + * > + * This is directly called at the same stage of connector->mode_valid &drm_connector_helper_funcs.mode_valid to make a proper link. Please also check the output of make htmldocs and make sure it looks pretty. > + * callback. > + * > + * NOTE: > + * > + * For a given set of crtc's in a drm_device, if at least one does not > + * have the mode_valid callback, or, at least one returns MODE_OK then > + * the mode will be probbed. > + * > + * RETURNS: > + * > + * drm_mode_status Enum > + */ > + 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 > -- > 1.9.1 > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: Jose Abreu <Jose.Abreu@synopsys.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, Alexey Brodkin <Alexey.Brodkin@synopsys.com>, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Carlos Palminha <CARLOS.PALMINHA@synopsys.com> Subject: Re: [PATCH 1/2] drm: Introduce crtc->mode_valid() callback Date: Tue, 2 May 2017 10:48:07 +0200 [thread overview] Message-ID: <20170502084807.k7ioo7a5j33xtnpj@phenom.ffwll.local> (raw) In-Reply-To: <3e1dba0cd5fc053e56f1c9c94d0cb8789ecca4ae.1493203049.git.joabreu@synopsys.com> 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. 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. -Daniel > --- > drivers/gpu/drm/drm_probe_helper.c | 44 ++++++++++++++++++++++++++++++++ > include/drm/drm_modeset_helper_vtables.h | 26 +++++++++++++++++++ > 2 files changed, 70 insertions(+) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index 1b0c14a..61eac30 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -80,6 +80,46 @@ > return MODE_OK; > } > > +static enum drm_mode_status drm_mode_validate_connector_crtc( > + struct drm_connector *connector, > + struct drm_display_mode *mode) > +{ > + const struct drm_crtc_helper_funcs *crtc_funcs = NULL; > + enum drm_mode_status mode_status = MODE_ERROR; > + struct drm_device *dev = connector->dev; > + struct drm_encoder *encoder; > + struct drm_crtc *crtc; > + bool callback_found = false; > + int i; > + > + for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) { > + encoder = drm_encoder_find(dev, connector->encoder_ids[i]); > + > + if (!encoder) > + continue; > + > + drm_for_each_crtc(crtc, dev) { > + crtc_funcs = crtc->helper_private; > + > + if (!drm_encoder_crtc_ok(encoder, crtc)) > + continue; > + if (!crtc_funcs || !crtc_funcs->mode_valid) > + continue; > + > + /* MODE_OK=0 and default mode_status=MODE_ERROR=-1 > + * so if at least one crtc accepts the mode we get > + * MODE_OK */ > + mode_status &= crtc_funcs->mode_valid(crtc, mode); > + callback_found |= true; > + } > + } > + > + /* We can reach here without calling mode_valid if there is no > + * registered crtc with this callback, lets return MODE_OK in this > + * case */ > + return callback_found ? mode_status : MODE_OK; > +} > + > static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) > { > struct drm_cmdline_mode *cmdline_mode; > @@ -431,6 +471,10 @@ 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) > + mode->status = drm_mode_validate_connector_crtc( > + connector, mode); > } > > prune: > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index c01c328..59fffba 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -106,6 +106,32 @@ struct drm_crtc_helper_funcs { > void (*commit)(struct drm_crtc *crtc); > > /** > + * @mode_valid: > + * > + * This callback should be implemented if the crtc has some sort of > + * restriction in the modes it can display. 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. > + * > + * This is directly called at the same stage of connector->mode_valid &drm_connector_helper_funcs.mode_valid to make a proper link. Please also check the output of make htmldocs and make sure it looks pretty. > + * callback. > + * > + * NOTE: > + * > + * For a given set of crtc's in a drm_device, if at least one does not > + * have the mode_valid callback, or, at least one returns MODE_OK then > + * the mode will be probbed. > + * > + * RETURNS: > + * > + * drm_mode_status Enum > + */ > + 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 > -- > 1.9.1 > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2017-05-02 8:48 UTC|newest] Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-04-26 10:48 [PATCH 0/2] Introduce crtc->mode_valid() callback Jose Abreu 2017-04-26 10:48 ` [PATCH 1/2] drm: " Jose Abreu 2017-04-27 10:05 ` Andrzej Hajda 2017-04-27 10:05 ` Andrzej Hajda 2017-04-27 12:34 ` Jose Abreu 2017-04-27 12:34 ` Jose Abreu 2017-04-28 11:41 ` Ville Syrjälä 2017-04-28 11:41 ` Ville Syrjälä 2017-04-28 12:30 ` Jose Abreu 2017-04-28 12:30 ` Jose Abreu 2017-04-28 12:58 ` Ville Syrjälä 2017-04-28 12:58 ` Ville Syrjälä 2017-05-02 8:48 ` Daniel Vetter [this message] 2017-05-02 8:48 ` Daniel Vetter 2017-05-02 9:29 ` Jose Abreu 2017-05-03 6:19 ` Daniel Vetter 2017-05-03 6:19 ` Daniel Vetter 2017-05-03 6:19 ` Daniel Vetter 2017-05-03 6:19 ` Daniel Vetter 2017-05-03 14:16 ` Jose Abreu 2017-05-03 15:00 ` Daniel Vetter 2017-05-03 15:00 ` Daniel Vetter 2017-05-03 15:21 ` Ville Syrjälä 2017-05-03 15:21 ` Ville Syrjälä 2017-05-04 12:49 ` Daniel Vetter 2017-05-04 12:49 ` Daniel Vetter 2017-05-04 13:08 ` Ville Syrjälä 2017-05-04 13:08 ` Ville Syrjälä 2017-05-04 10:21 ` Jose Abreu 2017-05-04 11:55 ` Jose Abreu 2017-05-04 12:48 ` Daniel Vetter 2017-05-04 12:48 ` Daniel Vetter 2017-04-26 10:48 ` [PATCH 2/2] drm: arcpgu: Use " Jose Abreu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170502084807.k7ioo7a5j33xtnpj@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=Alexey.Brodkin@synopsys.com \ --cc=CARLOS.PALMINHA@synopsys.com \ --cc=Jose.Abreu@synopsys.com \ --cc=airlied@linux.ie \ --cc=daniel.vetter@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=ville.syrjala@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.