[prev in list] [next in list] [prev in thread] [next in thread] 

List:       freedesktop-xorg-devel
Subject:    Re: [PATCH] Handle failures in setting a CRTC to a DRM mode properly
From:       Michel_Dänzer <michel () daenzer ! net>
Date:       2015-10-23 8:43:49
Message-ID: 5629F345.2020102 () daenzer ! net
[Download RAW message or body]

On 22.10.2015 03:52, cpaul@redhat.com wrote:
> From: Stephen Chandler Paul <cpaul@redhat.com>
> 
> This fixes a bug where running the card out of PPLL's when hotplugging
> another monitor would result in all of the displays going blank and
> failing to work properly until X was restarted or the user switched to
> another VT.
> 
> Signed-off-by: Stephen Chandler Paul <cpaul@redhat.com>
> ---
>  src/drmmode_display.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index 64e79d4..5d69fbf 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -762,10 +762,12 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
>  		}
>  		ret = drmModeSetCrtc(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id,
>  				     fb_id, x, y, output_ids, output_count, &kmode);
> -		if (ret)
> +		if (ret) {
>  			xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
>  				   "failed to set mode: %s", strerror(-ret));
> -		else
> +			ret = FALSE;
> +			goto done;
> +		} else
>  			ret = TRUE;
>  
>  		if (crtc->scrn->pScreen)
> 

Thanks for the good catch, again.

I think assigning the return value of drmModeSetCrtc to ret (which is
of type Bool) is just a bit confusing. How about this:

		if (drmModeSetCrtc(drmmode->fd,
				   drmmode_crtc->mode_crtc->crtc_id,
				   fb_id, x, y, output_ids,
				   output_count, &kmode) != 0) {
			xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
				   "failed to set mode: %s",
				   strerror(errno));
			ret = FALSE;
			goto done;
		} else
			ret = TRUE;


That would get my

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[prev in list] [next in list] [prev in thread] [next in thread]