Re: [Freedreno] [RFC PATCH v3 0/3] Support for Solid Fill Planes
Hi Daniel Thanks for looking into this series. On 1/6/2023 1:49 PM, Dmitry Baryshkov wrote: On Fri, 6 Jan 2023 at 20:41, Daniel Vetter wrote: On Fri, Jan 06, 2023 at 05:43:23AM +0200, Dmitry Baryshkov wrote: On Fri, 6 Jan 2023 at 02:38, Jessica Zhang wrote: On 1/5/2023 3:33 AM, Daniel Vetter wrote: On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote: Introduce and add support for a solid_fill property. When the solid_fill property is set, and the framebuffer is set to NULL, memory fetch will be disabled. In addition, loosen the NULL FB checks within the atomic commit callstack to allow a NULL FB when the solid_fill property is set and add FB checks in methods where the FB was previously assumed to be non-NULL. Finally, have the DPU driver use drm_plane_state.solid_fill and instead of dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit callstack to account for a NULL FB in cases where solid_fill is set. Some drivers support hardware that have optimizations for solid fill planes. This series aims to expose these capabilities to userspace as some compositors have a solid fill flag (ex. SOLID_COLOR in the Android hardware composer HAL) that can be set by apps like the Android Gears app. Userspace can set the solid_fill property to a blob containing the appropriate version number and solid fill color (in RGB323232 format) and setting the framebuffer to NULL. Note: Currently, there's only one version of the solid_fill blob property. However if other drivers want to support a similar feature, but require more than just the solid fill color, they can extend this feature by creating additional versions of the drm_solid_fill struct. Changes in V2: - Dropped SOLID_FILL_FORMAT property (Simon) - Switched to implementing solid_fill property as a blob (Simon, Dmitry) - Changed to checks for if solid_fill_blob is set (Dmitry) - Abstracted (plane_state && !solid_fill_blob) checks to helper method (Dmitry) - Removed DPU_PLANE_COLOR_FILL_FLAG - Fixed whitespace and indentation issues (Dmitry) Now that this is a blob, I do wonder again whether it's not cleaner to set the blob as the FB pointer. Or create some kind other kind of special data source objects (because solid fill is by far not the only such thing). We'd still end up in special cases like when userspace that doesn't understand solid fill tries to read out such a framebuffer, but these cases already exist anyway for lack of priviledges. So I still think that feels like the more consistent way to integrate this feature. Which doesn't mean it has to happen like that, but the patches/cover letter should at least explain why we don't do it like this. Hi Daniel, IIRC we were facing some issues with this check [1] when trying to set FB to a PROP_BLOB instead. Which is why we went with making it a separate property instead. Will mention this in the cover letter. What kind of issues? Could you please describe them? We switched from bitmask to enum style for prop types, which means it's not possible to express with the current uapi a property which accepts both an object or a blob. Which yeah sucks a bit ... But! blob properties are kms objects (like framebuffers), so it should be possible to stuff a blob into an object property as-is. Of course you need to update the validation code to make sure we accept either an fb or a blob for the internal representation. But that kind of split internally is required no matter what I think. I checked your idea and notes from Jessica. So while we can pass blobs to property objects, the prop_fb_id is created as an object property with the type DRM_MODE_OBJECT_FB. Passing DRM_MODE_OBJECT_BLOB would fail a check in drm_property_change_valid_get() -> __drm_mode_object_find(). And I don't think that we should break the existing validation code for this special case. Like Jessica wrote, re-using the FB_ID property to pass solid fill information will need modification of existing checks shown in [1] OR the property creation itself would fail. We just went with this approach, as it was less intrusive and would not affect the existing FB_ID path. Since both approaches need modifications of validation checks, adding a new property is less intrusive and safer than the already convoluted checks in drm_property_flags_valid(). Let us know if its a strong preference on your side to re-use FB_ID and if so why. Thanks Abhinav If you insist on using FB_ID for passing solid_fill information, I'd ask you to reconsider using a 1x1 framebuffer. It would be fully compatible with the existing userspace, which can then treat it seamlessly. -Daniel [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71
Re: [Freedreno] [RFC PATCH v3 3/3] drm/msm/dpu: Use color_fill property for DPU planes
On Fri, 6 Jan 2023 at 22:57, Jessica Zhang wrote:
> On 1/4/2023 6:16 PM, Dmitry Baryshkov wrote:
> > On 05/01/2023 01:40, Jessica Zhang wrote:
> >> Initialize and use the color_fill properties for planes in DPU driver. In
> >> addition, relax framebuffer requirements within atomic commit path and
> >> add checks for NULL framebuffers. Finally, drop DPU_PLANE_COLOR_FILL_FLAG
> >> as it's unused.
> >>
> >> Changes since V2:
> >> - Fixed dropped 'const' warning
> >> - Dropped use of solid_fill_format
> >> - Switched to using drm_plane_solid_fill_enabled helper method
> >> - Added helper to convert color fill to BGR888 (Rob)
> >> - Added support for solid fill on planes of varying sizes
> >> - Removed DPU_PLANE_COLOR_FILL_FLAG
> >>
> >> Signed-off-by: Jessica Zhang
> >> ---
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +++-
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 ++-
> >> 2 files changed, 49 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> index 13ce321283ff..0695b70ea1b7 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> @@ -409,6 +409,7 @@ static void _dpu_crtc_blend_setup_mixer(struct
> >> drm_crtc *crtc,
> >> struct drm_plane_state *state;
> >> struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
> >> struct dpu_plane_state *pstate = NULL;
> >> +const struct msm_format *fmt;
> >> struct dpu_format *format;
> >> struct dpu_hw_ctl *ctl = mixer->lm_ctl;
> >> @@ -441,7 +442,13 @@ static void _dpu_crtc_blend_setup_mixer(struct
> >> drm_crtc *crtc,
> >> sspp_idx - SSPP_VIG0,
> >> state->fb ? state->fb->base.id : -1);
> >> -format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
> >> +if (pstate->base.fb)
> >> +fmt = msm_framebuffer_format(pstate->base.fb);
> >> +else
> >> +fmt = dpu_get_msm_format(&_dpu_crtc_get_kms(crtc)->base,
> >> +DRM_FORMAT_ABGR, 0);
> >> +
> >> +format = to_dpu_format(fmt);
> >> if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
> >> bg_alpha_enable = true;
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> index 86719020afe2..51a7507373f7 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> @@ -44,7 +44,6 @@
> >> #define DPU_NAME_SIZE 12
> >> -#define DPU_PLANE_COLOR_FILL_FLAGBIT(31)
> >> #define DPU_ZPOS_MAX 255
> >> /* multirect rect index */
> >> @@ -105,7 +104,6 @@ struct dpu_plane {
> >> enum dpu_sspp pipe;
> >> struct dpu_hw_pipe *pipe_hw;
> >> -uint32_t color_fill;
> >> bool is_error;
> >> bool is_rt_pipe;
> >> const struct dpu_mdss_cfg *catalog;
> >> @@ -678,6 +676,17 @@ static void _dpu_plane_setup_scaler(struct
> >> dpu_plane *pdpu,
> >> _cfg);
> >> }
> >> +static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill
> >> solid_fill)
> >> +{
> >> +uint32_t ret = 0;
> >> +
> >> +ret |= ((uint8_t) solid_fill.b) << 16;
> >> +ret |= ((uint8_t) solid_fill.g) << 8;
> >> +ret |= ((uint8_t) solid_fill.r);
> >> +
> >> +return ret;
> >> +}
> >> +
> >> /**
> >>* _dpu_plane_color_fill - enables color fill on plane
> >>* @pdpu: Pointer to DPU plane object
> >> @@ -1001,12 +1010,17 @@ static int dpu_plane_atomic_check(struct
> >> drm_plane *plane,
> >> dst = drm_plane_state_dest(new_plane_state);
> >> -fb_rect.x2 = new_plane_state->fb->width;
> >> -fb_rect.y2 = new_plane_state->fb->height;
> >> +if (new_plane_state->fb) {
> >> +fb_rect.x2 = new_plane_state->fb->width;
> >> +fb_rect.y2 = new_plane_state->fb->height;
> >> +}
> >> max_linewidth = pdpu->catalog->caps->max_linewidth;
> >> -fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> >> +if (new_plane_state->fb)
> >> +fmt =
> >> to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> >> +else
> >> +fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR);
> >
> > I think this should be more explicit:
> >
> > if (solid_fill)
> > fmt = dpu_get_dpu_format(...)
> > else
> > fmt = to_dpu_format(msm_framebuffer_format(...).
> >
> > And in the _dpu_crtc_blend_setup_mixer() too.
>
> Hi Dmitry,
>
> Noted.
>
> >
> > Maybe the code can be extracted to a helper.
> >
> >> min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
> >> @@ -1018,7 +1032,7 @@ static int dpu_plane_atomic_check(struct
> >> drm_plane *plane,
> >> return -EINVAL;
> >> /* check src bounds */
> >> -} else if (!dpu_plane_validate_src(, _rect, min_src_size)) {
> >> +} else if (new_plane_state->fb && !dpu_plane_validate_src(,
> >> _rect,
Re: [Freedreno] [RFC PATCH v3 0/3] Support for Solid Fill Planes
On Fri, 6 Jan 2023 at 20:41, Daniel Vetter wrote: > > On Fri, Jan 06, 2023 at 05:43:23AM +0200, Dmitry Baryshkov wrote: > > On Fri, 6 Jan 2023 at 02:38, Jessica Zhang > > wrote: > > > > > > > > > > > > On 1/5/2023 3:33 AM, Daniel Vetter wrote: > > > > On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote: > > > >> Introduce and add support for a solid_fill property. When the > > > >> solid_fill > > > >> property is set, and the framebuffer is set to NULL, memory fetch will > > > >> be > > > >> disabled. > > > >> > > > >> In addition, loosen the NULL FB checks within the atomic commit > > > >> callstack > > > >> to allow a NULL FB when the solid_fill property is set and add FB > > > >> checks > > > >> in methods where the FB was previously assumed to be non-NULL. > > > >> > > > >> Finally, have the DPU driver use drm_plane_state.solid_fill and > > > >> instead of > > > >> dpu_plane_state.color_fill, and add extra checks in the DPU atomic > > > >> commit > > > >> callstack to account for a NULL FB in cases where solid_fill is set. > > > >> > > > >> Some drivers support hardware that have optimizations for solid fill > > > >> planes. This series aims to expose these capabilities to userspace as > > > >> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android > > > >> hardware composer HAL) that can be set by apps like the Android Gears > > > >> app. > > > >> > > > >> Userspace can set the solid_fill property to a blob containing the > > > >> appropriate version number and solid fill color (in RGB323232 format) > > > >> and > > > >> setting the framebuffer to NULL. > > > >> > > > >> Note: Currently, there's only one version of the solid_fill blob > > > >> property. > > > >> However if other drivers want to support a similar feature, but require > > > >> more than just the solid fill color, they can extend this feature by > > > >> creating additional versions of the drm_solid_fill struct. > > > >> > > > >> Changes in V2: > > > >> - Dropped SOLID_FILL_FORMAT property (Simon) > > > >> - Switched to implementing solid_fill property as a blob (Simon, > > > >> Dmitry) > > > >> - Changed to checks for if solid_fill_blob is set (Dmitry) > > > >> - Abstracted (plane_state && !solid_fill_blob) checks to helper method > > > >>(Dmitry) > > > >> - Removed DPU_PLANE_COLOR_FILL_FLAG > > > >> - Fixed whitespace and indentation issues (Dmitry) > > > > > > > > Now that this is a blob, I do wonder again whether it's not cleaner to > > > > set > > > > the blob as the FB pointer. Or create some kind other kind of special > > > > data > > > > source objects (because solid fill is by far not the only such thing). > > > > > > > > We'd still end up in special cases like when userspace that doesn't > > > > understand solid fill tries to read out such a framebuffer, but these > > > > cases already exist anyway for lack of priviledges. > > > > > > > > So I still think that feels like the more consistent way to integrate > > > > this > > > > feature. Which doesn't mean it has to happen like that, but the > > > > patches/cover letter should at least explain why we don't do it like > > > > this. > > > > > > Hi Daniel, > > > > > > IIRC we were facing some issues with this check [1] when trying to set > > > FB to a PROP_BLOB instead. Which is why we went with making it a > > > separate property instead. Will mention this in the cover letter. > > > > What kind of issues? Could you please describe them? > > We switched from bitmask to enum style for prop types, which means it's > not possible to express with the current uapi a property which accepts > both an object or a blob. > > Which yeah sucks a bit ... > > But! > > blob properties are kms objects (like framebuffers), so it should be > possible to stuff a blob into an object property as-is. Of course you need > to update the validation code to make sure we accept either an fb or a > blob for the internal representation. But that kind of split internally is > required no matter what I think. I checked your idea and notes from Jessica. So while we can pass blobs to property objects, the prop_fb_id is created as an object property with the type DRM_MODE_OBJECT_FB. Passing DRM_MODE_OBJECT_BLOB would fail a check in drm_property_change_valid_get() -> __drm_mode_object_find(). And I don't think that we should break the existing validation code for this special case. If you insist on using FB_ID for passing solid_fill information, I'd ask you to reconsider using a 1x1 framebuffer. It would be fully compatible with the existing userspace, which can then treat it seamlessly. > -Daniel > > > > > > > > > [1] > > > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71 -- With best wishes Dmitry
Re: [Freedreno] [RFC PATCH v3 3/3] drm/msm/dpu: Use color_fill property for DPU planes
On 1/4/2023 6:16 PM, Dmitry Baryshkov wrote:
On 05/01/2023 01:40, Jessica Zhang wrote:
Initialize and use the color_fill properties for planes in DPU driver. In
addition, relax framebuffer requirements within atomic commit path and
add checks for NULL framebuffers. Finally, drop DPU_PLANE_COLOR_FILL_FLAG
as it's unused.
Changes since V2:
- Fixed dropped 'const' warning
- Dropped use of solid_fill_format
- Switched to using drm_plane_solid_fill_enabled helper method
- Added helper to convert color fill to BGR888 (Rob)
- Added support for solid fill on planes of varying sizes
- Removed DPU_PLANE_COLOR_FILL_FLAG
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +++-
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 ++-
2 files changed, 49 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 13ce321283ff..0695b70ea1b7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -409,6 +409,7 @@ static void _dpu_crtc_blend_setup_mixer(struct
drm_crtc *crtc,
struct drm_plane_state *state;
struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
struct dpu_plane_state *pstate = NULL;
+ const struct msm_format *fmt;
struct dpu_format *format;
struct dpu_hw_ctl *ctl = mixer->lm_ctl;
@@ -441,7 +442,13 @@ static void _dpu_crtc_blend_setup_mixer(struct
drm_crtc *crtc,
sspp_idx - SSPP_VIG0,
state->fb ? state->fb->base.id : -1);
- format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
+ if (pstate->base.fb)
+ fmt = msm_framebuffer_format(pstate->base.fb);
+ else
+ fmt = dpu_get_msm_format(&_dpu_crtc_get_kms(crtc)->base,
+ DRM_FORMAT_ABGR, 0);
+
+ format = to_dpu_format(fmt);
if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
bg_alpha_enable = true;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 86719020afe2..51a7507373f7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -44,7 +44,6 @@
#define DPU_NAME_SIZE 12
-#define DPU_PLANE_COLOR_FILL_FLAG BIT(31)
#define DPU_ZPOS_MAX 255
/* multirect rect index */
@@ -105,7 +104,6 @@ struct dpu_plane {
enum dpu_sspp pipe;
struct dpu_hw_pipe *pipe_hw;
- uint32_t color_fill;
bool is_error;
bool is_rt_pipe;
const struct dpu_mdss_cfg *catalog;
@@ -678,6 +676,17 @@ static void _dpu_plane_setup_scaler(struct
dpu_plane *pdpu,
_cfg);
}
+static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill
solid_fill)
+{
+ uint32_t ret = 0;
+
+ ret |= ((uint8_t) solid_fill.b) << 16;
+ ret |= ((uint8_t) solid_fill.g) << 8;
+ ret |= ((uint8_t) solid_fill.r);
+
+ return ret;
+}
+
/**
* _dpu_plane_color_fill - enables color fill on plane
* @pdpu: Pointer to DPU plane object
@@ -1001,12 +1010,17 @@ static int dpu_plane_atomic_check(struct
drm_plane *plane,
dst = drm_plane_state_dest(new_plane_state);
- fb_rect.x2 = new_plane_state->fb->width;
- fb_rect.y2 = new_plane_state->fb->height;
+ if (new_plane_state->fb) {
+ fb_rect.x2 = new_plane_state->fb->width;
+ fb_rect.y2 = new_plane_state->fb->height;
+ }
max_linewidth = pdpu->catalog->caps->max_linewidth;
- fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
+ if (new_plane_state->fb)
+ fmt =
to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
+ else
+ fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR);
I think this should be more explicit:
if (solid_fill)
fmt = dpu_get_dpu_format(...)
else
fmt = to_dpu_format(msm_framebuffer_format(...).
And in the _dpu_crtc_blend_setup_mixer() too.
Hi Dmitry,
Noted.
Maybe the code can be extracted to a helper.
min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
@@ -1018,7 +1032,7 @@ static int dpu_plane_atomic_check(struct
drm_plane *plane,
return -EINVAL;
/* check src bounds */
- } else if (!dpu_plane_validate_src(, _rect, min_src_size)) {
+ } else if (new_plane_state->fb && !dpu_plane_validate_src(,
_rect, min_src_size)) {
DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
DRM_RECT_ARG());
return -E2BIG;
@@ -1086,9 +1100,10 @@ void dpu_plane_flush(struct drm_plane *plane)
if (pdpu->is_error)
/* force white frame with 100% alpha pipe output on error */
_dpu_plane_color_fill(pdpu, 0xFF, 0xFF);
- else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
+ else if (!(plane->state->fb) &&
drm_plane_solid_fill_enabled(plane->state))
/* force 100% alpha */
- _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
+
Re: [Freedreno] [RFC PATCH v3 2/3] drm: Adjust atomic checks for solid fill color
On 1/4/2023 5:57 PM, Dmitry Baryshkov wrote:
On 05/01/2023 01:40, Jessica Zhang wrote:
Loosen the requirements for atomic and legacy commit so that, in cases
where solid fill planes is enabled (and FB_ID is NULL), the commit can
still go through.
In addition, add framebuffer NULL checks in other areas to account for
FB being NULL when solid fill is enabled.
Changes in V2:
- Changed to checks for if solid_fill_blob is set (Dmitry)
- Abstracted (plane_state && !solid_fill_blob) checks to helper method
(Dmitry)
- Fixed indentation issue (Dmitry)
Changes in V3:
- Created drm_plane_has_visible_data() helper and corrected CRTC and FB
NULL-check logic (Dmitry)
- Merged `if (fb)` blocks in drm_atomic_plane_check() and abstracted
them into helper method (Dmitry)
- Inverted `if (solid_fill_enabled) else if (fb)` check order (Dmitry)
- Fixed indentation (Dmitry)
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic.c | 136
drivers/gpu/drm/drm_atomic_helper.c | 34 ---
drivers/gpu/drm/drm_plane.c | 8 +-
include/drm/drm_atomic_helper.h | 5 +-
include/drm/drm_plane.h | 19
5 files changed, 124 insertions(+), 78 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f197f59f6d99..63f34b430479 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -580,6 +580,76 @@ plane_switching_crtc(const struct drm_plane_state
*old_plane_state,
return true;
}
+static int drm_atomic_check_fb(const struct drm_plane_state *state)
This change should go to a separate patch. Please don't mix refactoring
(moving of the code) with the actual functionality changes.
Hi Dmitry,
Acked.
+{
+ struct drm_plane *plane = state->plane;
+ const struct drm_framebuffer *fb = state->fb;
+ struct drm_mode_rect *clips;
+
+ uint32_t num_clips;
+ unsigned int fb_width, fb_height;
+ int ret;
+
+ /* Check whether this plane supports the fb pixel format. */
+ ret = drm_plane_check_pixel_format(plane, fb->format->format,
+ fb->modifier);
+
+ if (ret) {
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] invalid pixel format %p4cc,
modifier 0x%llx\n",
+ plane->base.id, plane->name,
+ >format->format, fb->modifier);
+ return ret;
+ }
+
+ fb_width = fb->width << 16;
+ fb_height = fb->height << 16;
+
+ /* Make sure source coordinates are inside the fb. */
+ if (state->src_w > fb_width ||
+ state->src_x > fb_width - state->src_w ||
+ state->src_h > fb_height ||
+ state->src_y > fb_height - state->src_h) {
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] invalid source coordinates "
+ "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
+ plane->base.id, plane->name,
+ state->src_w >> 16,
+ ((state->src_w & 0x) * 15625) >> 10,
+ state->src_h >> 16,
+ ((state->src_h & 0x) * 15625) >> 10,
+ state->src_x >> 16,
+ ((state->src_x & 0x) * 15625) >> 10,
+ state->src_y >> 16,
+ ((state->src_y & 0x) * 15625) >> 10,
+ fb->width, fb->height);
+ return -ENOSPC;
+ }
+
+ clips = __drm_plane_get_damage_clips(state);
+ num_clips = drm_plane_get_damage_clips_count(state);
+
+ /* Make sure damage clips are valid and inside the fb. */
+ while (num_clips > 0) {
+ if (clips->x1 >= clips->x2 ||
+ clips->y1 >= clips->y2 ||
+ clips->x1 < 0 ||
+ clips->y1 < 0 ||
+ clips->x2 > fb_width ||
+ clips->y2 > fb_height) {
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] invalid damage clip %d %d %d
%d\n",
+ plane->base.id, plane->name, clips->x1,
+ clips->y1, clips->x2, clips->y2);
+ return -EINVAL;
+ }
+ clips++;
+ num_clips--;
+ }
+
+ return 0;
+}
+
/**
* drm_atomic_plane_check - check plane state
* @old_plane_state: old plane state to check
@@ -596,13 +666,12 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
struct drm_plane *plane = new_plane_state->plane;
struct drm_crtc *crtc = new_plane_state->crtc;
const struct drm_framebuffer *fb = new_plane_state->fb;
- unsigned int fb_width, fb_height;
- struct drm_mode_rect *clips;
- uint32_t num_clips;
int ret;
- /* either *both* CRTC and FB must be set, or neither */
- if (crtc && !fb) {
+ /* When solid_fill is disabled,
+ * either *both* CRTC and FB must be set, or neither
+ */
+ if (crtc && !drm_atomic_has_visible_data(new_plane_state)) {
drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set
Re: [Freedreno] [RFC PATCH v3 0/3] Support for Solid Fill Planes
On 1/5/2023 7:43 PM, Dmitry Baryshkov wrote: On Fri, 6 Jan 2023 at 02:38, Jessica Zhang wrote: On 1/5/2023 3:33 AM, Daniel Vetter wrote: On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote: Introduce and add support for a solid_fill property. When the solid_fill property is set, and the framebuffer is set to NULL, memory fetch will be disabled. In addition, loosen the NULL FB checks within the atomic commit callstack to allow a NULL FB when the solid_fill property is set and add FB checks in methods where the FB was previously assumed to be non-NULL. Finally, have the DPU driver use drm_plane_state.solid_fill and instead of dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit callstack to account for a NULL FB in cases where solid_fill is set. Some drivers support hardware that have optimizations for solid fill planes. This series aims to expose these capabilities to userspace as some compositors have a solid fill flag (ex. SOLID_COLOR in the Android hardware composer HAL) that can be set by apps like the Android Gears app. Userspace can set the solid_fill property to a blob containing the appropriate version number and solid fill color (in RGB323232 format) and setting the framebuffer to NULL. Note: Currently, there's only one version of the solid_fill blob property. However if other drivers want to support a similar feature, but require more than just the solid fill color, they can extend this feature by creating additional versions of the drm_solid_fill struct. Changes in V2: - Dropped SOLID_FILL_FORMAT property (Simon) - Switched to implementing solid_fill property as a blob (Simon, Dmitry) - Changed to checks for if solid_fill_blob is set (Dmitry) - Abstracted (plane_state && !solid_fill_blob) checks to helper method (Dmitry) - Removed DPU_PLANE_COLOR_FILL_FLAG - Fixed whitespace and indentation issues (Dmitry) Now that this is a blob, I do wonder again whether it's not cleaner to set the blob as the FB pointer. Or create some kind other kind of special data source objects (because solid fill is by far not the only such thing). We'd still end up in special cases like when userspace that doesn't understand solid fill tries to read out such a framebuffer, but these cases already exist anyway for lack of priviledges. So I still think that feels like the more consistent way to integrate this feature. Which doesn't mean it has to happen like that, but the patches/cover letter should at least explain why we don't do it like this. Hi Daniel, IIRC we were facing some issues with this check [1] when trying to set FB to a PROP_BLOB instead. Which is why we went with making it a separate property instead. Will mention this in the cover letter. What kind of issues? Could you please describe them? Hi Dmitry, PROP_BLOB is defined as a legacy type here [1], but FB_ID is a PROP_OBJECT which is defined as an extended type [2]. So, setting a property blob as the FB would fail drm_property_flags_valid() due to this check [3]. [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/include/uapi/drm/drm_mode.h#L523 [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/include/uapi/drm/drm_mode.h#L534 [3] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71 Thanks, Jessica Zhang [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71 Thanks, Jessica Zhang -Daniel Changes in V3: - Fixed some logic errors in atomic checks (Dmitry) - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() helper methods (Dmitry) Jessica Zhang (3): drm: Introduce solid fill property for drm plane drm: Adjust atomic checks for solid fill color drm/msm/dpu: Use color_fill property for DPU planes drivers/gpu/drm/drm_atomic.c | 136 +- drivers/gpu/drm/drm_atomic_helper.c | 34 +++--- drivers/gpu/drm/drm_atomic_state_helper.c | 9 ++ drivers/gpu/drm/drm_atomic_uapi.c | 59 ++ drivers/gpu/drm/drm_blend.c | 17 +++ drivers/gpu/drm/drm_plane.c | 8 +- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 +++ include/drm/drm_atomic_helper.h | 5 +- include/drm/drm_blend.h | 1 + include/drm/drm_plane.h | 62 ++ 11 files changed, 302 insertions(+), 103 deletions(-) -- 2.38.1 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch -- With best wishes Dmitry
Re: [Freedreno] [RFC PATCH v3 0/3] Support for Solid Fill Planes
On Fri, Jan 06, 2023 at 05:43:23AM +0200, Dmitry Baryshkov wrote: > On Fri, 6 Jan 2023 at 02:38, Jessica Zhang wrote: > > > > > > > > On 1/5/2023 3:33 AM, Daniel Vetter wrote: > > > On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote: > > >> Introduce and add support for a solid_fill property. When the solid_fill > > >> property is set, and the framebuffer is set to NULL, memory fetch will be > > >> disabled. > > >> > > >> In addition, loosen the NULL FB checks within the atomic commit callstack > > >> to allow a NULL FB when the solid_fill property is set and add FB checks > > >> in methods where the FB was previously assumed to be non-NULL. > > >> > > >> Finally, have the DPU driver use drm_plane_state.solid_fill and instead > > >> of > > >> dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit > > >> callstack to account for a NULL FB in cases where solid_fill is set. > > >> > > >> Some drivers support hardware that have optimizations for solid fill > > >> planes. This series aims to expose these capabilities to userspace as > > >> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android > > >> hardware composer HAL) that can be set by apps like the Android Gears > > >> app. > > >> > > >> Userspace can set the solid_fill property to a blob containing the > > >> appropriate version number and solid fill color (in RGB323232 format) and > > >> setting the framebuffer to NULL. > > >> > > >> Note: Currently, there's only one version of the solid_fill blob > > >> property. > > >> However if other drivers want to support a similar feature, but require > > >> more than just the solid fill color, they can extend this feature by > > >> creating additional versions of the drm_solid_fill struct. > > >> > > >> Changes in V2: > > >> - Dropped SOLID_FILL_FORMAT property (Simon) > > >> - Switched to implementing solid_fill property as a blob (Simon, Dmitry) > > >> - Changed to checks for if solid_fill_blob is set (Dmitry) > > >> - Abstracted (plane_state && !solid_fill_blob) checks to helper method > > >>(Dmitry) > > >> - Removed DPU_PLANE_COLOR_FILL_FLAG > > >> - Fixed whitespace and indentation issues (Dmitry) > > > > > > Now that this is a blob, I do wonder again whether it's not cleaner to set > > > the blob as the FB pointer. Or create some kind other kind of special data > > > source objects (because solid fill is by far not the only such thing). > > > > > > We'd still end up in special cases like when userspace that doesn't > > > understand solid fill tries to read out such a framebuffer, but these > > > cases already exist anyway for lack of priviledges. > > > > > > So I still think that feels like the more consistent way to integrate this > > > feature. Which doesn't mean it has to happen like that, but the > > > patches/cover letter should at least explain why we don't do it like this. > > > > Hi Daniel, > > > > IIRC we were facing some issues with this check [1] when trying to set > > FB to a PROP_BLOB instead. Which is why we went with making it a > > separate property instead. Will mention this in the cover letter. > > What kind of issues? Could you please describe them? We switched from bitmask to enum style for prop types, which means it's not possible to express with the current uapi a property which accepts both an object or a blob. Which yeah sucks a bit ... But! blob properties are kms objects (like framebuffers), so it should be possible to stuff a blob into an object property as-is. Of course you need to update the validation code to make sure we accept either an fb or a blob for the internal representation. But that kind of split internally is required no matter what I think. -Daniel > > > > > [1] > > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71 > > > > Thanks, > > > > Jessica Zhang > > > > > -Daniel > > > > > >> > > >> Changes in V3: > > >> - Fixed some logic errors in atomic checks (Dmitry) > > >> - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() > > >> helper > > >>methods (Dmitry) > > >> > > >> Jessica Zhang (3): > > >>drm: Introduce solid fill property for drm plane > > >>drm: Adjust atomic checks for solid fill color > > >>drm/msm/dpu: Use color_fill property for DPU planes > > >> > > >> drivers/gpu/drm/drm_atomic.c | 136 +- > > >> drivers/gpu/drm/drm_atomic_helper.c | 34 +++--- > > >> drivers/gpu/drm/drm_atomic_state_helper.c | 9 ++ > > >> drivers/gpu/drm/drm_atomic_uapi.c | 59 ++ > > >> drivers/gpu/drm/drm_blend.c | 17 +++ > > >> drivers/gpu/drm/drm_plane.c | 8 +- > > >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +- > > >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 +++ > > >> include/drm/drm_atomic_helper.h | 5 +- > > >> include/drm/drm_blend.h | 1 + > > >> include/drm/drm_plane.h
Re: [Freedreno] [PATCH v6 03/11] dt-bindings: display/msm: add sm8350 and sm8450 DSI PHYs
On 06/01/2023 17:39, Krzysztof Kozlowski wrote: On 07/12/2022 02:22, Dmitry Baryshkov wrote: SM8350 and SM8450 platforms use the same driver and same bindings as the existing 7nm DSI PHYs. Add corresponding compatibility strings. Acked-by: Krzysztof Kozlowski Signed-off-by: Dmitry Baryshkov --- Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml index c851770bbdf2..bffd161fedfd 100644 --- a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml +++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml @@ -15,6 +15,8 @@ allOf: properties: compatible: enum: + - qcom,dsi-phy-5nm-8350 + - qcom,dsi-phy-5nm-8450 If this patch was not merged (so far nothing in next), can we make it proper SoC compatible? Ack. Bjorn has merged the dtsi bits, but I'll send a fixup. qcom,sm8450-dsi-phy-5nm The SC7280 already uses such pattern. - qcom,dsi-phy-7nm - qcom,dsi-phy-7nm-8150 - qcom,sc7280-dsi-phy-7nm Best regards, Krzysztof -- With best wishes Dmitry
Re: [Freedreno] [PATCH 1/6] dt-bindings: display/msm: document the SM8550 DSI PHY
On 04/01/2023 10:08, Neil Armstrong wrote: > Document the SM8550 DSI PHY which is very close from the 7nm > and 5nm DSI PHYs found in earlier platforms. > > Signed-off-by: Neil Armstrong > --- > Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > index bffd161fedfd..f72727f81076 100644 > --- a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > +++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > @@ -15,6 +15,7 @@ allOf: > properties: >compatible: > enum: > + - qcom,dsi-phy-4nm-8550 >- qcom,dsi-phy-5nm-8350 >- qcom,dsi-phy-5nm-8450 Poor patterns once allowed like to keep growing... I commented here: https://lore.kernel.org/all/ccbb47e4-d780-0b1d-814e-27e86b6c3...@linaro.org/ so let's wait for response about other compatibles. >- qcom,dsi-phy-7nm > Best regards, Krzysztof
Re: [Freedreno] [PATCH v6 03/11] dt-bindings: display/msm: add sm8350 and sm8450 DSI PHYs
On 07/12/2022 02:22, Dmitry Baryshkov wrote: > SM8350 and SM8450 platforms use the same driver and same bindings as the > existing 7nm DSI PHYs. Add corresponding compatibility strings. > > Acked-by: Krzysztof Kozlowski > Signed-off-by: Dmitry Baryshkov > --- > Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > index c851770bbdf2..bffd161fedfd 100644 > --- a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > +++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > @@ -15,6 +15,8 @@ allOf: > properties: >compatible: > enum: > + - qcom,dsi-phy-5nm-8350 > + - qcom,dsi-phy-5nm-8450 If this patch was not merged (so far nothing in next), can we make it proper SoC compatible? qcom,sm8450-dsi-phy-5nm The SC7280 already uses such pattern. >- qcom,dsi-phy-7nm >- qcom,dsi-phy-7nm-8150 >- qcom,sc7280-dsi-phy-7nm Best regards, Krzysztof
Re: [Freedreno] [PATCH] drm/msm: Set preferred depth.
On Fri, Jan 06, 2023 at 09:18:21AM +0200, Dmitry Baryshkov wrote:
> On 06/01/2023 09:16, Steev Klimaszewski wrote:
> > As of commit 37c90d589dc0 ("drm/fb-helper: Fix single-probe color-format
> > selection"), if no supported color formats are found, it tries to use the
> > driver provided default, which msm didn't have set and leads to the
> > following output:
> >
> > msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not
> > supported
> > msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not
> > supported
> > msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not
> > supported
> > msm_dpu ae01000.display-controller: [drm] No compatible format found
> > [ cut here ]
> > WARNING: CPU: 0 PID: 73 at drivers/gpu/drm/drm_atomic.c:1604
> > __drm_atomic_helper_set_config+0x240/0x33c
> > Modules linked in: ext4 mbcache jbd2 msm mdt_loader ocmem gpu_sched
> > llcc_qcom gpio_keys qrtr
> > CPU: 0 PID: 73 Comm: kworker/u16:2 Not tainted 6.2.0-rc2-next-20230106 #53
> > Hardware name: LENOVO 21BX0015US/21BX0015US, BIOS N3HET74W (1.46 )
> > 10/12/2022
> > Workqueue: events_unbound deferred_probe_work_func
> > pstate: 8045 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > pc : __drm_atomic_helper_set_config+0x240/0x33c
> > lr : __drm_atomic_helper_set_config+0x68/0x33c
> > sp : 88a7b790
> > x29: 88a7b790 x28: 73ee3e130a00 x27:
> > x26: 73ee3d256e00 x25: 0038 x24: 73e6c0d65e00
> > x23: 73e6c17a7800 x22: 73e6c0d64e00 x21: 73e79c025e00
> > x20: c0d64e00 x19: 73ee3e130a00 x18:
> > x17: 662074616d726f66 x16: 20656c6269746170 x15:
> > x14: x13: x12:
> > x11: x10: x9 : a829144ff8bc
> > x8 : x7 : x6 :
> > x5 : 73e6c0d65f50 x4 : 73ee3d254950 x3 : 73e6c0d65ec0
> > x2 : 73ee3c953a00 x1 : 73e79c025580 x0 :
> > Call trace:
> > __drm_atomic_helper_set_config+0x240/0x33c
> > drm_client_modeset_commit_atomic+0x160/0x280
> > drm_client_modeset_commit_locked+0x64/0x194
> > drm_client_modeset_commit+0x38/0x60
> > __drm_fb_helper_initial_config_and_unlock+0x528/0x63c
> > drm_fb_helper_initial_config+0x54/0x64
> > msm_fbdev_init+0x94/0xfc [msm]
> > msm_drm_bind+0x548/0x614 [msm]
> > try_to_bring_up_aggregate_device+0x1e4/0x2d0
> > __component_add+0xc4/0x1c0
> > component_add+0x1c/0x2c
> > dp_display_probe+0x2a4/0x460 [msm]
> > platform_probe+0x70/0xcc
> > really_probe+0xc8/0x3e0
> > __driver_probe_device+0x84/0x190
> > driver_probe_device+0x44/0x120
> > __device_attach_driver+0xc4/0x160
> > bus_for_each_drv+0x84/0xe0
> > __device_attach+0xa4/0x1cc
> > device_initial_probe+0x1c/0x2c
> > bus_probe_device+0xa4/0xb0
> > deferred_probe_work_func+0xc0/0x114
> > process_one_work+0x1ec/0x470
> > worker_thread+0x74/0x410
> > kthread+0xfc/0x110
> > ret_from_fork+0x10/0x20
> > ---[ end trace ]---
> >
> > Signed-off-by: Steev Klimaszewski
> > ---
> > drivers/gpu/drm/msm/msm_drv.c | 1 +
> > 1 file changed, 1 insertion(+)
>
> Suggested-by: Dmitry Baryshkov
> Reviewed-by: Dmitry Baryshkov
I think a documentation patch that preferred_depth = 0 actually means
xrgb would be good, since we seem to have a serious confusion going on
here?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [Freedreno] [PATCH] drm/msm: Set preferred depth.
On Fri, Jan 06, 2023 at 09:31:31AM +0100, Thomas Zimmermann wrote:
> Am 06.01.23 um 08:16 schrieb Steev Klimaszewski:
> > As of commit 37c90d589dc0 ("drm/fb-helper: Fix single-probe color-format
> > selection"), if no supported color formats are found, it tries to use the
> > driver provided default, which msm didn't have set and leads to the
> > following output:
> >
> > msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not
> > supported
> > msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not
> > supported
> > msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not
> > supported
> > msm_dpu ae01000.display-controller: [drm] No compatible format found
> > [ cut here ]
> > WARNING: CPU: 0 PID: 73 at drivers/gpu/drm/drm_atomic.c:1604
> > __drm_atomic_helper_set_config+0x240/0x33c
> > Modules linked in: ext4 mbcache jbd2 msm mdt_loader ocmem gpu_sched
> > llcc_qcom gpio_keys qrtr
> > CPU: 0 PID: 73 Comm: kworker/u16:2 Not tainted 6.2.0-rc2-next-20230106 #53
> > Hardware name: LENOVO 21BX0015US/21BX0015US, BIOS N3HET74W (1.46 )
> > 10/12/2022
> > Workqueue: events_unbound deferred_probe_work_func
> > pstate: 8045 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > pc : __drm_atomic_helper_set_config+0x240/0x33c
> > lr : __drm_atomic_helper_set_config+0x68/0x33c
> > sp : 88a7b790
> > x29: 88a7b790 x28: 73ee3e130a00 x27:
> > x26: 73ee3d256e00 x25: 0038 x24: 73e6c0d65e00
> > x23: 73e6c17a7800 x22: 73e6c0d64e00 x21: 73e79c025e00
> > x20: c0d64e00 x19: 73ee3e130a00 x18:
> > x17: 662074616d726f66 x16: 20656c6269746170 x15:
> > x14: x13: x12:
> > x11: x10: x9 : a829144ff8bc
> > x8 : x7 : x6 :
> > x5 : 73e6c0d65f50 x4 : 73ee3d254950 x3 : 73e6c0d65ec0
> > x2 : 73ee3c953a00 x1 : 73e79c025580 x0 :
> > Call trace:
> > __drm_atomic_helper_set_config+0x240/0x33c
> > drm_client_modeset_commit_atomic+0x160/0x280
> > drm_client_modeset_commit_locked+0x64/0x194
> > drm_client_modeset_commit+0x38/0x60
> > __drm_fb_helper_initial_config_and_unlock+0x528/0x63c
> > drm_fb_helper_initial_config+0x54/0x64
> > msm_fbdev_init+0x94/0xfc [msm]
> > msm_drm_bind+0x548/0x614 [msm]
> > try_to_bring_up_aggregate_device+0x1e4/0x2d0
> > __component_add+0xc4/0x1c0
> > component_add+0x1c/0x2c
> > dp_display_probe+0x2a4/0x460 [msm]
> > platform_probe+0x70/0xcc
> > really_probe+0xc8/0x3e0
> > __driver_probe_device+0x84/0x190
> > driver_probe_device+0x44/0x120
> > __device_attach_driver+0xc4/0x160
> > bus_for_each_drv+0x84/0xe0
> > __device_attach+0xa4/0x1cc
> > device_initial_probe+0x1c/0x2c
> > bus_probe_device+0xa4/0xb0
> > deferred_probe_work_func+0xc0/0x114
> > process_one_work+0x1ec/0x470
> > worker_thread+0x74/0x410
> > kthread+0xfc/0x110
> > ret_from_fork+0x10/0x20
> > ---[ end trace ]---
> >
> > Signed-off-by: Steev Klimaszewski
> > ---
> > drivers/gpu/drm/msm/msm_drv.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 8b0b0ac74a6f..65c4c93c311e 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -479,6 +479,7 @@ static int msm_drm_init(struct device *dev, const
> > struct drm_driver *drv)
> > drm_helper_move_panel_connectors_to_head(ddev);
> > + ddev->mode_config.preferred_depth = 24;
>
> Reviewed-by: Thomas Zimmermann
preferred_depth is not a mandatory thing, we need to fix the fbdev patch,
not work around that in all the drivers. xrgb is the assumed default.
-Daniel
>
> Best regards
> Thomas
>
> > ddev->mode_config.funcs = _config_funcs;
> > ddev->mode_config.helper_private = _config_helper_funcs;
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[Freedreno] [PATCH RESEND] drm/msm: Add missing check and destroy for alloc_ordered_workqueue
Add check for the return value of alloc_ordered_workqueue as it may return
NULL pointer.
Moreover, use the destroy_workqueue in the later fails in order to avoid
memory leak.
Fixes: c8afe684c95c ("drm/msm: basic KMS driver for snapdragon")
Signed-off-by: Jiasheng Jiang
---
drivers/gpu/drm/msm/msm_drv.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 8b0b0ac74a6f..b82d938226ad 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -418,6 +418,8 @@ static int msm_drm_init(struct device *dev, const struct
drm_driver *drv)
priv->dev = ddev;
priv->wq = alloc_ordered_workqueue("msm", 0);
+ if (!priv->wq)
+ return -ENOMEM;
INIT_LIST_HEAD(>objects);
mutex_init(>obj_lock);
@@ -440,12 +442,12 @@ static int msm_drm_init(struct device *dev, const struct
drm_driver *drv)
ret = msm_init_vram(ddev);
if (ret)
- return ret;
+ goto err_destroy_workqueue;
/* Bind all our sub-components: */
ret = component_bind_all(dev, ddev);
if (ret)
- return ret;
+ goto err_destroy_workqueue;
dma_set_max_seg_size(dev, UINT_MAX);
@@ -540,6 +542,8 @@ static int msm_drm_init(struct device *dev, const struct
drm_driver *drv)
err_msm_uninit:
msm_drm_uninit(dev);
+err_destroy_workqueue:
+ destroy_workqueue(priv->wq);
return ret;
}
--
2.25.1
Re: [Freedreno] [PATCH] drm/msm: Set preferred depth.
Hi
Am 06.01.23 um 08:16 schrieb Steev Klimaszewski:
As of commit 37c90d589dc0 ("drm/fb-helper: Fix single-probe color-format
selection"), if no supported color formats are found, it tries to use the
driver provided default, which msm didn't have set and leads to the
following output:
msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not supported
msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not supported
msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not supported
msm_dpu ae01000.display-controller: [drm] No compatible format found
[ cut here ]
WARNING: CPU: 0 PID: 73 at drivers/gpu/drm/drm_atomic.c:1604
__drm_atomic_helper_set_config+0x240/0x33c
Modules linked in: ext4 mbcache jbd2 msm mdt_loader ocmem gpu_sched llcc_qcom
gpio_keys qrtr
CPU: 0 PID: 73 Comm: kworker/u16:2 Not tainted 6.2.0-rc2-next-20230106 #53
Hardware name: LENOVO 21BX0015US/21BX0015US, BIOS N3HET74W (1.46 ) 10/12/2022
Workqueue: events_unbound deferred_probe_work_func
pstate: 8045 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : __drm_atomic_helper_set_config+0x240/0x33c
lr : __drm_atomic_helper_set_config+0x68/0x33c
sp : 88a7b790
x29: 88a7b790 x28: 73ee3e130a00 x27:
x26: 73ee3d256e00 x25: 0038 x24: 73e6c0d65e00
x23: 73e6c17a7800 x22: 73e6c0d64e00 x21: 73e79c025e00
x20: c0d64e00 x19: 73ee3e130a00 x18:
x17: 662074616d726f66 x16: 20656c6269746170 x15:
x14: x13: x12:
x11: x10: x9 : a829144ff8bc
x8 : x7 : x6 :
x5 : 73e6c0d65f50 x4 : 73ee3d254950 x3 : 73e6c0d65ec0
x2 : 73ee3c953a00 x1 : 73e79c025580 x0 :
Call trace:
__drm_atomic_helper_set_config+0x240/0x33c
drm_client_modeset_commit_atomic+0x160/0x280
drm_client_modeset_commit_locked+0x64/0x194
drm_client_modeset_commit+0x38/0x60
__drm_fb_helper_initial_config_and_unlock+0x528/0x63c
drm_fb_helper_initial_config+0x54/0x64
msm_fbdev_init+0x94/0xfc [msm]
msm_drm_bind+0x548/0x614 [msm]
try_to_bring_up_aggregate_device+0x1e4/0x2d0
__component_add+0xc4/0x1c0
component_add+0x1c/0x2c
dp_display_probe+0x2a4/0x460 [msm]
platform_probe+0x70/0xcc
really_probe+0xc8/0x3e0
__driver_probe_device+0x84/0x190
driver_probe_device+0x44/0x120
__device_attach_driver+0xc4/0x160
bus_for_each_drv+0x84/0xe0
__device_attach+0xa4/0x1cc
device_initial_probe+0x1c/0x2c
bus_probe_device+0xa4/0xb0
deferred_probe_work_func+0xc0/0x114
process_one_work+0x1ec/0x470
worker_thread+0x74/0x410
kthread+0xfc/0x110
ret_from_fork+0x10/0x20
---[ end trace ]---
Signed-off-by: Steev Klimaszewski
---
drivers/gpu/drm/msm/msm_drv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 8b0b0ac74a6f..65c4c93c311e 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -479,6 +479,7 @@ static int msm_drm_init(struct device *dev, const struct
drm_driver *drv)
drm_helper_move_panel_connectors_to_head(ddev);
+ ddev->mode_config.preferred_depth = 24;
Reviewed-by: Thomas Zimmermann
Best regards
Thomas
ddev->mode_config.funcs = _config_funcs;
ddev->mode_config.helper_private = _config_helper_funcs;
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
