| Message ID | 9fa7d9826e3793989fb52a2911d37b41ddfc0580.1494347165.git.joabreu@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 09, 2017 at 06:00:09PM +0100, Jose Abreu wrote: > Add a new helper to call crtc->mode_valid callback. > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > 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> > --- > drivers/gpu/drm/drm_crtc.c | 22 ++++++++++++++++++++++ > drivers/gpu/drm/drm_crtc_internal.h | 3 +++ > 2 files changed, 25 insertions(+) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 5af25ce..07ae705 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -38,6 +38,7 @@ > #include <drm/drm_crtc.h> > #include <drm/drm_edid.h> > #include <drm/drm_fourcc.h> > +#include <drm/drm_modeset_helper_vtables.h> > #include <drm/drm_modeset_lock.h> > #include <drm/drm_atomic.h> > #include <drm/drm_auth.h> > @@ -741,3 +742,24 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj, > > return ret; > } > + > +/** > + * drm_crtc_mode_valid - call crtc->mode_valid callback, if any. > + * @crtc: crtc > + * @mode: mode to be validated > + * > + * If no mode_valid callback is available this will return MODE_OK. > + * > + * Returns: drm_mode_status Enum > + */ > +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc, > + const struct drm_display_mode *mode) > +{ > + const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; This is clearly a helper func, but you place it into the core and EXPORT_SYMBOL it. Imo this should be entirely internal to the helpers, perhaps just stuff them all into drm_probe_helpers.c? Header file would be drm_crtc_helper_internal.h. That also means no need for kernel-doc (only the driver api is formally documented) and then these 3 patches are so tiny it's better to squash them into the patch that adds their users. Thanks, Daniel > + > + if (!crtc_funcs || !crtc_funcs->mode_valid) > + return MODE_OK; > + > + return crtc_funcs->mode_valid(crtc, mode); > +} > +EXPORT_SYMBOL(drm_crtc_mode_valid); > diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h > index d077c54..3800abd 100644 > --- a/drivers/gpu/drm/drm_crtc_internal.h > +++ b/drivers/gpu/drm/drm_crtc_internal.h > @@ -45,6 +45,9 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc, > > struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc); > > +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc, > + const struct drm_display_mode *mode); > + > /* IOCTLs */ > int drm_mode_getcrtc(struct drm_device *dev, > void *data, struct drm_file *file_priv); > -- > 1.9.1 > >
Hi Daniel, On 10-05-2017 08:59, Daniel Vetter wrote: > On Tue, May 09, 2017 at 06:00:09PM +0100, Jose Abreu wrote: >> Add a new helper to call crtc->mode_valid callback. >> >> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> 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> >> --- >> drivers/gpu/drm/drm_crtc.c | 22 ++++++++++++++++++++++ >> drivers/gpu/drm/drm_crtc_internal.h | 3 +++ >> 2 files changed, 25 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index 5af25ce..07ae705 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -38,6 +38,7 @@ >> #include <drm/drm_crtc.h> >> #include <drm/drm_edid.h> >> #include <drm/drm_fourcc.h> >> +#include <drm/drm_modeset_helper_vtables.h> >> #include <drm/drm_modeset_lock.h> >> #include <drm/drm_atomic.h> >> #include <drm/drm_auth.h> >> @@ -741,3 +742,24 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj, >> >> return ret; >> } >> + >> +/** >> + * drm_crtc_mode_valid - call crtc->mode_valid callback, if any. >> + * @crtc: crtc >> + * @mode: mode to be validated >> + * >> + * If no mode_valid callback is available this will return MODE_OK. >> + * >> + * Returns: drm_mode_status Enum >> + */ >> +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc, >> + const struct drm_display_mode *mode) >> +{ >> + const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; > This is clearly a helper func, but you place it into the core and > EXPORT_SYMBOL it. Imo this should be entirely internal to the helpers, > perhaps just stuff them all into drm_probe_helpers.c? Header file would be > drm_crtc_helper_internal.h. Yeah, at first I was not planning to export it but then I saw that drm_bridge_mode_fixup() is exported (and is in drm_bridge.c) so it kind of felt right to place this in drm_crtc.c. Anyway, I will move them to drm_probe_helpers.c, indeed there is no point in exporting this. > > That also means no need for kernel-doc (only the driver api is formally > documented) and then these 3 patches are so tiny it's better to squash > them into the patch that adds their users. Ok, will remove the docs but I think its better to have a single patch which adds all the helpers so that I can use the suggested-by tag. Thanks! Best regards, Jose Miguel Abreu > > Thanks, Daniel >> + >> + if (!crtc_funcs || !crtc_funcs->mode_valid) >> + return MODE_OK; >> + >> + return crtc_funcs->mode_valid(crtc, mode); >> +} >> +EXPORT_SYMBOL(drm_crtc_mode_valid); >> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h >> index d077c54..3800abd 100644 >> --- a/drivers/gpu/drm/drm_crtc_internal.h >> +++ b/drivers/gpu/drm/drm_crtc_internal.h >> @@ -45,6 +45,9 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc, >> >> struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc); >> >> +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc, >> + const struct drm_display_mode *mode); >> + >> /* IOCTLs */ >> int drm_mode_getcrtc(struct drm_device *dev, >> void *data, struct drm_file *file_priv); >> -- >> 1.9.1 >> >>
On Wed, May 10, 2017 at 09:57:30AM +0100, Jose Abreu wrote: > Hi Daniel, > > > On 10-05-2017 08:59, Daniel Vetter wrote: > > On Tue, May 09, 2017 at 06:00:09PM +0100, Jose Abreu wrote: > >> Add a new helper to call crtc->mode_valid callback. > >> > >> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> 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> > >> --- > >> drivers/gpu/drm/drm_crtc.c | 22 ++++++++++++++++++++++ > >> drivers/gpu/drm/drm_crtc_internal.h | 3 +++ > >> 2 files changed, 25 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > >> index 5af25ce..07ae705 100644 > >> --- a/drivers/gpu/drm/drm_crtc.c > >> +++ b/drivers/gpu/drm/drm_crtc.c > >> @@ -38,6 +38,7 @@ > >> #include <drm/drm_crtc.h> > >> #include <drm/drm_edid.h> > >> #include <drm/drm_fourcc.h> > >> +#include <drm/drm_modeset_helper_vtables.h> > >> #include <drm/drm_modeset_lock.h> > >> #include <drm/drm_atomic.h> > >> #include <drm/drm_auth.h> > >> @@ -741,3 +742,24 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj, > >> > >> return ret; > >> } > >> + > >> +/** > >> + * drm_crtc_mode_valid - call crtc->mode_valid callback, if any. > >> + * @crtc: crtc > >> + * @mode: mode to be validated > >> + * > >> + * If no mode_valid callback is available this will return MODE_OK. > >> + * > >> + * Returns: drm_mode_status Enum > >> + */ > >> +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc, > >> + const struct drm_display_mode *mode) > >> +{ > >> + const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; > > This is clearly a helper func, but you place it into the core and > > EXPORT_SYMBOL it. Imo this should be entirely internal to the helpers, > > perhaps just stuff them all into drm_probe_helpers.c? Header file would be > > drm_crtc_helper_internal.h. > > Yeah, at first I was not planning to export it but then I saw > that drm_bridge_mode_fixup() is exported (and is in drm_bridge.c) > so it kind of felt right to place this in drm_crtc.c. Anyway, I > will move them to drm_probe_helpers.c, indeed there is no point > in exporting this. Bridge is a bit special, since there's the bridge integration through atomic/legacy helpers, but drivers could also wire up a bridge on their own (i.e. without using the drm_encoder->bridge pointer). Not sure anyone is doing that right now, but that was the idea behind having the helpers all exported. -Daniel
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 5af25ce..07ae705 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -38,6 +38,7 @@ #include <drm/drm_crtc.h> #include <drm/drm_edid.h> #include <drm/drm_fourcc.h> +#include <drm/drm_modeset_helper_vtables.h> #include <drm/drm_modeset_lock.h> #include <drm/drm_atomic.h> #include <drm/drm_auth.h> @@ -741,3 +742,24 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj, return ret; } + +/** + * drm_crtc_mode_valid - call crtc->mode_valid callback, if any. + * @crtc: crtc + * @mode: mode to be validated + * + * If no mode_valid callback is available this will return MODE_OK. + * + * Returns: drm_mode_status Enum + */ +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc, + const struct drm_display_mode *mode) +{ + const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; + + if (!crtc_funcs || !crtc_funcs->mode_valid) + return MODE_OK; + + return crtc_funcs->mode_valid(crtc, mode); +} +EXPORT_SYMBOL(drm_crtc_mode_valid); diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index d077c54..3800abd 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -45,6 +45,9 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc, struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc); +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc, + const struct drm_display_mode *mode); + /* IOCTLs */ int drm_mode_getcrtc(struct drm_device *dev, void *data, struct drm_file *file_priv);
Add a new helper to call crtc->mode_valid callback. Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> 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> --- drivers/gpu/drm/drm_crtc.c | 22 ++++++++++++++++++++++ drivers/gpu/drm/drm_crtc_internal.h | 3 +++ 2 files changed, 25 insertions(+)