Hi Sebastian,
On Monday, 10 December 2018 00:19:22 EET Sebastian Reichel wrote:
> Hi,
>
> On Wed, Dec 05, 2018 at 05:00:17PM +0200, Laurent Pinchart wrote:
> > The encoder .atomic_check() and connector .mode_valid() operations both
> > walk through the dss devices in the pipeline to validate the mode.
> > Factor out the common code in a new omap_drm_connector_mode_fixup()
> > function.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
>
> This is a bit tricky to review. It would probably be easier to
> review, if the changes were split into two commits:
>
> 1. introduce common function
> 2. move drm_display_mode/videomode conversion further up the stack
I agree that the connector changes are not very nicely formatted. I however
don't like splitting patches, especially when small, in such a way.
One issue preventing a better diff formatting is the change from int to enum
drm_mode_status for the omap_connector_mode_valid() function. Without that
change, the patch can be better formatted with
git diff --anchored="static int omap_connector_mode_valid"
I agree it's still not perfect.
If you think the above change is worth it I'll submit a split version,
otherwise I'll leave it as is.
> Anyways:
>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
>
> -- Sebastian
>
> > drivers/gpu/drm/omapdrm/omap_connector.c | 55 +++++++++++++-----------
> > drivers/gpu/drm/omapdrm/omap_connector.h | 5 +++
> > drivers/gpu/drm/omapdrm/omap_encoder.c | 30 ++++---------
> > 3 files changed, 45 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c
> > b/drivers/gpu/drm/omapdrm/omap_connector.c index
> > dd1e0f2e8ffc..9882a4b1402b 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_connector.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_connector.c
> > @@ -241,50 +241,57 @@ static int omap_connector_get_modes(struct
> > drm_connector *connector)>
> > return 0;
> >
> > }
> >
> > -static int omap_connector_mode_valid(struct drm_connector *connector,
> > - struct drm_display_mode *mode)
> > +enum drm_mode_status omap_connector_mode_fixup(struct omap_dss_device
> > *dssdev, + const struct drm_display_mode *mode,
> > + struct drm_display_mode *adjusted_mode)
> >
> > {
> >
> > - struct omap_connector *omap_connector = to_omap_connector(connector);
> > - struct omap_dss_device *dssdev;
> > - struct videomode vm = {0};
> > - struct drm_device *dev = connector->dev;
> > - struct drm_display_mode *new_mode;
> > - int r, ret = MODE_BAD;
> > + struct videomode vm = { 0 };
> > + int ret;
> >
> > drm_display_mode_to_videomode(mode, &vm);
> >
> > - mode->vrefresh = drm_mode_vrefresh(mode);
> >
> > - for (dssdev = omap_connector->output; dssdev; dssdev = dssdev->next) {
> > + for (; dssdev; dssdev = dssdev->next) {
> >
> > if (!dssdev->ops->check_timings)
> >
> > continue;
> >
> > - r = dssdev->ops->check_timings(dssdev, &vm);
> > - if (r)
> > - goto done;
> > + ret = dssdev->ops->check_timings(dssdev, &vm);
> > + if (ret)
> > + return MODE_BAD;
> >
> > }
> >
> > - /* check if vrefresh is still valid */
> > - new_mode = drm_mode_duplicate(dev, mode);
> > - if (!new_mode)
> > - return MODE_BAD;
> > + if (adjusted_mode)
> > + drm_display_mode_from_videomode(&vm, adjusted_mode);
> >
> > - new_mode->clock = vm.pixelclock / 1000;
> > - new_mode->vrefresh = 0;
> > - if (mode->vrefresh == drm_mode_vrefresh(new_mode))
> > - ret = MODE_OK;
> > - drm_mode_destroy(dev, new_mode);
> > + return MODE_OK;
> > +}
> > +
> > +static enum drm_mode_status omap_connector_mode_valid(struct
> > drm_connector *connector, + struct drm_display_mode *mode)
> > +{
> > + struct omap_connector *omap_connector = to_omap_connector(connector);
> > + struct drm_display_mode new_mode = { { 0 } };
> > + enum drm_mode_status status;
> > +
> > + status = omap_connector_mode_fixup(omap_connector->output, mode,
> > + &new_mode);
> > + if (status != MODE_OK)
> > + goto done;
> > +
> > + /* Check if vrefresh is still valid. */
> > + if (drm_mode_vrefresh(mode) != drm_mode_vrefresh(&new_mode))
> > + status = MODE_NOCLOCK;
> >
> > done:
> > DBG("connector: mode %s: "
> >
> > "%d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x",
> >
> > - (ret == MODE_OK) ? "valid" : "invalid",
> > + (status == MODE_OK) ? "valid" : "invalid",
> >
> > mode->base.id, mode->name, mode->vrefresh, mode->clock,
> > mode->hdisplay, mode->hsync_start,
> > mode->hsync_end, mode->htotal,
> > mode->vdisplay, mode->vsync_start,
> > mode->vsync_end, mode->vtotal, mode->type, mode->flags);
> >
> > - return ret;
> > + return status;
> >
> > }
> >
> > static const struct drm_connector_funcs omap_connector_funcs = {
> >
> > diff --git a/drivers/gpu/drm/omapdrm/omap_connector.h
> > b/drivers/gpu/drm/omapdrm/omap_connector.h index
> > 4a1dcd0f031b..6b7d4d95e32b 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_connector.h
> > +++ b/drivers/gpu/drm/omapdrm/omap_connector.h
> > @@ -22,6 +22,8 @@
> >
> > #include <linux/types.h>
> >
> > +enum drm_mode_status;
> > +
> >
> > struct drm_connector;
> > struct drm_device;
> > struct drm_encoder;
> >
> > @@ -34,5 +36,8 @@ struct drm_connector *omap_connector_init(struct
> > drm_device *dev,>
> > bool omap_connector_get_hdmi_mode(struct drm_connector *connector);
> > void omap_connector_enable_hpd(struct drm_connector *connector);
> > void omap_connector_disable_hpd(struct drm_connector *connector);
> >
> > +enum drm_mode_status omap_connector_mode_fixup(struct omap_dss_device
> > *dssdev, + const struct drm_display_mode *mode,
> > + struct drm_display_mode *adjusted_mode);
> >
> > #endif /* __OMAPDRM_CONNECTOR_H__ */
> >
> > diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c
> > b/drivers/gpu/drm/omapdrm/omap_encoder.c index 7d01df6fa723..086963088201
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_encoder.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_encoder.c
> > @@ -198,29 +198,17 @@ static int omap_encoder_atomic_check(struct
> > drm_encoder *encoder,>
> > struct drm_connector_state *conn_state)
> >
> > {
> >
> > struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
> >
> > - struct drm_device *dev = encoder->dev;
> > - struct omap_dss_device *dssdev;
> > - struct videomode vm = { 0 };
> > - int ret;
> > -
> > - drm_display_mode_to_videomode(&crtc_state->mode, &vm);
> > -
> > - for (dssdev = omap_encoder->output; dssdev; dssdev = dssdev->next) {
> > - if (!dssdev->ops->check_timings)
> > - continue;
> > -
> > - ret = dssdev->ops->check_timings(dssdev, &vm);
> > - if (ret)
> > - goto done;
> > + enum drm_mode_status status;
> > +
> > + status = omap_connector_mode_fixup(omap_encoder->output,
> > + &crtc_state->mode,
> > + &crtc_state->adjusted_mode);
> > + if (status != MODE_OK) {
> > + dev_err(encoder->dev->dev, "invalid timings: %d\n", status);
> > + return -EINVAL;
> >
> > }
> >
> > - drm_display_mode_from_videomode(&vm, &crtc_state->adjusted_mode);
> > -
> > -done:
> > - if (ret)
> > - dev_err(dev->dev, "invalid timings: %d\n", ret);
> > -
> > - return ret;
> > + return 0;
> >
> > }
> >
> > static const struct drm_encoder_helper_funcs omap_encoder_helper_funcs =
> > {
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
