| Message ID | 20180913103923.14013-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Remove fb pitch limit for no display | expand |
On Thu, Sep 13, 2018 at 11:39:23AM +0100, Chris Wilson wrote: > If there is not a display (and so no CRTCs) then there is no upper limit > to the framebuffer pitch imposed by the CRTC. Should we still allow you to create framebuffers in that case? If yes then my plan to also query the planes which pixel formats/modifiers to accept in addfb is going to hit hard times. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_display.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3be5fa0acee8..7db14086fb02 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14403,9 +14403,9 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = { > .dirty = intel_user_framebuffer_dirty, > }; > > -static > -u32 intel_fb_pitch_limit(struct drm_i915_private *dev_priv, > - uint64_t fb_modifier, uint32_t pixel_format) > +static u32 > +intel_fb_pitch_limit(struct drm_i915_private *dev_priv, > + uint64_t fb_modifier, uint32_t pixel_format) > { > struct intel_crtc *crtc; > struct intel_plane *plane; > @@ -14415,6 +14415,9 @@ u32 intel_fb_pitch_limit(struct drm_i915_private *dev_priv, > * the highest stride limits of them all. > */ > crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A); > + if (!crtc) > + return U32_MAX; > + > plane = to_intel_plane(crtc->base.primary); > > return plane->max_stride(plane, pixel_format, fb_modifier, > -- > 2.19.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Ville Syrjälä (2018-09-13 11:56:56) > On Thu, Sep 13, 2018 at 11:39:23AM +0100, Chris Wilson wrote: > > If there is not a display (and so no CRTCs) then there is no upper limit > > to the framebuffer pitch imposed by the CRTC. > > Should we still allow you to create framebuffers in that case? Up to you, imho is that fb are just bo with a bit of description. I didn't see much harm in creating an fb even if it was never going to be attached to any pipe. I don't have a solid usecase, just feels like it reduces the impact on the API. Hmm, however if we if (num_pipes == 0) driver_features &= ~DRIVER_MODESET; we will kill the unusable API at the ioctl boundary. > If yes then my plan to also query the planes which pixel formats/modifiers > to accept in addfb is going to hit hard times. Spreading an ugly if !plane around :( -Chris
On Thu, Sep 13, 2018 at 12:05:49PM +0100, Chris Wilson wrote: > Quoting Ville Syrjälä (2018-09-13 11:56:56) > > On Thu, Sep 13, 2018 at 11:39:23AM +0100, Chris Wilson wrote: > > > If there is not a display (and so no CRTCs) then there is no upper limit > > > to the framebuffer pitch imposed by the CRTC. > > > > Should we still allow you to create framebuffers in that case? > > Up to you, imho is that fb are just bo with a bit of description. I > didn't see much harm in creating an fb even if it was never going to be > attached to any pipe. I don't have a solid usecase, just feels like it > reduces the impact on the API. To me it feels a bit like giving userspace false hope that they can actually do something with the fb later. > > Hmm, however if we > if (num_pipes == 0) driver_features &= ~DRIVER_MODESET; > we will kill the unusable API at the ioctl boundary. That seems a bit wrong. We'd really want to device_features for something like that. Not sure how many things we have in the driver struct that really ought to be under the device. > > > If yes then my plan to also query the planes which pixel formats/modifiers > > to accept in addfb is going to hit hard times. > > Spreading an ugly if !plane around :( Should just be for_each_plane() and I guess if none are there the addfb would get rejected as the format wouldn't match anything. But I haven't actually figured out how to do this in the best way. Another option would be to cache the union of all format/modifier combos of all planes somewhere and check against that (should be slightly more efficient as we wouldn't check the same thing many times). And in that case I guess we could always add some kind of fallback of say just XRGB8888 for the num_pipes==0 case, should we think there is some benefit to allowing it.
Quoting Ville Syrjälä (2018-09-13 13:26:48) > On Thu, Sep 13, 2018 at 12:05:49PM +0100, Chris Wilson wrote: > > Quoting Ville Syrjälä (2018-09-13 11:56:56) > > > On Thu, Sep 13, 2018 at 11:39:23AM +0100, Chris Wilson wrote: > > > > If there is not a display (and so no CRTCs) then there is no upper limit > > > > to the framebuffer pitch imposed by the CRTC. > > > > > > Should we still allow you to create framebuffers in that case? > > > > Up to you, imho is that fb are just bo with a bit of description. I > > didn't see much harm in creating an fb even if it was never going to be > > attached to any pipe. I don't have a solid usecase, just feels like it > > reduces the impact on the API. > > To me it feels a bit like giving userspace false hope that they > can actually do something with the fb later. > > > > > Hmm, however if we > > if (num_pipes == 0) driver_features &= ~DRIVER_MODESET; > > we will kill the unusable API at the ioctl boundary. > > That seems a bit wrong. We'd really want to device_features for > something like that. Not sure how many things we have in the driver > struct that really ought to be under the device. Hah, yes it is one level too deep. I think the current set of driver_features is more or less device_features? That seems like an easy job though -- though some may point out that this smells of midlayer ;) -Chris
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3be5fa0acee8..7db14086fb02 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14403,9 +14403,9 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = { .dirty = intel_user_framebuffer_dirty, }; -static -u32 intel_fb_pitch_limit(struct drm_i915_private *dev_priv, - uint64_t fb_modifier, uint32_t pixel_format) +static u32 +intel_fb_pitch_limit(struct drm_i915_private *dev_priv, + uint64_t fb_modifier, uint32_t pixel_format) { struct intel_crtc *crtc; struct intel_plane *plane; @@ -14415,6 +14415,9 @@ u32 intel_fb_pitch_limit(struct drm_i915_private *dev_priv, * the highest stride limits of them all. */ crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A); + if (!crtc) + return U32_MAX; + plane = to_intel_plane(crtc->base.primary); return plane->max_stride(plane, pixel_format, fb_modifier,
If there is not a display (and so no CRTCs) then there is no upper limit to the framebuffer pitch imposed by the CRTC. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_display.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)