Re: [PATCH i-g-t 4/5] tests/kms_cursor_crc: Test the SIZE_HINTS property

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Apr 04, 2024 at 03:52:50PM +0300, Pekka Paalanen wrote:
> On Thu, 4 Apr 2024 13:56:02 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Thu, Apr 04, 2024 at 11:51:05AM +0300, Pekka Paalanen wrote:
> > > On Fri, 15 Mar 2024 21:15:04 +0200
> > > Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > >   
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Make sure the driver accepts all the sizes declared in the
> > > > plane's SIZE_HINTS property.
> > > > 
> > > > TODO: Actually test that each reported size works fully?  
> > > 
> > > What does this mean? What does cursor_size_supported() do?  
> > 
> > It does a TEST_ONLY commit using data->fb and the specified size.
> > So just confirms that the kernel accepts it, not that it actually
> > looks correct on screen.
> 
> Good, that's better than my first impression.
> 
> > > I don't know that, so it looks like you just create an FB and destroy
> > > it, never putting it on the plane where the SIZE_HINTS were found.
> > >   
> > > >       Would need a bunch of restructuring of the test,
> > > >       and for the moment i915 is the only driver that
> > > >       supports SIZE_HINTS and it's already >100% covered
> > > >       by the existing tests. So I'll leave this for the
> > > >       next guy whose driver has different needs...  
> > > 
> > > How would they know to fix this test? They just see the test pass and
> > > are happy, right?  
> > 
> > I would expect them to actually care about testing their driver
> > properly. But perhaps my expectations are a bit high.
> 
> Yeah, a screenshot or CRC test would be best.

Yep. We have CRC tests, but the impelementation would need to be
completely flipped on its head to iterate the sizes as the inner
loop rather than as the outer loop. Not impossible, just a bunch
of work (and wouldn't extend the coverage for i915 at all).

-- 
Ville Syrjälä
Intel