[PATCH v2] Handle failures in setting a CRTC to a DRM mode properly

[Emil Velikov]

On 27 October 2015 at 02:07, Michel Dänzer <michel at daenzer.net> wrote:
> On 26.10.2015 20:01, Emil Velikov wrote:
>> On 26 October 2015 at 10:55, Daniel Martin <consume.noise at gmail.com> wrote:
>>> On 26 October 2015 at 10:39, Michel Dänzer <michel at daenzer.net> wrote:
>>>>> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
>>>>> index 64e79d4..f0f121e 100644
>>>>> --- a/src/drmmode_display.c
>>>>> +++ b/src/drmmode_display.c
>>>>> @@ -760,12 +760,15 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
>>>>>                               radeon_bo_wait(drmmode_crtc->scanout[0].bo);
>>>>>                       }
>>>>>               }
>>>>> -             ret = drmModeSetCrtc(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id,
>>>>> -                                  fb_id, x, y, output_ids, output_count, &kmode);
>>>>> -             if (ret)
>>>>> +             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(-ret));
>>>>
>>>> errno must be passed instead of -ret to strerror. I pushed the patch
>>>> with that fixed, thanks!
>>>
>>> While you're at it, can you add the missing linebreak to the message?
>>> Just hit it and it looks suspicious:
>>>     (EE) modeset(0): failed to set mode: Invalid argument(--) RandR disabled
>>> or
>>>     (EE) modeset(0): failed to set mode: Invalid argument(II)
>>> modeset(0): Setting screen physical size to 677 x 381
>>>
>> Mildly related:
>>  - Using -ret opposed to errno -> intel, nouveau, modeset ddx
>
> The -ret => errno change is only necessary because we're no longer
> assigning the return value of drmModeSetCrtc (which is -errno) to ret.
> This doesn't apply to intel or nouveau (or to modesetting without the
> fix above).
>
>From my archaeology experiences in libdrm, not all of its APIs return
-errno on error. So I got a bit confused there.
The modeset case(s) those seems to be set and consistent afaict.

>
>>  - Incorrectly handled failure and missing \n -> modeset ddx
>
> Yep, in fact Daniel is using the modesetting driver, not radeon.
>
> Patches welcome for both cases. :)
>
... as does amdgpu. I'll let those using the said drivers to port the fix :-P

Cheers,
Emil