[PATCH] drm/crtc: constify drm_crtc_index parameter

[PATCH] drm/crtc: constify drm_crtc_index parameter

[Jani Nikula]

Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

I needed this for some stuff that turned out to be a dead end. But this
seems to be the right thing to do anyway.
---
 include/drm/drm_crtc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 61932f55f788..0aa292526567 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1342,7 +1342,7 @@ extern void drm_crtc_cleanup(struct drm_crtc *crtc);
  * Given a registered CRTC, return the index of that CRTC within a DRM
  * device's list of CRTCs.
  */
-static inline unsigned int drm_crtc_index(struct drm_crtc *crtc)
+static inline unsigned int drm_crtc_index(const struct drm_crtc *crtc)
 {
 	return crtc->index;
 }
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/crtc: constify drm_crtc_index parameter

[Daniel Vetter]

On Mon, Oct 10, 2016 at 06:26:10PM +0300, Jani Nikula wrote:
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> I needed this for some stuff that turned out to be a dead end. But this
> seems to be the right thing to do anyway.

Applied to drm-misc. There's also the connector and plane versions of
this, care to spin them too?
-Daniel

> ---
>  include/drm/drm_crtc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 61932f55f788..0aa292526567 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1342,7 +1342,7 @@ extern void drm_crtc_cleanup(struct drm_crtc *crtc);
>   * Given a registered CRTC, return the index of that CRTC within a DRM
>   * device's list of CRTCs.
>   */
> -static inline unsigned int drm_crtc_index(struct drm_crtc *crtc)
> +static inline unsigned int drm_crtc_index(const struct drm_crtc *crtc)
>  {
>  	return crtc->index;
>  }
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/crtc: constify drm_crtc_index parameter

[Chris Wilson]

On Mon, Oct 10, 2016 at 06:26:10PM +0300, Jani Nikula wrote:
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Plus equivalents in drm_plane.h, drm_encoder.h ...

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/crtc: constify drm_crtc_index parameter

[Ville Syrjälä]

On Mon, Oct 10, 2016 at 06:26:10PM +0300, Jani Nikula wrote:
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> I needed this for some stuff that turned out to be a dead end. But this
> seems to be the right thing to do anyway.
> ---
>  include/drm/drm_crtc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 61932f55f788..0aa292526567 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1342,7 +1342,7 @@ extern void drm_crtc_cleanup(struct drm_crtc *crtc);
>   * Given a registered CRTC, return the index of that CRTC within a DRM
>   * device's list of CRTCs.
>   */
> -static inline unsigned int drm_crtc_index(struct drm_crtc *crtc)
> +static inline unsigned int drm_crtc_index(const struct drm_crtc *crtc)
>  {
>  	return crtc->index;

BTW speaking about the index stuff. It dawned on me recently that calling
drm_crtc_cleanup() & co. is totally not safe except maybe during the
final cleanup.

If you would do something like:
a = drm_crtc_init();
b = drm_crtc_init();
drm_crtc_cleanup(a);
c = drm_crtc_init();

b and c would end up with the same index.


Or if you would do just
a = drm_crtc_init();
b = drm_crtc_init();
drm_crtc_cleanup(a);

We'd end up with num_crtc==1, but b->index==1, so we'd actually access
beyond the allocated arrays in a bunch of places.

This would need to fixed somehow, or at least documented clearly that if
you have to call any of the cleanup functions during init, you have to
abort the entire thing.

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

Re: [PATCH] drm/crtc: constify drm_crtc_index parameter

[Daniel Vetter]

On Mon, Oct 10, 2016 at 6:04 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Mon, Oct 10, 2016 at 06:26:10PM +0300, Jani Nikula wrote:
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>
>> ---
>>
>> I needed this for some stuff that turned out to be a dead end. But this
>> seems to be the right thing to do anyway.
>> ---
>>  include/drm/drm_crtc.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 61932f55f788..0aa292526567 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -1342,7 +1342,7 @@ extern void drm_crtc_cleanup(struct drm_crtc *crtc);
>>   * Given a registered CRTC, return the index of that CRTC within a DRM
>>   * device's list of CRTCs.
>>   */
>> -static inline unsigned int drm_crtc_index(struct drm_crtc *crtc)
>> +static inline unsigned int drm_crtc_index(const struct drm_crtc *crtc)
>>  {
>>       return crtc->index;
>
> BTW speaking about the index stuff. It dawned on me recently that calling
> drm_crtc_cleanup() & co. is totally not safe except maybe during the
> final cleanup.
>
> If you would do something like:
> a = drm_crtc_init();
> b = drm_crtc_init();
> drm_crtc_cleanup(a);
> c = drm_crtc_init();
>
> b and c would end up with the same index.
>
>
> Or if you would do just
> a = drm_crtc_init();
> b = drm_crtc_init();
> drm_crtc_cleanup(a);
>
> We'd end up with num_crtc==1, but b->index==1, so we'd actually access
> beyond the allocated arrays in a bunch of places.
>
> This would need to fixed somehow, or at least documented clearly that if
> you have to call any of the cleanup functions during init, you have to
> abort the entire thing.

You can't hotplug CRTCs, like you can't hotplug anything else than
connectors. And for connectors we have an ida to make sure you never
end up with duplicated indizes. So I think we're safe.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/crtc: constify drm_crtc_index parameter

[Ville Syrjälä]

On Mon, Oct 10, 2016 at 06:10:42PM +0200, Daniel Vetter wrote:
> On Mon, Oct 10, 2016 at 6:04 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Oct 10, 2016 at 06:26:10PM +0300, Jani Nikula wrote:
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >>
> >> ---
> >>
> >> I needed this for some stuff that turned out to be a dead end. But this
> >> seems to be the right thing to do anyway.
> >> ---
> >>  include/drm/drm_crtc.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >> index 61932f55f788..0aa292526567 100644
> >> --- a/include/drm/drm_crtc.h
> >> +++ b/include/drm/drm_crtc.h
> >> @@ -1342,7 +1342,7 @@ extern void drm_crtc_cleanup(struct drm_crtc *crtc);
> >>   * Given a registered CRTC, return the index of that CRTC within a DRM
> >>   * device's list of CRTCs.
> >>   */
> >> -static inline unsigned int drm_crtc_index(struct drm_crtc *crtc)
> >> +static inline unsigned int drm_crtc_index(const struct drm_crtc *crtc)
> >>  {
> >>       return crtc->index;
> >
> > BTW speaking about the index stuff. It dawned on me recently that calling
> > drm_crtc_cleanup() & co. is totally not safe except maybe during the
> > final cleanup.
> >
> > If you would do something like:
> > a = drm_crtc_init();
> > b = drm_crtc_init();
> > drm_crtc_cleanup(a);
> > c = drm_crtc_init();
> >
> > b and c would end up with the same index.
> >
> >
> > Or if you would do just
> > a = drm_crtc_init();
> > b = drm_crtc_init();
> > drm_crtc_cleanup(a);
> >
> > We'd end up with num_crtc==1, but b->index==1, so we'd actually access
> > beyond the allocated arrays in a bunch of places.
> >
> > This would need to fixed somehow, or at least documented clearly that if
> > you have to call any of the cleanup functions during init, you have to
> > abort the entire thing.
> 
> You can't hotplug CRTCs, like you can't hotplug anything else than
> connectors. And for connectors we have an ida to make sure you never
> end up with duplicated indizes. So I think we're safe.

Except in i915 we don't abort the driver load if we would fail part
way into the crtc init. So we'd just keep going with bogus indexes
and whatnot. I think I wrote a patch to change that, but I'd have
find it again to be sure. Other drivers may or may not have the same
problem.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/crtc: constify drm_crtc_index parameter

[Daniel Vetter]

On Mon, Oct 10, 2016 at 6:30 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>> You can't hotplug CRTCs, like you can't hotplug anything else than
>> connectors. And for connectors we have an ida to make sure you never
>> end up with duplicated indizes. So I think we're safe.
>
> Except in i915 we don't abort the driver load if we would fail part
> way into the crtc init. So we'd just keep going with bogus indexes
> and whatnot. I think I wrote a patch to change that, but I'd have
> find it again to be sure. Other drivers may or may not have the same
> problem.

Hm, we should at least make sure we don't leave any partially
initialized modeset objects behind when the modeset part of the driver
fails to load.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Fi.CI.BAT: warning for drm/crtc: constify drm_crtc_index parameter

[Patchwork]

== Series Details ==

Series: drm/crtc: constify drm_crtc_index parameter
URL   : https://patchwork.freedesktop.org/series/13537/
State : warning

== Summary ==

Series 13537v1 drm/crtc: constify drm_crtc_index parameter
https://patchwork.freedesktop.org/api/1.0/series/13537/revisions/1/mbox/

Test kms_cursor_legacy:
        Subgroup basic-flip-before-cursor-varying-size:
                pass       -> DMESG-WARN (fi-byt-n2820)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> DMESG-WARN (fi-byt-n2820)
Test vgem_basic:
        Subgroup unload:
                pass       -> SKIP       (fi-skl-6770hq)

fi-bdw-5557u     total:248  pass:231  dwarn:0   dfail:0   fail:0   skip:17 
fi-bsw-n3050     total:248  pass:204  dwarn:0   dfail:0   fail:0   skip:44 
fi-bxt-t5700     total:248  pass:217  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-j1900     total:248  pass:214  dwarn:1   dfail:0   fail:1   skip:32 
fi-byt-n2820     total:248  pass:209  dwarn:2   dfail:0   fail:1   skip:36 
fi-hsw-4770      total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-hsw-4770r     total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-ilk-650       total:248  pass:185  dwarn:0   dfail:0   fail:2   skip:61 
fi-ivb-3520m     total:248  pass:221  dwarn:0   dfail:0   fail:0   skip:27 
fi-ivb-3770      total:248  pass:207  dwarn:0   dfail:0   fail:0   skip:41 
fi-kbl-7200u     total:248  pass:222  dwarn:0   dfail:0   fail:0   skip:26 
fi-skl-6260u     total:248  pass:232  dwarn:0   dfail:0   fail:0   skip:16 
fi-skl-6700hq    total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6700k     total:248  pass:221  dwarn:1   dfail:0   fail:0   skip:26 
fi-skl-6770hq    total:248  pass:230  dwarn:1   dfail:0   fail:1   skip:16 
fi-snb-2520m     total:248  pass:211  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2600      total:248  pass:209  dwarn:0   dfail:0   fail:0   skip:39 

Results at /archive/results/CI_IGT_test/Patchwork_2664/

e37a15c8d775e79dddc8345a0f6afdcfe1f607d9 drm-intel-nightly: 2016y-10m-10d-14h-33m-29s UTC integration manifest
31cdd5e drm/crtc: constify drm_crtc_index parameter

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx