Re: [PATCH v2 01/14] drm/fb-helper: keep the .gamma_store updated in drm_fb_helper_setcmap

[Daniel Vetter]

On Thu, Jun 22, 2017 at 08:06:24AM +0200, Peter Rosin wrote:
> I think the gamma_store can end up invalid on error. But the way I read
> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should
> this pesky legacy fbdev stuff be any better?
> 
> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 574af01..25315fb 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1223,12 +1223,16 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  	const struct drm_crtc_helper_funcs *crtc_funcs;
>  	u16 *red, *green, *blue, *transp;
>  	struct drm_crtc *crtc;
> +	u16 *r, *g, *b;
>  	int i, j, rc = 0;
>  	int start;
>  
>  	if (oops_in_progress)
>  		return -EBUSY;
>  
> +	if (cmap->start + cmap->len < cmap->start)
> +		return -EINVAL;

Doesn't the fbdev core check this for us?

> +
>  	drm_modeset_lock_all(dev);
>  	if (!drm_fb_helper_is_bound(fb_helper)) {
>  		drm_modeset_unlock_all(dev);
> @@ -1245,6 +1249,29 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  		transp = cmap->transp;
>  		start = cmap->start;
>  
> +		if (info->fix.visual != FB_VISUAL_TRUECOLOR) {

I think switching to a positive check of visual == PSEUDOCOLOR is a bit
clearer. Also this makes more sense once we move over to the main
gamma_set function, since as-is it's unclear that setcolreg only does
something for PSEUDOCOLOR. But interim I guess that's ok.
-Daniel

> +			if (!crtc->gamma_size) {
> +				rc = -EINVAL;
> +				goto out;
> +			}
> +
> +			if (cmap->start + cmap->len > crtc->gamma_size) {
> +				rc = -EINVAL;
> +				goto out;
> +			}
> +
> +			r = crtc->gamma_store;
> +			g = r + crtc->gamma_size;
> +			b = g + crtc->gamma_size;
> +
> +			memcpy(r + cmap->start, cmap->red,
> +			       cmap->len * sizeof(u16));
> +			memcpy(g + cmap->start, cmap->green,
> +			       cmap->len * sizeof(u16));
> +			memcpy(b + cmap->start, cmap->blue,
> +			       cmap->len * sizeof(u16));
> +		}
> +
>  		for (j = 0; j < cmap->len; j++) {
>  			u16 hred, hgreen, hblue, htransp = 0xffff;
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch