From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Jose Abreu <Jose.Abreu@synopsys.com> Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Daniel Vetter <daniel.vetter@ffwll.ch>, Alexey Brodkin <Alexey.Brodkin@synopsys.com>, Carlos Palminha <CARLOS.PALMINHA@synopsys.com> Subject: Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper Date: Sun, 14 May 2017 14:04:24 +0300 [thread overview] Message-ID: <1874234.uZ7HLpxeJ2@avalon> (raw) In-Reply-To: <65a1928f-a5e8-b668-0925-ff233c21c9fe@synopsys.com> Hi Jose, On Friday 12 May 2017 17:06:14 Jose Abreu wrote: > On 12-05-2017 10:35, Laurent Pinchart wrote: > > On Tuesday 09 May 2017 18:00:12 Jose Abreu wrote: > >> This changes the connector probe helper function to use the new > >> encoder->mode_valid() and crtc->mode_valid() helper callbacks to > >> validate the modes. > >> > >> The new callbacks are optional so the behaviour remains the same > >> if they are not implemented. If they are, then the code loops > >> through all the connector's encodersXcrtcs and calls the > >> callback. > >> > >> If at least a valid encoderXcrtc combination is found which > >> accepts the mode then the function returns MODE_OK. > >> > >> 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> > >> Cc: Andrzej Hajda <a.hajda@samsung.com> > >> Cc: Archit Taneja <architt@codeaurora.org> > >> --- > >> > >> Changes v1->v2: > >> - Use new helpers suggested by Ville > >> - Change documentation (Daniel) > >> > >> drivers/gpu/drm/drm_probe_helper.c | 60 ++++++++++++++++++++++++++++++-- > >> 1 file changed, 57 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_probe_helper.c > >> b/drivers/gpu/drm/drm_probe_helper.c index 1b0c14a..de47413 100644 > >> --- a/drivers/gpu/drm/drm_probe_helper.c > >> +++ b/drivers/gpu/drm/drm_probe_helper.c [snip] > >> +static enum drm_mode_status > >> +drm_mode_validate_connector(struct drm_connector *connector, > >> + struct drm_display_mode *mode) > > > > This does more than validating the mode against the connector, it > > validates it against the whole pipeline. I would call the function > > drm_mode_validate_pipeline() (or any other similar name). > > Yeah, in previous version I had something similar but I changed > in order to address review comments. I can change again though... Sorry, I haven't seen v1. I think it makes more sense to reflect in its name the fact that the function validates the mode against the whole pipeline, but I'll let others disagree. > >> +{ > >> + struct drm_device *dev = connector->dev; > >> + uint32_t *ids = connector->encoder_ids; > >> + enum drm_mode_status ret = MODE_OK; > >> + unsigned int i; > >> + > >> + /* Step 1: Validate against connector */ > >> + ret = drm_connector_mode_valid(connector, mode); > >> + if (ret != MODE_OK) > >> + return ret; > >> + > >> + /* Step 2: Validate against encoders and crtcs */ > >> + for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) { > >> + struct drm_encoder *encoder = drm_encoder_find(dev, ids[i]); > >> + struct drm_crtc *crtc; > >> + > >> + if (!encoder) > >> + continue; > >> + > >> + ret = drm_encoder_mode_valid(encoder, mode); > >> + if (ret != MODE_OK) { > >> + /* No point in continuing for crtc check as this > > > > encoder > > > >> + * will not accept the mode anyway. If all encoders > >> + * reject the mode then, at exit, ret will not be > >> + * MODE_OK. */ > >> + continue; > >> + } > >> + > >> + drm_for_each_crtc(crtc, dev) { > >> + if (!drm_encoder_crtc_ok(encoder, crtc)) > >> + continue; > >> + > >> + ret = drm_crtc_mode_valid(crtc, mode); > >> + if (ret == MODE_OK) { > >> + /* If we get to this point there is at least > >> + * one combination of encoder+crtc that works > >> + * for this mode. Lets return now. */ > >> + return ret; > >> + } > >> + } > >> + } > >> + > >> + return ret; > >> +} [snip] > >> @@ -428,8 +482,8 @@ int > >> drm_helper_probe_single_connector_modes(struct drm_connector *connector, > >> > >> if (mode->status == MODE_OK) > >> > >> mode->status = drm_mode_validate_flag(mode, > >> > >> mode_flags); > >> > >> - if (mode->status == MODE_OK && connector_funcs->mode_valid) > >> - mode->status = connector_funcs->mode_valid(connector, > >> + if (mode->status == MODE_OK) > >> + mode->status = drm_mode_validate_connector(connector, > >> > >> mode); > > > > I would reverse the arguments order to match the style of the other > > validation functions. > > Hmm, I think it makes more sense to pass connector first and then > mode ... I disagree, as this function validates a mode against a pipeline, the same way the other validation functions validate a mode against other parameters, but it's your patch :-) -- Regards, Laurent Pinchart
WARNING: multiple messages have this Message-ID (diff)
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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 v2 5/8] drm: Use new mode_valid() helpers in connector probe helper Date: Sun, 14 May 2017 14:04:24 +0300 [thread overview] Message-ID: <1874234.uZ7HLpxeJ2@avalon> (raw) In-Reply-To: <65a1928f-a5e8-b668-0925-ff233c21c9fe@synopsys.com> Hi Jose, On Friday 12 May 2017 17:06:14 Jose Abreu wrote: > On 12-05-2017 10:35, Laurent Pinchart wrote: > > On Tuesday 09 May 2017 18:00:12 Jose Abreu wrote: > >> This changes the connector probe helper function to use the new > >> encoder->mode_valid() and crtc->mode_valid() helper callbacks to > >> validate the modes. > >> > >> The new callbacks are optional so the behaviour remains the same > >> if they are not implemented. If they are, then the code loops > >> through all the connector's encodersXcrtcs and calls the > >> callback. > >> > >> If at least a valid encoderXcrtc combination is found which > >> accepts the mode then the function returns MODE_OK. > >> > >> 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> > >> Cc: Andrzej Hajda <a.hajda@samsung.com> > >> Cc: Archit Taneja <architt@codeaurora.org> > >> --- > >> > >> Changes v1->v2: > >> - Use new helpers suggested by Ville > >> - Change documentation (Daniel) > >> > >> drivers/gpu/drm/drm_probe_helper.c | 60 ++++++++++++++++++++++++++++++-- > >> 1 file changed, 57 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_probe_helper.c > >> b/drivers/gpu/drm/drm_probe_helper.c index 1b0c14a..de47413 100644 > >> --- a/drivers/gpu/drm/drm_probe_helper.c > >> +++ b/drivers/gpu/drm/drm_probe_helper.c [snip] > >> +static enum drm_mode_status > >> +drm_mode_validate_connector(struct drm_connector *connector, > >> + struct drm_display_mode *mode) > > > > This does more than validating the mode against the connector, it > > validates it against the whole pipeline. I would call the function > > drm_mode_validate_pipeline() (or any other similar name). > > Yeah, in previous version I had something similar but I changed > in order to address review comments. I can change again though... Sorry, I haven't seen v1. I think it makes more sense to reflect in its name the fact that the function validates the mode against the whole pipeline, but I'll let others disagree. > >> +{ > >> + struct drm_device *dev = connector->dev; > >> + uint32_t *ids = connector->encoder_ids; > >> + enum drm_mode_status ret = MODE_OK; > >> + unsigned int i; > >> + > >> + /* Step 1: Validate against connector */ > >> + ret = drm_connector_mode_valid(connector, mode); > >> + if (ret != MODE_OK) > >> + return ret; > >> + > >> + /* Step 2: Validate against encoders and crtcs */ > >> + for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) { > >> + struct drm_encoder *encoder = drm_encoder_find(dev, ids[i]); > >> + struct drm_crtc *crtc; > >> + > >> + if (!encoder) > >> + continue; > >> + > >> + ret = drm_encoder_mode_valid(encoder, mode); > >> + if (ret != MODE_OK) { > >> + /* No point in continuing for crtc check as this > > > > encoder > > > >> + * will not accept the mode anyway. If all encoders > >> + * reject the mode then, at exit, ret will not be > >> + * MODE_OK. */ > >> + continue; > >> + } > >> + > >> + drm_for_each_crtc(crtc, dev) { > >> + if (!drm_encoder_crtc_ok(encoder, crtc)) > >> + continue; > >> + > >> + ret = drm_crtc_mode_valid(crtc, mode); > >> + if (ret == MODE_OK) { > >> + /* If we get to this point there is at least > >> + * one combination of encoder+crtc that works > >> + * for this mode. Lets return now. */ > >> + return ret; > >> + } > >> + } > >> + } > >> + > >> + return ret; > >> +} [snip] > >> @@ -428,8 +482,8 @@ int > >> drm_helper_probe_single_connector_modes(struct drm_connector *connector, > >> > >> if (mode->status == MODE_OK) > >> > >> mode->status = drm_mode_validate_flag(mode, > >> > >> mode_flags); > >> > >> - if (mode->status == MODE_OK && connector_funcs->mode_valid) > >> - mode->status = connector_funcs->mode_valid(connector, > >> + if (mode->status == MODE_OK) > >> + mode->status = drm_mode_validate_connector(connector, > >> > >> mode); > > > > I would reverse the arguments order to match the style of the other > > validation functions. > > Hmm, I think it makes more sense to pass connector first and then > mode ... I disagree, as this function validates a mode against a pipeline, the same way the other validation functions validate a mode against other parameters, but it's your patch :-) -- Regards, Laurent Pinchart _______________________________________________ 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-14 11:04 UTC|newest] Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-05-09 17:00 [PATCH v2 0/8] Introduce new mode validation callbacks Jose Abreu 2017-05-09 17:00 ` [PATCH v2 1/8] drm: Add crtc/encoder/bridge->mode_valid() callbacks Jose Abreu 2017-05-10 8:03 ` Daniel Vetter 2017-05-10 8:03 ` Daniel Vetter 2017-05-10 9:05 ` Jose Abreu 2017-05-12 8:24 ` Laurent Pinchart 2017-05-12 8:24 ` Laurent Pinchart 2017-05-15 6:46 ` Daniel Vetter 2017-05-15 6:46 ` Daniel Vetter 2017-05-09 17:00 ` [PATCH v2 2/8] drm: Add drm_crtc_mode_valid() Jose Abreu 2017-05-10 7:59 ` Daniel Vetter 2017-05-10 7:59 ` Daniel Vetter 2017-05-10 8:57 ` Jose Abreu 2017-05-10 15:12 ` Daniel Vetter 2017-05-10 15:12 ` Daniel Vetter 2017-05-09 17:00 ` [PATCH v2 3/8] drm: Add drm_encoder_mode_valid() Jose Abreu 2017-05-09 17:00 ` [PATCH v2 4/8] drm: Add drm_connector_mode_valid() Jose Abreu 2017-05-09 17:00 ` [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper Jose Abreu 2017-05-10 8:01 ` Daniel Vetter 2017-05-10 8:01 ` Daniel Vetter 2017-05-10 8:59 ` Jose Abreu 2017-05-12 9:35 ` Laurent Pinchart 2017-05-12 9:35 ` Laurent Pinchart 2017-05-12 16:06 ` Jose Abreu 2017-05-12 16:06 ` Jose Abreu 2017-05-14 11:04 ` Laurent Pinchart [this message] 2017-05-14 11:04 ` Laurent Pinchart 2017-05-15 6:47 ` Daniel Vetter 2017-05-15 6:47 ` Daniel Vetter 2017-05-15 7:05 ` Laurent Pinchart 2017-05-15 7:05 ` Laurent Pinchart 2017-05-16 5:11 ` Jose Abreu 2017-05-16 5:11 ` Jose Abreu 2017-05-09 17:00 ` [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid() Jose Abreu 2017-05-10 13:41 ` Ville Syrjälä 2017-05-10 13:41 ` Ville Syrjälä 2017-05-10 14:01 ` Jose Abreu 2017-05-10 14:01 ` Jose Abreu 2017-05-10 14:07 ` Jose Abreu 2017-05-10 14:07 ` Jose Abreu 2017-05-10 15:14 ` Daniel Vetter 2017-05-10 15:14 ` Daniel Vetter 2017-05-12 9:38 ` Laurent Pinchart 2017-05-12 9:38 ` Laurent Pinchart 2017-05-12 10:50 ` Archit Taneja 2017-05-12 10:50 ` Archit Taneja 2017-05-12 11:01 ` Laurent Pinchart 2017-05-12 11:01 ` Laurent Pinchart 2017-05-15 4:18 ` Archit Taneja 2017-05-15 4:18 ` Archit Taneja 2017-05-15 6:49 ` Daniel Vetter 2017-05-15 6:49 ` Daniel Vetter 2017-05-09 17:00 ` [PATCH v2 7/8] drm: Use mode_valid() in atomic modeset Jose Abreu 2017-05-10 16:08 ` Archit Taneja 2017-05-10 16:08 ` Archit Taneja 2017-05-10 17:55 ` Daniel Vetter 2017-05-10 17:55 ` Daniel Vetter 2017-05-12 9:53 ` Laurent Pinchart 2017-05-12 9:53 ` Laurent Pinchart 2017-05-15 6:50 ` Daniel Vetter 2017-05-09 17:00 ` [PATCH v2 8/8] drm: arc: Use crtc->mode_valid() callback Jose Abreu 2017-05-12 9:57 ` Laurent Pinchart 2017-05-15 1:40 ` Jose Abreu 2017-05-15 1:40 ` 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=1874234.uZ7HLpxeJ2@avalon \ --to=laurent.pinchart@ideasonboard.com \ --cc=Alexey.Brodkin@synopsys.com \ --cc=CARLOS.PALMINHA@synopsys.com \ --cc=Jose.Abreu@synopsys.com \ --cc=daniel.vetter@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ /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.