Re: [PATCH v3 16/16] drm: Replace mode->export_head with a boolean

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Sam Ravnborg <sam@ravnborg.org>,
	ML dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v3 16/16] drm: Replace mode->export_head with a boolean
Date: Tue, 1 Sep 2020 13:58:00 +0300	[thread overview]
Message-ID: <20200901105800.GE6112@intel.com> (raw)
In-Reply-To: <CACvgo50i8sqhDAyWawcaPUSd=GkKLFWJ_DVSHeq8UvJBh3OwRQ@mail.gmail.com>

On Thu, Apr 30, 2020 at 02:50:52PM +0100, Emil Velikov wrote:
> Hi Ville
> 
> I don't fully grok the i915 changes to provide meaningful review.
> There are couple of small comments below, but regardless of those

Sorry, forgot to reply to this in a timely manner.

> 
> Patches 01-11 and 14-16 are:
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> 
> On Tue, 28 Apr 2020 at 18:20, Ville Syrjala
> <ville.syrjala@linux.intel.com> wrote:
> 
> > The downside is that drm_mode_expose_to_userspace() gets to
> > iterate a few more modes. It already was O(n^2), now it's
> > a slightly worse O(n^2).
> >
> Personally I'd drop the O() sentence, or change it to
> It already was O(n^2), now it's slightly worse O((n+y)^2).

Dropped.

> 
> > Another alternative would be a temp bitmask so we wouldn't have
> > to have anything in the mode struct itself. The main issue is
> > how large of a bitmask do we need? I guess we could allocate
> > it dynamically but that means an extra kcalloc() and an extra
> > loop through the modes to count them first (or grow the bitmask
> > with krealloc() as needed).
> >
> If the walk is even remotely close to being an issue, we could
> consider the bitmask.
> I don't think that's the case yet.
> 
> 
> Hmm the original code never discards any entries from export_head.
> I wonder if there's some corner case where we could end with an "old"
> mode showing in the list?

No. export_list starts out empty so only the modes we explicitly add
to the list can be reached. Thus any dangling pointers in some other
mode's export_head are of no concern.

Pushed the last few patches to drm-misc-next. Thanks for the reviews
everyone.

> 
> For example:
> - creates a user mode via drmModeCreatePropertyBlob()
> - calls drmModeGetConnector() and sees their mode
> - optional (?) does a modeset to and away from said mode
> - removes their blob drmModeDestroyPropertyBlob()
> - calls drmModeGetConnector() and still sees their removed mode.
> 
> If this is a bug (?) that we care about, we might want to add an igt
> test for it.
> Conversely, if this is a behaviour we want to keep this patch needs some work.
> 
> HTH
> 
> Emil

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel