Re: [igt-dev] [PATCH i-g-t 2/4] tests/kms_plane_multiple: Add tiled-none plane position subtests

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

On Tue, Nov 27, 2018 at 06:36:53PM +0000, Kazlauskas, Nicholas wrote:
> On 11/13/18 12:15 PM, Ville Syrjälä wrote:
> > On Tue, Nov 13, 2018 at 05:02:04PM +0000, Kazlauskas, Nicholas wrote:
> >> On 11/13/18 11:58 AM, Ville Syrjälä wrote:
> >>> On Tue, Nov 13, 2018 at 10:23:48AM -0500, Nicholas Kazlauskas wrote:
> >>>> The per-pipe plane position subtests are capable of running on
> >>>> AMDGPU as long as they're not using i915 specific tiling formats.
> >>>>
> >>>> The test setup already supports being invoked with different tiling
> >>>> modes so this patch introduces the new 'tiled-none' subtest that runs
> >>>> without any tiling.
> >>>>
> >>>> The tiled-none tests are skipped on i915 to retain existing test
> >>>> coverage and behavior on i915.
> >>>>
> >>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> >>>> ---
> >>>>    tests/kms_plane_multiple.c | 12 ++++++++++--
> >>>>    1 file changed, 10 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
> >>>> index 721afe59..7c8adf94 100644
> >>>> --- a/tests/kms_plane_multiple.c
> >>>> +++ b/tests/kms_plane_multiple.c
> >>>> @@ -291,12 +291,16 @@ test_plane_position(data_t *data, enum pipe pipe, uint64_t tiling)
> >>>>    {
> >>>>    	igt_output_t *output;
> >>>>    	int connected_outs;
> >>>> -	int devid = intel_get_drm_devid(data->drm_fd);
> >>>>    	int n_planes = data->display.pipes[pipe].n_planes;
> >>>>    
> >>>> +	if (is_i915_device(data->drm_fd))
> >>>> +		igt_skip_on(tiling == LOCAL_DRM_FORMAT_MOD_NONE);
> >>>> +	else
> >>>> +		igt_skip_on(tiling != LOCAL_DRM_FORMAT_MOD_NONE);
> >>>
> >>> Might be nice to switch this over to using igt_plane_has_format_mod().
> >>> We'd need to push it down a bit further since we'd need to query each
> >>> plane in turn.
> >>
> >> That definitely seems like a more elegant solution, I'll fix that up for
> >> this and the other patch too.
> > 
> > I forget, does amggpu support modifiers? If not you will need this:
> > https://patchwork.freedesktop.org/patch/239808/
> 
> There's no IN_FORMATS support for AMDGPU yet. I had tried adding it with 
> a quick patch but it causes a lot of breakages in userspace so it's not 
> as simple as I had hoped.
> 
> I have v2s that make use of the new helpers but without IN_FOMRATS 
> support they skip. The patch series you linked would be really nice to 
> have in this case since it still allows the helpers to be used without 
> it. I don't mind leaving a R-B on the patch you linked.
> 
> But if there isn't any plan to have it merged I can always just do 
> something like:
> 
> igt_skip_on(tiling != LOCAL_DRM_FORMAT_MOD_NONE && 
> !igt_plane_has_format_mod(...))
> 
> Let me know what you think.

I'd like to have that patch merged. So r-b away please.

IIRC the point of contention was whether my proposed behaviour is
actually acceptable for driver w/o modifiers. Since you seem to have
one I think an r-b from you would be a good indicator that my
plan was in fact sane ;)


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