thr3ads.net - Nouveau - [Nouveau] [PATCH 1/2] drm/nouveau: Disable atomic support on a per-device basis [Sep 2018]

If this information is useful, please help other people find it:
Share via:

Ville Syrjala

2018-Sep-13 16:31 UTC

[Nouveau] [PATCH 1/2] drm/nouveau: Disable atomic support on a per-device basis

From: Ville Syrjälä <ville.syrjala at linux.intel.com>

We now have per-device driver_features, so let's use that
to disable atomic only for pre-nv50.

Cc: Ben Skeggs <bskeggs at redhat.com>
Cc: Lyude Paul <lyude at redhat.com>
Cc: nouveau at lists.freedesktop.org
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
---
 drivers/gpu/drm/nouveau/dispnv04/disp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.c
b/drivers/gpu/drm/nouveau/dispnv04/disp.c
index 70dce544984e..670535a68d3b 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/disp.c
@@ -56,7 +56,7 @@ nv04_display_create(struct drm_device *dev)
 	nouveau_display(dev)->fini = nv04_display_fini;
 
 	/* Pre-nv50 doesn't support atomic, so don't expose the ioctls */
-	dev->driver->driver_features &= ~DRIVER_ATOMIC;
+	dev->driver_features &= ~DRIVER_ATOMIC;
 
 	nouveau_hw_save_vga_fonts(dev, 1);
 
-- 
2.16.4

Lyude Paul

2018-Sep-13 21:02 UTC

head link

[Nouveau] [PATCH 1/2] drm/nouveau: Disable atomic support on a per-device basis

Hm, one nitpick here. Since /sys/kernel/debug/dri/*/state creation depends on
the driver supporting atomic, maybe it would be good to make it so that we set
DRIVER_ATOMIC in the driver_stub structure, then disable it per-device depending
on the nouveau_atomic setting + hw support. That way we can always have the
state debugfs file while maintaining nouveau's current behavior with
exposing
atomic ioctls. Assuming that wouldn't have any unintended side-effects of
course

On Thu, 2018-09-13 at 19:31 +0300, Ville Syrjala wrote:> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> We now have per-device driver_features, so let's use that
> to disable atomic only for pre-nv50.
> 
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: Lyude Paul <lyude at redhat.com>
> Cc: nouveau at lists.freedesktop.org
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/nouveau/dispnv04/disp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.c
> b/drivers/gpu/drm/nouveau/dispnv04/disp.c
> index 70dce544984e..670535a68d3b 100644
> --- a/drivers/gpu/drm/nouveau/dispnv04/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv04/disp.c
> @@ -56,7 +56,7 @@ nv04_display_create(struct drm_device *dev)
>  	nouveau_display(dev)->fini = nv04_display_fini;
>  
>  	/* Pre-nv50 doesn't support atomic, so don't expose the ioctls */
> -	dev->driver->driver_features &= ~DRIVER_ATOMIC;
> +	dev->driver_features &= ~DRIVER_ATOMIC;
>  
>  	nouveau_hw_save_vga_fonts(dev, 1);
>

kbuild test robot

2018-Sep-14 07:45 UTC

head link

[Nouveau] [PATCH 1/2] drm/nouveau: Disable atomic support on a per-device basis

Hi Ville,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.19-rc3 next-20180913]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]

url:   
https://github.com/0day-ci/linux/commits/Ville-Syrjala/drm-nouveau-Disable-atomic-support-on-a-per-device-basis/20180914-111059
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/nouveau/dispnv04/disp.c: In function
'nv04_display_create':>> drivers/gpu/drm/nouveau/dispnv04/disp.c:59:5: error: 'struct
drm_device' has no member named 'driver_features'     dev->driver_features &= ~DRIVER_ATOMIC;
        ^~

vim +59 drivers/gpu/drm/nouveau/dispnv04/disp.c

    33	
    34	int
    35	nv04_display_create(struct drm_device *dev)
    36	{
    37		struct nouveau_drm *drm = nouveau_drm(dev);
    38		struct nvkm_i2c *i2c = nvxx_i2c(&drm->client.device);
    39		struct dcb_table *dcb = &drm->vbios.dcb;
    40		struct drm_connector *connector, *ct;
    41		struct drm_encoder *encoder;
    42		struct nouveau_encoder *nv_encoder;
    43		struct nouveau_crtc *crtc;
    44		struct nv04_display *disp;
    45		int i, ret;
    46	
    47		disp = kzalloc(sizeof(*disp), GFP_KERNEL);
    48		if (!disp)
    49			return -ENOMEM;
    50	
    51		nvif_object_map(&drm->client.device.object, NULL, 0);
    52	
    53		nouveau_display(dev)->priv = disp;
    54		nouveau_display(dev)->dtor = nv04_display_destroy;
    55		nouveau_display(dev)->init = nv04_display_init;
    56		nouveau_display(dev)->fini = nv04_display_fini;
    57	
    58		/* Pre-nv50 doesn't support atomic, so don't expose the ioctls
*/
  > 59		dev->driver_features &= ~DRIVER_ATOMIC;
    60	
    61		nouveau_hw_save_vga_fonts(dev, 1);
    62	
    63		nv04_crtc_create(dev, 0);
    64		if (nv_two_heads(dev))
    65			nv04_crtc_create(dev, 1);
    66	
    67		for (i = 0; i < dcb->entries; i++) {
    68			struct dcb_output *dcbent = &dcb->entry[i];
    69	
    70			connector = nouveau_connector_create(dev, dcbent->connector);
    71			if (IS_ERR(connector))
    72				continue;
    73	
    74			switch (dcbent->type) {
    75			case DCB_OUTPUT_ANALOG:
    76				ret = nv04_dac_create(connector, dcbent);
    77				break;
    78			case DCB_OUTPUT_LVDS:
    79			case DCB_OUTPUT_TMDS:
    80				ret = nv04_dfp_create(connector, dcbent);
    81				break;
    82			case DCB_OUTPUT_TV:
    83				if (dcbent->location == DCB_LOC_ON_CHIP)
    84					ret = nv17_tv_create(connector, dcbent);
    85				else
    86					ret = nv04_tv_create(connector, dcbent);
    87				break;
    88			default:
    89				NV_WARN(drm, "DCB type %d not known\n", dcbent->type);
    90				continue;
    91			}
    92	
    93			if (ret)
    94				continue;
    95		}
    96	
    97		list_for_each_entry_safe(connector, ct,
    98					 &dev->mode_config.connector_list, head) {
    99			if (!connector->encoder_ids[0]) {
   100				NV_WARN(drm, "%s has no encoders, removing\n",
   101					connector->name);
   102				connector->funcs->destroy(connector);
   103			}
   104		}
   105	
   106		list_for_each_entry(encoder, &dev->mode_config.encoder_list,
head) {
   107			struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
   108			struct nvkm_i2c_bus *bus    109				nvkm_i2c_bus_find(i2c,
nv_encoder->dcb->i2c_index);
   110			nv_encoder->i2c = bus ? &bus->i2c : NULL;
   111		}
   112	
   113		/* Save previous state */
   114		list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
   115			crtc->save(&crtc->base);
   116	
   117		list_for_each_entry(nv_encoder, &dev->mode_config.encoder_list,
base.base.head)
   118			nv_encoder->enc_save(&nv_encoder->base.base);
   119	
   120		nouveau_overlay_init(dev);
   121	
   122		return 0;
   123	}
   124	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 65150 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20180914/038f0406/attachment-0001.gz>

Daniel Vetter

2018-Sep-14 08:11 UTC

head link

[Nouveau] [PATCH 1/2] drm/nouveau: Disable atomic support on a per-device basis

On Thu, Sep 13, 2018 at 11:02 PM, Lyude Paul <lyude at redhat.com>
wrote:> Hm, one nitpick here. Since /sys/kernel/debug/dri/*/state creation depends
on
> the driver supporting atomic, maybe it would be good to make it so that we
set
> DRIVER_ATOMIC in the driver_stub structure, then disable it per-device
depending
> on the nouveau_atomic setting + hw support. That way we can always have the
> state debugfs file while maintaining nouveau's current behavior with
exposing
> atomic ioctls. Assuming that wouldn't have any unintended side-effects
of course
dri/*/state only works with atomic drivers. There's no explicit state
with legacy drivers at all, it's all just implicit in hw and some
random driver structures.

We should make sure though that the debugfs stuff looks at
drm_drv_uses_atomic_modsetting(), and not DRIVER_ATOMIC. Former is
about the internals (i915 is internally atomic everywhere), latter
about the uapi (some old platforms aren't properly validated for full
atomic features, hence why it's disabled).
-Daniel
> On Thu, 2018-09-13 at 19:31 +0300, Ville Syrjala wrote:
>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>
>> We now have per-device driver_features, so let's use that
>> to disable atomic only for pre-nv50.
>>
>> Cc: Ben Skeggs <bskeggs at redhat.com>
>> Cc: Lyude Paul <lyude at redhat.com>
>> Cc: nouveau at lists.freedesktop.org
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> ---
>>  drivers/gpu/drm/nouveau/dispnv04/disp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.c
>> b/drivers/gpu/drm/nouveau/dispnv04/disp.c
>> index 70dce544984e..670535a68d3b 100644
>> --- a/drivers/gpu/drm/nouveau/dispnv04/disp.c
>> +++ b/drivers/gpu/drm/nouveau/dispnv04/disp.c
>> @@ -56,7 +56,7 @@ nv04_display_create(struct drm_device *dev)
>>       nouveau_display(dev)->fini = nv04_display_fini;
>>
>>       /* Pre-nv50 doesn't support atomic, so don't expose the
ioctls */
>> -     dev->driver->driver_features &= ~DRIVER_ATOMIC;
>> +     dev->driver_features &= ~DRIVER_ATOMIC;
>>
>>       nouveau_hw_save_vga_fonts(dev, 1);
>>
>


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

Ville Syrjälä

2018-Sep-14 15:04 UTC

head link

[Nouveau] [PATCH 1/2] drm/nouveau: Disable atomic support on a per-device basis

On Thu, Sep 13, 2018 at 05:02:05PM -0400, Lyude Paul
wrote:> Hm, one nitpick here. Since /sys/kernel/debug/dri/*/state creation depends
on
> the driver supporting atomic, maybe it would be good to make it so that we
set
> DRIVER_ATOMIC in the driver_stub structure, then disable it per-device
depending
> on the nouveau_atomic setting + hw support. That way we can always have the
> state debugfs file while maintaining nouveau's current behavior with
exposing
> atomic ioctls. Assuming that wouldn't have any unintended side-effects
of course
I'm not sure why there are three driver structs in nouveau.
Maybe someone can just nuke two of them?
> 
> On Thu, 2018-09-13 at 19:31 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > We now have per-device driver_features, so let's use that
> > to disable atomic only for pre-nv50.
> > 
> > Cc: Ben Skeggs <bskeggs at redhat.com>
> > Cc: Lyude Paul <lyude at redhat.com>
> > Cc: nouveau at lists.freedesktop.org
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/nouveau/dispnv04/disp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.c
> > b/drivers/gpu/drm/nouveau/dispnv04/disp.c
> > index 70dce544984e..670535a68d3b 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv04/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv04/disp.c
> > @@ -56,7 +56,7 @@ nv04_display_create(struct drm_device *dev)
> >  	nouveau_display(dev)->fini = nv04_display_fini;
> >  
> >  	/* Pre-nv50 doesn't support atomic, so don't expose the
ioctls */
> > -	dev->driver->driver_features &= ~DRIVER_ATOMIC;
> > +	dev->driver_features &= ~DRIVER_ATOMIC;
> >  
> >  	nouveau_hw_save_vga_fonts(dev, 1);
> >  
-- 
Ville Syrjälä
Intel

Reasonably Related Threads

Search for more apparently analagous threads

Nouveau - Sep 2018 - [PATCH 1/2] drm/nouveau: Disable atomic support on a per-device basis

[Nouveau] [PATCH 1/2] drm/nouveau: Disable atomic support on a per-device basis

[Nouveau] [PATCH 1/2] drm/nouveau: Disable atomic support on a per-device basis

[Nouveau] [PATCH 1/2] drm/nouveau: Disable atomic support on a per-device basis

[Nouveau] [PATCH 1/2] drm/nouveau: Disable atomic support on a per-device basis

[Nouveau] [PATCH 1/2] drm/nouveau: Disable atomic support on a per-device basis

Reasonably Related Threads