Re: [PATCH 25/29] drm/omap: Pass drm_display_mode to .check_timings() and .set_timings()

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

Hi Sebastian,

On Monday, 10 December 2018 00:26:28 EET Sebastian Reichel wrote:
> On Wed, Dec 05, 2018 at 05:00:18PM +0200, Laurent Pinchart wrote:
> > The omap_dss_device .check_timings() and .set_timings() operations
> > operate on struct videomode, while the DRM API operates on struct
> > drm_display_mode. This forces conversion from to videomode in the
> > callers. While that's not a problem per se, it creates a difference with
> > the drm_bridge API.
> > 
> > Replace the videomode parameter to the .check_timings() and
> > .set_timings() operations with a drm_display_mode. This pushed the
> > conversion to videomode down to the DSS devices in some cases. If needed
> > they will be converted to operate on drm_display_mode natively.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > [...]
> > diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c
> > b/drivers/gpu/drm/omapdrm/omap_connector.c index
> > 9882a4b1402b..487603c56b08 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_connector.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_connector.c
> > @@ -245,23 +245,19 @@ 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 videomode vm = { 0 };
> >  	int ret;
> > 
> > -	drm_display_mode_to_videomode(mode, &vm);
> > +	drm_mode_copy(adjusted_mode, mode);
> 
> This will explode if adjusted_mode is NULL?

Correct, but this function should never (and is never as far as I can tell) 
called with adjusted_mode equal to NULL.

> >  	for (; dssdev; dssdev = dssdev->next) {
> >  		if (!dssdev->ops->check_timings)
> >  			continue;
> > 
> > -		ret = dssdev->ops->check_timings(dssdev, &vm);
> > +		ret = dssdev->ops->check_timings(dssdev, adjusted_mode);
> > 
> >  		if (ret)
> >  			return MODE_BAD;
> >  	}
> > 
> > -	if (adjusted_mode)
> > -		drm_display_mode_from_videomode(&vm, adjusted_mode);
> > -
> >  	return MODE_OK;
> >  }
> 
> Old code assumed, that adjusted_mode may be NULL. If adjusted_mode
> cannot be NULL, it should be mentioned in the patch description.

I will instead remove the check in the previous patch where it was needlessly 
introduced.

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel