Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <CGME20180727101302epcas5p22561edd1d60101e611933ccff3e3d008@epcas5p2.samsung.com>
 <20180727101302epcas5p22561edd1d60101e611933ccff3e3d008~FMiUBZnCN2101321013epcas5p2Z@epcas5p2.samsung.com>
In-Reply-To: <20180727101302epcas5p22561edd1d60101e611933ccff3e3d008~FMiUBZnCN2101321013epcas5p2Z@epcas5p2.samsung.com>
From:   Daniel Stone <daniel@fooishbar.org>
Date:   Fri, 24 Aug 2018 14:58:16 +0100
Message-ID: <CAPj87rPY6gTjTVdxZ4qtwtNhQPKyrfxeN2_=Z6qH3SoO9QYw+g@mail.gmail.com>
Subject: Re: [PATCH] drm/kms/crtc: Improving the func drm_mode_setcrtc
To:     Satendra Singh Thakur <satendra.t@samsung.com>
Cc:     Gustavo Padovan <gustavo@padovan.org>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Sean Paul <seanpaul@chromium.org>,
        David Airlie <airlied@linux.ie>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        vineet.j@samsung.com, hemanshu.s@samsung.com, sst2005@gmail.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Satendra,

On Fri, 27 Jul 2018 at 11:13, Satendra Singh Thakur
<satendra.t@samsung.com> wrote:
> Following changes are done to this func:

I would certainly agree with Sean, Martin, and Jani's comments. This
patch was difficult to follow as it made so many changes at once.
Being a crucial function which has many subtle details, it is
important to keep the _changes_ as readable as possible: not just the
final result, but the intermediate patches.

Another thing you might consider is running the igt test suite after
your patches, particularly when touching core code. Running igt
would've flagged the below:

> +       /* Check if the flag is set*/
> +       if (!crtc_req->mode_valid) {
> +               DRM_DEBUG_KMS("mode_valid flag [%d] is not set\n",
> +               crtc_req->mode_valid);
> +               return -EINVAL;
> +       }

This is a change from the previous behaviour, and not correct.
mode_valid is allowed to be NULL if there are also no connectors and
there is no FB: this disables the CRTC.

> +       /* Check the validity of count_connectors supplied by user */
> +       if (!crtc_req->count_connectors ||
> +               crtc_req->count_connectors > config->num_connector) {
> +               DRM_DEBUG_KMS("Invalid connectors' count %d\n",
> +               crtc_req->count_connectors);
> +               return -EINVAL;
> +       }

Same comment applies here.

Cheers,
Daniel