| Message ID | beb31d3a042458e8132553b36f4664e407ff021c.1494347165.git.joabreu@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 09, 2017 at 06:00:12PM +0100, 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 > @@ -39,6 +39,8 @@ > #include <drm/drm_fb_helper.h> > #include <drm/drm_edid.h> > > +#include "drm_crtc_internal.h" > + > /** > * DOC: output probing helper overview > * > @@ -80,6 +82,54 @@ > return MODE_OK; > } > > +static enum drm_mode_status > +drm_mode_validate_connector(struct drm_connector *connector, > + struct drm_display_mode *mode) > +{ > + 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; > + } One thing I've forgotten the last time around: Please also check bridge->mode_valid here. The encoder->bridge mapping is fixed. Otherwise I think this looks good. -Daniel > + > + 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; > +} > + > static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) > { > struct drm_cmdline_mode *cmdline_mode; > @@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev) > * - drm_mode_validate_flag() checks the modes against basic connector > * capabilities (interlace_allowed,doublescan_allowed,stereo_allowed) > * - the optional &drm_connector_helper_funcs.mode_valid helper can perform > - * driver and/or hardware specific checks > + * driver and/or sink specific checks > + * - the optional &drm_crtc_helper_funcs.mode_valid and > + * &drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or > + * source specific checks which are also enforced by the modeset/atomic > + * helpers > * > * 5. Any mode whose status is not OK is pruned from the connector's modes list, > * accompanied by a debug message indicating the reason for the mode's > @@ -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); > } > > -- > 1.9.1 > >
Hi Daniel, On 10-05-2017 09:01, Daniel Vetter wrote: > On Tue, May 09, 2017 at 06:00:12PM +0100, 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 >> @@ -39,6 +39,8 @@ >> #include <drm/drm_fb_helper.h> >> #include <drm/drm_edid.h> >> >> +#include "drm_crtc_internal.h" >> + >> /** >> * DOC: output probing helper overview >> * >> @@ -80,6 +82,54 @@ >> return MODE_OK; >> } >> >> +static enum drm_mode_status >> +drm_mode_validate_connector(struct drm_connector *connector, >> + struct drm_display_mode *mode) >> +{ >> + 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; >> + } > One thing I've forgotten the last time around: Please also check > bridge->mode_valid here. The encoder->bridge mapping is fixed. Ok, will add in next version. Best regards, Jose Miguel Abreu > > Otherwise I think this looks good. > -Daniel > >> + >> + 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; >> +} >> + >> static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) >> { >> struct drm_cmdline_mode *cmdline_mode; >> @@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev) >> * - drm_mode_validate_flag() checks the modes against basic connector >> * capabilities (interlace_allowed,doublescan_allowed,stereo_allowed) >> * - the optional &drm_connector_helper_funcs.mode_valid helper can perform >> - * driver and/or hardware specific checks >> + * driver and/or sink specific checks >> + * - the optional &drm_crtc_helper_funcs.mode_valid and >> + * &drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or >> + * source specific checks which are also enforced by the modeset/atomic >> + * helpers >> * >> * 5. Any mode whose status is not OK is pruned from the connector's modes list, >> * accompanied by a debug message indicating the reason for the mode's >> @@ -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); >> } >> >> -- >> 1.9.1 >> >>
Hi Jose, Thank you for the patch. 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 > @@ -39,6 +39,8 @@ > #include <drm/drm_fb_helper.h> > #include <drm/drm_edid.h> > > +#include "drm_crtc_internal.h" > + > /** > * DOC: output probing helper overview > * > @@ -80,6 +82,54 @@ > return MODE_OK; > } > > +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). > +{ > + 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; > +} > + > static int drm_helper_probe_add_cmdline_mode(struct drm_connector > *connector) > { > struct drm_cmdline_mode *cmdline_mode; > @@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev) > * - drm_mode_validate_flag() checks the modes against basic connector > * capabilities (interlace_allowed,doublescan_allowed,stereo_allowed) > * - the optional &drm_connector_helper_funcs.mode_valid helper can > perform > - * driver and/or hardware specific checks > + * driver and/or sink specific checks > + * - the optional &drm_crtc_helper_funcs.mode_valid and > + * &drm_encoder_helper_funcs.mode_valid helpers can perform driver > and/or > + * source specific checks which are also enforced by the > modeset/atomic > + * helpers > * > * 5. Any mode whose status is not OK is pruned from the connector's modes > list, > * accompanied by a debug message indicating the reason for the mode's > @@ -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. > }
Hi Laurent, On 12-05-2017 10:35, Laurent Pinchart wrote: > Hi Jose, > > Thank you for the patch. > > 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 >> @@ -39,6 +39,8 @@ >> #include <drm/drm_fb_helper.h> >> #include <drm/drm_edid.h> >> >> +#include "drm_crtc_internal.h" >> + >> /** >> * DOC: output probing helper overview >> * >> @@ -80,6 +82,54 @@ >> return MODE_OK; >> } >> >> +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... > >> +{ >> + 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; >> +} >> + >> static int drm_helper_probe_add_cmdline_mode(struct drm_connector >> *connector) >> { >> struct drm_cmdline_mode *cmdline_mode; >> @@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev) >> * - drm_mode_validate_flag() checks the modes against basic connector >> * capabilities (interlace_allowed,doublescan_allowed,stereo_allowed) >> * - the optional &drm_connector_helper_funcs.mode_valid helper can >> perform >> - * driver and/or hardware specific checks >> + * driver and/or sink specific checks >> + * - the optional &drm_crtc_helper_funcs.mode_valid and >> + * &drm_encoder_helper_funcs.mode_valid helpers can perform driver >> and/or >> + * source specific checks which are also enforced by the >> modeset/atomic >> + * helpers >> * >> * 5. Any mode whose status is not OK is pruned from the connector's modes >> list, >> * accompanied by a debug message indicating the reason for the mode's >> @@ -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 ... Best regards, Jose Miguel Abreu > >> }
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 :-)
On Sun, May 14, 2017 at 02:04:24PM +0300, Laurent Pinchart wrote: > 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: > > >> + 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 :-) Call it drm_connector_validate_mode, because the first argument is generally the object we operate on :-) -Daniel
On Monday 15 May 2017 08:47:49 Daniel Vetter wrote: > On Sun, May 14, 2017 at 02:04:24PM +0300, Laurent Pinchart wrote: > > 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: > >>>> + 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 :-) > > Call it drm_connector_validate_mode, because the first argument is > generally the object we operate on :-) But the function doesn't validate a mode for a connector, it validates a mode for a complete pipeline...
Hi Laurent, On 15-05-2017 08:05, Laurent Pinchart wrote: > On Monday 15 May 2017 08:47:49 Daniel Vetter wrote: >> On Sun, May 14, 2017 at 02:04:24PM +0300, Laurent Pinchart wrote: >>> 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: >>>>>> + 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 :-) >> Call it drm_connector_validate_mode, because the first argument is >> generally the object we operate on :-) > But the function doesn't validate a mode for a connector, it validates a mode > for a complete pipeline... > Hmm, but note that in the same function there is drm_mode_validate_size() and drm_mode_validate_flag() calls, which take as first argument the mode and then the object to validate (I hadn't seen this). So, maybe leave it as drm_mode_validate_connector() as it takes a connector as argument or change to drm_mode_validate_pipeline() as you said, or even drm_mode_validate_datapath(), drm_mode_validate_videopath(), drm_mode_validate_components() ? Best regards, Jose Miguel Abreu
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 @@ -39,6 +39,8 @@ #include <drm/drm_fb_helper.h> #include <drm/drm_edid.h> +#include "drm_crtc_internal.h" + /** * DOC: output probing helper overview * @@ -80,6 +82,54 @@ return MODE_OK; } +static enum drm_mode_status +drm_mode_validate_connector(struct drm_connector *connector, + struct drm_display_mode *mode) +{ + 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; +} + static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) { struct drm_cmdline_mode *cmdline_mode; @@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev) * - drm_mode_validate_flag() checks the modes against basic connector * capabilities (interlace_allowed,doublescan_allowed,stereo_allowed) * - the optional &drm_connector_helper_funcs.mode_valid helper can perform - * driver and/or hardware specific checks + * driver and/or sink specific checks + * - the optional &drm_crtc_helper_funcs.mode_valid and + * &drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or + * source specific checks which are also enforced by the modeset/atomic + * helpers * * 5. Any mode whose status is not OK is pruned from the connector's modes list, * accompanied by a debug message indicating the reason for the mode's @@ -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); }
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(-)