| Message ID | 20221004131916.233474-4-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/5] drm/i915: remove the TODO in pin_and_fence_fb_obj | expand |
On Tue, Oct 04, 2022 at 02:19:15PM +0100, Matthew Auld wrote: > For these types of display buffers, we need to able to CPU access some > part of the backing memory in prepare_plane_clear_colors(). As a result > we need to ensure we always place in the mappable part of lmem, which > becomes necessary on small-bar systems. > > v2(Nirmoy & Ville): > - Add some commentary for why we need to CPU access the buffer. > - Split out the other changes, so we just consider the display change > here. > v3: > - Handle this in the dpt path. > v4(Ville): > - Drop the intel_fb_rc_ccs_cc_plane() sanity check in > pin_and_fence_fb_obj(), since we can also trigger this on DG1 it > seems. On some hw I think there was also a chicken bit to disable DPT, and the DPT simulation situation was always a mess (you had to enable it using some custom config). So in theory someone might have the need to use the non-DPT path on more modern hw as well, but I'm OK not considering it here. If someone thinks that small-bar+no-DPT is important they can deal with it in a separate patch. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Fixes: eb1c535f0d69 ("drm/i915: turn on small BAR support") > Reported-by: Jianshui Yu <jianshui.yu@intel.com> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Nirmoy Das <nirmoy.das@intel.com> > --- > drivers/gpu/drm/i915/display/intel_fb_pin.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c > index 5031ee5695dd..e12339f46640 100644 > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c > @@ -50,7 +50,18 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, > continue; > > if (HAS_LMEM(dev_priv)) { > - ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0); > + unsigned int flags = obj->flags; > + > + /* > + * For this type of buffer we need to able to read from the CPU > + * the clear color value found in the buffer, hence we need to > + * ensure it is always in the mappable part of lmem, if this is > + * a small-bar device. > + */ > + if (intel_fb_rc_ccs_cc_plane(fb) >= 0) > + flags &= ~I915_BO_ALLOC_GPU_ONLY; > + ret = __i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0, > + flags); > if (ret) > continue; > } > -- > 2.37.3
On 10/4/2022 3:19 PM, Matthew Auld wrote: > For these types of display buffers, we need to able to CPU access some > part of the backing memory in prepare_plane_clear_colors(). As a result > we need to ensure we always place in the mappable part of lmem, which > becomes necessary on small-bar systems. > > v2(Nirmoy & Ville): > - Add some commentary for why we need to CPU access the buffer. > - Split out the other changes, so we just consider the display change > here. > v3: > - Handle this in the dpt path. > v4(Ville): > - Drop the intel_fb_rc_ccs_cc_plane() sanity check in > pin_and_fence_fb_obj(), since we can also trigger this on DG1 it > seems. > > Fixes: eb1c535f0d69 ("drm/i915: turn on small BAR support") > Reported-by: Jianshui Yu <jianshui.yu@intel.com> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Nirmoy Das <nirmoy.das@intel.com> Acked-by: Nirmoy Das <nirmoy.das@intel.com> > --- > drivers/gpu/drm/i915/display/intel_fb_pin.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c > index 5031ee5695dd..e12339f46640 100644 > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c > @@ -50,7 +50,18 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, > continue; > > if (HAS_LMEM(dev_priv)) { > - ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0); > + unsigned int flags = obj->flags; > + > + /* > + * For this type of buffer we need to able to read from the CPU > + * the clear color value found in the buffer, hence we need to > + * ensure it is always in the mappable part of lmem, if this is > + * a small-bar device. > + */ > + if (intel_fb_rc_ccs_cc_plane(fb) >= 0) > + flags &= ~I915_BO_ALLOC_GPU_ONLY; > + ret = __i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0, > + flags); > if (ret) > continue; > }
Hi Matt, On 04/10/2022 14:19, Matthew Auld wrote: > For these types of display buffers, we need to able to CPU access some > part of the backing memory in prepare_plane_clear_colors(). As a result > we need to ensure we always place in the mappable part of lmem, which > becomes necessary on small-bar systems. > > v2(Nirmoy & Ville): > - Add some commentary for why we need to CPU access the buffer. > - Split out the other changes, so we just consider the display change > here. > v3: > - Handle this in the dpt path. > v4(Ville): > - Drop the intel_fb_rc_ccs_cc_plane() sanity check in > pin_and_fence_fb_obj(), since we can also trigger this on DG1 it > seems. > > Fixes: eb1c535f0d69 ("drm/i915: turn on small BAR support") That one landed in 6.0 - do you want to send this (with pre-requisite(s)) to stable? Or if not do you want me to send for 6.1 as part of fixes flow? In which case what are the per-requisites? Regards, Tvrtko > Reported-by: Jianshui Yu <jianshui.yu@intel.com> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Nirmoy Das <nirmoy.das@intel.com> > --- > drivers/gpu/drm/i915/display/intel_fb_pin.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c > index 5031ee5695dd..e12339f46640 100644 > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c > @@ -50,7 +50,18 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, > continue; > > if (HAS_LMEM(dev_priv)) { > - ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0); > + unsigned int flags = obj->flags; > + > + /* > + * For this type of buffer we need to able to read from the CPU > + * the clear color value found in the buffer, hence we need to > + * ensure it is always in the mappable part of lmem, if this is > + * a small-bar device. > + */ > + if (intel_fb_rc_ccs_cc_plane(fb) >= 0) > + flags &= ~I915_BO_ALLOC_GPU_ONLY; > + ret = __i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0, > + flags); > if (ret) > continue; > }
Hi, On 11/10/2022 14:54, Tvrtko Ursulin wrote: > > Hi Matt, > > On 04/10/2022 14:19, Matthew Auld wrote: >> For these types of display buffers, we need to able to CPU access some >> part of the backing memory in prepare_plane_clear_colors(). As a result >> we need to ensure we always place in the mappable part of lmem, which >> becomes necessary on small-bar systems. >> >> v2(Nirmoy & Ville): >> - Add some commentary for why we need to CPU access the buffer. >> - Split out the other changes, so we just consider the display change >> here. >> v3: >> - Handle this in the dpt path. >> v4(Ville): >> - Drop the intel_fb_rc_ccs_cc_plane() sanity check in >> pin_and_fence_fb_obj(), since we can also trigger this on DG1 it >> seems. >> >> Fixes: eb1c535f0d69 ("drm/i915: turn on small BAR support") > > That one landed in 6.0 - do you want to send this (with > pre-requisite(s)) to stable? Or if not do you want me to send for 6.1 as > part of fixes flow? In which case what are the per-requisites? This one is only for DG2, which is still hidden behind force_probe, so not too sure if it needs stable? I think the only pre-requisite is 999f45620772 ("drm/i915: allow control over the flags when migrating"), but again I'm not too sure how much we care about fixes for platforms hidden behind force_probe? What do you think? > > Regards, > > Tvrtko > >> Reported-by: Jianshui Yu <jianshui.yu@intel.com> >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Nirmoy Das <nirmoy.das@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_fb_pin.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c >> b/drivers/gpu/drm/i915/display/intel_fb_pin.c >> index 5031ee5695dd..e12339f46640 100644 >> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c >> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c >> @@ -50,7 +50,18 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, >> continue; >> if (HAS_LMEM(dev_priv)) { >> - ret = i915_gem_object_migrate(obj, &ww, >> INTEL_REGION_LMEM_0); >> + unsigned int flags = obj->flags; >> + >> + /* >> + * For this type of buffer we need to able to read from >> the CPU >> + * the clear color value found in the buffer, hence we >> need to >> + * ensure it is always in the mappable part of lmem, if >> this is >> + * a small-bar device. >> + */ >> + if (intel_fb_rc_ccs_cc_plane(fb) >= 0) >> + flags &= ~I915_BO_ALLOC_GPU_ONLY; >> + ret = __i915_gem_object_migrate(obj, &ww, >> INTEL_REGION_LMEM_0, >> + flags); >> if (ret) >> continue; >> }
On 11/10/2022 15:39, Matthew Auld wrote: > Hi, > > On 11/10/2022 14:54, Tvrtko Ursulin wrote: >> >> Hi Matt, >> >> On 04/10/2022 14:19, Matthew Auld wrote: >>> For these types of display buffers, we need to able to CPU access some >>> part of the backing memory in prepare_plane_clear_colors(). As a result >>> we need to ensure we always place in the mappable part of lmem, which >>> becomes necessary on small-bar systems. >>> >>> v2(Nirmoy & Ville): >>> - Add some commentary for why we need to CPU access the buffer. >>> - Split out the other changes, so we just consider the display change >>> here. >>> v3: >>> - Handle this in the dpt path. >>> v4(Ville): >>> - Drop the intel_fb_rc_ccs_cc_plane() sanity check in >>> pin_and_fence_fb_obj(), since we can also trigger this on DG1 it >>> seems. >>> >>> Fixes: eb1c535f0d69 ("drm/i915: turn on small BAR support") >> >> That one landed in 6.0 - do you want to send this (with >> pre-requisite(s)) to stable? Or if not do you want me to send for 6.1 >> as part of fixes flow? In which case what are the per-requisites? > > This one is only for DG2, which is still hidden behind force_probe, so > not too sure if it needs stable? I think the only pre-requisite is > 999f45620772 ("drm/i915: allow control over the flags when migrating"), > but again I'm not too sure how much we care about fixes for platforms > hidden behind force_probe? What do you think? It is certainly not mandatory, but now that cards are about to ship and reach end users it may be nice to have if not too hard - at least for 6.1 release candidates. I am not clear on the importance of the fix to say for sure. Like what goes bang and under what circumstances. So I do basically defer to someone who knows those answers. Regards, Tvrtko > >> >> Regards, >> >> Tvrtko >> >>> Reported-by: Jianshui Yu <jianshui.yu@intel.com> >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Cc: Nirmoy Das <nirmoy.das@intel.com> >>> --- >>> drivers/gpu/drm/i915/display/intel_fb_pin.c | 13 ++++++++++++- >>> 1 file changed, 12 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c >>> b/drivers/gpu/drm/i915/display/intel_fb_pin.c >>> index 5031ee5695dd..e12339f46640 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c >>> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c >>> @@ -50,7 +50,18 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, >>> continue; >>> if (HAS_LMEM(dev_priv)) { >>> - ret = i915_gem_object_migrate(obj, &ww, >>> INTEL_REGION_LMEM_0); >>> + unsigned int flags = obj->flags; >>> + >>> + /* >>> + * For this type of buffer we need to able to read from >>> the CPU >>> + * the clear color value found in the buffer, hence we >>> need to >>> + * ensure it is always in the mappable part of lmem, if >>> this is >>> + * a small-bar device. >>> + */ >>> + if (intel_fb_rc_ccs_cc_plane(fb) >= 0) >>> + flags &= ~I915_BO_ALLOC_GPU_ONLY; >>> + ret = __i915_gem_object_migrate(obj, &ww, >>> INTEL_REGION_LMEM_0, >>> + flags); >>> if (ret) >>> continue; >>> }
On 11/10/2022 16:03, Tvrtko Ursulin wrote: > > On 11/10/2022 15:39, Matthew Auld wrote: >> Hi, >> >> On 11/10/2022 14:54, Tvrtko Ursulin wrote: >>> >>> Hi Matt, >>> >>> On 04/10/2022 14:19, Matthew Auld wrote: >>>> For these types of display buffers, we need to able to CPU access some >>>> part of the backing memory in prepare_plane_clear_colors(). As a result >>>> we need to ensure we always place in the mappable part of lmem, which >>>> becomes necessary on small-bar systems. >>>> >>>> v2(Nirmoy & Ville): >>>> - Add some commentary for why we need to CPU access the buffer. >>>> - Split out the other changes, so we just consider the display change >>>> here. >>>> v3: >>>> - Handle this in the dpt path. >>>> v4(Ville): >>>> - Drop the intel_fb_rc_ccs_cc_plane() sanity check in >>>> pin_and_fence_fb_obj(), since we can also trigger this on DG1 it >>>> seems. >>>> >>>> Fixes: eb1c535f0d69 ("drm/i915: turn on small BAR support") >>> >>> That one landed in 6.0 - do you want to send this (with >>> pre-requisite(s)) to stable? Or if not do you want me to send for 6.1 >>> as part of fixes flow? In which case what are the per-requisites? >> >> This one is only for DG2, which is still hidden behind force_probe, so >> not too sure if it needs stable? I think the only pre-requisite is >> 999f45620772 ("drm/i915: allow control over the flags when >> migrating"), but again I'm not too sure how much we care about fixes >> for platforms hidden behind force_probe? What do you think? > > It is certainly not mandatory, but now that cards are about to ship and > reach end users it may be nice to have if not too hard - at least for > 6.1 release candidates. I am not clear on the importance of the fix to > say for sure. Like what goes bang and under what circumstances. So I do > basically defer to someone who knows those answers. It's important for small-bar DG2 systems, where it will potentially oops somewhere in the driver without this, so if we want to support DG2 as a platform in 6.1, then we also need this fix, along with 999f45620772. > > Regards, > > Tvrtko > >> >>> >>> Regards, >>> >>> Tvrtko >>> >>>> Reported-by: Jianshui Yu <jianshui.yu@intel.com> >>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>> Cc: Nirmoy Das <nirmoy.das@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/display/intel_fb_pin.c | 13 ++++++++++++- >>>> 1 file changed, 12 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c >>>> b/drivers/gpu/drm/i915/display/intel_fb_pin.c >>>> index 5031ee5695dd..e12339f46640 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c >>>> @@ -50,7 +50,18 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, >>>> continue; >>>> if (HAS_LMEM(dev_priv)) { >>>> - ret = i915_gem_object_migrate(obj, &ww, >>>> INTEL_REGION_LMEM_0); >>>> + unsigned int flags = obj->flags; >>>> + >>>> + /* >>>> + * For this type of buffer we need to able to read from >>>> the CPU >>>> + * the clear color value found in the buffer, hence we >>>> need to >>>> + * ensure it is always in the mappable part of lmem, if >>>> this is >>>> + * a small-bar device. >>>> + */ >>>> + if (intel_fb_rc_ccs_cc_plane(fb) >= 0) >>>> + flags &= ~I915_BO_ALLOC_GPU_ONLY; >>>> + ret = __i915_gem_object_migrate(obj, &ww, >>>> INTEL_REGION_LMEM_0, >>>> + flags); >>>> if (ret) >>>> continue; >>>> }
On 11/10/2022 16:28, Matthew Auld wrote: > On 11/10/2022 16:03, Tvrtko Ursulin wrote: >> >> On 11/10/2022 15:39, Matthew Auld wrote: >>> Hi, >>> >>> On 11/10/2022 14:54, Tvrtko Ursulin wrote: >>>> >>>> Hi Matt, >>>> >>>> On 04/10/2022 14:19, Matthew Auld wrote: >>>>> For these types of display buffers, we need to able to CPU access some >>>>> part of the backing memory in prepare_plane_clear_colors(). As a >>>>> result >>>>> we need to ensure we always place in the mappable part of lmem, which >>>>> becomes necessary on small-bar systems. >>>>> >>>>> v2(Nirmoy & Ville): >>>>> - Add some commentary for why we need to CPU access the buffer. >>>>> - Split out the other changes, so we just consider the display >>>>> change >>>>> here. >>>>> v3: >>>>> - Handle this in the dpt path. >>>>> v4(Ville): >>>>> - Drop the intel_fb_rc_ccs_cc_plane() sanity check in >>>>> pin_and_fence_fb_obj(), since we can also trigger this on DG1 it >>>>> seems. >>>>> >>>>> Fixes: eb1c535f0d69 ("drm/i915: turn on small BAR support") >>>> >>>> That one landed in 6.0 - do you want to send this (with >>>> pre-requisite(s)) to stable? Or if not do you want me to send for >>>> 6.1 as part of fixes flow? In which case what are the per-requisites? >>> >>> This one is only for DG2, which is still hidden behind force_probe, >>> so not too sure if it needs stable? I think the only pre-requisite is >>> 999f45620772 ("drm/i915: allow control over the flags when >>> migrating"), but again I'm not too sure how much we care about fixes >>> for platforms hidden behind force_probe? What do you think? >> >> It is certainly not mandatory, but now that cards are about to ship >> and reach end users it may be nice to have if not too hard - at least >> for 6.1 release candidates. I am not clear on the importance of the >> fix to say for sure. Like what goes bang and under what circumstances. >> So I do basically defer to someone who knows those answers. > > It's important for small-bar DG2 systems, where it will potentially oops > somewhere in the driver without this, so if we want to support DG2 as a > platform in 6.1, then we also need this fix, along with 999f45620772. Now that you mention an oops I will definitely aim to pull it in. Thanks! Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c index 5031ee5695dd..e12339f46640 100644 --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c @@ -50,7 +50,18 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, continue; if (HAS_LMEM(dev_priv)) { - ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0); + unsigned int flags = obj->flags; + + /* + * For this type of buffer we need to able to read from the CPU + * the clear color value found in the buffer, hence we need to + * ensure it is always in the mappable part of lmem, if this is + * a small-bar device. + */ + if (intel_fb_rc_ccs_cc_plane(fb) >= 0) + flags &= ~I915_BO_ALLOC_GPU_ONLY; + ret = __i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0, + flags); if (ret) continue; }
For these types of display buffers, we need to able to CPU access some part of the backing memory in prepare_plane_clear_colors(). As a result we need to ensure we always place in the mappable part of lmem, which becomes necessary on small-bar systems. v2(Nirmoy & Ville): - Add some commentary for why we need to CPU access the buffer. - Split out the other changes, so we just consider the display change here. v3: - Handle this in the dpt path. v4(Ville): - Drop the intel_fb_rc_ccs_cc_plane() sanity check in pin_and_fence_fb_obj(), since we can also trigger this on DG1 it seems. Fixes: eb1c535f0d69 ("drm/i915: turn on small BAR support") Reported-by: Jianshui Yu <jianshui.yu@intel.com> Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Nirmoy Das <nirmoy.das@intel.com> --- drivers/gpu/drm/i915/display/intel_fb_pin.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)