Re: [PATCH 24/29] drm/omap: Factor out common mode validation code

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@ideasonboard.com>
> > ---
> 
> 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@collabora.com>
> 
> -- 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel