linux-kernel - Re: [PATCH 01/20] drm/atomic: Fix remaining places where !funcs->best

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 2 Jun 2016 23:57:02 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:	Boris Brezillon <boris.brezillon@...e-electrons.com>,
	David Airlie <airlied@...ux.ie>,
	dri-devel <dri-devel@...ts.freedesktop.org>,
	Daniel Vetter <daniel.vetter@...el.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Kukjin Kim <kgene@...nel.org>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
	intel-gfx <intel-gfx@...ts.freedesktop.org>,
	Jani Nikula <jani.nikula@...ux.intel.com>,
	Alexey Brodkin <abrodkin@...opsys.com>,
	Inki Dae <inki.dae@...sung.com>,
	Joonyoung Shim <jy0922.shim@...sung.com>,
	Seung-Woo Kim <sw0312.kim@...sung.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Stefan Agner <stefan@...er.ch>,
	Alison Wang <alison.wang@...escale.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Rob Clark <robdclark@...il.com>,
	Mark Yao <mark.yao@...k-chips.com>,
	Heiko Stuebner <heiko@...ech.de>,
	Benjamin Gaignard <benjamin.gaignard@...aro.org>,
	Vincent Abriou <vincent.abriou@...com>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Chen-Yu Tsai <wens@...e.org>,
	Thierry Re ding <thierry.reding@...il.com>,
	Stephen Warren <swarren@...dotorg.org>,
	Alexandre Courbot <gnurou@...il.com>,
	Eric Anholt <eric@...olt.net>,
	Gerd Hoffmann <kraxel@...hat.com>,
	linux-mediatek@...ts.infradead.org,
	linux-arm-msm <linux-arm-msm@...r.kernel.org>,
	freedreno@...ts.freedesktop.org,
	"open list:DRM DRIVERS FOR RENESAS" 
	<linux-renesas-soc@...r.kernel.org>,
	"open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"open list:VIRTIO CORE, NET..." 
	<virtualization@...ts.linux-foundation.org>
Subject: Re: [PATCH 01/20] drm/atomic: Fix remaining places where
 !funcs->best_encoder is valid

On Thu, Jun 2, 2016 at 11:05 PM, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
> Hi Boris,
>
> Thank you for the patch.
>
> On Thursday 02 Jun 2016 16:31:28 Boris Brezillon wrote:
>> Adapt drm_pick_crtcs() and update_connector_routing() to fallback to
>> drm_atomic_helper_best_encoder() if funcs->best_encoder() is NULL so
>> that DRM drivers can leave this hook unassigned if they know they want
>> to use drm_atomic_helper_best_encoder().
>
> Could you please update include/drm/drm_modeset_helper_vtables.h to document
> this new behaviour ?

Thanks for reminding me. Please update hooks for both
atomic_best_encoder and best_encoder. Also mention that it's only
optional for atomic drivers. There's lots of examples in that file for
the wording usually used.

> The only drawback I see in this patch is the additional object lookup
> performed by drm_atomic_helper_best_encoder() at runtime. I wonder if we could
> somehow cache the information, as the assignment can't change when there's a
> 1:1 correspondence between encoders and connectors.

drm_encoder_find is an idr lookup. That should be plenty fast,
especially for modeset code. Usually what's too expensive even for
modeset code is linear list walks. But Chris just submitted patches to
convert most of them into simple lookups.
-Daniel

>>> Signed-off-by: Boris Brezillon <boris.brezillon@...e-electrons.com>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c |  4 +++-
>>  drivers/gpu/drm/drm_fb_helper.c     | 13 ++++++++++++-
>>  2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c index f6a3350..849d029 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -300,8 +300,10 @@ update_connector_routing(struct drm_atomic_state
>> *state, if (funcs->atomic_best_encoder)
>>               new_encoder = funcs->atomic_best_encoder(connector,
>>                                                        connector_state);
>> -     else
>> +     else if (funcs->best_encoder)
>>               new_encoder = funcs->best_encoder(connector);
>> +     else
>> +             new_encoder = drm_atomic_helper_best_encoder(connector);
>>
>>       if (!new_encoder) {
>>               DRM_DEBUG_ATOMIC("No suitable encoder found for [CONNECTOR:%d:%s]\n",
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> b/drivers/gpu/drm/drm_fb_helper.c index 7c2eb75..d44389a 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -2000,7 +2000,18 @@ static int drm_pick_crtcs(struct drm_fb_helper
>> *fb_helper, my_score++;
>>
>>       connector_funcs = connector->helper_private;
>> -     encoder = connector_funcs->best_encoder(connector);
>> +
>> +     /*
>> +      * If the DRM device implements atomic hooks and ->best_encoder() is
>> +      * NULL we fallback to the default drm_atomic_helper_best_encoder()
>> +      * helper.
>> +      */
>> +     if (fb_helper->dev->mode_config.funcs->atomic_commit &&
>> +         !connector_funcs->best_encoder)
>> +             encoder = drm_atomic_helper_best_encoder(connector);
>> +     else
>> +             encoder = connector_funcs->best_encoder(connector);
>> +
>>       if (!encoder)
>>               goto out;
>
> --
> Regards,
>
> Laurent Pinchart
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch