DRM_MODE_PAGE_FLIP_ASYNC for atomic IOCTL

[Drew Davenport]

My understanding is that DRM_MODE_PAGE_FLIP_ASYNC is currently only
supported for the legacy DRM_IOCTL_MODE_PAGE_FLIP ioctl. Comments in
related code suggest that while ASYNC is not currently supported for
DRM_IOCTL_MODE_ATOMIC, there could be support for it at some point in
the future.

Is anyone currently working on adding DRM_IOCTL_MODE_PAGE_FLIP support
to DRM_IOCTL_MODE_ATOMIC?

If not, has there already been any discussion as to what is required to
make it happen?

Thanks.

Re: [Intel-gfx] [PATCH] drm/i915/display: Check source height is > 0

[Drew Davenport]

On Tue, Dec 27, 2022 at 05:55:17PM +, Teres Alexis, Alan Previn wrote:
> Is there a better place for this check higher up the intel specific 
> atomic-check? (so the check won't be skl specific - i notice that 
> intel_adjusted_rate is also called by
> ilk_foo as well and non-backend-specific functions). Else, perhaps 
> intel_adjusted_rate should add a check + WARN? (if we are trying to propagate 
> this slowly across HW).

Would intel_plane_atomic_check_with_state be a more appropriate
place to check that the src width and height are at least 1? This is
where skl_plane_check and other HW's foo_plane_check functions are called
from.

I don't think that the potential divide-by-zero will be hit in the case
where intel_adjusted_rate is called from ilk_pipe_pixel_rate because the
src rect will not have a fractional part to it in this case. I'm assuming
that something earlier on would catch and reject a src with zero
width/height.

Drew

> 
> 
> ...alan 
> 
> On Mon, 2022-12-26 at 22:53 -0700, Drew Davenport wrote:
> > The error message suggests that the height of the src rect must be at
> > least 1. Reject source with height of 0.
> > 
> > Signed-off-by: Drew Davenport 
> > 
> > ---
> > I was investigating some divide-by-zero crash reports on ChromeOS which
> > pointed to the intel_adjusted_rate function. Further prodding showed
> > that I could reproduce this in a simple test program if I made src_h
> > some value less than 1 but greater than 0.
> > 
> > This seemed to be a sensible place to check that the source height is at
> > least 1. I tried to repro this issue on an amd device I had on hand, and
> > the configuration was rejected.
> > 
> > Would it make sense to add a check that source dimensions are at least 1
> > somewhere in core, like in drm_atomic_plane_check? Or is that a valid
> > use case on some devices, and thus any such check should be done on a
> > per-driver basis?
> > 
> > Thanks.
> > 
> >  drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c 
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 4b79c2d2d6177..9b172a1e90deb 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct 
> > intel_plane_state *plane_state)
> > u32 offset;
> > int ret;
> >  
> > -   if (w > max_width || w < min_width || h > max_height) {
> > +   if (w > max_width || w < min_width || h > max_height || h < 1) {
> > drm_dbg_kms(_priv->drm,
> > "requested Y/RGB source size %dx%d outside limits 
> > (min: %dx1 max: %dx%d)\n",
> > w, h, min_width, max_width, max_height);
> > -- 
> > 2.39.0.314.g84b9a713c41-goog
> > 
> 

Re: [PATCH] drm/i915/display: Check source height is > 0

[Drew Davenport]

On Tue, Jan 03, 2023 at 12:42:43PM +0200, Juha-Pekka Heikkila wrote:
> Hi Drew,

Hi Juha-Pekka, sorry for the late response since I was on vacation.

> 
> this is good find. I went looking where the problem is in and saw what you
> probably also saw earlier.
> 
> I was wondering if diff below would be better fix? I assume this would end
> up with einval or erange in your case but code flow otherwise would stay as
> is while fixing all future callers for same issue:

Yes, the function you identify below is where I encountered
divide-by-zero errors. If width/height less than 1 is a legitimate use
case, then your proposed solution looks like it would be better. It
should have no risk of regression in userspace either, which is nice.

I tested your patch, and as expected I did not hit the divide-by-zero
error, though the test commit was rejected due to a check further along
inside skl_update_scaler. Perhaps there is some other configuration
which would pass the test commit with a width/height less than 1, but I
didn't dig much further.

> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 10e1fc9d0698..a9948e8d3543 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -144,7 +144,7 @@ unsigned int intel_adjusted_rate(const struct drm_rect
> *src,
>  const struct drm_rect *dst,
>  unsigned int rate)
>  {
> -   unsigned int src_w, src_h, dst_w, dst_h;
> +   unsigned int src_w, src_h, dst_w, dst_h, dst_wh;
> 
> src_w = drm_rect_width(src) >> 16;
> src_h = drm_rect_height(src) >> 16;
> @@ -155,8 +155,10 @@ unsigned int intel_adjusted_rate(const struct drm_rect
> *src,
> dst_w = min(src_w, dst_w);
> dst_h = min(src_h, dst_h);
> 
> -   return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h),
> -   dst_w * dst_h);
> +   /* in case src contained only fractional part */
> +   dst_wh = max(dst_w * dst_h, (unsigned) 1);
> +
> +   return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), dst_wh);
>  }
> 
>  unsigned int intel_plane_pixel_rate(const struct intel_crtc_state
> *crtc_state,
> 
> 
> What do you think? I'll in any case come up with some test for this in igt.

I see that you've posted your fix to the list already. Adding a
test to cover this in IGT also sounds great. Thanks!

Breadcrumbs to Juha-Pekka's patch for anyone following this
thread: https://patchwork.freedesktop.org/series/112396/

> 
> /Juha-Pekka
> 
> On 27.12.2022 7.53, Drew Davenport wrote:
> > The error message suggests that the height of the src rect must be at
> > least 1. Reject source with height of 0.
> > 
> > Signed-off-by: Drew Davenport 
> > 
> > ---
> > I was investigating some divide-by-zero crash reports on ChromeOS which
> > pointed to the intel_adjusted_rate function. Further prodding showed
> > that I could reproduce this in a simple test program if I made src_h
> > some value less than 1 but greater than 0.
> > 
> > This seemed to be a sensible place to check that the source height is at
> > least 1. I tried to repro this issue on an amd device I had on hand, and
> > the configuration was rejected.
> > 
> > Would it make sense to add a check that source dimensions are at least 1
> > somewhere in core, like in drm_atomic_plane_check? Or is that a valid
> > use case on some devices, and thus any such check should be done on a
> > per-driver basis?
> > 
> > Thanks.
> > 
> >   drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c 
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 4b79c2d2d6177..9b172a1e90deb 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct 
> > intel_plane_state *plane_state)
> > u32 offset;
> > int ret;
> > -   if (w > max_width || w < min_width || h > max_height) {
> > +   if (w > max_width || w < min_width || h > max_height || h < 1) {
> > drm_dbg_kms(_priv->drm,
> > "requested Y/RGB source size %dx%d outside limits 
> > (min: %dx1 max: %dx%d)\n",
> > w, h, min_width, max_width, max_height);
> 

[PATCH] drm/i915/display: Check source height is > 0

[Drew Davenport]

The error message suggests that the height of the src rect must be at
least 1. Reject source with height of 0.

Signed-off-by: Drew Davenport 

---
I was investigating some divide-by-zero crash reports on ChromeOS which
pointed to the intel_adjusted_rate function. Further prodding showed
that I could reproduce this in a simple test program if I made src_h
some value less than 1 but greater than 0.

This seemed to be a sensible place to check that the source height is at
least 1. I tried to repro this issue on an amd device I had on hand, and
the configuration was rejected.

Would it make sense to add a check that source dimensions are at least 1
somewhere in core, like in drm_atomic_plane_check? Or is that a valid
use case on some devices, and thus any such check should be done on a
per-driver basis?

Thanks.

 drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c 
b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 4b79c2d2d6177..9b172a1e90deb 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct 
intel_plane_state *plane_state)
u32 offset;
int ret;
 
-   if (w > max_width || w < min_width || h > max_height) {
+   if (w > max_width || w < min_width || h > max_height || h < 1) {
drm_dbg_kms(_priv->drm,
"requested Y/RGB source size %dx%d outside limits 
(min: %dx1 max: %dx%d)\n",
w, h, min_width, max_width, max_height);
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 5/5] drm/panel-samsung-atna33xc20: Extend autosuspend delay

[Drew Davenport]

Avoid the panel oscillating on and off during boot. In some cases it
will be more than 1000ms between powering the panel to read the EDID early
during boot, and enabling the panel for display. Extending the
autosuspend delay avoids autosuspending during this interval.

Signed-off-by: Drew Davenport 

---

 drivers/gpu/drm/panel/panel-samsung-atna33xc20.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c 
b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
index f4616f0367846..5703f4712d96e 100644
--- a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
+++ b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
@@ -310,7 +310,7 @@ static int atana33xc20_probe(struct dp_aux_ep_device 
*aux_ep)
ret = devm_add_action_or_reset(dev,  atana33xc20_runtime_disable, dev);
if (ret)
return ret;
-   pm_runtime_set_autosuspend_delay(dev, 1000);
+   pm_runtime_set_autosuspend_delay(dev, 2000);
pm_runtime_use_autosuspend(dev);
ret = devm_add_action_or_reset(dev,  atana33xc20_dont_use_autosuspend, 
dev);
if (ret)
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 2/5] drm/panel-samsung-atna33xc20: Use ktime_get_boottime for delays

[Drew Davenport]

ktime_get_boottime continues while the device is suspended. This change
ensures that the resume path will not be delayed if the power off delay
has already been met while the device is suspended

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/panel/panel-samsung-atna33xc20.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c 
b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
index 5a8b978c64158..f4616f0367846 100644
--- a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
+++ b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
@@ -53,7 +53,7 @@ static void atana33xc20_wait(ktime_t start_ktime, unsigned 
int min_ms)
ktime_t now_ktime, min_ktime;
 
min_ktime = ktime_add(start_ktime, ms_to_ktime(min_ms));
-   now_ktime = ktime_get();
+   now_ktime = ktime_get_boottime();
 
if (ktime_before(now_ktime, min_ktime))
msleep(ktime_to_ms(ktime_sub(min_ktime, now_ktime)) + 1);
@@ -75,7 +75,7 @@ static int atana33xc20_suspend(struct device *dev)
ret = regulator_disable(p->supply);
if (ret)
return ret;
-   p->powered_off_time = ktime_get();
+   p->powered_off_time = ktime_get_boottime();
p->el3_was_on = false;
 
return 0;
@@ -93,7 +93,7 @@ static int atana33xc20_resume(struct device *dev)
ret = regulator_enable(p->supply);
if (ret)
return ret;
-   p->powered_on_time = ktime_get();
+   p->powered_on_time = ktime_get_boottime();
 
if (p->no_hpd) {
msleep(HPD_MAX_MS);
@@ -142,7 +142,7 @@ static int atana33xc20_disable(struct drm_panel *panel)
return 0;
 
gpiod_set_value_cansleep(p->el_on3_gpio, 0);
-   p->el_on3_off_time = ktime_get();
+   p->el_on3_off_time = ktime_get_boottime();
p->enabled = false;
 
/*
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 3/5] drm/panel-simple: Use ktime_get_boottime for delays

[Drew Davenport]

ktime_get_boottime continues while the device is suspended. This change
ensures that the resume path will not be delayed if the power off delay
has already been met while the device is suspended

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/panel/panel-simple.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c 
b/drivers/gpu/drm/panel/panel-simple.c
index 8a3b685c2fcc0..065f378bba9d2 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -280,7 +280,7 @@ static void panel_simple_wait(ktime_t start_ktime, unsigned 
int min_ms)
return;
 
min_ktime = ktime_add(start_ktime, ms_to_ktime(min_ms));
-   now_ktime = ktime_get();
+   now_ktime = ktime_get_boottime();
 
if (ktime_before(now_ktime, min_ktime))
msleep(ktime_to_ms(ktime_sub(min_ktime, now_ktime)) + 1);
@@ -307,7 +307,7 @@ static int panel_simple_suspend(struct device *dev)
 
gpiod_set_value_cansleep(p->enable_gpio, 0);
regulator_disable(p->supply);
-   p->unprepared_time = ktime_get();
+   p->unprepared_time = ktime_get_boottime();
 
kfree(p->edid);
p->edid = NULL;
@@ -351,7 +351,7 @@ static int panel_simple_resume(struct device *dev)
if (p->desc->delay.prepare)
msleep(p->desc->delay.prepare);
 
-   p->prepared_time = ktime_get();
+   p->prepared_time = ktime_get_boottime();
 
return 0;
 }
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 4/5] drm/bridge/parade-ps8640: Extend autosuspend

[Drew Davenport]

Same change as done for panel-samsung-atna33xc20. Extend the autosuspend
delay to avoid oscillating between power status during boot.

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/bridge/parade-ps8640.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c 
b/drivers/gpu/drm/bridge/parade-ps8640.c
index 6a614e54b383c..f74090a9cc9e8 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -734,13 +734,13 @@ static int ps8640_probe(struct i2c_client *client)
pm_runtime_enable(dev);
/*
 * Powering on ps8640 takes ~300ms. To avoid wasting time on power
-* cycling ps8640 too often, set autosuspend_delay to 1000ms to ensure
+* cycling ps8640 too often, set autosuspend_delay to 2000ms to ensure
 * the bridge wouldn't suspend in between each _aux_transfer_msg() call
 * during EDID read (~20ms in my experiment) and in between the last
 * _aux_transfer_msg() call during EDID read and the _pre_enable() call
 * (~100ms in my experiment).
 */
-   pm_runtime_set_autosuspend_delay(dev, 1000);
+   pm_runtime_set_autosuspend_delay(dev, 2000);
pm_runtime_use_autosuspend(dev);
pm_suspend_ignore_children(dev, true);
ret = devm_add_action_or_reset(dev, ps8640_runtime_disable, dev);
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 1/5] drm/panel-edp: Use ktime_get_boottime for delays

[Drew Davenport]

ktime_get is based on CLOCK_MONOTONIC which stops on suspend. On
suspend, the time that the panel was powerd off is recorded with
ktime_get, and on resume this time is compared to the current ktime_get
time to determine if the driver should wait for the panel to power down
completely before re-enabling it.

Because we're using ktime_get, this delay doesn't account for the time
that the device is suspended, during which the power down delay may have
already elapsed.

Change to use ktime_get_boottime throughout, which uses CLOCK_BOOTTIME
which does not stop when suspended. This ensures that the resume path
will not be delayed if the power off delay has already been met while
the device is suspended.

Signed-off-by: Drew Davenport 

---

 drivers/gpu/drm/panel/panel-edp.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-edp.c 
b/drivers/gpu/drm/panel/panel-edp.c
index 5cb8dc2ebe184..a0a7ab35e08c9 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -351,7 +351,7 @@ static void panel_edp_wait(ktime_t start_ktime, unsigned 
int min_ms)
return;
 
min_ktime = ktime_add(start_ktime, ms_to_ktime(min_ms));
-   now_ktime = ktime_get();
+   now_ktime = ktime_get_boottime();
 
if (ktime_before(now_ktime, min_ktime))
msleep(ktime_to_ms(ktime_sub(min_ktime, now_ktime)) + 1);
@@ -378,7 +378,7 @@ static int panel_edp_suspend(struct device *dev)
 
gpiod_set_value_cansleep(p->enable_gpio, 0);
regulator_disable(p->supply);
-   p->unprepared_time = ktime_get();
+   p->unprepared_time = ktime_get_boottime();
 
return 0;
 }
@@ -464,14 +464,14 @@ static int panel_edp_prepare_once(struct panel_edp *p)
}
}
 
-   p->prepared_time = ktime_get();
+   p->prepared_time = ktime_get_boottime();
 
return 0;
 
 error:
gpiod_set_value_cansleep(p->enable_gpio, 0);
regulator_disable(p->supply);
-   p->unprepared_time = ktime_get();
+   p->unprepared_time = ktime_get_boottime();
 
return err;
 }
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH] drm/panel-edp: Use ktime_get_boottime for delays

[Drew Davenport]

ktime_get is based on CLOCK_MONOTONIC which stops on suspend. On
suspend, the time that the panel was powerd off is recorded with
ktime_get, and on resume this time is compared to the current ktime_get
time to determine if the driver should wait for the panel to power down
completely before re-enabling it.

Because we're using ktime_get, this delay doesn't account for the time
that the device is suspended, during which the power down delay may have
already elapsed.

Change to use ktime_get_boottime throughout, which uses CLOCK_BOOTTIME
which does not stop when suspended. This ensures that the resume path
will not be delayed if the power off delay has already been met while
the device is suspended.

Signed-off-by: Drew Davenport 

---

 drivers/gpu/drm/panel/panel-edp.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-edp.c 
b/drivers/gpu/drm/panel/panel-edp.c
index 5cb8dc2ebe184..a0a7ab35e08c9 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -351,7 +351,7 @@ static void panel_edp_wait(ktime_t start_ktime, unsigned 
int min_ms)
return;
 
min_ktime = ktime_add(start_ktime, ms_to_ktime(min_ms));
-   now_ktime = ktime_get();
+   now_ktime = ktime_get_boottime();
 
if (ktime_before(now_ktime, min_ktime))
msleep(ktime_to_ms(ktime_sub(min_ktime, now_ktime)) + 1);
@@ -378,7 +378,7 @@ static int panel_edp_suspend(struct device *dev)
 
gpiod_set_value_cansleep(p->enable_gpio, 0);
regulator_disable(p->supply);
-   p->unprepared_time = ktime_get();
+   p->unprepared_time = ktime_get_boottime();
 
return 0;
 }
@@ -464,14 +464,14 @@ static int panel_edp_prepare_once(struct panel_edp *p)
}
}
 
-   p->prepared_time = ktime_get();
+   p->prepared_time = ktime_get_boottime();
 
return 0;
 
 error:
gpiod_set_value_cansleep(p->enable_gpio, 0);
regulator_disable(p->supply);
-   p->unprepared_time = ktime_get();
+   p->unprepared_time = ktime_get_boottime();
 
return err;
 }
-- 
2.38.1.493.g58b659f92b-goog

Re: [v7 1/5] drm/edid: seek for available CEA and DisplayID block from specific EDID block index

[Drew Davenport]

On Tue, Mar 15, 2022 at 03:21:05PM +, Lee, Shawn C wrote:
> On Tuesday, March 15, 2022 8:33 PM, Nikula, Jani  
> wrote:
> >On Mon, 14 Mar 2022, Drew Davenport  wrote:
> >> On Mon, Mar 14, 2022 at 10:40:47AM +0200, Jani Nikula wrote:
> >>> On Sun, 13 Mar 2022, Lee Shawn C  wrote:
> >>> > drm_find_cea_extension() always look for a top level CEA block. 
> >>> > Pass ext_index from caller then this function to search next 
> >>> > available CEA ext block from a specific EDID block pointer.
> >>> >
> >>> > v2: save proper extension block index if CTA data information
> >>> > was found in DispalyID block.
> >>> > v3: using different parameters to store CEA and DisplayID block index.
> >>> > configure DisplayID extansion block index before search available
> >>> > DisplayID block.
> >>> >
> >>> > Cc: Jani Nikula 
> >>> > Cc: Ville Syrjala 
> >>> > Cc: Ankit Nautiyal 
> >>> > Cc: Drew Davenport 
> >>> > Cc: intel-gfx 
> >>> > Signed-off-by: Lee Shawn C 
> >>> > ---
> >>> >  drivers/gpu/drm/drm_displayid.c | 10 +--
> >>> >  drivers/gpu/drm/drm_edid.c  | 53 ++---
> >>> >  include/drm/drm_displayid.h |  4 +--
> >>> >  3 files changed, 39 insertions(+), 28 deletions(-)
> >>> >
> >>> > diff --git a/drivers/gpu/drm/drm_displayid.c 
> >>> > b/drivers/gpu/drm/drm_displayid.c index 32da557b960f..31c3e6d7d549 
> >>> > 100644
> >>> > --- a/drivers/gpu/drm/drm_displayid.c
> >>> > +++ b/drivers/gpu/drm/drm_displayid.c
> >>> > @@ -59,11 +59,14 @@ static const u8 
> >>> > *drm_find_displayid_extension(const struct edid *edid,  }
> >>> >  
> >>> >  void displayid_iter_edid_begin(const struct edid *edid,
> >>> > -  struct displayid_iter *iter)
> >>> > +  struct displayid_iter *iter, int 
> >>> > *ext_index)
> >>> 
> >>> Please don't do this. This just ruins the clean approach displayid 
> >>> iterator added.
> >>> 
> >>> Instead of making the displayid iterator ugly, and leaking its 
> >>> abstractions, I'll repeat what I said should be done in reply to the 
> >>> very first version of this patch series [1]:
> >>> 
> >>> "I think we're going to need abstracted EDID iteration similar to 
> >>> what I've done for DisplayID iteration. We can't have all places 
> >>> reimplementing the iteration like we have now."
> >>> 
> >>> This isn't a problem that should be solved by having all the callers 
> >>> hold a bunch of local variables and pass them around to all the 
> >>> functions. Nobody's going to be able to keep track of this anymore. 
> >>> And this series, as it is, makes it harder to fix this properly later on.
> >>
> >> I missed your original review comment, so apologies for repeating what 
> >> you said there already.
> >>
> >> I'd agree that passing a starting index to the displayid_iter_* 
> >> functions is probably not the right direction here. More thoughts below.
> >>
> >>> 
> >>> 
> >>> BR,
> >>> Jani.
> >>> 
> >>> 
> >>> [1] https://lore.kernel.org/r/87czjf8dik@intel.com
> >>> 
> >>> 
> >>> 
> >>> >  {
> >>> > memset(iter, 0, sizeof(*iter));
> >>> >  
> >>> > iter->edid = edid;
> >>> > +
> >>> > +   if (ext_index)
> >>> > +   iter->ext_index = *ext_index;
> >>> >  }
> >>> >  
> >>> >  static const struct displayid_block * @@ -126,7 +129,10 @@ 
> >>> > __displayid_iter_next(struct displayid_iter *iter)
> >>> > }
> >>> >  }
> >>> >  
> >>> > -void displayid_iter_end(struct displayid_iter *iter)
> >>> > +void displayid_iter_end(struct displayid_iter *iter, int 
> >>> > +*ext_index)
> >>> >  {
> >>> > +   if (ext_index)
> >>> > +   *ext_index = iter->ext_index;
> >>> > +
> >>> > memset(iter, 

Re: [v7 1/5] drm/edid: seek for available CEA and DisplayID block from specific EDID block index

[Drew Davenport]

On Mon, Mar 14, 2022 at 10:40:47AM +0200, Jani Nikula wrote:
> On Sun, 13 Mar 2022, Lee Shawn C  wrote:
> > drm_find_cea_extension() always look for a top level CEA block. Pass
> > ext_index from caller then this function to search next available
> > CEA ext block from a specific EDID block pointer.
> >
> > v2: save proper extension block index if CTA data information
> > was found in DispalyID block.
> > v3: using different parameters to store CEA and DisplayID block index.
> > configure DisplayID extansion block index before search available
> > DisplayID block.
> >
> > Cc: Jani Nikula 
> > Cc: Ville Syrjala 
> > Cc: Ankit Nautiyal 
> > Cc: Drew Davenport 
> > Cc: intel-gfx 
> > Signed-off-by: Lee Shawn C 
> > ---
> >  drivers/gpu/drm/drm_displayid.c | 10 +--
> >  drivers/gpu/drm/drm_edid.c  | 53 ++---
> >  include/drm/drm_displayid.h |  4 +--
> >  3 files changed, 39 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_displayid.c 
> > b/drivers/gpu/drm/drm_displayid.c
> > index 32da557b960f..31c3e6d7d549 100644
> > --- a/drivers/gpu/drm/drm_displayid.c
> > +++ b/drivers/gpu/drm/drm_displayid.c
> > @@ -59,11 +59,14 @@ static const u8 *drm_find_displayid_extension(const 
> > struct edid *edid,
> >  }
> >  
> >  void displayid_iter_edid_begin(const struct edid *edid,
> > -  struct displayid_iter *iter)
> > +  struct displayid_iter *iter, int *ext_index)
> 
> Please don't do this. This just ruins the clean approach displayid
> iterator added.
> 
> Instead of making the displayid iterator ugly, and leaking its
> abstractions, I'll repeat what I said should be done in reply to the
> very first version of this patch series [1]:
> 
> "I think we're going to need abstracted EDID iteration similar to what
> I've done for DisplayID iteration. We can't have all places
> reimplementing the iteration like we have now."
> 
> This isn't a problem that should be solved by having all the callers
> hold a bunch of local variables and pass them around to all the
> functions. Nobody's going to be able to keep track of this anymore. And
> this series, as it is, makes it harder to fix this properly later on.

I missed your original review comment, so apologies for repeating what you
said there already.

I'd agree that passing a starting index to the displayid_iter_*
functions is probably not the right direction here. More thoughts below.

> 
> 
> BR,
> Jani.
> 
> 
> [1] https://lore.kernel.org/r/87czjf8dik@intel.com
> 
> 
> 
> >  {
> > memset(iter, 0, sizeof(*iter));
> >  
> > iter->edid = edid;
> > +
> > +   if (ext_index)
> > +   iter->ext_index = *ext_index;
> >  }
> >  
> >  static const struct displayid_block *
> > @@ -126,7 +129,10 @@ __displayid_iter_next(struct displayid_iter *iter)
> > }
> >  }
> >  
> > -void displayid_iter_end(struct displayid_iter *iter)
> > +void displayid_iter_end(struct displayid_iter *iter, int *ext_index)
> >  {
> > +   if (ext_index)
> > +   *ext_index = iter->ext_index;
> > +
> > memset(iter, 0, sizeof(*iter));
> >  }
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 561f53831e29..78c415aa6889 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -3353,28 +3353,27 @@ const u8 *drm_find_edid_extension(const struct edid 
> > *edid,
> > return edid_ext;
> >  }
> >  
> > -static const u8 *drm_find_cea_extension(const struct edid *edid)
> > +static const u8 *drm_find_cea_extension(const struct edid *edid,
> > +   int *cea_ext_index, int 
> > *displayid_ext_index)

As discussed above, passing both indices into this function may not be
the best approach here. But I think we need to keep track of some kind
of state in order to know which was the last CEA block that was
returned, and thus this function can return the next one after that,
whether it's in the CEA extension block or DisplayID block.

What do you think of using the pointer returned from this function as
that state? The caller could pass in a u8* that was returned from a
previous call. This function would iterate through the extension blocks
and skip the CEA blocks it finds until it finds the passed in pointer,
and then return the next one after (or NULL).

An alternative might be to create a linked list of ptrs to the edid
extension blocks, and allow a caller to iterate over

Re: [v6 1/5] drm/edid: seek for available CEA block from specific EDID block index

[Drew Davenport]

On Fri, Mar 11, 2022 at 09:22:14AM +0800, Lee Shawn C wrote:
> drm_find_cea_extension() always look for a top level CEA block. Pass
> ext_index from caller then this function to search next available
> CEA ext block from a specific EDID block pointer.
> 
> v2: save proper extension block index if CTA data information
> was found in DispalyID block.
> 
> Cc: Jani Nikula 
> Cc: Ville Syrjala 
> Cc: Ankit Nautiyal 
> Cc: intel-gfx 
> Signed-off-by: Lee Shawn C 
> ---
>  drivers/gpu/drm/drm_edid.c | 43 +++---
>  1 file changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 561f53831e29..e267d31d5c87 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3353,16 +3353,14 @@ const u8 *drm_find_edid_extension(const struct edid 
> *edid,
>   return edid_ext;
>  }
>  
> -static const u8 *drm_find_cea_extension(const struct edid *edid)
> +static const u8 *drm_find_cea_extension(const struct edid *edid, int 
> *ext_index)
>  {
>   const struct displayid_block *block;
>   struct displayid_iter iter;
>   const u8 *cea;
> - int ext_index = 0;
>  
> - /* Look for a top level CEA extension block */
> - /* FIXME: make callers iterate through multiple CEA ext blocks? */
> - cea = drm_find_edid_extension(edid, CEA_EXT, _index);
> + /* Look for a CEA extension block from ext_index */
> + cea = drm_find_edid_extension(edid, CEA_EXT, ext_index);
>   if (cea)
>   return cea;
>  
> @@ -3370,6 +3368,7 @@ static const u8 *drm_find_cea_extension(const struct 
> edid *edid)
>   displayid_iter_edid_begin(edid, );
>   displayid_iter_for_each(block, ) {
>   if (block->tag == DATA_BLOCK_CTA) {
> + *ext_index = iter.ext_index;
This could still end up in an infinite loop in patch 2 in the case that
there is no CEA_EXT block in the edid, but there is a CEA block in the
DisplayId block.

Repeating my review comment from elsewhere, consider the case:
- If there are no cea extension blocks in the EDID,
  drm_find_edid_extension returns NULL
- drm_find_cea_extension will then return the first DisplayId block
  with tag DATA_BLOCK_CTA

If the version of the cea data from DisplayId block is less than 3, the
loop will restart and call drm_find_cea_extension the same way, returning
the same DisplayID block every time.

Setting *ext_index inside the display_iter_for_each block doesn't change this,
since we're not checking it.

But I don't think we want to use the same *ext_index both to pass into
drm_find_edid_extension and for tracking the next DisplayId block to check.
This might end up in similar infinite loops or skipping DisplayId blocks.

Maybe you'll need to pass in two indexes to drm_find_cea_extension, one which
is passed to drm_find_edid_extension, and the other to keep track of the next
DisplayId block to check.
>   cea = (const u8 *)block;
>   break;
>   }
> @@ -3643,10 +3642,10 @@ add_alternate_cea_modes(struct drm_connector 
> *connector, struct edid *edid)
>   struct drm_device *dev = connector->dev;
>   struct drm_display_mode *mode, *tmp;
>   LIST_HEAD(list);
> - int modes = 0;
> + int modes = 0, ext_index = 0;
>  
>   /* Don't add CEA modes if the CEA extension block is missing */
> - if (!drm_find_cea_extension(edid))
> + if (!drm_find_cea_extension(edid, _index))
>   return 0;
>  
>   /*
> @@ -4321,11 +4320,11 @@ static void drm_parse_y420cmdb_bitmap(struct 
> drm_connector *connector,
>  static int
>  add_cea_modes(struct drm_connector *connector, struct edid *edid)
>  {
> - const u8 *cea = drm_find_cea_extension(edid);
> - const u8 *db, *hdmi = NULL, *video = NULL;
> + const u8 *cea, *db, *hdmi = NULL, *video = NULL;
>   u8 dbl, hdmi_len, video_len = 0;
> - int modes = 0;
> + int modes = 0, ext_index = 0;
>  
> + cea = drm_find_cea_extension(edid, _index);
>   if (cea && cea_revision(cea) >= 3) {
>   int i, start, end;
>  
> @@ -4562,7 +4561,7 @@ static void drm_edid_to_eld(struct drm_connector 
> *connector, struct edid *edid)
>   uint8_t *eld = connector->eld;
>   const u8 *cea;
>   const u8 *db;
> - int total_sad_count = 0;
> + int total_sad_count = 0, ext_index = 0;
>   int mnl;
>   int dbl;
>  
> @@ -4571,7 +4570,7 @@ static void drm_edid_to_eld(struct drm_connector 
> *connector, struct edid *edid)
>   if (!edid)
>   return;
>  
> - cea = drm_find_cea_extension(edid);
> + cea = drm_find_cea_extension(edid, _index);
>   if (!cea) {
>   DRM_DEBUG_KMS("ELD: no CEA Extension found\n");
>   return;
> @@ -4655,11 +4654,11 @@ static void drm_edid_to_eld(struct drm_connector 
> *connector, struct edid *edid)
>   */
>  int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads)
>  {

[PATCH] drm/edid: Update comments for drm_find_edid_extension

[Drew Davenport]

In (40d9b043a89e drm/connector: store tile information from displayid (v3))
this function was changed to find EDID extensions by id, but the comments
still are specific to the CEA extension.

Signed-off-by: Drew Davenport 

---

 drivers/gpu/drm/drm_edid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 561f53831e29..1afe73fbf3e0 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3326,7 +3326,7 @@ add_detailed_modes(struct drm_connector *connector, 
struct edid *edid,
 #define EDID_CEA_VCDB_QS   (1 << 6)
 
 /*
- * Search EDID for CEA extension block.
+ * Search EDID for the extension block with id @ext_id.
  */
 const u8 *drm_find_edid_extension(const struct edid *edid,
  int ext_id, int *ext_index)
@@ -3338,7 +3338,7 @@ const u8 *drm_find_edid_extension(const struct edid *edid,
if (edid == NULL || edid->extensions == 0)
return NULL;
 
-   /* Find CEA extension */
+   /* Find extension that matches @ext_id */
for (i = *ext_index; i < edid->extensions; i++) {
edid_ext = (const u8 *)edid + EDID_LENGTH * (i + 1);
if (edid_ext[0] == ext_id)
-- 
2.35.1.723.g4982287a31-goog

[PATCH] drm: bridge: it66121: Remove redundant check

[Drew Davenport]

ctx->next_bridge is checked for NULL twice in a row. The second
conditional is redundant, so remove it.

Signed-off-by: Drew Davenport 
---
 drivers/gpu/drm/bridge/ite-it66121.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c 
b/drivers/gpu/drm/bridge/ite-it66121.c
index 06b59b422c69..69288cf894b9 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -936,9 +936,6 @@ static int it66121_probe(struct i2c_client *client,
return -EPROBE_DEFER;
}
 
-   if (!ctx->next_bridge)
-   return -EPROBE_DEFER;
-
i2c_set_clientdata(client, ctx);
mutex_init(>lock);
 
-- 
2.34.1.703.g22d0c6ccf7-goog

[PATCH 2/4] drm/msm/dpu: Refactor rm iterator

[Drew Davenport]

Make iterator implementation private, and add function to
query resources assigned to an encoder.

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 59 ---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h   | 10 
 .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h   | 10 
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 31 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h| 47 ++-
 5 files changed, 76 insertions(+), 81 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index f8ac3bf60fd60..6cadeff456f09 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -957,11 +957,11 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder 
*drm_enc,
struct drm_connector *conn = NULL, *conn_iter;
struct drm_crtc *drm_crtc;
struct dpu_crtc_state *cstate;
-   struct dpu_rm_hw_iter hw_iter;
struct msm_display_topology topology;
-   struct dpu_hw_ctl *hw_ctl[MAX_CHANNELS_PER_ENC] = { NULL };
-   struct dpu_hw_mixer *hw_lm[MAX_CHANNELS_PER_ENC] = { NULL };
-   int num_lm = 0, num_ctl = 0;
+   struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC];
+   struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC];
+   struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
+   int num_lm, num_ctl, num_pp;
int i, j, ret;
 
if (!drm_enc) {
@@ -1005,42 +1005,31 @@ static void dpu_encoder_virt_mode_set(struct 
drm_encoder *drm_enc,
return;
}
 
-   dpu_rm_init_hw_iter(_iter, drm_enc->base.id, DPU_HW_BLK_PINGPONG);
-   for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
-   dpu_enc->hw_pp[i] = NULL;
-   if (!dpu_rm_get_hw(_kms->rm, _iter))
-   break;
-   dpu_enc->hw_pp[i] = (struct dpu_hw_pingpong *) hw_iter.hw;
-   }
-
-   dpu_rm_init_hw_iter(_iter, drm_enc->base.id, DPU_HW_BLK_CTL);
-   for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
-   if (!dpu_rm_get_hw(_kms->rm, _iter))
-   break;
-   hw_ctl[i] = (struct dpu_hw_ctl *)hw_iter.hw;
-   num_ctl++;
-   }
+   num_pp = dpu_rm_get_assigned_resources(_kms->rm, drm_enc->base.id,
+   DPU_HW_BLK_PINGPONG, hw_pp, ARRAY_SIZE(hw_pp));
+   num_ctl = dpu_rm_get_assigned_resources(_kms->rm, drm_enc->base.id,
+   DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl));
+   num_lm = dpu_rm_get_assigned_resources(_kms->rm, drm_enc->base.id,
+   DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm));
 
-   dpu_rm_init_hw_iter(_iter, drm_enc->base.id, DPU_HW_BLK_LM);
-   for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
-   if (!dpu_rm_get_hw(_kms->rm, _iter))
-   break;
-   hw_lm[i] = (struct dpu_hw_mixer *)hw_iter.hw;
-   num_lm++;
-   }
+   for (i = 0; i < MAX_CHANNELS_PER_ENC; i++)
+   dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i])
+   : NULL;
 
cstate = to_dpu_crtc_state(drm_crtc->state);
 
for (i = 0; i < num_lm; i++) {
int ctl_idx = (i < num_ctl) ? i : (num_ctl-1);
 
-   cstate->mixers[i].hw_lm = hw_lm[i];
-   cstate->mixers[i].lm_ctl = hw_ctl[ctl_idx];
+   cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]);
+   cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]);
}
 
cstate->num_mixers = num_lm;
 
for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+   int num_blk;
+   struct dpu_hw_blk *hw_blk[MAX_CHANNELS_PER_ENC];
struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
 
if (!dpu_enc->hw_pp[i]) {
@@ -1056,17 +1045,15 @@ static void dpu_encoder_virt_mode_set(struct 
drm_encoder *drm_enc,
}
 
phys->hw_pp = dpu_enc->hw_pp[i];
-   phys->hw_ctl = hw_ctl[i];
+   phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
 
-   dpu_rm_init_hw_iter(_iter, drm_enc->base.id,
-   DPU_HW_BLK_INTF);
-   for (j = 0; j < MAX_CHANNELS_PER_ENC; j++) {
+   num_blk = dpu_rm_get_assigned_resources(_kms->rm,
+   drm_enc->base.id, DPU_HW_BLK_INTF, hw_blk,
+   ARRAY_SIZE(hw_blk));
+   for (j = 0; j < num_blk; j++) {
struct dpu_hw_intf *hw_intf;
 
-   if (!dpu_rm_get_hw(_kms->rm, _iter))
-   break;
-
-   hw_intf = (struct dpu_hw_intf *)hw_iter.hw;
+   hw_intf = to_dpu_hw_intf(hw_blk[i]);
if (h

[PATCH 3/4] drm/msm/dpu: Refactor resource manager

[Drew Davenport]

Track hardware resource objects in arrays rather than
a list and remove the resource manager's iterator idiom. Separate
the mapping of hardware resources to an encoder ID into a different
array.

Use an implicit mapping between the hardware blocks' ids, which
are 1-based, and array indices in these arrays to replace iteration
with index lookups in several places.

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 553 +++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h |  22 +-
 2 files changed, 255 insertions(+), 320 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 779df26dc81ae..f1483b00b7423 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -12,8 +12,12 @@
 #include "dpu_encoder.h"
 #include "dpu_trace.h"
 
-#define RESERVED_BY_OTHER(h, r)  \
-   ((h)->enc_id && (h)->enc_id != r)
+
+static inline bool reserved_by_other(uint32_t *res_map, int idx,
+uint32_t enc_id)
+{
+   return res_map[idx] && res_map[idx] != enc_id;
+}
 
 /**
  * struct dpu_rm_requirements - Reservation requirements parameter bundle
@@ -25,126 +29,40 @@ struct dpu_rm_requirements {
struct dpu_encoder_hw_resources hw_res;
 };
 
-
-/**
- * struct dpu_rm_hw_blk - hardware block tracking list member
- * @list:  List head for list of all hardware blocks tracking items
- * @id:Hardware ID number, within it's own space, ie. LM_X
- * @enc_id:Encoder id to which this blk is binded
- * @hw:Pointer to the hardware register access object for this 
block
- */
-struct dpu_rm_hw_blk {
-   struct list_head list;
-   uint32_t id;
-   uint32_t enc_id;
-   struct dpu_hw_blk *hw;
-};
-
-/**
- * struct dpu_rm_hw_iter - iterator for use with dpu_rm
- * @hw: dpu_hw object requested, or NULL on failure
- * @blk: dpu_rm internal block representation. Clients ignore. Used as 
iterator.
- * @enc_id: DRM ID of Encoder client wishes to search for, or 0 for Any Encoder
- * @type: Hardware Block Type client wishes to search for.
- */
-struct dpu_rm_hw_iter {
-   void *hw;
-   struct dpu_rm_hw_blk *blk;
-   uint32_t enc_id;
-   enum dpu_hw_blk_type type;
-};
-
-static void dpu_rm_init_hw_iter(
-   struct dpu_rm_hw_iter *iter,
-   uint32_t enc_id,
-   enum dpu_hw_blk_type type)
-{
-   memset(iter, 0, sizeof(*iter));
-   iter->enc_id = enc_id;
-   iter->type = type;
-}
-
-static bool _dpu_rm_get_hw_locked(struct dpu_rm *rm, struct dpu_rm_hw_iter *i)
+int dpu_rm_destroy(struct dpu_rm *rm)
 {
-   struct list_head *blk_list;
+   int i;
 
-   if (!rm || !i || i->type >= DPU_HW_BLK_MAX) {
-   DPU_ERROR("invalid rm\n");
-   return false;
-   }
+   for (i = 0; i < ARRAY_SIZE(rm->pingpong_blks); i++) {
+   struct dpu_hw_pingpong *hw;
 
-   i->hw = NULL;
-   blk_list = >hw_blks[i->type];
-
-   if (i->blk && (>blk->list == blk_list)) {
-   DPU_DEBUG("attempt resume iteration past last\n");
-   return false;
+   if (rm->pingpong_blks[i]) {
+   hw = to_dpu_hw_pingpong(rm->pingpong_blks[i]);
+   dpu_hw_pingpong_destroy(hw);
+   }
}
+   for (i = 0; i < ARRAY_SIZE(rm->mixer_blks); i++) {
+   struct dpu_hw_mixer *hw;
 
-   i->blk = list_prepare_entry(i->blk, blk_list, list);
-
-   list_for_each_entry_continue(i->blk, blk_list, list) {
-   if (i->enc_id == i->blk->enc_id) {
-   i->hw = i->blk->hw;
-   DPU_DEBUG("found type %d id %d for enc %d\n",
-   i->type, i->blk->id, i->enc_id);
-   return true;
+   if (rm->mixer_blks[i]) {
+   hw = to_dpu_hw_mixer(rm->mixer_blks[i]);
+   dpu_hw_lm_destroy(hw);
}
}
+   for (i = 0; i < ARRAY_SIZE(rm->ctl_blks); i++) {
+   struct dpu_hw_ctl *hw;
 
-   DPU_DEBUG("no match, type %d for enc %d\n", i->type, i->enc_id);
-
-   return false;
-}
-
-static bool dpu_rm_get_hw(struct dpu_rm *rm, struct dpu_rm_hw_iter *i)
-{
-   bool ret;
-
-   mutex_lock(>rm_lock);
-   ret = _dpu_rm_get_hw_locked(rm, i);
-   mutex_unlock(>rm_lock);
-
-   return ret;
-}
-
-static void _dpu_rm_hw_destroy(enum dpu_hw_blk_type type, void *hw)
-{
-   switch (type) {
-   case DPU_HW_BLK_LM:
-   dpu_hw_lm_destroy(hw);
-   break;
-   case DPU_HW_BLK_CTL:
-   dpu_hw_ctl_destroy(hw);
-   

[PATCH 4/4] drm/msm/dpu: Track resources in global state

[Drew Davenport]

Move mapping of resources to encoder ids from the resource manager to a
new dpu_global_state struct. Store this struct in global atomic state.

Before this patch, atomic test would be performed by modifying global
state (resource manager), and backing out any changes if the test fails.
By using drm atomic global state, this is not necessary as any changes
to the global state will be discarded if the test fails.

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  65 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c |  84 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |  26 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c  | 121 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h  |  22 ++--
 5 files changed, 207 insertions(+), 111 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 6cadeff456f09..5afde2e7fef08 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -164,7 +164,6 @@ enum dpu_enc_rc_states {
  * clks and resources after IDLE_TIMEOUT time.
  * @vsync_event_work:  worker to handle vsync event for autorefresh
  * @topology:   topology of the display
- * @mode_set_complete:  flag to indicate modeset completion
  * @idle_timeout:  idle timeout duration in milliseconds
  */
 struct dpu_encoder_virt {
@@ -202,7 +201,6 @@ struct dpu_encoder_virt {
struct delayed_work delayed_off_work;
struct kthread_work vsync_event_work;
struct msm_display_topology topology;
-   bool mode_set_complete;
 
u32 idle_timeout;
 };
@@ -563,6 +561,7 @@ static int dpu_encoder_virt_atomic_check(
const struct drm_display_mode *mode;
struct drm_display_mode *adj_mode;
struct msm_display_topology topology;
+   struct dpu_global_state *global_state;
int i = 0;
int ret = 0;
 
@@ -579,6 +578,7 @@ static int dpu_encoder_virt_atomic_check(
dpu_kms = to_dpu_kms(priv->kms);
mode = _state->mode;
adj_mode = _state->adjusted_mode;
+   global_state = dpu_kms_get_existing_global_state(dpu_kms);
trace_dpu_enc_atomic_check(DRMID(drm_enc));
 
/*
@@ -610,17 +610,15 @@ static int dpu_encoder_virt_atomic_check(
 
topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
 
-   /* Reserve dynamic resources now. Indicating AtomicTest phase */
+   /* Reserve dynamic resources now. */
if (!ret) {
/*
 * Avoid reserving resources when mode set is pending. Topology
 * info may not be available to complete reservation.
 */
-   if (drm_atomic_crtc_needs_modeset(crtc_state)
-   && dpu_enc->mode_set_complete) {
-   ret = dpu_rm_reserve(_kms->rm, drm_enc, crtc_state,
-topology, true);
-   dpu_enc->mode_set_complete = false;
+   if (drm_atomic_crtc_needs_modeset(crtc_state)) {
+   ret = dpu_rm_reserve(_kms->rm, global_state,
+   drm_enc, crtc_state, topology);
}
}
 
@@ -957,12 +955,13 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder 
*drm_enc,
struct drm_connector *conn = NULL, *conn_iter;
struct drm_crtc *drm_crtc;
struct dpu_crtc_state *cstate;
+   struct dpu_global_state *global_state;
struct msm_display_topology topology;
struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC];
struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC];
struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
int num_lm, num_ctl, num_pp;
-   int i, j, ret;
+   int i, j;
 
if (!drm_enc) {
DPU_ERROR("invalid encoder\n");
@@ -976,6 +975,12 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder 
*drm_enc,
dpu_kms = to_dpu_kms(priv->kms);
connector_list = _kms->dev->mode_config.connector_list;
 
+   global_state = dpu_kms_get_existing_global_state(dpu_kms);
+   if (IS_ERR_OR_NULL(global_state)) {
+   DPU_ERROR("Failed to get global state");
+   return;
+   }
+
trace_dpu_enc_mode_set(DRMID(drm_enc));
 
list_for_each_entry(conn_iter, connector_list, head)
@@ -996,21 +1001,14 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder 
*drm_enc,
 
topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
 
-   /* Reserve dynamic resources now. Indicating non-AtomicTest phase */
-   ret = dpu_rm_reserve(_kms->rm, drm_enc, drm_crtc->state,
-topology, false);
-   if (ret) {
-   DPU_ERROR_ENC(dpu_enc,
-  

[PATCH 1/4] drm/msm/dpu: Remove unused function arguments

[Drew Davenport]

Several functions arguments in the resource manager are unused, so
remove them.

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 37 ++
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 23f5b1433b357..dea1dba441fe7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -144,8 +144,7 @@ static int _dpu_rm_hw_blk_create(
const struct dpu_mdss_cfg *cat,
void __iomem *mmio,
enum dpu_hw_blk_type type,
-   uint32_t id,
-   const void *hw_catalog_info)
+   uint32_t id)
 {
struct dpu_rm_hw_blk *blk;
void *hw;
@@ -223,7 +222,7 @@ int dpu_rm_init(struct dpu_rm *rm,
}
 
rc = _dpu_rm_hw_blk_create(rm, cat, mmio, DPU_HW_BLK_LM,
-   cat->mixer[i].id, >mixer[i]);
+   cat->mixer[i].id);
if (rc) {
DPU_ERROR("failed: lm hw not available\n");
goto fail;
@@ -244,7 +243,7 @@ int dpu_rm_init(struct dpu_rm *rm,
 
for (i = 0; i < cat->pingpong_count; i++) {
rc = _dpu_rm_hw_blk_create(rm, cat, mmio, DPU_HW_BLK_PINGPONG,
-   cat->pingpong[i].id, >pingpong[i]);
+   cat->pingpong[i].id);
if (rc) {
DPU_ERROR("failed: pp hw not available\n");
goto fail;
@@ -258,7 +257,7 @@ int dpu_rm_init(struct dpu_rm *rm,
}
 
rc = _dpu_rm_hw_blk_create(rm, cat, mmio, DPU_HW_BLK_INTF,
-   cat->intf[i].id, >intf[i]);
+   cat->intf[i].id);
if (rc) {
DPU_ERROR("failed: intf hw not available\n");
goto fail;
@@ -267,7 +266,7 @@ int dpu_rm_init(struct dpu_rm *rm,
 
for (i = 0; i < cat->ctl_count; i++) {
rc = _dpu_rm_hw_blk_create(rm, cat, mmio, DPU_HW_BLK_CTL,
-   cat->ctl[i].id, >ctl[i]);
+   cat->ctl[i].id);
if (rc) {
DPU_ERROR("failed: ctl hw not available\n");
goto fail;
@@ -293,7 +292,6 @@ static bool _dpu_rm_needs_split_display(const struct 
msm_display_topology *top)
  * pingpong
  * @rm: dpu resource manager handle
  * @enc_id: encoder id requesting for allocation
- * @reqs: proposed use case requirements
  * @lm: proposed layer mixer, function checks if lm, and all other hardwired
  *  blocks connected to the lm (pp) is available and appropriate
  * @pp: output parameter, pingpong block attached to the layer mixer.
@@ -305,7 +303,6 @@ static bool _dpu_rm_needs_split_display(const struct 
msm_display_topology *top)
 static bool _dpu_rm_check_lm_and_get_connected_blks(
struct dpu_rm *rm,
uint32_t enc_id,
-   struct dpu_rm_requirements *reqs,
struct dpu_rm_hw_blk *lm,
struct dpu_rm_hw_blk **pp,
struct dpu_rm_hw_blk *primary_lm)
@@ -384,7 +381,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm, uint32_t 
enc_id,
lm[lm_count] = iter_i.blk;
 
if (!_dpu_rm_check_lm_and_get_connected_blks(
-   rm, enc_id, reqs, lm[lm_count],
+   rm, enc_id, lm[lm_count],
[lm_count], NULL))
continue;
 
@@ -399,7 +396,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm, uint32_t 
enc_id,
continue;
 
if (!_dpu_rm_check_lm_and_get_connected_blks(
-   rm, enc_id, reqs, iter_j.blk,
+   rm, enc_id, iter_j.blk,
[lm_count], iter_i.blk))
continue;
 
@@ -480,20 +477,19 @@ static int _dpu_rm_reserve_ctls(
 static int _dpu_rm_reserve_intf(
struct dpu_rm *rm,
uint32_t enc_id,
-   uint32_t id,
-   enum dpu_hw_blk_type type)
+   uint32_t id)
 {
struct dpu_rm_hw_iter iter;
int ret = 0;
 
/* Find the block entry in the rm, and note the reservation */
-   dpu_rm_init_hw_iter(, 0, type);
+   dpu_rm_init_hw_iter(, 0, DPU_HW_BLK_INTF);
while (_dpu_rm_get_hw_locked(rm, )) {
if (iter.blk->id != id)
continue;
 
if (RESERVED_BY_OTHER(iter.blk, enc_id)) {
-   DPU_ERROR("type %d id %d alrea

[PATCH 2/6] drm/msm/dpu: Remove unnecessary NULL checks

[Drew Davenport]

dpu_crtc_mixer.lm_ctl will never be NULL, so don't bother checking

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index f197dce54576..b9ed8285ab39 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -197,8 +197,8 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
DPU_DEBUG("%s\n", dpu_crtc->name);
 
for (i = 0; i < cstate->num_mixers; i++) {
-   if (!mixer[i].hw_lm || !mixer[i].lm_ctl) {
-   DPU_ERROR("invalid lm or ctl assigned to mixer\n");
+   if (!mixer[i].hw_lm) {
+   DPU_ERROR("invalid lm assigned to mixer\n");
return;
}
mixer[i].mixer_op_mode = 0;
@@ -1115,8 +1115,6 @@ static int _dpu_debugfs_status_show(struct seq_file *s, 
void *data)
m = >mixers[i];
if (!m->hw_lm)
seq_printf(s, "\tmixer[%d] has no lm\n", i);
-   else if (!m->lm_ctl)
-   seq_printf(s, "\tmixer[%d] has no ctl\n", i);
else
seq_printf(s, "\tmixer:%d ctl:%d width:%d height:%d\n",
m->hw_lm->idx - LM_0, m->lm_ctl->idx - CTL_0,
-- 
2.21.0

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

[PATCH 6/6] drm/msm/dpu: Remove unnecessary NULL checks

[Drew Davenport]

The dpu_encoder_phys * argument passed to these functions will never be
NULL so don't check.

Signed-off-by: Drew Davenport 
---

 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 69 ---
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 44 +---
 2 files changed, 17 insertions(+), 96 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index cc2ecf327582..39e1e280ba44 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -45,8 +45,7 @@ static bool dpu_encoder_phys_cmd_mode_fixup(
const struct drm_display_mode *mode,
struct drm_display_mode *adj_mode)
 {
-   if (phys_enc)
-   DPU_DEBUG_CMDENC(to_dpu_encoder_phys_cmd(phys_enc), "\n");
+   DPU_DEBUG_CMDENC(to_dpu_encoder_phys_cmd(phys_enc), "\n");
return true;
 }
 
@@ -58,9 +57,6 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
struct dpu_hw_ctl *ctl;
struct dpu_hw_intf_cfg intf_cfg = { 0 };
 
-   if (!phys_enc)
-   return;
-
ctl = phys_enc->hw_ctl;
if (!ctl->ops.setup_intf_cfg)
return;
@@ -79,7 +75,7 @@ static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, 
int irq_idx)
int new_cnt;
u32 event = DPU_ENCODER_FRAME_EVENT_DONE;
 
-   if (!phys_enc || !phys_enc->hw_pp)
+   if (!phys_enc->hw_pp)
return;
 
DPU_ATRACE_BEGIN("pp_done_irq");
@@ -106,7 +102,7 @@ static void dpu_encoder_phys_cmd_pp_rd_ptr_irq(void *arg, 
int irq_idx)
struct dpu_encoder_phys *phys_enc = arg;
struct dpu_encoder_phys_cmd *cmd_enc;
 
-   if (!phys_enc || !phys_enc->hw_pp)
+   if (!phys_enc->hw_pp)
return;
 
DPU_ATRACE_BEGIN("rd_ptr_irq");
@@ -125,9 +121,6 @@ static void dpu_encoder_phys_cmd_ctl_start_irq(void *arg, 
int irq_idx)
 {
struct dpu_encoder_phys *phys_enc = arg;
 
-   if (!phys_enc)
-   return;
-
DPU_ATRACE_BEGIN("ctl_start_irq");
 
atomic_add_unless(_enc->pending_ctlstart_cnt, -1, 0);
@@ -141,9 +134,6 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg, 
int irq_idx)
 {
struct dpu_encoder_phys *phys_enc = arg;
 
-   if (!phys_enc)
-   return;
-
if (phys_enc->parent_ops->handle_underrun_virt)
phys_enc->parent_ops->handle_underrun_virt(phys_enc->parent,
phys_enc);
@@ -179,7 +169,7 @@ static void dpu_encoder_phys_cmd_mode_set(
struct dpu_encoder_phys_cmd *cmd_enc =
to_dpu_encoder_phys_cmd(phys_enc);
 
-   if (!phys_enc || !mode || !adj_mode) {
+   if (!mode || !adj_mode) {
DPU_ERROR("invalid args\n");
return;
}
@@ -198,7 +188,7 @@ static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
u32 frame_event = DPU_ENCODER_FRAME_EVENT_ERROR;
bool do_log = false;
 
-   if (!phys_enc || !phys_enc->hw_pp)
+   if (!phys_enc->hw_pp)
return -EINVAL;
 
cmd_enc->pp_timeout_report_cnt++;
@@ -247,11 +237,6 @@ static int _dpu_encoder_phys_cmd_wait_for_idle(
struct dpu_encoder_wait_info wait_info;
int ret;
 
-   if (!phys_enc) {
-   DPU_ERROR("invalid encoder\n");
-   return -EINVAL;
-   }
-
wait_info.wq = _enc->pending_kickoff_wq;
wait_info.atomic_cnt = _enc->pending_kickoff_cnt;
wait_info.timeout_ms = KICKOFF_TIMEOUT_MS;
@@ -273,7 +258,7 @@ static int dpu_encoder_phys_cmd_control_vblank_irq(
int ret = 0;
int refcount;
 
-   if (!phys_enc || !phys_enc->hw_pp) {
+   if (!phys_enc->hw_pp) {
DPU_ERROR("invalid encoder\n");
return -EINVAL;
}
@@ -314,9 +299,6 @@ static int dpu_encoder_phys_cmd_control_vblank_irq(
 static void dpu_encoder_phys_cmd_irq_control(struct dpu_encoder_phys *phys_enc,
bool enable)
 {
-   if (!phys_enc)
-   return;
-
trace_dpu_enc_phys_cmd_irq_ctrl(DRMID(phys_enc->parent),
phys_enc->hw_pp->idx - PINGPONG_0,
enable, atomic_read(_enc->vblank_refcount));
@@ -351,7 +333,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
u32 vsync_hz;
struct dpu_kms *dpu_kms;
 
-   if (!phys_enc || !phys_enc->hw_pp) {
+   if (!phys_enc->hw_pp) {
DPU_ERROR("invalid encoder\n");
return;
}
@@ -428,8 +410,7 @@ static void _dpu_encoder_phys_cmd_pingpong_config(
struct dpu_encoder_phys_cmd *cmd_enc =
to_dpu_encoder_phys_cmd(phys_enc);
 
-   if (!phys_enc || !phys_enc->hw_pp
-  

[PATCH 4/6] drm/msm/dpu: Remove unnecessary NULL check

[Drew Davenport]

dpu_encoder_virt.phys_encs[0:num_phys_encs-1] will not be NULL so don't
check.

Also fix multiline strings that caused checkpatch warning.

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 162 
 1 file changed, 61 insertions(+), 101 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 45a87757e766..e9f8fe66af7b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -233,7 +233,7 @@ int dpu_encoder_helper_wait_for_irq(struct dpu_encoder_phys 
*phys_enc,
u32 irq_status;
int ret;
 
-   if (!phys_enc || !wait_info || intr_idx >= INTR_IDX_MAX) {
+   if (!wait_info || intr_idx >= INTR_IDX_MAX) {
DPU_ERROR("invalid params\n");
return -EINVAL;
}
@@ -308,7 +308,7 @@ int dpu_encoder_helper_register_irq(struct dpu_encoder_phys 
*phys_enc,
struct dpu_encoder_irq *irq;
int ret = 0;
 
-   if (!phys_enc || intr_idx >= INTR_IDX_MAX) {
+   if (intr_idx >= INTR_IDX_MAX) {
DPU_ERROR("invalid params\n");
return -EINVAL;
}
@@ -363,10 +363,6 @@ int dpu_encoder_helper_unregister_irq(struct 
dpu_encoder_phys *phys_enc,
struct dpu_encoder_irq *irq;
int ret;
 
-   if (!phys_enc) {
-   DPU_ERROR("invalid encoder\n");
-   return -EINVAL;
-   }
irq = _enc->irq[intr_idx];
 
/* silently skip irqs that weren't registered */
@@ -415,7 +411,7 @@ void dpu_encoder_get_hw_resources(struct drm_encoder 
*drm_enc,
for (i = 0; i < dpu_enc->num_phys_encs; i++) {
struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
 
-   if (phys && phys->ops.get_hw_resources)
+   if (phys->ops.get_hw_resources)
phys->ops.get_hw_resources(phys, hw_res);
}
 }
@@ -438,7 +434,7 @@ static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
for (i = 0; i < dpu_enc->num_phys_encs; i++) {
struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
 
-   if (phys && phys->ops.destroy) {
+   if (phys->ops.destroy) {
phys->ops.destroy(phys);
--dpu_enc->num_phys_encs;
dpu_enc->phys_encs[i] = NULL;
@@ -464,7 +460,7 @@ void dpu_encoder_helper_split_config(
struct dpu_hw_mdp *hw_mdptop;
struct msm_display_info *disp_info;
 
-   if (!phys_enc || !phys_enc->hw_mdptop || !phys_enc->parent) {
+   if (!phys_enc->hw_mdptop || !phys_enc->parent) {
DPU_ERROR("invalid arg(s), encoder %d\n", phys_enc != 0);
return;
}
@@ -528,16 +524,11 @@ static struct msm_display_topology 
dpu_encoder_get_topology(
struct drm_display_mode *mode)
 {
struct msm_display_topology topology;
-   int i, intf_count = 0;
-
-   for (i = 0; i < MAX_PHYS_ENCODERS_PER_VIRTUAL; i++)
-   if (dpu_enc->phys_encs[i])
-   intf_count++;
 
/* User split topology for width > 1080 */
topology.num_lm = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1;
topology.num_enc = 0;
-   topology.num_intf = intf_count;
+   topology.num_intf = dpu_enc->num_phys_encs;
 
return topology;
 }
@@ -583,10 +574,10 @@ static int dpu_encoder_virt_atomic_check(
for (i = 0; i < dpu_enc->num_phys_encs; i++) {
struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
 
-   if (phys && phys->ops.atomic_check)
+   if (phys->ops.atomic_check)
ret = phys->ops.atomic_check(phys, crtc_state,
conn_state);
-   else if (phys && phys->ops.mode_fixup)
+   else if (phys->ops.mode_fixup)
if (!phys->ops.mode_fixup(phys, mode, adj_mode))
ret = -EINVAL;
 
@@ -682,7 +673,7 @@ static void _dpu_encoder_irq_control(struct drm_encoder 
*drm_enc, bool enable)
for (i = 0; i < dpu_enc->num_phys_encs; i++) {
struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
 
-   if (phys && phys->ops.irq_control)
+   if (phys->ops.irq_control)
phys->ops.irq_control(phys, enable);
}
 
@@ -1032,46 +1023,43 @@ static void dpu_encoder_virt_mode_set(struct 
drm_encoder *drm_enc,
for (i = 0; i < dpu_enc->num_phys_encs; i++) {
struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
 
-   if (phys) {
- 

[PATCH 3/6] drm/msm/dpu: Remove unnecessary NULL checks

[Drew Davenport]

dpu_crtc_mixer.hw_lm will never be NULL, so don't check.

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index b9ed8285ab39..bf513411b243 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -197,10 +197,6 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
DPU_DEBUG("%s\n", dpu_crtc->name);
 
for (i = 0; i < cstate->num_mixers; i++) {
-   if (!mixer[i].hw_lm) {
-   DPU_ERROR("invalid lm assigned to mixer\n");
-   return;
-   }
mixer[i].mixer_op_mode = 0;
mixer[i].flush_mask = 0;
if (mixer[i].lm_ctl->ops.clear_all_blendstages)
@@ -1113,12 +1109,9 @@ static int _dpu_debugfs_status_show(struct seq_file *s, 
void *data)
 
for (i = 0; i < cstate->num_mixers; ++i) {
m = >mixers[i];
-   if (!m->hw_lm)
-   seq_printf(s, "\tmixer[%d] has no lm\n", i);
-   else
-   seq_printf(s, "\tmixer:%d ctl:%d width:%d height:%d\n",
-   m->hw_lm->idx - LM_0, m->lm_ctl->idx - CTL_0,
-   out_width, mode->vdisplay);
+   seq_printf(s, "\tmixer:%d ctl:%d width:%d height:%d\n",
+   m->hw_lm->idx - LM_0, m->lm_ctl->idx - CTL_0,
+   out_width, mode->vdisplay);
}
 
seq_puts(s, "\n");
-- 
2.21.0

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

[PATCH 5/6] drm/msm/dpu: Remove unreachable code

[Drew Davenport]

The return statement follows another return statement, so will never be
reached.

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index cfd01b0ac7f1..cc2ecf327582 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -816,6 +816,4 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
DPU_DEBUG_CMDENC(cmd_enc, "created\n");
 
return phys_enc;
-
-   return ERR_PTR(ret);
 }
-- 
2.21.0

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

[PATCH 1/6] drm/msm/dpu: Remove unnecessary NULL checks

[Drew Davenport]

dpu_hw_ctl* is checked for NULL when passed as an argument
to several functions. It will never be NULL, so remove the
checks.

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c  | 10 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 12 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c |  8 +++-
 3 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index f96e142c4361..45a87757e766 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1419,7 +1419,7 @@ static void _dpu_encoder_trigger_flush(struct drm_encoder 
*drm_enc,
}
 
ctl = phys->hw_ctl;
-   if (!ctl || !ctl->ops.trigger_flush) {
+   if (!ctl->ops.trigger_flush) {
DPU_ERROR("missing trigger cb\n");
return;
}
@@ -1469,7 +1469,7 @@ void dpu_encoder_helper_trigger_start(struct 
dpu_encoder_phys *phys_enc)
}
 
ctl = phys_enc->hw_ctl;
-   if (ctl && ctl->ops.trigger_start) {
+   if (ctl->ops.trigger_start) {
ctl->ops.trigger_start(ctl);
trace_dpu_enc_trigger_start(DRMID(phys_enc->parent), ctl->idx);
}
@@ -1513,7 +1513,7 @@ static void dpu_encoder_helper_hw_reset(struct 
dpu_encoder_phys *phys_enc)
dpu_enc = to_dpu_encoder_virt(phys_enc->parent);
ctl = phys_enc->hw_ctl;
 
-   if (!ctl || !ctl->ops.reset)
+   if (!ctl->ops.reset)
return;
 
DRM_DEBUG_KMS("id:%u ctl %d reset\n", DRMID(phys_enc->parent),
@@ -1554,8 +1554,6 @@ static void _dpu_encoder_kickoff_phys(struct 
dpu_encoder_virt *dpu_enc)
continue;
 
ctl = phys->hw_ctl;
-   if (!ctl)
-   continue;
 
/*
 * This is cleared in frame_done worker, which isn't invoked
@@ -1603,7 +1601,7 @@ void dpu_encoder_trigger_kickoff_pending(struct 
drm_encoder *drm_enc)
for (i = 0; i < dpu_enc->num_phys_encs; i++) {
phys = dpu_enc->phys_encs[i];
 
-   if (phys && phys->hw_ctl) {
+   if (phys) {
ctl = phys->hw_ctl;
if (ctl->ops.clear_pending_flush)
ctl->ops.clear_pending_flush(ctl);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 047960949fbb..cfd01b0ac7f1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -62,7 +62,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
return;
 
ctl = phys_enc->hw_ctl;
-   if (!ctl || !ctl->ops.setup_intf_cfg)
+   if (!ctl->ops.setup_intf_cfg)
return;
 
intf_cfg.intf = phys_enc->intf_idx;
@@ -125,7 +125,7 @@ static void dpu_encoder_phys_cmd_ctl_start_irq(void *arg, 
int irq_idx)
 {
struct dpu_encoder_phys *phys_enc = arg;
 
-   if (!phys_enc || !phys_enc->hw_ctl)
+   if (!phys_enc)
return;
 
DPU_ATRACE_BEGIN("ctl_start_irq");
@@ -198,7 +198,7 @@ static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
u32 frame_event = DPU_ENCODER_FRAME_EVENT_ERROR;
bool do_log = false;
 
-   if (!phys_enc || !phys_enc->hw_pp || !phys_enc->hw_ctl)
+   if (!phys_enc || !phys_enc->hw_pp)
return -EINVAL;
 
cmd_enc->pp_timeout_report_cnt++;
@@ -428,7 +428,7 @@ static void _dpu_encoder_phys_cmd_pingpong_config(
struct dpu_encoder_phys_cmd *cmd_enc =
to_dpu_encoder_phys_cmd(phys_enc);
 
-   if (!phys_enc || !phys_enc->hw_ctl || !phys_enc->hw_pp
+   if (!phys_enc || !phys_enc->hw_pp
|| !phys_enc->hw_ctl->ops.setup_intf_cfg) {
DPU_ERROR("invalid arg(s), enc %d\n", phys_enc != 0);
return;
@@ -458,7 +458,7 @@ static void dpu_encoder_phys_cmd_enable_helper(
struct dpu_hw_ctl *ctl;
u32 flush_mask = 0;
 
-   if (!phys_enc || !phys_enc->hw_ctl || !phys_enc->hw_pp) {
+   if (!phys_enc || !phys_enc->hw_pp) {
DPU_ERROR("invalid arg(s), encoder %d\n", phys_enc != 0);
return;
}
@@ -614,7 +614,7 @@ static int _dpu_encoder_phys_cmd_wait_for_ctl_start(
struct dpu_encoder_wait_info wait_info;
int ret;
 
-   if (!phys_enc || !phys_enc->hw_ctl) {
+   if (!phys_enc) {
DPU_ERROR("invalid argument(s)\n");
return -EINVAL;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_

[PATCH] drm/msm: Remove unused function arguments

[Drew Davenport]

The arguments related to IOMMU port name have been unused since
commit 944fc36c31ed ("drm/msm: use upstream iommu") and can be removed.

Signed-off-by: Drew Davenport 
---
Rob, in the original commit the port name stuff was left intentionally.
Would it be alright to remove it now?

 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 10 ++
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 10 ++
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 10 ++
 drivers/gpu/drm/msm/msm_gpu.c|  5 ++---
 drivers/gpu/drm/msm/msm_gpummu.c |  6 ++
 drivers/gpu/drm/msm/msm_iommu.c  |  6 ++
 drivers/gpu/drm/msm/msm_mmu.h|  4 ++--
 7 files changed, 14 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 58b0485dc375..3165c2db2541 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -30,10 +30,6 @@
 #define CREATE_TRACE_POINTS
 #include "dpu_trace.h"
 
-static const char * const iommu_ports[] = {
-   "mdp_0",
-};
-
 /*
  * To enable overall DRM driver logging
  * # echo 0x2 > /sys/module/drm/parameters/debug
@@ -725,8 +721,7 @@ static void _dpu_kms_mmu_destroy(struct dpu_kms *dpu_kms)
 
mmu = dpu_kms->base.aspace->mmu;
 
-   mmu->funcs->detach(mmu, (const char **)iommu_ports,
-   ARRAY_SIZE(iommu_ports));
+   mmu->funcs->detach(mmu);
msm_gem_address_space_put(dpu_kms->base.aspace);
 
dpu_kms->base.aspace = NULL;
@@ -752,8 +747,7 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
return PTR_ERR(aspace);
}
 
-   ret = aspace->mmu->funcs->attach(aspace->mmu, iommu_ports,
-   ARRAY_SIZE(iommu_ports));
+   ret = aspace->mmu->funcs->attach(aspace->mmu);
if (ret) {
DPU_ERROR("failed to attach iommu %d\n", ret);
msm_gem_address_space_put(aspace);
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c 
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 50711ccc8691..dda05436f716 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -157,10 +157,6 @@ static long mdp4_round_pixclk(struct msm_kms *kms, 
unsigned long rate,
}
 }
 
-static const char * const iommu_ports[] = {
-   "mdp_port0_cb0", "mdp_port1_cb0",
-};
-
 static void mdp4_destroy(struct msm_kms *kms)
 {
struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
@@ -172,8 +168,7 @@ static void mdp4_destroy(struct msm_kms *kms)
drm_gem_object_put_unlocked(mdp4_kms->blank_cursor_bo);
 
if (aspace) {
-   aspace->mmu->funcs->detach(aspace->mmu,
-   iommu_ports, ARRAY_SIZE(iommu_ports));
+   aspace->mmu->funcs->detach(aspace->mmu);
msm_gem_address_space_put(aspace);
}
 
@@ -524,8 +519,7 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
 
kms->aspace = aspace;
 
-   ret = aspace->mmu->funcs->attach(aspace->mmu, iommu_ports,
-   ARRAY_SIZE(iommu_ports));
+   ret = aspace->mmu->funcs->attach(aspace->mmu);
if (ret)
goto fail;
} else {
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c 
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 91cd76a2bab1..f8bd0bfcf4b0 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -19,10 +19,6 @@
 #include "msm_mmu.h"
 #include "mdp5_kms.h"
 
-static const char *iommu_ports[] = {
-   "mdp_0",
-};
-
 static int mdp5_hw_init(struct msm_kms *kms)
 {
struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
@@ -233,8 +229,7 @@ static void mdp5_kms_destroy(struct msm_kms *kms)
mdp5_pipe_destroy(mdp5_kms->hwpipes[i]);
 
if (aspace) {
-   aspace->mmu->funcs->detach(aspace->mmu,
-   iommu_ports, ARRAY_SIZE(iommu_ports));
+   aspace->mmu->funcs->detach(aspace->mmu);
msm_gem_address_space_put(aspace);
}
 }
@@ -737,8 +732,7 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
 
kms->aspace = aspace;
 
-   ret = aspace->mmu->funcs->attach(aspace->mmu, iommu_ports,
-   ARRAY_SIZE(iommu_ports));
+   ret = aspace->mmu->funcs->attach(aspace->mmu);
if (ret) {
DRM_DEV_ERROR(>dev, "failed to attach iommu: 
%d\n",
ret);
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm

[PATCH 6/6] drm/msm/dpu: Remove unnecessary NULL checks

[Drew Davenport]

dpu_kms.dev will never be NULL, so don't bother checking.

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c  |  8 -
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |  4 ---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   | 30 +--
 3 files changed, 1 insertion(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
index a53517abf15c..283d5a48fd13 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
@@ -343,10 +343,6 @@ void dpu_core_irq_preinstall(struct dpu_kms *dpu_kms)
struct msm_drm_private *priv;
int i;
 
-   if (!dpu_kms->dev) {
-   DPU_ERROR("invalid drm device\n");
-   return;
-   }
priv = dpu_kms->dev->dev_private;
 
pm_runtime_get_sync(_kms->pdev->dev);
@@ -376,10 +372,6 @@ void dpu_core_irq_uninstall(struct dpu_kms *dpu_kms)
struct msm_drm_private *priv;
int i;
 
-   if (!dpu_kms->dev) {
-   DPU_ERROR("invalid drm device\n");
-   return;
-   }
priv = dpu_kms->dev->dev_private;
 
pm_runtime_get_sync(_kms->pdev->dev);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 39fc39cd2439..d5532836b5b9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -373,10 +373,6 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
}
 
dpu_kms = phys_enc->dpu_kms;
-   if (!dpu_kms->dev) {
-   DPU_ERROR("invalid device\n");
-   return;
-   }
priv = dpu_kms->dev->dev_private;
 
/*
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 9d6429fa6229..fbb154d7c81c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -72,7 +72,7 @@ static int _dpu_danger_signal_status(struct seq_file *s,
struct dpu_danger_safe_status status;
int i;
 
-   if (!kms->dev || !kms->hw_mdp) {
+   if (!kms->hw_mdp) {
DPU_ERROR("invalid arg(s)\n");
return 0;
}
@@ -153,9 +153,6 @@ static int _dpu_debugfs_show_regset32(struct seq_file *s, 
void *data)
return 0;
 
dev = dpu_kms->dev;
-   if (!dev)
-   return 0;
-
priv = dev->dev_private;
base = dpu_kms->mmio + regset->offset;
 
@@ -288,9 +285,6 @@ static void dpu_kms_prepare_commit(struct msm_kms *kms,
return;
dpu_kms = to_dpu_kms(kms);
dev = dpu_kms->dev;
-
-   if (!dev)
-   return;
priv = dev->dev_private;
 
/* Call prepare_commit for all affected encoders */
@@ -461,10 +455,6 @@ static void _dpu_kms_drm_obj_destroy(struct dpu_kms 
*dpu_kms)
struct msm_drm_private *priv;
int i;
 
-   if (!dpu_kms->dev) {
-   DPU_ERROR("invalid dev\n");
-   return;
-   }
priv = dpu_kms->dev->dev_private;
 
for (i = 0; i < priv->num_crtcs; i++)
@@ -496,7 +486,6 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
 
int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret;
int max_crtc_count;
-
dev = dpu_kms->dev;
priv = dev->dev_private;
catalog = dpu_kms->catalog;
@@ -576,8 +565,6 @@ static void _dpu_kms_hw_destroy(struct dpu_kms *dpu_kms)
int i;
 
dev = dpu_kms->dev;
-   if (!dev)
-   return;
 
if (dpu_kms->hw_intr)
dpu_hw_intr_destroy(dpu_kms->hw_intr);
@@ -794,11 +781,6 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
 
dpu_kms = to_dpu_kms(kms);
dev = dpu_kms->dev;
-   if (!dev) {
-   DPU_ERROR("invalid device\n");
-   return rc;
-   }
-
priv = dev->dev_private;
 
atomic_set(_kms->bandwidth_ref, 0);
@@ -1051,11 +1033,6 @@ static int __maybe_unused dpu_runtime_suspend(struct 
device *dev)
struct dss_module_power *mp = _kms->mp;
 
ddev = dpu_kms->dev;
-   if (!ddev) {
-   DPU_ERROR("invalid drm_device\n");
-   return rc;
-   }
-
rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, false);
if (rc)
DPU_ERROR("clock disable failed rc:%d\n", rc);
@@ -1073,11 +1050,6 @@ static int __maybe_unused dpu_runtime_resume(struct 
device *dev)
struct dss_module_power *mp = _kms->mp;
 
ddev = dpu_kms->dev;
-   if (!ddev) {
-   DPU_ERROR("invalid

[PATCH 2/6] drm/msm/dpu: Remove unused macro

[Drew Davenport]

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 4c889aabdaf9..6ceba33a179e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -139,10 +139,6 @@ struct vsync_info {
 
 #define to_dpu_kms(x) container_of(x, struct dpu_kms, base)
 
-/* get struct msm_kms * from drm_device * */
-#define ddev_to_msm_kms(D) ((D) && (D)->dev_private ? \
-   ((struct msm_drm_private *)((D)->dev_private))->kms : NULL)
-
 /**
  * Debugfs functions - extra helper functions for debugfs support
  *
-- 
2.20.1

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

[PATCH 5/6] drm/msm/dpu: Remove unnecessary NULL checks

[Drew Davenport]

msm_drm_private.kms will only be NULL in the dummy headless case, so
there is no need to check it in the dpu display driver.

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c  | 23 ---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 +++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 14 ++-
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |  2 +-
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  5 +---
 drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c  |  6 +
 7 files changed, 19 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
index 17f917b718ce..a53517abf15c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
@@ -55,8 +55,7 @@ static void dpu_core_irq_callback_handler(void *arg, int 
irq_idx)
 int dpu_core_irq_idx_lookup(struct dpu_kms *dpu_kms,
enum dpu_intr_type intr_type, u32 instance_idx)
 {
-   if (!dpu_kms || !dpu_kms->hw_intr ||
-   !dpu_kms->hw_intr->ops.irq_idx_lookup)
+   if (!dpu_kms->hw_intr || !dpu_kms->hw_intr->ops.irq_idx_lookup)
return -EINVAL;
 
return dpu_kms->hw_intr->ops.irq_idx_lookup(intr_type,
@@ -73,7 +72,7 @@ static int _dpu_core_irq_enable(struct dpu_kms *dpu_kms, int 
irq_idx)
unsigned long irq_flags;
int ret = 0, enable_count;
 
-   if (!dpu_kms || !dpu_kms->hw_intr ||
+   if (!dpu_kms->hw_intr ||
!dpu_kms->irq_obj.enable_counts ||
!dpu_kms->irq_obj.irq_counts) {
DPU_ERROR("invalid params\n");
@@ -114,7 +113,7 @@ int dpu_core_irq_enable(struct dpu_kms *dpu_kms, int 
*irq_idxs, u32 irq_count)
 {
int i, ret = 0, counts;
 
-   if (!dpu_kms || !irq_idxs || !irq_count) {
+   if (!irq_idxs || !irq_count) {
DPU_ERROR("invalid params\n");
return -EINVAL;
}
@@ -138,7 +137,7 @@ static int _dpu_core_irq_disable(struct dpu_kms *dpu_kms, 
int irq_idx)
 {
int ret = 0, enable_count;
 
-   if (!dpu_kms || !dpu_kms->hw_intr || !dpu_kms->irq_obj.enable_counts) {
+   if (!dpu_kms->hw_intr || !dpu_kms->irq_obj.enable_counts) {
DPU_ERROR("invalid params\n");
return -EINVAL;
}
@@ -169,7 +168,7 @@ int dpu_core_irq_disable(struct dpu_kms *dpu_kms, int 
*irq_idxs, u32 irq_count)
 {
int i, ret = 0, counts;
 
-   if (!dpu_kms || !irq_idxs || !irq_count) {
+   if (!irq_idxs || !irq_count) {
DPU_ERROR("invalid params\n");
return -EINVAL;
}
@@ -186,7 +185,7 @@ int dpu_core_irq_disable(struct dpu_kms *dpu_kms, int 
*irq_idxs, u32 irq_count)
 
 u32 dpu_core_irq_read(struct dpu_kms *dpu_kms, int irq_idx, bool clear)
 {
-   if (!dpu_kms || !dpu_kms->hw_intr ||
+   if (!dpu_kms->hw_intr ||
!dpu_kms->hw_intr->ops.get_interrupt_status)
return 0;
 
@@ -205,7 +204,7 @@ int dpu_core_irq_register_callback(struct dpu_kms *dpu_kms, 
int irq_idx,
 {
unsigned long irq_flags;
 
-   if (!dpu_kms || !dpu_kms->irq_obj.irq_cb_tbl) {
+   if (!dpu_kms->irq_obj.irq_cb_tbl) {
DPU_ERROR("invalid params\n");
return -EINVAL;
}
@@ -240,7 +239,7 @@ int dpu_core_irq_unregister_callback(struct dpu_kms 
*dpu_kms, int irq_idx,
 {
unsigned long irq_flags;
 
-   if (!dpu_kms || !dpu_kms->irq_obj.irq_cb_tbl) {
+   if (!dpu_kms->irq_obj.irq_cb_tbl) {
DPU_ERROR("invalid params\n");
return -EINVAL;
}
@@ -274,8 +273,7 @@ int dpu_core_irq_unregister_callback(struct dpu_kms 
*dpu_kms, int irq_idx,
 
 static void dpu_clear_all_irqs(struct dpu_kms *dpu_kms)
 {
-   if (!dpu_kms || !dpu_kms->hw_intr ||
-   !dpu_kms->hw_intr->ops.clear_all_irqs)
+   if (!dpu_kms->hw_intr || !dpu_kms->hw_intr->ops.clear_all_irqs)
return;
 
dpu_kms->hw_intr->ops.clear_all_irqs(dpu_kms->hw_intr);
@@ -283,8 +281,7 @@ static void dpu_clear_all_irqs(struct dpu_kms *dpu_kms)
 
 static void dpu_disable_all_irqs(struct dpu_kms *dpu_kms)
 {
-   if (!dpu_kms || !dpu_kms->hw_intr ||
-   !dpu_kms->hw_intr->ops.disable_all_irqs)
+   if (!dpu_kms->hw_intr || !dpu_kms->hw_intr->ops.disable_all_irqs)
return;
 
dpu_kms->hw_intr->ops.disable_all_irqs(dpu_kms->hw_intr);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 839887a3a80c..e96843010dff 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_

[PATCH 1/6] drm/msm/dpu: Remove unused variables

[Drew Davenport]

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 5 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 ---
 2 files changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index ce59adff06aa..2ece11262943 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1288,13 +1288,8 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, 
struct drm_plane *plane,
 {
struct drm_crtc *crtc = NULL;
struct dpu_crtc *dpu_crtc = NULL;
-   struct msm_drm_private *priv = NULL;
-   struct dpu_kms *kms = NULL;
int i;
 
-   priv = dev->dev_private;
-   kms = to_dpu_kms(priv->kms);
-
dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL);
if (!dpu_crtc)
return ERR_PTR(-ENOMEM);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index d82ea994063f..4d2cacd0ce3d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1914,8 +1914,6 @@ static int _dpu_encoder_debugfs_status_open(struct inode 
*inode,
 static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
 {
struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
-   struct msm_drm_private *priv;
-   struct dpu_kms *dpu_kms;
int i;
 
static const struct file_operations debugfs_status_fops = {
@@ -1932,9 +1930,6 @@ static int _dpu_encoder_init_debugfs(struct drm_encoder 
*drm_enc)
return -EINVAL;
}
 
-   priv = drm_enc->dev->dev_private;
-   dpu_kms = to_dpu_kms(priv->kms);
-
snprintf(name, DPU_NAME_SIZE, "encoder%u", drm_enc->base.id);
 
/* create overall sub-directory for the encoder */
@@ -2133,14 +2128,12 @@ static void dpu_encoder_frame_done_timeout(struct 
timer_list *t)
struct dpu_encoder_virt *dpu_enc = from_timer(dpu_enc, t,
frame_done_timer);
struct drm_encoder *drm_enc = _enc->base;
-   struct msm_drm_private *priv;
u32 event;
 
if (!drm_enc->dev || !drm_enc->dev->dev_private) {
DPU_ERROR("invalid parameters\n");
return;
}
-   priv = drm_enc->dev->dev_private;
 
if (!dpu_enc->frame_busy_mask[0] || !dpu_enc->crtc_frame_event_cb) {
DRM_DEBUG_KMS("id:%u invalid timeout frame_busy_mask=%lu\n",
-- 
2.20.1

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

[PATCH 4/6] drm/msm/dpu: Remove unnecessary NULL checks

[Drew Davenport]

drm_crtc.dev will never be NULL, so no need to check it.

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 5 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 6 +++---
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 094d74635fb7..839887a3a80c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -33,11 +33,6 @@ static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc 
*crtc)
 {
struct msm_drm_private *priv;
 
-   if (!crtc->dev) {
-   DPU_ERROR("invalid device\n");
-   return NULL;
-   }
-
priv = crtc->dev->dev_private;
if (!priv->kms) {
DPU_ERROR("invalid kms\n");
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index ead7d657097c..0b9dc042d2e2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -266,7 +266,7 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc 
*crtc)
 {
struct drm_encoder *encoder;
 
-   if (!crtc || !crtc->dev) {
+   if (!crtc) {
DPU_ERROR("invalid crtc\n");
return INTF_MODE_NONE;
}
@@ -694,7 +694,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
unsigned long flags;
bool release_bandwidth = false;
 
-   if (!crtc || !crtc->dev || !crtc->state) {
+   if (!crtc || !crtc->state) {
DPU_ERROR("invalid crtc\n");
return;
}
@@ -766,7 +766,7 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
struct msm_drm_private *priv;
bool request_bandwidth;
 
-   if (!crtc || !crtc->dev) {
+   if (!crtc) {
DPU_ERROR("invalid crtc\n");
return;
}
-- 
2.20.1

[PATCH 3/6] drm/msm/dpu: Remove unnecessary NULL checks

[Drew Davenport]

drm_device.dev_private is set to a non-NULL msm_drm_private
struct in msm_drm_init. Successful initialization of msm means
that dev_private is non-NULL so there is no need to check it
everywhere.

Signed-off-by: Drew Davenport 
---

 drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c |  6 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c|  4 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c |  4 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c  | 15 +--
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  2 +-
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c |  3 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 16 +++-
 7 files changed, 14 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
index cdbea38b8697..17f917b718ce 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
@@ -349,9 +349,6 @@ void dpu_core_irq_preinstall(struct dpu_kms *dpu_kms)
if (!dpu_kms->dev) {
DPU_ERROR("invalid drm device\n");
return;
-   } else if (!dpu_kms->dev->dev_private) {
-   DPU_ERROR("invalid device private\n");
-   return;
}
priv = dpu_kms->dev->dev_private;
 
@@ -385,9 +382,6 @@ void dpu_core_irq_uninstall(struct dpu_kms *dpu_kms)
if (!dpu_kms->dev) {
DPU_ERROR("invalid drm device\n");
return;
-   } else if (!dpu_kms->dev->dev_private) {
-   DPU_ERROR("invalid device private\n");
-   return;
}
priv = dpu_kms->dev->dev_private;
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 09a49b59bb5b..094d74635fb7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -33,13 +33,13 @@ static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc 
*crtc)
 {
struct msm_drm_private *priv;
 
-   if (!crtc->dev || !crtc->dev->dev_private) {
+   if (!crtc->dev) {
DPU_ERROR("invalid device\n");
return NULL;
}
 
priv = crtc->dev->dev_private;
-   if (!priv || !priv->kms) {
+   if (!priv->kms) {
DPU_ERROR("invalid kms\n");
return NULL;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 2ece11262943..ead7d657097c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -694,7 +694,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
unsigned long flags;
bool release_bandwidth = false;
 
-   if (!crtc || !crtc->dev || !crtc->dev->dev_private || !crtc->state) {
+   if (!crtc || !crtc->dev || !crtc->state) {
DPU_ERROR("invalid crtc\n");
return;
}
@@ -766,7 +766,7 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
struct msm_drm_private *priv;
bool request_bandwidth;
 
-   if (!crtc || !crtc->dev || !crtc->dev->dev_private) {
+   if (!crtc || !crtc->dev) {
DPU_ERROR("invalid crtc\n");
return;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 4d2cacd0ce3d..7b37d1eeeab5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -735,8 +735,7 @@ static int dpu_encoder_resource_control(struct drm_encoder 
*drm_enc,
struct msm_drm_private *priv;
bool is_vid_mode = false;
 
-   if (!drm_enc || !drm_enc->dev || !drm_enc->dev->dev_private ||
-   !drm_enc->crtc) {
+   if (!drm_enc || !drm_enc->dev || !drm_enc->crtc) {
DPU_ERROR("invalid parameters\n");
return -EINVAL;
}
@@ -1092,7 +1091,7 @@ static void _dpu_encoder_virt_enable_helper(struct 
drm_encoder *drm_enc)
struct msm_drm_private *priv;
struct dpu_kms *dpu_kms;
 
-   if (!drm_enc || !drm_enc->dev || !drm_enc->dev->dev_private) {
+   if (!drm_enc || !drm_enc->dev) {
DPU_ERROR("invalid parameters\n");
return;
}
@@ -1193,9 +1192,6 @@ static void dpu_encoder_virt_disable(struct drm_encoder 
*drm_enc)
} else if (!drm_enc->dev) {
DPU_ERROR("invalid dev\n");
return;
-   } else if (!drm_enc->dev->dev_private) {
-   DPU_ERROR("invalid dev_private\n");
-   return;
}
 
dpu_enc = to_dpu_encoder_v