[PATCH] drm: don't override possible_crtcs for primary/cursor planes

[Rob Clark]

On Thu, Oct 20, 2016 at 3:19 PM, Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
> On Thu, Oct 20, 2016 at 03:06:06PM -0400, Rob Clark wrote:
>> It is kind of a pointless restriction.  If userspace does silly things
>> like using crtcA's cursor plane on crtcB, and then setcursor on crtcA,
>> it will end up with the overlay disabled on crtcB.  But userspace is
>> allowed to shoot itself like this.
>>
>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>> ---
>> Note that cursor and primary planes are slightly useless to non-atomic
>> userspace, since userspace has no way to know *which* crtc a primary
>> or cursor plane belongs to.  So lighting up a 2nd display may or may
>> not make your overlay planes go *poof*.
>>
>> So basically use of cursor/primary planes plus legacy ioctls is an
>> undefined behavior if we go with this patch.
>>
>> *Maybe* we want to restrict this to atomic userspace.  (Ie. advertise
>> the more limited possible_crtcs for legacy userspace.)  Depends on
>> whether we can (slightly retroactively) declare that mixing atomic
>> and legacy APIs is undefined behavior.  Or if really needed, add
>> DRM_CLIENT_CAP_REALLY_REALLY_ATOMIC_NO_LEGACY_PLS.  (Or just pass in
>> value 2 for existing DRM_CLIENT_CAP_ATOMIC)
>>
>>  drivers/gpu/drm/drm_crtc.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index c215802..1e6d37f 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -707,9 +707,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>>       crtc->primary = primary;
>>       crtc->cursor = cursor;
>>       if (primary)
>> -             primary->possible_crtcs = 1 << drm_crtc_index(crtc);
>> +             WARN_ON(!(primary->possible_crtcs & (1 << drm_crtc_index(crtc))));
>>       if (cursor)
>> -             cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
>> +             WARN_ON(!(cursor->possible_crtcs & (1 << drm_crtc_index(crtc))));
>
> The slight issue here is that this would require the caller to know the
> upcoming crtc index before its even assigned. So might be better just
> having the caller populate these however it likes after the crtc_init()?

hmm, bleh.. I wanted to make sure we catch drivers that left it as zero..

but how 'bout:

  if (cursor && !cursor->possible_crtcs)
     cursor->possible_crtcs = 1 << drm_crtc_index(crtc);

BR,
-R

>>
>>       if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>>               drm_object_attach_property(&crtc->base, config->prop_active, 0);
>> --
>> 2.7.4
>
> --
> Ville Syrjälä
> Intel OTC